1200798 - Refactor sources and breakpoints to use redux

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

Just realized I never filed a bug for this, but I'm working on this right now. This is the bulk of the debugger cleanup. Once this is done, everything concerning sources and breakpoints will be going through the fluxify library that landed in 1177891, which isn't too dissimilar from how perf tools works from what I hear. I already did a lot of this refactor back in June, but that was mostly the views. I'm also splitting apart debugger-controller.js and turning the controllers more into "stores" which just manage state transitions.

This shouldn't take too much longer (2 weeks?) but I suspect updating all the tests will take a while. Updating the tests could be done in parallel though so if Eddy or someone helps it shouldn't be too bad.

After this is done, we'll have some good code to show how various use cases work in a flux-style environment. I'll work more on docs and see what people think then.

Comment 1

[James Long (:jlongster)]

Attachment: 1200798.patch

For anyone following allowing, here's my current progress. I haven't gotten it working yet as I've mostly just been refactoring the code for the past week or so. I'm not going to keep fixing bugs until it runs and then keep polishing.

So much of the complicated async work has vanished. Some of this is hindsight and the benefit of fresh code, but the flux-style stuff has also been key to simplifying things.

Comment 2

[James Long (:jlongster)]

Attachment: 1200798.patch

Comment 3

[James Long (:jlongster)]

Attachment: 1200798.patch

a much more robust patch

Comment 4

[James Long (:jlongster)]

Attachment: 1200798.patch

Here's a much more up-to-date patch. Note that it's still rough, but I finally rebased on top of the massive file migration and everything seems to work again. Now I'm off to write some docs, look at how much the tests need fixing up, and make it easy to involve others.

Comment 5

[James Long (:jlongster)]

Tracking spreadsheet for fixing tests: https://docs.google.com/spreadsheets/d/10cGSYR7Ztytw2ZPLgC9-a9It2U2bVFOAoI6Cj0X6_wc/edit

Comment 6

[James Long (:jlongster)]

Found a test that's been failing silently. Needs a fix to the addon-sdk tabs, this new bug does that.

Comment 7

[James Long (:jlongster)]

I am 77% of the way through the tests, which means most of them are passing. I think the ones left should go fast, and I'm planning on landing this right after the uplift in Nov.

https://docs.google.com/spreadsheets/d/10cGSYR7Ztytw2ZPLgC9-a9It2U2bVFOAoI6Cj0X6_wc/edit#gid=0

Comment 8

[James Long (:jlongster)]

I'll attach updated patches in a few minutes.

Comment 9

[James Long (:jlongster)]

Attachment: 1177891.patch

Here's the latest work. Getting really close, about 80% of the tests passing and the rest shouldn't take too long.

Comment 10

[James Long (:jlongster)]

Attachment: 1177891-tests.patch

And here are all the tests cleaned up/refactored.

Comment 11

[James Long (:jlongster)]

Attachment: 1200798.patch

This is almost the final code. All tests pass now! This does not include the tests. Eddy, you said you wanted to look at it. Feel free to ask me any questions. I'm also going to sit down and write documentation about this, so feel free to wait until later this week until you can read that too.

Note that this isn't ready to be committed; I haven't done a final polish passover to fix any eslint/style issues, etc

Comment 12

[James Long (:jlongster)]

Attachment: 1200798-tests.patch

Cleaned up tests!

Comment 13

[James Long (:jlongster)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c98ff04835d

Comment 14

[James Long (:jlongster)]

Another: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8158c8ec04e9

Comment 15

[James Long (:jlongster)]

3rd: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ed0b5e35fe2

Just fixing a few other tools that call into the debugger, most things look good!

Comment 16

[James Long (:jlongster)]

Almost there, just some webconsole/toolbox stuff: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bb984861c64

Comment 17

[James Long (:jlongster)]

Another: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd5fa0bd2c19

Comment 18

[James Long (:jlongster)]

I think logging all the actions actually makes some tests run too slowly :( See if that helps the few that are left: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e9eb8240898

Comment 19

[James Long (:jlongster)]

There is one test that's giving me trouble. Hard to tell what's going on because only part of the system is going through redux (where we can log all actions). Added some more logging, hopefully I'll understand once this is done: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb905ef20ae

Comment 20

[James Long (:jlongster)]

Maybe? https://treeherder.mozilla.org/#/jobs?repo=try&revision=aef88313c653

Comment 21

[James Long (:jlongster)]

Oops, accidentally removed a line in one of the server conditional bp tests. Looks like most of the failures are related to that test. New one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=601d1b2ddb9f

Comment 22

[James Long (:jlongster)]

Attachment: 1200798-tests.patch

Cleaned up code

Comment 23

[James Long (:jlongster)]

Attachment: 1200798.patch

Comment 24

[James Long (:jlongster)]

Eddy I cleaned up a few parts. I'm still planning on going through it more tomorrow night. But please go ahead and do a high-level review, I'm not going to change anything significant from here on.

Comment 25

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8690649 [details] [diff] [review]
1200798.patch

Review of attachment 8690649 [details] [diff] [review]:
-----------------------------------------------------------------

There is a lot of code here, so this is necessarily only a high-level review. That said, the overall patch looks good to me. I have a couple of nits, some comments, and several questions. If we can go over those over Vidyo tomorrow, I think we can give this an r+ tomorrow.

I'd still need to go over debugger-controller.js and debugger-view.js, and I'd like to at least go over some of the tests in the other patch. I'll do that tomorrow before our Vidyo meeting.

::: devtools/client/debugger/content/actions/breakpoints.js
@@ +10,5 @@
> +const {
> +  getSource, getBreakpoint, getBreakpoints, makeLocationId
> +} = require('../queries');
> +
> +function addBreakpoints(bps) {

Is the following correct?

1. addBreakpoints is an action creator.
2. An action creator normally returns just one action.
3. However, if an action creator returns a function, the middleware will call that function instead.
4. The function returned by an action creator takes a dispatch function as argument.
5. We can use the dispatch function to dispatch multiple actions with a single action creator.

@@ +18,5 @@
> +    });
> +  };
> +}
> +
> +// TODO: this is a hack for now

It would be useful to explain *why* this is a hack, as it is not obvious to someone who is seeing this code for the first time.

@@ +26,5 @@
> +  BREAKPOINT_CLIENT_STORE.set(actor, client);
> +}
> +
> +function getBreakpointClient(actor) {
> +  return BREAKPOINT_CLIENT_STORE.get(actor);

Couldn't we just get the clients from gThreadClient?

@@ +40,5 @@
> +  const currentBp = getBreakpoint(state, location);
> +
> +  if(currentBp) {
> +    if(currentBp.disabled) {
> +      // A disabled breakpoint exists, so reuse it so any properties

Nit: don't use 'so' in the same sentence twice.

@@ +45,5 @@
> +      // like conditions survive across disabling/enabling
> +      return currentBp;
> +    }
> +    else {
> +      // Do nothing because the breakpoint already exists

I was slightly confused by this comment, because this function doesn't do anything if the breakpoint *does* exist either. This function doesn't really make a breakpoint, it tries to get a breakpoint if it already exists. Perhaps getBreakpoint (or even maybeGetBreakpoint) would be a better name for this?

@@ +54,5 @@
> +  return { location, condition };
> +}
> +
> +function addBreakpoint(location, condition) {
> +  return (dispatch, getState) => {

Is the getState argument automatically passed by the middleware, just like dispatch?

@@ +68,5 @@
> +      breakpoint: bp,
> +      condition: condition,
> +      [PROMISE]: Task.spawn(function*() {
> +        const sourceClient = gThreadClient.source(
> +          getSource(getState(), bp.location.actor)

Because of the way redux works, the state is always in a consistent state at this point, correct?

@@ +78,5 @@
> +        });
> +        const { isPending, actualLocation } = response;
> +
> +        // Save the client instance
> +        setBreakpointClient(bpClient.actor, bpClient);

I assume this is necessary because we can only store JSON data in the store, so we need a separate place to store client objects? Assuming that's correct, this would have been nice to add as an explanation to your hack comment earlier.

@@ +80,5 @@
> +
> +        // Save the client instance
> +        setBreakpointClient(bpClient.actor, bpClient);
> +
> +        return {

I am somewhat confused about this return statement. I assume that, since we're returning a value from a Task, that this is translated to a DONE action for the corresponding Promise. Is this correct?

@@ +102,5 @@
> +function removeBreakpoint(location) {
> +  return _removeBreakpoint(location);
> +}
> +
> +function _removeBreakpoint(location, isDisabled) {

How about renaming this removeOrDisableBreakpoint? _ carries no semantic meaning.

@@ +111,5 @@
> +    }
> +    if (bp.loading) {
> +      // TODO(jwl): make this wait until the breakpoint is saved if it
> +      // is still loading
> +      throw new Error('attempt to remove unsaved breakpoint');

This looks like a regression to me. Didn't we use to handle this case correctly before?

Arguably, this is not a huge deal, since saving breakpoints is something that happens quickly from the user's point of view, so this will likely only cause problems if the user adds and then immediately removes a breakpoint again. Still, this seems like a case we should handle correctly, especially since we used to do so before.

What do you think?

@@ +161,5 @@
> +    return dispatch({
> +      type: constants.SET_BREAKPOINT_CONDITION,
> +      breakpoint: bp,
> +      condition: condition,
> +      [PROMISE]: Task.spawn(function*() {

This task is run within the same tick of the event loop as the call to setBreakpointCondition, right? Otherwise, it would be possible for bpClient to be removed by another action before the task gets to run, causing the call to setCondition to fail.

::: devtools/client/debugger/content/actions/sources.js
@@ +32,5 @@
> +
> +    // Signal that a new source has been added.
> +    window.emit(EVENTS.NEW_SOURCE);
> +
> +    console.log('JWL ADD_SOURCE ' + JSON.stringify(source));

We probably shouldn't be using console.log here?

@@ +46,5 @@
> +  console.log('JWL SELECT_SOURCE (' + id + ') ');
> +  return (dispatch, getState) => {
> +    if(!gThreadClient) {
> +      // No connection, do nothing. This happens when the debugger is
> +      // shutdown too fast and it tries to display a default source.

Nit: is shut down

@@ +50,5 @@
> +      // shutdown too fast and it tries to display a default source.
> +      return;
> +    }
> +
> +    console.log('JWL [' + id + '] ' + source.actor);

Should we be using console.log here?

@@ +54,5 @@
> +    console.log('JWL [' + id + '] ' + source.actor);
> +    source = getSource(getState(), source.actor);
> +    console.log('JWL [' + id + '] ' + JSON.stringify(source));
> +
> +    dispatch(loadSourceText(source));

I assume the loadSourceText action is necessary so that the source actually exists in the UI before we select it? This is somewhat confusing: how can a source for which we don't have the source text be selected? I feel like this could use some clarification in a comment. Do you agree?

@@ +75,5 @@
> +      // engine may pause in the middle of loading all the sources.
> +      // This is relatively harmless, as individual `newSource`
> +      // notifications are fired for each script and they will be
> +      // added to the UI through that.
> +      if(!response.sources) {

What kind of packet would we receive in this case instead of the expected reply? A paused packet? This could use some clarification.

@@ +185,5 @@
> +
> +    return dispatch({
> +      type: constants.LOAD_SOURCE_TEXT,
> +      source: source,
> +      //[HISTOGRAM_ID]: "DEVTOOLS_DEBUGGER_DISPLAY_SOURCE_%_MS",

Why is this commented out?

@@ +225,5 @@
> +
> +    // Can't use promise.all, because if one fetch operation is rejected, then
> +    // everything is considered rejected, thus no other subsequent source will
> +    // be getting fetched. We don't want that. Something like Q's allSettled
> +    // would work like a charm here.

I agree. I feel like this should be abstracted into a function in DevToolsUtils (or similar). Doing so would make the code here below much easier to read. Agreed?

@@ +235,5 @@
> +        onFetch([source, text, contentType]);
> +      }, err => {
> +        onError(source, err);
> +      });
> +      setTimeout(onTimeout.bind(null, source),

Assuming I'm reading this right: if were fetching N sources, where N is a large number, we end up setting N timeouts in very short succession (the only delay between these timeouts is the time it takes to execute the loop). These timeouts will all fire in very short succession.

Wouldn't it be better to just use a single timeout in this case? Any sources that have not been loaded by the time that timeout expires could be marked as timed out.

::: devtools/client/debugger/content/globalActions.js
@@ +4,5 @@
>  "use strict";
>  
>  const constants = require('./constants');
>  
> +function unload() {

This needs a comment. It's not obvious from the code what this is unloading, or why.

::: devtools/client/debugger/content/reducers/breakpoints.js
@@ +26,5 @@
> +        loading: true,
> +        condition: action.condition || bp.condition || undefined
> +      }));
> +
> +      emitChange(existingBp ? "breakpoint-enabled" : "breakpoint-added",

We're emitting this event so that the UI can update it's views to show to newly added breakpoint, right? I wonder if this isn't too early: since the breakpoint is still loading at this point, it could theoretically happen that we pass the breakpoint after it has been displayed, but before it has been added on the server.

This is probably not a huge deal, since I the interval between displaying the breakpoint and adding it to the server is usually too small for this to matter. On top of that, I think we did the same thing in the old version, right? Anyway, feel free to ignore this comment, but its something to keep in mind.

@@ +37,5 @@
> +
> +      // If the breakpoint moved, update the map
> +      if(actualLocation) {
> +        // TODO: The `setBreakpoint` RDP request rdp request returns
> +        // an `actualLocation` field it doesn't conform to the regular

Nit: that/which doesn't?

@@ +39,5 @@
> +      if(actualLocation) {
> +        // TODO: The `setBreakpoint` RDP request rdp request returns
> +        // an `actualLocation` field it doesn't conform to the regular
> +        // { actor, line } location shape, but it has a `source`
> +        // field. We should fix that.

Please open a bug for this TODO and add the bug number here.

@@ +51,5 @@
> +        const prevLocation = action.breakpoint.location;
> +        const newBp = currentBp.merge({ location: actualLocation });
> +        state = setIn(state, ['breakpoints', movedId], newBp);
> +
> +        emitChange('breakpoint-moved', {

The only reason this event seems to be necessary because we show the breakpoint in the UI too early, before it's final location has been determined. This makes me wonder again if we shouldn't just wait with displaying the breakpoint until it has been added to the server.

Again, feel free to ignore this comment; I understand that this is legacy from our old version, but I think it's something worth thinking about.

@@ +65,5 @@
> +      state = mergeIn(state, ['breakpoints', finalLocationId], {
> +        disabled: false,
> +        loading: false,
> +        actor: actor,
> +        text: text

Why do we need to store this text field in each breakpoint?

@@ +111,5 @@
> +    else if(action.status === 'done') {
> +      return mergeIn(state, ['breakpoints', id], {
> +        loading: false,
> +        // Setting a condition creates a new breakpoint client as of
> +        // now, so we need to update the actor

Does it? That seems wrong. Setting a condition on an existing breakpoint actor should be done in-place, so the underlying actor should not change.

::: devtools/client/debugger/content/views/sources-view.js
@@ +85,5 @@
> +    this._blackBoxCheckboxTooltip = L10N.getStr("blackBoxCheckboxTooltip");
> +
> +    this._commandset = document.getElementById("debuggerCommands");
> +    this._popupset = document.getElementById("debuggerPopupset");
> +    this._cmPopup = document.getElementById("sourceEditorContextMenu");

Nit: not a big fan of the two letter acronyms here.

@@ +95,5 @@
> +    this._toggleBreakpointsButton = document.getElementById("toggle-breakpoints");
> +    this._newTabMenuItem = document.getElementById("debugger-sources-context-newtab");
> +    this._copyUrlMenuItem = document.getElementById("debugger-sources-context-copyurl");
> +
> +    this._noResultsFoundToolTip = new Tooltip(document);

Nit: inconsistent capitalisation (ToolTip vs Tooltip)

@@ +197,5 @@
> +      if(source) {
> +        this.actions.selectSource(source);
> +      }
> +      else {
> +        setNamedTimeout("new-source", NEW_SOURCE_DISPLAY_DELAY, () => {

What does this timeout accomplish?

@@ +265,5 @@
> +    });
> +  },
> +
> +  _parseUrl: function(aSource) {
> +    let fullUrl = aSource.url;

Don't we have a generic URL parser for this? Or is that overkill?

@@ +289,5 @@
> +  },
> +
> +  renderBreakpoint: function(breakpoint, removed) {
> +    if(removed) {
> +      if(this._getBreakpoint(breakpoint)) {

Why does this need to be wrapped in an if-check? If the app state is always in a consistent state when actions are executed, then a breakpoint should always exist when it is being removed, should it not?

If that's not a case, a comment that explains why would be useful here.

@@ +294,5 @@
> +        this._removeBreakpoint(breakpoint)
> +      }
> +    }
> +    else {
> +      if(this._getBreakpoint(breakpoint)) {

Same here.

@@ +316,5 @@
> +    let disabled = breakpoint.disabled;
> +    let location = breakpoint.location;
> +
> +    // Get the source item to which the breakpoint should be attached.
> +    let sourceItem = this.getItemByValue(location.actor);

Under what circumstances would the sourceItem for a breakpoint not exist?

@@ +342,5 @@
> +      // menupopup and commandset are also destroyed.
> +      finalize: this._onBreakpointRemoved
> +    });
> +
> +    if(typeof breakpoint.condition === "string") {

Why do we only highlight a breakpoint if it has a condition?

@@ +361,5 @@
> +   *        @see DebuggerController.Breakpoints.addBreakpoint
> +   */
> +  _removeBreakpoint: function(breakpoint) {
> +    // When a parent source item is removed, all the child breakpoint items are
> +    // also automagically removed.

How?

@@ +927,5 @@
> +        definitions: resultList
> +      };
> +    }
> +
> +    //functionDefinitions is a list with an object full of metadata, extract the

Nit: add a space between // and functionDefinitions

@@ +929,5 @@
> +    }
> +
> +    //functionDefinitions is a list with an object full of metadata, extract the
> +    //data and use to construct a more useful, less cluttered, contextual list
> +    for (let i=0; i<functionDefinitions.length; i++) {

Nit: operators should be separated by whitespace

@@ +976,5 @@
> +
> +    if(source) {
> +      let location = { actor: source.actor, line: start };
> +      if (getBreakpoint(this.getState(), location) && start == end) {
> +        this.highlightBreakpoint(location, { noEditorUpdate: true });

I'm not sure why we are highlighting/unhighlighting the breakpoint here?

@@ +985,5 @@
> +  },
> +
> +  /*
> +   * Uses function definition data to perform actions in different
> +   * cases of how many locations were found: zero, one, or mulitple definitions

Nit: multiple, not mulitple

@@ +1006,5 @@
> +
> +    } else if(definitions.length == 1) {
> +      this.DebuggerView.setEditorLocation(definitions[0].source, definitions[0].startLine);
> +    } else {
> +      //multiple definitions found, do something else

We're not currently doing something else, so perhaps add TODO in front of this?

::: devtools/client/debugger/debugger-controller.js
@@ +184,3 @@
>      this._onTabDetached = this._onTabDetached.bind(this);
> +
> +    const broadcaster = makeStateBroadcaster(() => !!this.activeThread);

Remind me what the function of this code was exactly?

::: devtools/client/debugger/debugger.xul
@@ +495,5 @@
>        <textbox id="conditional-breakpoint-panel-textbox"/>
>      </vbox>
>    </panel>
>  
> +  <panel style="position:fixed;top:0;right:0;" id="redux-devtools"></panel>

I just realised that we didn't really talk about the XUL parts of this refactor, so I don't really understand what this panel is for.

::: devtools/client/debugger/panel.js
@@ +106,5 @@
> +    const { dispatch } =  this._controller;
> +    const deferred = promise.defer();
> +
> +    dispatch({
> +      type: this.panelWin.services.WAIT_UNTIL,

I don't understand the purpose of these fields here.

::: devtools/client/jar.mn
@@ -70,5 @@
>      content/debugger/debugger.css (debugger/debugger.css)
>      content/debugger/debugger-controller.js (debugger/debugger-controller.js)
>      content/debugger/debugger-view.js (debugger/debugger-view.js)
>      content/debugger/views/workers-view.js (debugger/views/workers-view.js)
> -    content/debugger/views/sources-view.js (debugger/views/sources-view.js)

Not sure what this file (jar.mn) is for?

::: devtools/client/shared/redux/middleware/promise.js
@@ +15,5 @@
>      if (!(PROMISE in action)) {
>        return next(action);
>      }
>  
> +    const promiseInst = action[PROMISE];

What does Inst mean? Instance? If so, then why not use that?

Comment 26

[James Long (:jlongster)]

Great review, thanks! I'll go through each comment individually. Note that some of this code was just pre-existing.

(In reply to Eddy Bruel [:ejpbruel] from comment #25)
> 
> ::: devtools/client/debugger/content/actions/breakpoints.js
> @@ +10,5 @@
> > +const {
> > +  getSource, getBreakpoint, getBreakpoints, makeLocationId
> > +} = require('../queries');
> > +
> > +function addBreakpoints(bps) {
> 
> Is the following correct?
> 
> 1. addBreakpoints is an action creator.
> 2. An action creator normally returns just one action.
> 3. However, if an action creator returns a function, the middleware will
> call that function instead.
> 4. The function returned by an action creator takes a dispatch function as
> argument.
> 5. We can use the dispatch function to dispatch multiple actions with a
> single action creator.

Yes, that's exactly right. You can look at the middleware that handles the returned function here: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/redux/middleware/thunk.js. It's super simple.

Note that `addBreakpoints` actually isn't used anywhere any more. I'm removing it now.

> 
> @@ +18,5 @@
> > +    });
> > +  };
> > +}
> > +
> > +// TODO: this is a hack for now
> 
> It would be useful to explain *why* this is a hack, as it is not obvious to
> someone who is seeing this code for the first time.

I will flesh out the comment. It's not really a hack, we just need to think through the interaction of state and clients. This keeps an internal "database" (just a Map) of clients that we can look up based on actor id. We could probably generalize this in the future so it's easy to get a client based on some state.

> 
> @@ +26,5 @@
> > +  BREAKPOINT_CLIENT_STORE.set(actor, client);
> > +}
> > +
> > +function getBreakpointClient(actor) {
> > +  return BREAKPOINT_CLIENT_STORE.get(actor);
> 
> Couldn't we just get the clients from gThreadClient?

I don't see anywhere where it keeps breakpoint clients. It keeps source clients, so we can call `threadClient.source(source)` on it, but not breakpoints. Again we just need to sit down and think about how we want to do this.

> 
> @@ +40,5 @@
> > +  const currentBp = getBreakpoint(state, location);
> > +
> > +  if(currentBp) {
> > +    if(currentBp.disabled) {
> > +      // A disabled breakpoint exists, so reuse it so any properties
> 
> Nit: don't use 'so' in the same sentence twice.
> 
> @@ +45,5 @@
> > +      // like conditions survive across disabling/enabling
> > +      return currentBp;
> > +    }
> > +    else {
> > +      // Do nothing because the breakpoint already exists
> 
> I was slightly confused by this comment, because this function doesn't do
> anything if the breakpoint *does* exist either. This function doesn't really
> make a breakpoint, it tries to get a breakpoint if it already exists.
> Perhaps getBreakpoint (or even maybeGetBreakpoint) would be a better name
> for this?

What do you mean? "because this function doesn't do anything if the breakpoint *does* exist either" that is the exact case that this comment refers to. The breakpoint *does* exist, so return null. This function is really very much *making* a breakpoint, but if it returns `null` it didn't make one because it already exists. When we use this we check for `null` and if that's true we know a breakpoint already exists and we can return early (see `addBreakpoint`)

You're right though that it's not the best way to signal those conditions. I have changed the code to be:

function _doesBreakpointExist(state, location) {
  const currentBp = getBreakpoint(state, location);
  return currentBp && !currentBp.disabled;
}

function _getOrCreateBreakpoint(state, location, condition) {
  return getBreakpoint(state, location) || { location, condition };
}


and used like this:

    if(_breakpointExists(getState(), location) {
      return;
    }

    const bp = _getOrCreateBreakpoint(getState(), location, condition);

> @@ +54,5 @@
> > +  return { location, condition };
> > +}
> > +
> > +function addBreakpoint(location, condition) {
> > +  return (dispatch, getState) => {
> 
> Is the getState argument automatically passed by the middleware, just like
> dispatch?

Yep! It's an optional second argument.

> @@ +68,5 @@
> > +      breakpoint: bp,
> > +      condition: condition,
> > +      [PROMISE]: Task.spawn(function*() {
> > +        const sourceClient = gThreadClient.source(
> > +          getSource(getState(), bp.location.actor)
> 
> Because of the way redux works, the state is always in a consistent state at
> this point, correct?

Yes. I mean, bad async code can still have a reference to an actor that goes away later on. So you can still mess up, but it's a lot harder. And easier to debug when you do.

> 
> @@ +78,5 @@
> > +        });
> > +        const { isPending, actualLocation } = response;
> > +
> > +        // Save the client instance
> > +        setBreakpointClient(bpClient.actor, bpClient);
> 
> I assume this is necessary because we can only store JSON data in the store,
> so we need a separate place to store client objects? Assuming that's
> correct, this would have been nice to add as an explanation to your hack
> comment earlier.

Yeah, I fleshed out the comment.

> 
> @@ +80,5 @@
> > +
> > +        // Save the client instance
> > +        setBreakpointClient(bpClient.actor, bpClient);
> > +
> > +        return {
> 
> I am somewhat confused about this return statement. I assume that, since
> we're returning a value from a Task, that this is translated to a DONE
> action for the corresponding Promise. Is this correct?

Exactly. The final value is the `value` field of the "done" action, so it would look like this: { status: "done", type: ADD_BREAKPOINT, value: { actor: ..., etc }, breakpoint: bp, condition: condition }

Note that the action still has the `breakpoint` and `condition` field from when the whole process started. This is really helpful because you can always inspect the arguments that the async work started with. It's especially helpful in tests (I can call this and have direct access to both the params and the returned fields, helpful for checking things like moving breakpoints)

> 
> @@ +102,5 @@
> > +function removeBreakpoint(location) {
> > +  return _removeBreakpoint(location);
> > +}
> > +
> > +function _removeBreakpoint(location, isDisabled) {
> 
> How about renaming this removeOrDisableBreakpoint? _ carries no semantic
> meaning.

Done.

> 
> @@ +111,5 @@
> > +    }
> > +    if (bp.loading) {
> > +      // TODO(jwl): make this wait until the breakpoint is saved if it
> > +      // is still loading
> > +      throw new Error('attempt to remove unsaved breakpoint');
> 
> This looks like a regression to me. Didn't we use to handle this case
> correctly before?
> 
> Arguably, this is not a huge deal, since saving breakpoints is something
> that happens quickly from the user's point of view, so this will likely only
> cause problems if the user adds and then immediately removes a breakpoint
> again. Still, this seems like a case we should handle correctly, especially
> since we used to do so before.
> 
> What do you think?

Good catch. Technically we handled it, yes. But this actually never came up in tests, and there's why I would think it would come up because UI events are firing as fast as possible. I think that dbg-client actually solves this for us; I'm vague on the details but it makes sure to order packets correctly (the responses will be ordered the way that requests were made). So this should Just Work; a remove request comes in after the add request and the add response comes back and then the remove response.

It didn't come up in a single test, where a lot of adding/removing happens, so I'm not sure it's worth the cost. It's something we can certainly do in the future... Perhaps I'm just eager to land this, I dunno. 

> 
> @@ +161,5 @@
> > +    return dispatch({
> > +      type: constants.SET_BREAKPOINT_CONDITION,
> > +      breakpoint: bp,
> > +      condition: condition,
> > +      [PROMISE]: Task.spawn(function*() {
> 
> This task is run within the same tick of the event loop as the call to
> setBreakpointCondition, right? Otherwise, it would be possible for bpClient
> to be removed by another action before the task gets to run, causing the
> call to setCondition to fail.

As far as I know, as Task.spawn executes the task immediately.

Note that if any of these async requests fail, we have a chance to properly handle the UI in the reducers, and sometimes we ignore them because failures are so rare. (We can do whatever we like. The point is we can decide if it's harmless or not, which the above would be)

> 
> ::: devtools/client/debugger/content/actions/sources.js
> @@ +32,5 @@
> > +
> > +    // Signal that a new source has been added.
> > +    window.emit(EVENTS.NEW_SOURCE);
> > +
> > +    console.log('JWL ADD_SOURCE ' + JSON.stringify(source));
> 
> We probably shouldn't be using console.log here?

Yeesh. Yeah I was debugging a nasty thing on try.

> @@ +46,5 @@
> > +  console.log('JWL SELECT_SOURCE (' + id + ') ');
> > +  return (dispatch, getState) => {
> > +    if(!gThreadClient) {
> > +      // No connection, do nothing. This happens when the debugger is
> > +      // shutdown too fast and it tries to display a default source.
> 
> Nit: is shut down
> 
> @@ +50,5 @@
> > +      // shutdown too fast and it tries to display a default source.
> > +      return;
> > +    }
> > +
> > +    console.log('JWL [' + id + '] ' + source.actor);
> 
> Should we be using console.log here?
> 
> @@ +54,5 @@
> > +    console.log('JWL [' + id + '] ' + source.actor);
> > +    source = getSource(getState(), source.actor);
> > +    console.log('JWL [' + id + '] ' + JSON.stringify(source));
> > +
> > +    dispatch(loadSourceText(source));
> 
> I assume the loadSourceText action is necessary so that the source actually
> exists in the UI before we select it? This is somewhat confusing: how can a
> source for which we don't have the source text be selected? I feel like this
> could use some clarification in a comment. Do you agree?

I can add a comment, but personally I think it's straight-forward: a source existing in the UI is just the name being added to the left pane. The text of the source in the editor is another matter. This just ensures that the text of the source starts loading when its selected.

It can be selected by just clicking the name in the left panel. Doesn't matter if we have the text or not.

`loadSourceText` only loads the text if it hasn't already been loaded, too. 

> 
> @@ +75,5 @@
> > +      // engine may pause in the middle of loading all the sources.
> > +      // This is relatively harmless, as individual `newSource`
> > +      // notifications are fired for each script and they will be
> > +      // added to the UI through that.
> > +      if(!response.sources) {
> 
> What kind of packet would we receive in this case instead of the expected
> reply? A paused packet? This could use some clarification.

Good point, I actually can't remember now but it was something strange; didn't have an `error` but also didn't have `sources`. Probably worth looking into more in the future.

This was actually copied over from previous code: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/debugger-controller.js#L1284 I made the error message more explicit though. I think I was able to this this, but I can't remember how. I just tried and couldn't get it. But basically: this way of checking was copied from before.

> 
> @@ +185,5 @@
> > +
> > +    return dispatch({
> > +      type: constants.LOAD_SOURCE_TEXT,
> > +      source: source,
> > +      //[HISTOGRAM_ID]: "DEVTOOLS_DEBUGGER_DISPLAY_SOURCE_%_MS",
> 
> Why is this commented out?

Because the promise middleware used to do histograms automatically but it doesn't anymore. I can submit the histogram manually.

> @@ +225,5 @@
> > +
> > +    // Can't use promise.all, because if one fetch operation is rejected, then
> > +    // everything is considered rejected, thus no other subsequent source will
> > +    // be getting fetched. We don't want that. Something like Q's allSettled
> > +    // would work like a charm here.
> 
> I agree. I feel like this should be abstracted into a function in
> DevToolsUtils (or similar). Doing so would make the code here below much
> easier to read. Agreed?

This part of the code was actually totally ripped from the previous code: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/debugger-controller.js#L1487

I'd rather do this in a follow-up and not get tied down to cleaning up everything.

> 
> @@ +235,5 @@
> > +        onFetch([source, text, contentType]);
> > +      }, err => {
> > +        onError(source, err);
> > +      });
> > +      setTimeout(onTimeout.bind(null, source),
> 
> Assuming I'm reading this right: if were fetching N sources, where N is a
> large number, we end up setting N timeouts in very short succession (the
> only delay between these timeouts is the time it takes to execute the loop).
> These timeouts will all fire in very short succession.
> 
> Wouldn't it be better to just use a single timeout in this case? Any sources
> that have not been loaded by the time that timeout expires could be marked
> as timed out.

Good point. I was trying to translate old code into this. I changed it to have a single timeout.

> 
> ::: devtools/client/debugger/content/globalActions.js
> @@ +4,5 @@
> >  "use strict";
> >  
> >  const constants = require('./constants');
> >  
> > +function unload() {
> 
> This needs a comment. It's not obvious from the code what this is unloading,
> or why.

Done.

> 
> ::: devtools/client/debugger/content/reducers/breakpoints.js
> @@ +26,5 @@
> > +        loading: true,
> > +        condition: action.condition || bp.condition || undefined
> > +      }));
> > +
> > +      emitChange(existingBp ? "breakpoint-enabled" : "breakpoint-added",
> 
> We're emitting this event so that the UI can update it's views to show to
> newly added breakpoint, right? I wonder if this isn't too early: since the
> breakpoint is still loading at this point, it could theoretically happen
> that we pass the breakpoint after it has been displayed, but before it has
> been added on the server.
> 
> This is probably not a huge deal, since I the interval between displaying
> the breakpoint and adding it to the server is usually too small for this to
> matter. On top of that, I think we did the same thing in the old version,
> right? Anyway, feel free to ignore this comment, but its something to keep
> in mind.

Yeah, we do optimistic rendering on purpose. It just potentially makes it "feel faster", especially if you're on a remote debugger setup. We remove the breakpoint later if an error came through the system.

It's worth talking about in the future, if it gets too complex we can rethink this. But it's how it worked before so I wanted to maintain integrity.

> 
> @@ +37,5 @@
> > +
> > +      // If the breakpoint moved, update the map
> > +      if(actualLocation) {
> > +        // TODO: The `setBreakpoint` RDP request rdp request returns
> > +        // an `actualLocation` field it doesn't conform to the regular
> 
> Nit: that/which doesn't?
> 
> @@ +39,5 @@
> > +      if(actualLocation) {
> > +        // TODO: The `setBreakpoint` RDP request rdp request returns
> > +        // an `actualLocation` field it doesn't conform to the regular
> > +        // { actor, line } location shape, but it has a `source`
> > +        // field. We should fix that.
> 
> Please open a bug for this TODO and add the bug number here.

Done.

> 
> @@ +51,5 @@
> > +        const prevLocation = action.breakpoint.location;
> > +        const newBp = currentBp.merge({ location: actualLocation });
> > +        state = setIn(state, ['breakpoints', movedId], newBp);
> > +
> > +        emitChange('breakpoint-moved', {
> 
> The only reason this event seems to be necessary because we show the
> breakpoint in the UI too early, before it's final location has been
> determined. This makes me wonder again if we shouldn't just wait with
> displaying the breakpoint until it has been added to the server.
> 
> Again, feel free to ignore this comment; I understand that this is legacy
> from our old version, but I think it's something worth thinking about.

Actually that is the main reason we show it early. Its a smooth user experience when the breakpoint moves: it's added where you clicked but we use a CSS transition to slide it down to where it was actually moved. It would feel very odd to click on 1 line and the breakpoint just appear 4 lines down.

It is more complex, yeah, but I think the UX deserves it.

> 
> @@ +65,5 @@
> > +      state = mergeIn(state, ['breakpoints', finalLocationId], {
> > +        disabled: false,
> > +        loading: false,
> > +        actor: actor,
> > +        text: text
> 
> Why do we need to store this text field in each breakpoint?

The text is a snippet of the line where the breakpoint was added so we can show that snippet in the source/breakpoints list. Something like "var a = ...". Ported from previous code, where it also just had `text` on a bp instance.

> 
> @@ +111,5 @@
> > +    else if(action.status === 'done') {
> > +      return mergeIn(state, ['breakpoints', id], {
> > +        loading: false,
> > +        // Setting a condition creates a new breakpoint client as of
> > +        // now, so we need to update the actor
> 
> Does it? That seems wrong. Setting a condition on an existing breakpoint
> actor should be done in-place, so the underlying actor should not change.

Yeah, I've always thought that was bizarre. I'm pretty sure we have a bug for it already. I can't remember why we did this, but it was a lot easier at the time.

> 
> ::: devtools/client/debugger/content/views/sources-view.js
> @@ +85,5 @@
> > +    this._blackBoxCheckboxTooltip = L10N.getStr("blackBoxCheckboxTooltip");
> > +
> > +    this._commandset = document.getElementById("debuggerCommands");
> > +    this._popupset = document.getElementById("debuggerPopupset");
> > +    this._cmPopup = document.getElementById("sourceEditorContextMenu");
> 
> Nit: not a big fan of the two letter acronyms here.

This is totally old code that I never touched :)

> 
> @@ +95,5 @@
> > +    this._toggleBreakpointsButton = document.getElementById("toggle-breakpoints");
> > +    this._newTabMenuItem = document.getElementById("debugger-sources-context-newtab");
> > +    this._copyUrlMenuItem = document.getElementById("debugger-sources-context-copyurl");
> > +
> > +    this._noResultsFoundToolTip = new Tooltip(document);
> 
> Nit: inconsistent capitalisation (ToolTip vs Tooltip)

Old code again :) I will continually be cleaning this up.

> 
> @@ +197,5 @@
> > +      if(source) {
> > +        this.actions.selectSource(source);
> > +      }
> > +      else {
> > +        setNamedTimeout("new-source", NEW_SOURCE_DISPLAY_DELAY, () => {
> 
> What does this timeout accomplish?

This is ported from old code as well; on first startup, there is no source selected. So it waits a little bit (trying to wait for all sources to be listed) and then selects one automatically. The timeout is there to try and let all the sources be listed.

> 
> @@ +265,5 @@
> > +    });
> > +  },
> > +
> > +  _parseUrl: function(aSource) {
> > +    let fullUrl = aSource.url;
> 
> Don't we have a generic URL parser for this? Or is that overkill?

This is old code again, but this isn't actually parsing the URL scheme. It just gets various information from the URL, like what label and group to use. I think the `SourceUtils.getSourceLabel` functions that it uses does do a full URL parse.

> 
> @@ +289,5 @@
> > +  },
> > +
> > +  renderBreakpoint: function(breakpoint, removed) {
> > +    if(removed) {
> > +      if(this._getBreakpoint(breakpoint)) {
> 
> Why does this need to be wrapped in an if-check? If the app state is always
> in a consistent state when actions are executed, then a breakpoint should
> always exist when it is being removed, should it not?
> 
> If that's not a case, a comment that explains why would be useful here.

App state is consistent with itself. One part will always be consistent with another, unlike if we divided app state into multiple places. However, we can never guarantee that app state will be consistent between async actions, because anything can happen in an async world. Requests might come in the wrong order because you did something bad. This is just being defensive because we shouldn't care about that kind of consistency at this point; we are just the view and we'll do what we are told. I can add a comment.

> 
> @@ +294,5 @@
> > +        this._removeBreakpoint(breakpoint)
> > +      }
> > +    }
> > +    else {
> > +      if(this._getBreakpoint(breakpoint)) {
> 
> Same here.

It's going to be a pretty common pattern. Here's we're checking whether to add or update an existing breakpoint. I think that's pretty straight-forward.

> @@ +316,5 @@
> > +    let disabled = breakpoint.disabled;
> > +    let location = breakpoint.location;
> > +
> > +    // Get the source item to which the breakpoint should be attached.
> > +    let sourceItem = this.getItemByValue(location.actor);
> 
> Under what circumstances would the sourceItem for a breakpoint not exist?

I think this is just being defensive again. We're trying to emulate a declarative React-style component. It's just going to render the data that it's given if it can.

> 
> @@ +342,5 @@
> > +      // menupopup and commandset are also destroyed.
> > +      finalize: this._onBreakpointRemoved
> > +    });
> > +
> > +    if(typeof breakpoint.condition === "string") {
> 
> Why do we only highlight a breakpoint if it has a condition?

Because highlighting the breakpoint opens up the popup to enter a conditional value. All that highlighting does is selects the breakpoint in the left pane, it's not normally done. But if you select it and there's a conditional expression an overlay will pop up with an input. I hate this UI and we're going to fix it soon.

> 
> @@ +361,5 @@
> > +   *        @see DebuggerController.Breakpoints.addBreakpoint
> > +   */
> > +  _removeBreakpoint: function(breakpoint) {
> > +    // When a parent source item is removed, all the child breakpoint items are
> > +    // also automagically removed.
> 
> How?

I dunno, this is all old code.

> 
> @@ +927,5 @@
> > +        definitions: resultList
> > +      };
> > +    }
> > +
> > +    //functionDefinitions is a list with an object full of metadata, extract the
> 
> Nit: add a space between // and functionDefinitions

Old code, but I'll fix.

> 
> @@ +929,5 @@
> > +    }
> > +
> > +    //functionDefinitions is a list with an object full of metadata, extract the
> > +    //data and use to construct a more useful, less cluttered, contextual list
> > +    for (let i=0; i<functionDefinitions.length; i++) {
> 
> Nit: operators should be separated by whitespace
> 
> @@ +976,5 @@
> > +
> > +    if(source) {
> > +      let location = { actor: source.actor, line: start };
> > +      if (getBreakpoint(this.getState(), location) && start == end) {
> > +        this.highlightBreakpoint(location, { noEditorUpdate: true });
> 
> I'm not sure why we are highlighting/unhighlighting the breakpoint here?

It just selects the breakpoint on the left. If you move the cursor on a line with a breakpoint it'll highlight. Same as before.

> 
> @@ +985,5 @@
> > +  },
> > +
> > +  /*
> > +   * Uses function definition data to perform actions in different
> > +   * cases of how many locations were found: zero, one, or mulitple definitions
> 
> Nit: multiple, not mulitple
> 
> @@ +1006,5 @@
> > +
> > +    } else if(definitions.length == 1) {
> > +      this.DebuggerView.setEditorLocation(definitions[0].source, definitions[0].startLine);
> > +    } else {
> > +      //multiple definitions found, do something else
> 
> We're not currently doing something else, so perhaps add TODO in front of
> this?

Old code :) will add though

> 
> ::: devtools/client/debugger/debugger-controller.js
> @@ +184,3 @@
> >      this._onTabDetached = this._onTabDetached.bind(this);
> > +
> > +    const broadcaster = makeStateBroadcaster(() => !!this.activeThread);
> 
> Remind me what the function of this code was exactly?

The broadcaster is the thing that reducers fire state change events on, and views can listen for specific state changes. It will automatically suppress all events when the function you pass in returns `false`, so when shutting down it won't emit any events (so no more weird UI errors if changes come in later).

> 
> ::: devtools/client/debugger/debugger.xul
> @@ +495,5 @@
> >        <textbox id="conditional-breakpoint-panel-textbox"/>
> >      </vbox>
> >    </panel>
> >  
> > +  <panel style="position:fixed;top:0;right:0;" id="redux-devtools"></panel>
> 
> I just realised that we didn't really talk about the XUL parts of this
> refactor, so I don't really understand what this panel is for.

Good catch. I think this is the only XUL change, but I just removed it. I used it to use the redux devtools, but we'll add that later.

> 
> ::: devtools/client/debugger/panel.js
> @@ +106,5 @@
> > +    const { dispatch } =  this._controller;
> > +    const deferred = promise.defer();
> > +
> > +    dispatch({
> > +      type: this.panelWin.services.WAIT_UNTIL,
> 
> I don't understand the purpose of these fields here.

Read the docs here: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/redux/middleware/wait-service.js

> 
> ::: devtools/client/jar.mn
> @@ -70,5 @@
> >      content/debugger/debugger.css (debugger/debugger.css)
> >      content/debugger/debugger-controller.js (debugger/debugger-controller.js)
> >      content/debugger/debugger-view.js (debugger/debugger-view.js)
> >      content/debugger/views/workers-view.js (debugger/views/workers-view.js)
> > -    content/debugger/views/sources-view.js (debugger/views/sources-view.js)
> 
> Not sure what this file (jar.mn) is for?

It's for files packaged under the chrome:// URL, which I'm moving away from.

> 
> ::: devtools/client/shared/redux/middleware/promise.js
> @@ +15,5 @@
> >      if (!(PROMISE in action)) {
> >        return next(action);
> >      }
> >  
> > +    const promiseInst = action[PROMISE];
> 
> What does Inst mean? Instance? If so, then why not use that?

Yeah, I changed it from `promise` because that's too confusing with the import. `instance` is such a generic term. But I don't really care either way.

Comment 27

[James Long (:jlongster)]

Attachment: 1200798.patch

Cleaned up in response to Eddy's review

Comment 28

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8691229 [details] [diff] [review]
1200798.patch

Review of attachment 8691229 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look closely at debugger-controller.js and debugger-view.js last time, so I only reviewed those this time around. I trust you to address my review comments from the previous pass, so let's just go quickly over those tonight. There are way too many changes in debugger-view.js for me to be able to review that file quickly, so let's go over that as well.

Like I said before, this patch is so large that the best I can do here is a high-level review. Keeping that in mind, I don't have any high-level issues with the patch, so I will likely r+ it after we talk about it tonight. I do want to take a quick look at the tests too, though, just in case.

I have one overall nit that I'd like for you to address before we land this though: please put a space after the if keyword everywhere.

::: devtools/client/debugger/debugger-controller.js
@@ +122,5 @@
> +  getTargetClient: () => DebuggerController.client,
> +  log: false
> +});
> +const {
> +  makeStateBroadcaster,

Can we go over what these things are for?

@@ +207,5 @@
>      if (this._startup) {
>        return;
>      }
>  
> +    DebuggerView.initialize();

I'm sure you had a good reason for this, but why do we no longer yield here? This was originally useful for tests so that we could guarantee that initialisation was complete. Do we have a different mechanism for that now?

@@ +319,5 @@
>        return;
>      }
>  
> +    this.client.removeListener("newGlobal");
> +    this.activeThread.removeListener("newSource");

What if _startDebuggingTab was never called?

@@ +346,3 @@
>      }
> +
> +    // Discard all the cached sources *before* the target starts navigating.

You're no longer discarding SourceScripts below, so this comment looks obsolete to me.

@@ +486,5 @@
>        }
>  
>        // Reset the view and fetch all the sources again.
>        DebuggerView.handleTabNavigation();
> +      this.dispatch(actions.unload());

It's still not clear to me what unload does here. You claim that loadSources forces the server to traverse all sources, so I would expect that to unload all previous sources?

@@ +500,5 @@
>  
> +  waitForSourcesLoaded: function() {
> +    const deferred = promise.defer();
> +    this.dispatch({
> +      type: services.WAIT_UNTIL,

Why is this implemented as an action?

@@ +867,5 @@
>          this._currentEvaluation = null;
>          this._currentException = null;
>          this._currentReturnedValue = null;
>          break;
> +    case FRAME_TYPE.CONDITIONAL_BREAKPOINT_EVAL:

Nit: inconsistent indentation

@@ -1160,5 @@
>  /**
> - * Keeps the source script list up-to-date, using the thread client's
> - * source script cache.
> - */
> -function SourceScripts() {

This is not necessary anymore because we now store sources and breakpoints in the store, right?

::: devtools/client/debugger/debugger-view.js
@@ -50,5 @@
>  const EventListenersView = require('./content/views/event-listeners-view');
> -const actions = require('./content/actions/event-listeners');
> -
> -Object.defineProperties(this, {
> -  "store": {

Why do we define these things as properties on this if they are already defined as globals?

@@ +94,5 @@
> +      "breakpoint-moved": ({ breakpoint, prevLocation }) => {
> +        const selectedSource = queries.getSelectedSource(getState());
> +        const { location } = breakpoint;
> +
> +        console.log('prevLocation', prevLocation);

Please remove this.

@@ +379,5 @@
>    /**
>     * Display the progress bar.
>     */
>    showProgressBar: function() {
> +    dump("OH YEAH\n");

Please remove this.

@@ +462,4 @@
>  
> +    if(source.isBlackBoxed) {
> +      this.showBlackBoxMessage();
> +      setTimeout(() => {

Why the timeout 0 here? It wasn't there before.

Comment 29

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8691229 [details] [diff] [review]
1200798.patch

Review of attachment 8691229 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look closely at debugger-controller.js and debugger-view.js last time, so I only reviewed those this time around. I trust you to address my review comments from the previous pass, so let's just go quickly over those tonight. There are way too many changes in debugger-view.js for me to be able to review that file quickly, so let's go over that as well.

Like I said before, this patch is so large that the best I can do here is a high-level review. Keeping that in mind, I don't have any high-level issues with the patch, so I will likely r+ it after we talk about it tonight. I do want to take a quick look at the tests too, though, just in case.

I have one overall nit that I'd like for you to address before we land this though: please put a space after the if keyword everywhere.

::: devtools/client/debugger/debugger-controller.js
@@ +122,5 @@
> +  getTargetClient: () => DebuggerController.client,
> +  log: false
> +});
> +const {
> +  makeStateBroadcaster,

Can we go over what these things are for?

@@ +207,5 @@
>      if (this._startup) {
>        return;
>      }
>  
> +    DebuggerView.initialize();

I'm sure you had a good reason for this, but why do we no longer yield here? This was originally useful for tests so that we could guarantee that initialisation was complete. Do we have a different mechanism for that now?

@@ +319,5 @@
>        return;
>      }
>  
> +    this.client.removeListener("newGlobal");
> +    this.activeThread.removeListener("newSource");

What if _startDebuggingTab was never called?

@@ +346,3 @@
>      }
> +
> +    // Discard all the cached sources *before* the target starts navigating.

You're no longer discarding SourceScripts below, so this comment looks obsolete to me.

@@ +486,5 @@
>        }
>  
>        // Reset the view and fetch all the sources again.
>        DebuggerView.handleTabNavigation();
> +      this.dispatch(actions.unload());

It's still not clear to me what unload does here. You claim that loadSources forces the server to traverse all sources, so I would expect that to unload all previous sources?

@@ +500,5 @@
>  
> +  waitForSourcesLoaded: function() {
> +    const deferred = promise.defer();
> +    this.dispatch({
> +      type: services.WAIT_UNTIL,

Why is this implemented as an action?

@@ +867,5 @@
>          this._currentEvaluation = null;
>          this._currentException = null;
>          this._currentReturnedValue = null;
>          break;
> +    case FRAME_TYPE.CONDITIONAL_BREAKPOINT_EVAL:

Nit: inconsistent indentation

@@ -1160,5 @@
>  /**
> - * Keeps the source script list up-to-date, using the thread client's
> - * source script cache.
> - */
> -function SourceScripts() {

This is not necessary anymore because we now store sources and breakpoints in the store, right?

::: devtools/client/debugger/debugger-view.js
@@ -50,5 @@
>  const EventListenersView = require('./content/views/event-listeners-view');
> -const actions = require('./content/actions/event-listeners');
> -
> -Object.defineProperties(this, {
> -  "store": {

Why do we define these things as properties on this if they are already defined as globals?

@@ +94,5 @@
> +      "breakpoint-moved": ({ breakpoint, prevLocation }) => {
> +        const selectedSource = queries.getSelectedSource(getState());
> +        const { location } = breakpoint;
> +
> +        console.log('prevLocation', prevLocation);

Please remove this.

@@ +379,5 @@
>    /**
>     * Display the progress bar.
>     */
>    showProgressBar: function() {
> +    dump("OH YEAH\n");

Please remove this.

@@ +462,4 @@
>  
> +    if(source.isBlackBoxed) {
> +      this.showBlackBoxMessage();
> +      setTimeout(() => {

Why the timeout 0 here? It wasn't there before.

Comment 30

[James Long (:jlongster)]

Thanks for the great review! I'm impressed with how far you were able to go through it in a short amount of time.

> 
> @@ +207,5 @@
> >      if (this._startup) {
> >        return;
> >      }
> >  
> > +    DebuggerView.initialize();
> 
> I'm sure you had a good reason for this, but why do we no longer yield here?
> This was originally useful for tests so that we could guarantee that
> initialisation was complete. Do we have a different mechanism for that now?

The DebuggerView `initialize` function never actually did anything async. It just created a promise and return it. I want to remove promises as much as we can, so I'm simplifying it.

The panel still emits the "ready" event just like before, but we never actually use that in tests. Most of the tests use `waitForSourceShown` to wait for the debugger to load and a source to be shown.

> 
> @@ +319,5 @@
> >        return;
> >      }
> >  
> > +    this.client.removeListener("newGlobal");
> > +    this.activeThread.removeListener("newSource");
> 
> What if _startDebuggingTab was never called?

There is a check above that if a client does not exist it returns early.

> 
> @@ +346,3 @@
> >      }
> > +
> > +    // Discard all the cached sources *before* the target starts navigating.
> 
> You're no longer discarding SourceScripts below, so this comment looks
> obsolete to me.

I clarified the comment; it now just purges the parser cache.

> 
> @@ +486,5 @@
> >        }
> >  
> >        // Reset the view and fetch all the sources again.
> >        DebuggerView.handleTabNavigation();
> > +      this.dispatch(actions.unload());
> 
> It's still not clear to me what unload does here. You claim that loadSources
> forces the server to traverse all sources, so I would expect that to unload
> all previous sources?

This is just `unload`, not `unloadSources` or anything. `unload` is what happens when the page is navigated or something like that, and UI should be "reset". This will be very simple to rename in the future if we want. I also added more comments where this action is defined and used.

> 
> @@ +500,5 @@
> >  
> > +  waitForSourcesLoaded: function() {
> > +    const deferred = promise.defer();
> > +    this.dispatch({
> > +      type: services.WAIT_UNTIL,
> 
> Why is this implemented as an action?

Because we're tapping until the action stream to wait until a certain one passes through the system. You can do this to avoid all the promise shenanigans; you don't need to store promises anywhere for this to get them. You can just wait on an action. Docs about this middleware here: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/redux/middleware/wait-service.js

> 
> @@ +867,5 @@
> >          this._currentEvaluation = null;
> >          this._currentException = null;
> >          this._currentReturnedValue = null;
> >          break;
> > +    case FRAME_TYPE.CONDITIONAL_BREAKPOINT_EVAL:
> 
> Nit: inconsistent indentation
> 
> @@ -1160,5 @@
> >  /**
> > - * Keeps the source script list up-to-date, using the thread client's
> > - * source script cache.
> > - */
> > -function SourceScripts() {
> 
> This is not necessary anymore because we now store sources and breakpoints
> in the store, right?

Right.

> 
> ::: devtools/client/debugger/debugger-view.js
> @@ -50,5 @@
> >  const EventListenersView = require('./content/views/event-listeners-view');
> > -const actions = require('./content/actions/event-listeners');
> > -
> > -Object.defineProperties(this, {
> > -  "store": {
> 
> Why do we define these things as properties on this if they are already
> defined as globals?

This code has been removed. But shu converted globals into this because `const` does not define global variables anymore. (he did this across the entire firefox codebase)

> 
> @@ +94,5 @@
> > +      "breakpoint-moved": ({ breakpoint, prevLocation }) => {
> > +        const selectedSource = queries.getSelectedSource(getState());
> > +        const { location } = breakpoint;
> > +
> > +        console.log('prevLocation', prevLocation);
> 
> Please remove this.
> 
> @@ +379,5 @@
> >    /**
> >     * Display the progress bar.
> >     */
> >    showProgressBar: function() {
> > +    dump("OH YEAH\n");
> 
> Please remove this.
> 
> @@ +462,4 @@
> >  
> > +    if(source.isBlackBoxed) {
> > +      this.showBlackBoxMessage();
> > +      setTimeout(() => {
> 
> Why the timeout 0 here? It wasn't there before.

Because I'm getting away from events being synchronous. They are a recipe for race conditions in tests; tests will wait for an event but resume *in the middle* of debugger code executing because the event was fired. This is really hard to rationalize about, and the action system fixes this. Waiting on an action always resumes once all reducers and views are finished updating. I change a few of the deprecated events to mimic this behavior; in the future I will be removing all events and using only actions.

Comment 31

[James Long (:jlongster)]

I got it working on try before, but here's a new push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41eb46c24c59

Comment 32

[James Long (:jlongster)]

Oops, left in a bad build config: https://treeherder.mozilla.org/#/jobs?repo=try&revision=179b1c3b7de6

Comment 33

[James Long (:jlongster)]

bad conflict: https://treeherder.mozilla.org/#/jobs?repo=try&revision=baea5004845d

Comment 34

[James Long (:jlongster)]

working try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b512044bfd9e

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/34c9087517ea

Comment 36

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/23eb26d374c6 - you'll want to include e10s dt tests in your trychooser syntax, so things like https://treeherder.mozilla.org/logviewer.html#?job_id=5916668&repo=fx-team don't come as a surprise.

Comment 37

[Phil Ringnalda (:philor)]

Probably also a slowdown most felt in the already horrific Linux32 debug chunk which runs debugger tests, currently (at least on your push, it changes chunks constantly) dt4, where you got two instances of "browser_dbg_search-popup-jank.js | Test timed out" and four instances of running the suite over 80 minutes and getting it killed, out of ten runs.

Comment 38

[James Long (:jlongster)]

Thanks Phil. That's the PGO build, isn't that the one that there's a 1/4 chance the try server actually builds that? I don't see the error in any of the other builds (but there is a log of information there).

I did have e10s on in earlier try runs, sorry. (this is only touching frontend so theoretically it shouldn't matter, but theory doesn't mean anything)

Comment 39

[Phil Ringnalda (:philor)]

Fun. No, there's a zero chance of PGO on try, unless you force it with https://wiki.mozilla.org/ReleaseEngineering/TryChooser#What_if_I_want_PGO_for_my_build (although those are not very good instructions, since they'll give you bustage on all the platforms that don't even do PGO, so doing it there rather than in the specific mozconfigs for Linux/Windows is only a good idea if you are restricting your push to just those platforms).

Comment 40

[James Long (:jlongster)]

Yeah, I already did that and saw the bustage on at least OSX. For some reason it's not running the e10s devtools tests though... https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8dd5d167f57

But I was able to reproduce it locally finally. It's an intermittent so I think it was luck that it only happened on PGO. Thanks for catching that.

Comment 41

[James Long (:jlongster)]

Fixed one or two more intermittents and rebased: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a392b9f56de5. None of those failures are related to mine; I've seen all of them while trying to land other patches as well.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/258cff340229

Comment 43

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/37f3907090aa

Comment 44

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5973975&repo=fx-team

Comment 45

[James Long (:jlongster)]

There's only a single failure for that, right? Looking at the log, the way this test works is known to introduce intermittents so I'm not surprised it happened. If it only happens 1/10 runs can land this but file it as a new intermittent? I triggered more dt1 on windows 7 runs to see.

Comment 46

[James Long (:jlongster)]

Alright I triggered 8 more and 2 of them failed for the same reason. This is something I should fix here. Luckily, it is easy to fix and this should work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33f8d976e48f

Comment 47

[James Long (:jlongster)]

Oops, last run was broken, new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed114f1ace62

Comment 48

[James Long (:jlongster)]

Argh there was an infra DDOS that messed up the last run. Instead of retriggering everything just gonna push a new one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0dbc2c2b339

Comment 49

[Phil Ringnalda (:philor)]

There's also, again, https://treeherder.mozilla.org/#/jobs?repo=fx-team&tochange=258cff340229&fromchange=a5ad0e73b4bf&filter-searchStr=eaa61068ef4450a516d963d5765eb501d02f70ba

Maybe the debugger tests on Linux32 debug are just doomed, and you should go ahead and land slowing them down, so we can get to the point of disabling them that much quicker, dunno.

Comment 50

[James Long (:jlongster)]

What do you mean "land slowing them down"? How do I do that?

Linux32 debug has always had problems, and it's not inconceivable that this makes code run over more event loops ticks than before (not much, and is not a problem in production, in fact it makes a lot of race conditions go away). How can we fix the tests?

I really need to land this ASAP.

Comment 51

[James Long (:jlongster)]

Oh, hah, I think you meant just go ahead and land this patch. Yeah, they've been a pain usually.

On the other hand, it does tend to exacerbate race conditions which is good to make them stand out more. But if they are just constantly timing out like that, that doesn't help at all.

Now that I think about it, this patch definitely makes it "slower" not because it runs code over more event loop ticks but it's also a little more computationally complex. But all completely within normal range.

I hope we can go ahead and land this and we can just figure something for those tests?

Comment 52

[Phil Ringnalda (:philor)]

Yes, I meant "land, slowing them down."

There's really no question about what's going to happen: when you land, the bug 1137757 failures will go from an unacceptable 20% to a totally unacceptable 40%, and I will just disable the entire debugger/ test directory on Linux32 debug (well, there's some question about whether someone other than me will notice that you doubled the failure rate and back you out over it, forcing you to do the disabling yourself to be able to land and stick).

Then the good thing would be someone actually fixing them, getting rid of the ~70 assertion failures because asserting and printing stacks is slow, and then looking at the slowest tests and either rewriting them to run faster or rewriting the code they are testing to run faster. Probably won't happen, but there's no more possibility to just add more chunks like we've always done before because now the debugger/ chunk is essentially nothing but debugger/, so the only thing left other than actually speeding them up will be to move the tests into more than one directory, so that they can spread across multiple chunks.

Comment 53

[James Long (:jlongster)]

Ok. I'm going to go ahead and disable the debugger tests on linux32 debug. This is a short-term solution; this gives me even more motivation to clean up the tests in Q1. But the tests were already timing out a lot, so we need to just fix this. I'm not going to block this on that.

Filed bug 1229483 to re-enable them. We also really need to get debugger tests running in e10s in debug.

Same as before, but with it disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8159212fa9

Comment 54

[James Long (:jlongster)]

Well, the skip-if syntax was wrong in the last try push but looking at other files I found the right one that I'm sure will work. Here is the latest try push that does not disable linux32 debug, but beside that failure everything looks pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0dbc2c2b339. I don't think any of those oranges are my fault.

I'm going to try to land this again right now.

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0e47cb064701

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e47cb064701

Comment 57

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8690648 [details] [diff] [review]
1200798-tests.patch

This no longer needs review.