Add windowless workers UI #7525

bhackett1024 · 2018-12-17T16:13:53Z

Fixes bug 1241958

Summary of Changes

Add windowless worker UI which is used when the devtools.debugger.features.windowless-workers is set (defaults to off).

Test Plan

Tests still need to be added. Since workers don't work in the standalone debugger.html, it seemed better to add new mochitests after this lands.

jasonLaster · 2018-12-17T16:56:35Z

src/reducers/pause.js

 function update(
-  state: PauseState = createPauseState(),
+  allState: AllPauseState = createAllPauseState(),


i think i'd prefer keeping this state as that is a convention we use in other places

jasonLaster · 2018-12-17T16:57:42Z

src/reducers/pause.js

+): AllPauseState {
+  // Actions need to specify any thread they are operating on. These helpers
+  // manage updating the pause state for that thread.
+  const state = () => {


perhaps we can call this getThreadState?

this is a minor thing, but maybe it's possible to only assert the thread action in the update function, this way we can use an expression.

getThreadState => allState.threads[action.thread] || createInitialPauseState()

jasonLaster · 2018-12-17T16:59:49Z

src/reducers/pause.js

+    const threads = { ...allState.threads };
+    threads[action.thread] = newState;
+    return { ...allState, threads };
+  };


I think this is a bit simpler

const updateThreadState = threadState => { if (!action.thread) { throw new Error(`Missing thread in action ${action.type}`); } return { ...state, threads: { ...state.threads, [action.thread]: threadState } }; };

jasonLaster · 2018-12-17T17:04:26Z

src/components/PrimaryPanes/SourcesTreeItem.js

+    const { item, depth } = this.props;
+
+    const name =
+      depth == 0 ? item.name.substr(item.name.indexOf(":") + 1) : item.name;


what is this doing?

This is unmangling the addition of the thread to the URL information which was made in src/utils/sources-tree/addToTree.js

jasonLaster · 2018-12-17T17:05:25Z

src/client/firefox/create.js

@@ -54,12 +58,14 @@ export function createSource(
    isBlackBoxed: false,
    loadedState: "unloaded"
  };
+  clientCommands.registerSource(createdSource);


fortunately i think we will simplify this pretty soon

#7365

jasonLaster · 2018-12-17T17:07:26Z

src/client/firefox/commands.js

+        actor,
+        url: workerClients[actor].url,
+        // Ci.nsIWorkerDebugger.TYPE_DEDICATED
+        type: 0


it might be nice to have a createWorker method.

jasonLaster · 2018-12-17T17:08:10Z

src/client/firefox/commands.js

+  }
+
+  workerClients = newWorkerClients;
+}


it would be nice to move this to its own util because it will likely be extracted later anyways ...

jasonLaster · 2018-12-17T17:31:37Z

src/reducers/pause.js

+  currentThread: string,
+  debuggeeUrl: string,
+  canRewind: boolean,
+  threads: Object


can we type this?

I think so, it looks like this can be { [string]: PauseState }

yep - seems right

jasonLaster · 2018-12-17T17:31:48Z

src/reducers/pause.js

+  threads: Object
+};
+
+export const createAllPauseState = (): PauseState => ({


createPauseState

jasonLaster · 2018-12-17T17:32:36Z

src/reducers/pause.js

+      return {
+        ...state,
+        currentThread: actor
+      };


return {...state, currentThread: action.thread }

jasonLaster · 2018-12-17T17:34:16Z

src/reducers/pause.js

          generated
        }
      };
+
+      return updateThreadState(newState);


updateThreadState({ ...threadState(), frameScopes: { ...threadState().frameScopes, generated } })

jasonLaster · 2018-12-17T17:34:49Z

src/reducers/pause.js

          original,
          mappings
        }
      };
+
+      return updateThreadState(newState);


I think i prefer passing newState directly into updateThreadState as i did before

jasonLaster · 2018-12-17T17:35:00Z

src/reducers/pause.js

    }

    case "BREAK_ON_NEXT":
-      return { ...state, isWaitingOnBreak: true };
+      return updateThreadState({ ...threadState(), isWaitingOnBreak: true });


jasonLaster · 2018-12-17T17:35:37Z

src/reducers/pause.js

+      // Workaround for threads resuming before the initial connection.
+      if (!action.thread && !state.currentThread) {
+        return state;
+      }


jasonLaster · 2018-12-17T17:37:12Z

src/reducers/pause.js

-        ...state,
+    case "EVALUATE_EXPRESSION": {
+      const newState = {
+        ...threadState(),


it feels like we still have a lot of boilerplate
this feels better

return updateThreadState({ ...threadState(), command: action.status === "start" ? "expression" : null });

... but i wonder if we could do a bit more

updateThreadState({ command: action.status === "start" ? "expression" : null });

this means will always compute the threadState inside of update

const updateThreadState = threadState => { if (!action.thread) { throw new Error(`Missing thread in action ${action.type}`); } return { ...state, threads: { ...state.threads, [action.thread]: { ...threadState(), ...threadState } } }; };

jasonLaster · 2018-12-17T17:39:31Z

src/reducers/pause.js

-const getPauseState = state => state.pause;
+function getCurrentPauseState(state: OuterState): ThreadPauseState {
+  return (
+    state.pause.threads[state.pause.currentThread] || createInitialPauseState()


do we need this? createInitialPauseState()

maybe we create this state when we see the new worker?

This could be removed but we would need to make sure that the pause state includes entries for all threads. Right now they are lazily initialized.

i think it would be nice to do that, maybe by observing connect and SET_WORKERS.

bhackett1024 · 2018-12-18T17:53:00Z

The pushes above fix the following issues:

Hacks for separating source trees associated with different threads have been removed. Right now the sources for different threads will be shown in the same source tree as each other, and while this won't be the final design it seems better to do for now to minimize the complexity of this patch.
Suggested changes have been made to reducers/pause.js to simplify and tighten up the logic.
Unit tests have been fixed so that they all pass when running locally.

…ndowless-workers pref.

jasonLaster · 2018-12-18T18:49:00Z

My guess is that we just need to tweak the panel

failed test:  devtools/client/framework/test/browser_browser_toolbox_debugger.js 
failed test:  devtools/client/framework/test/browser_keybindings_01.js 
failed test:  devtools/client/framework/test/browser_toolbox_getpanelwhenready.js 
failed test:  devtools/client/inspector/markup/test/browser_markup_links_06.js 
failed test:  devtools/client/netmonitor/test/browser_net_open_in_debugger.js 
failed test:  devtools/client/netmonitor/test/browser_net_view-source-debugger.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_in_debugger_stackframe.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_eval_in_debugger_stackframe2.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_location_debugger_link.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_sourcemap_nosource.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_stacktrace_location_debugger_link.js

bhackett1024 · 2018-12-18T19:39:34Z

My guess is that we just need to tweak the panel

failed test:  devtools/client/framework/test/browser_browser_toolbox_debugger.js 
failed test:  devtools/client/framework/test/browser_keybindings_01.js 
failed test:  devtools/client/framework/test/browser_toolbox_getpanelwhenready.js 
failed test:  devtools/client/inspector/markup/test/browser_markup_links_06.js 
failed test:  devtools/client/netmonitor/test/browser_net_open_in_debugger.js 
failed test:  devtools/client/netmonitor/test/browser_net_view-source-debugger.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_in_debugger_stackframe.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_eval_in_debugger_stackframe2.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_location_debugger_link.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_sourcemap_nosource.js 
failed test:  devtools/client/webconsole/test/mochitest/browser_webconsole_stacktrace_location_debugger_link.js

AFAICT these failures (well, the first failure at least) is a problem with the new workers.js file:

Error: Module resource://devtools/client/debugger/new/src/client/firefox/workers/index.js is not found at resource://devtools/client/debugger/new/src/client/firefox/workers/index.js

I don't know where it came up with firefox/workers/index.js from, as the new file is firefox/workers.js. I had to update my m-c to add workers.js to the moz.build, and after that the browser_browser_toolbox_debugger.js test passes with the other changes that have been made here.

ochameau

Sorry for the late review, but I have a few comments about the code related to targets and thread client.

ochameau · 2018-12-18T22:04:40Z

src/client/firefox/workers.js

+  const supportsListWorkers = await checkServerSupportsListWorkers({
+    tabTarget,
+    debuggerClient
+  });


We should be able to get rid of that method as we only support up to current release version, which is now 64.
The following code is going to warn when connecting to an old device.

ochameau · 2018-12-18T22:12:31Z

src/client/firefox/workers.js

+    !supportsListWorkers
+  ) {
+    return newWorkerClients;
+  }


threadClient._parent is equivalent to tabTarget.activeTab.
While tabTarget is a Target class instance, its activeTab property refer to the target front.
While tabTarget will always be a Target class instance, activeTab will be a front specific to which target the toolbox is connected to.
If a regular web toolbox, it will be a BrowsingContextTargetFront,
if a worker window toolbox, it will be a WorkerTargetFront,
if a web extension, it will be a WebExtensionTargetFront and so on.

It would be clear if here and elsewhere, you were using tabTarget.activeTab rather than threadClient._parent. Everywhere where you have access to tabTarget in the scope, otherwise, if you only have the threadClient, that's ok, we may try to followup on that.

A much better check here would be to whitelist by target kind, rather than doing this duck typing.
We know which are the target kinds that supports workers.
All browsing context inherited ones as well as content process.
So here we could replace this check by:

if (!tabTarget.isBrowsingContext && !tabTarget.isContentProcess) { return newWorkerClients; }

Note that I'm actively working on https://bugzilla.mozilla.org/show_bug.cgi?id=1465635 which aims to merge tabTarget and its activeTab attribute. So that it will be easier to use and think about this type of code.

ochameau · 2018-12-18T22:16:08Z

src/client/firefox/workers.js

+      const [, consoleClient] = await debuggerClient.attachConsole(
+        workerTargetFront.targetForm.consoleActor,
+        []
+      );


Oh, I have a patch in flight to stop doing that. Stop duplicating console client instances like that.
https://phabricator.services.mozilla.com/D14278

So here, please fetch the console client like this:

const consoleClient = target.activeConsole;

activeConsole will be defined if you called target.attach(). You do this a few lines before this one.

Hmm, I just tried in the browser, and workerTargetFront.activeConsole is undefined at this point.

ochameau · 2018-12-18T22:20:46Z

src/client/firefox/workers.js

+      newWorkerClients[workerThread.actor] = workerClients[workerThread.actor];
+    } else {
+      addThreadEventListeners(workerThread);
+      workerThread.url = workerTargetFront.url;


It looks quite hacky to put url on ThreadClient right here.
Is that really necessary? Can't we use the url attribute we set in newWorkerClients dictionary? If not, a comment would be helpful here.

Oops, we are supposed to be using the url attribute in newWorkerClients instead of this hack. I'll create a PR to fix this.

jasonLaster reviewed Dec 17, 2018

View reviewed changes

jasonLaster force-pushed the windowless-workers branch from 0b351e9 to 2c1e50a Compare December 17, 2018 17:29

jasonLaster reviewed Dec 17, 2018

View reviewed changes

darkwing added the 📚 pr-wip label Dec 17, 2018

jasonLaster force-pushed the windowless-workers branch 2 times, most recently from 0349e0a to 49e298c Compare December 18, 2018 18:02

Add windowless workers UI, gated on the devtools.debugger.features.wi…

e82c4f7

…ndowless-workers pref.

jasonLaster force-pushed the windowless-workers branch from 49e298c to e82c4f7 Compare December 18, 2018 18:04

bhackett1024 and others added 2 commits December 18, 2018 20:58

Don't ignore workers.js

33fc599

Add prefs

d9abb97

jasonLaster force-pushed the windowless-workers branch from 78e8b00 to d9abb97 Compare December 18, 2018 21:18

jasonLaster merged commit 22467a9 into firefox-devtools:master Dec 18, 2018

ochameau reviewed Dec 18, 2018

View reviewed changes

jasonLaster mentioned this pull request Dec 18, 2018

Multi-target worker debugging #7552

Closed

7 tasks

bhackett1024 deleted the windowless-workers branch December 19, 2018 00:30

jasonLaster pushed a commit that referenced this pull request Dec 21, 2018

Add windowless workers UI (#7525)

54bd024

jasonLaster pushed a commit that referenced this pull request Dec 23, 2018

Add windowless workers UI (#7525)

24c919d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add windowless workers UI #7525

Add windowless workers UI #7525

bhackett1024 commented Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

bhackett1024 Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

bhackett1024 Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

jasonLaster Dec 17, 2018

bhackett1024 Dec 17, 2018

jasonLaster Dec 17, 2018

bhackett1024 commented Dec 18, 2018

jasonLaster commented Dec 18, 2018 •

edited

bhackett1024 commented Dec 18, 2018

ochameau left a comment

ochameau Dec 18, 2018

ochameau Dec 18, 2018

ochameau Dec 18, 2018

bhackett1024 Dec 19, 2018

ochameau Dec 18, 2018

bhackett1024 Dec 18, 2018

Add windowless workers UI #7525

Add windowless workers UI #7525

Conversation

bhackett1024 commented Dec 17, 2018

Summary of Changes

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhackett1024 commented Dec 18, 2018

jasonLaster commented Dec 18, 2018 • edited

bhackett1024 commented Dec 18, 2018

ochameau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonLaster commented Dec 18, 2018 •

edited