1169343 - Implement DebuggerView.Workers

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

Once bug 1168853 lands we'll have all the functionality we need to start building a UI for worker debugging. The plan is to land this UI incrementally and early, hiding it behind a pref for the time being.

This bug will be about adding the pref to enable worker debugging, and displaying a list of workers for the current tab.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Work in progress

This patch isn't quite ready for review yet, but I wanted to give you the opportunity to get an early look at it, so you'll have some idea of what I'm doing.

This patch does two things:
1. It adds a preference to the toolbox options to enable worker debugging
2. It splits the sources view into a worker view and sources view.

One subtlety is that when a worker is frozen (because its page moves into the bfcache), it needs to be hidden in the worker view, even though its still in the worker list (this is a result of the fact that we use the tab window to filter the list of all workers, and that window didn't change when the page was cached).

I would like to improve the patch so that the worker list takes up as little space as possible, and shows (No workers) when the list is empty. Also, I'm getting a weird error message about _itemsByElement not being defined when the worker list is updated right after enabling worker debugging in the preferences pane (this shouldn't happen, because _itemsByElement is defined by WidgetMethods).

Comment 2

[James Long (:jlongster)]

Cool, mind taking a screenshot of what it looks like? I can try to build the patch in a bit if you don't see this soon.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Implement DebuggerView.Workers

Alright, I think this patch is now polished enough to be ready for review.

Comment 4

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #2)
> Cool, mind taking a screenshot of what it looks like? I can try to build the
> patch in a bit if you don't see this soon.

Do you still need a screenshot of this? I've shown you what it looks like during the devtools team meeting :-)

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> (In reply to James Long (:jlongster) from comment #2)
> > Cool, mind taking a screenshot of what it looks like? I can try to build the
> > patch in a bit if you don't see this soon.
> 
> Do you still need a screenshot of this? I've shown you what it looks like
> during the devtools team meeting :-)

FWIW it's great to have a screenshot on the bug for future reference.  Plus I missed the demo due to connection issues so I'd like to see it also if you don't mind.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Screenshot

Comment 7

[James Long (:jlongster)]

Comment on attachment 8615986 [details] [diff] [review]
Implement DebuggerView.Workers

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

Looks generally OK, and shouldn't conflict much with my refactor.

::: browser/devtools/debugger/test/browser_dbg_panel-size.js
@@ +16,5 @@
>      gTab = aTab;
>      gPanel = aPanel;
>      gDebugger = gPanel.panelWin;
>      gPrefs = gDebugger.Prefs;
> +    gWorkersAndSources = gDebugger.document.getElementById("workers-and-sources-pane");

Did you actually need to change anything in this test other than this ID? I would prefer to minimize changing variable names because we are most likely going to end up with a different UI anyway (that list really need to go somewhere else, the left panel is already too complicated, though I'm not sure where yet)

It's a little annoying to land this with a "quick" UI that we know will change, but I understand why we should do that. I'd like to minimize the amount of backtracking we will do when we finalize the UI though.

Comment 8

[Eddy Bruel [:ejpbruel]]

Try push for the DebuggerView.Workers patch (with comments by jlong addressed):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bb9c6ce236e

Comment 9

[Eddy Bruel [:ejpbruel]]

The try push shows some test failures for browser_graphs-11b.js but these look like they were caused by earlier commits that have since been backed out (see 288cd0b9c9a3).

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/38869fcc5305

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38869fcc5305