New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add windowless workers UI #7525

Merged
merged 3 commits into from Dec 18, 2018

Conversation

Projects
None yet
4 participants
@bhackett1024
Copy link
Contributor

bhackett1024 commented Dec 17, 2018

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.

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

perhaps we can call this getThreadState?

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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()

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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
      }
    };
  };
const { item, depth } = this.props;

const name =
depth == 0 ? item.name.substr(item.name.indexOf(":") + 1) : item.name;

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

what is this doing?

This comment has been minimized.

@bhackett1024

bhackett1024 Dec 17, 2018

Author Contributor

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

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

fortunately i think we will simplify this pretty soon

#7365

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

it might be nice to have a createWorker method.

}

workerClients = newWorkerClients;
}

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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

@jasonLaster jasonLaster force-pushed the bhackett1024:windowless-workers branch from 0b351e9 to 2c1e50a Dec 17, 2018

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

can we type this?

This comment has been minimized.

@bhackett1024

bhackett1024 Dec 17, 2018

Author Contributor

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

yep - seems right

threads: Object
};

export const createAllPauseState = (): PauseState => ({

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

createPauseState

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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

generated
}
};

return updateThreadState(newState);

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor
updateThreadState({
  ...threadState(),
  frameScopes: {
    ...threadState().frameScopes,
    generated
  }
})
original,
mappings
}
};

return updateThreadState(newState);

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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

}

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

👍

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

yikes

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

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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 }
      }
    };
  };
const getPauseState = state => state.pause;
function getCurrentPauseState(state: OuterState): ThreadPauseState {
return (
state.pause.threads[state.pause.currentThread] || createInitialPauseState()

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

do we need this? createInitialPauseState()

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

This comment has been minimized.

@bhackett1024

bhackett1024 Dec 17, 2018

Author Contributor

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.

This comment has been minimized.

@jasonLaster

jasonLaster Dec 17, 2018

Contributor

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

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

@bhackett1024

This comment has been minimized.

Copy link
Contributor Author

bhackett1024 commented Dec 18, 2018

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.

@jasonLaster jasonLaster force-pushed the bhackett1024:windowless-workers branch 2 times, most recently from 0349e0a to 49e298c Dec 18, 2018

@jasonLaster jasonLaster force-pushed the bhackett1024:windowless-workers branch from 49e298c to e82c4f7 Dec 18, 2018

@jasonLaster

This comment has been minimized.

Copy link
Contributor

jasonLaster commented Dec 18, 2018

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

This comment has been minimized.

Copy link
Contributor Author

bhackett1024 commented Dec 18, 2018

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.

bhackett1024 and others added some commits Dec 18, 2018

@jasonLaster jasonLaster force-pushed the bhackett1024:windowless-workers branch from 78e8b00 to d9abb97 Dec 18, 2018

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

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ochameau
Copy link
Contributor

ochameau left a comment

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

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

This comment has been minimized.

@ochameau

ochameau Dec 18, 2018

Contributor

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.

!supportsListWorkers
) {
return newWorkerClients;
}

This comment has been minimized.

@ochameau

ochameau Dec 18, 2018

Contributor

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.

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

This comment has been minimized.

@ochameau

ochameau Dec 18, 2018

Contributor

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.

This comment has been minimized.

@bhackett1024

bhackett1024 Dec 19, 2018

Author Contributor

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

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

This comment has been minimized.

@ochameau

ochameau Dec 18, 2018

Contributor

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.

This comment has been minimized.

@bhackett1024

bhackett1024 Dec 18, 2018

Author Contributor

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 jasonLaster referenced this pull request Dec 18, 2018

Open

Multi-target worker debugging #7552

1 of 7 tasks complete

@bhackett1024 bhackett1024 deleted the bhackett1024:windowless-workers branch Dec 19, 2018

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment