Closed
Bug 1169343
Opened 9 years ago
Closed 9 years ago
Implement DebuggerView.Workers
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
39.44 KB,
patch
|
jlong
:
review+
|
Details | Diff | Splinter Review |
364.24 KB,
image/png
|
Details |
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•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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.
Updated•9 years ago
|
Blocks: ServiceWorkers-B2G
Assignee | ||
Comment 3•9 years ago
|
||
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•9 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 :-)
Comment 5•9 years ago
|
||
(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•9 years ago
|
||
Comment 7•9 years ago
|
||
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•9 years ago
|
Attachment #8615986 -
Attachment description: Implement WorkersView → Implement DebuggerView.Workers
Assignee | ||
Updated•9 years ago
|
Summary: Implement a UI to list the workers in a tab → Implement DebuggerView.Workers
Assignee | ||
Comment 8•9 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•9 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).
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/38869fcc5305
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•