Closed Bug 1169343 Opened 7 years ago Closed 7 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.
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).
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.
Alright, I think this patch is now polished enough to be ready for review.
(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 :-)
(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 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+
Attachment #8615986 - Attachment description: Implement WorkersView → Implement DebuggerView.Workers
Summary: Implement a UI to list the workers in a tab → Implement DebuggerView.Workers
Try push for the DebuggerView.Workers patch (with comments by jlong addressed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bb9c6ce236e
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).
You need to log in before you can comment on or make changes to this bug.