Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Use source map service from toolbox in Firefox #2506

Merged
merged 1 commit into from
Mar 31, 2017

Conversation

jryans
Copy link
Contributor

@jryans jryans commented Mar 30, 2017

Summary of Changes

  • When used as a Firefox panel, the debugger gets a sourceMapService object from the toolbox, which manages the worker itself, etc. Those details are hidden from the debugger.
  • When used in a tab, the debugger starts the source map worker like it did before.

Test Plan

  • Verified source maps work when running in a tab
  • Verified source maps work when running as a panel

@codecov
Copy link

codecov bot commented Mar 30, 2017

Codecov Report

Merging #2506 into master will decrease coverage by 0.58%.
The diff coverage is 59.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2506      +/-   ##
==========================================
- Coverage   55.58%   54.99%   -0.59%     
==========================================
  Files          39       39              
  Lines        1801     1813      +12     
  Branches      379      376       -3     
==========================================
- Hits         1001      997       -4     
- Misses        800      816      +16
Impacted Files Coverage Δ
src/actions/pause.js 8.77% <0%> (-0.88%) ⬇️
src/actions/navigation.js 0% <0%> (ø) ⬆️
src/utils/test-head.js 63.63% <100%> (+1.73%) ⬆️
src/actions/breakpoints.js 46.85% <57.14%> (-4.64%) ⬇️
src/actions/sources.js 61.53% <76.47%> (-1.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc025ca...dbc87db. Read the comment docs.

stopSourceMapWorker();
stopPrettyPrintWorker();
stopParserWorker();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for moving this to bootstrap? I was thinking of doing the same thing, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was helpful to see both bootstrap and teardown in the same file. For example with this change, we need to use the same Firefox conditional for the source map worker. With it all in one file, it's easy to see they work the same way.

@@ -1,11 +1,12 @@
// @flow

/**
* Utils for mochitest
* Utils for Jest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

@jryans jryans force-pushed the source-map-from-toolbox branch 2 times, most recently from 0d891b5 to adbf644 Compare March 31, 2017 16:07
Copy link
Contributor

@jasonLaster jasonLaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Lets wait for the mochitest fix to land to make sure this doesn't break anything else

clearSourceMaps();
clearDocuments();
return async function({ dispatch, getState, client, sourceMaps }: ThunkArgs) {
sourceMaps.clearSourceMaps();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this use await

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, probably so since it goes through the worker messaging. I'll update it.

let { frames, why, loadedObjects } = pauseInfo;
frames = await updateFrameLocations(frames);
frames = await updateFrameLocations(frames, sourceMaps);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


bootstrapWorker();
bootstrapWorkers();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

menuOptions.push(jumpLabel);
}
// TODO: Find a new way to only add this for mapped sources?
menuOptions.push(jumpLabel);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - this will be tough, the way we're doing it now. But that is okay. This will just be a no-op in the context menu.

bootstrap(React, ReactDOM).then(onConnect);
window.L10N = L10N;
// $FlowIgnore:
window.L10N.setBundle(require("../assets/panel/debugger.properties"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


export function teardownWorkers() {
if (!isFirefoxPanel()) {
// When used in Firefox, the toolbox manages the source map worker.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jryans jryans merged commit e3e1e6e into firefox-devtools:master Mar 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants