Implement DebuggerView.Workers

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 41
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8612924 [details] [diff] [review]
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).
Attachment #8612924 - Flags: feedback?(jlong)
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.
Blocks: 1131322
(Assignee)

Comment 3

4 years ago
Created attachment 8615986 [details] [diff] [review]
Implement DebuggerView.Workers

Alright, I think this patch is now polished enough to be ready for review.
Attachment #8612924 - Attachment is obsolete: true
Attachment #8612924 - Flags: feedback?(jlong)
Attachment #8615986 - Flags: review?(jlong)
(Assignee)

Comment 4

4 years ago
(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 :-)
(Assignee)

Updated

4 years ago
Blocks: 1171967
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 8616024 [details]
Screenshot
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.
Attachment #8615986 - Flags: review?(jlong) → review+
(Assignee)

Updated

4 years ago
Attachment #8615986 - Attachment description: Implement WorkersView → Implement DebuggerView.Workers
(Assignee)

Updated

4 years ago
Summary: Implement a UI to list the workers in a tab → Implement DebuggerView.Workers
(Assignee)

Comment 8

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

Comment 9

4 years ago
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).
https://hg.mozilla.org/mozilla-central/rev/38869fcc5305
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

4 years ago
Depends on: 1181345

Updated

4 years ago
No longer depends on: 1181345

Updated

3 years ago
Depends on: 1278551

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.