In bug 697762 comment 16, Mihai made a good suggestion: _createPropertyElement() currently creates a DIV DOM element and adds its own methods. Why not have a PropertyElement object that you create instances of? That object could have the methods and properties it needs, have it keep a ref to some DIV and work with the DOM elements in a cleaner approach. Where custom props and methods are needed just make the changes to the PropertyElement instance - not to the DOM element like now. Ideally, each type of PropertyElement object would have its own object that inherits from PropertyElement, but that would be overkill.

This work will start after we land the initial debugger in m-c.

This is a good bug for making a bit broader changes. As Panos pointed out the whole frontend feels slow, because of too many reasons, and there's a lot of stuff that we can be more smart about.

I have started working on this bug. Beware.

Boosting priority because other depending bugs, like 794823.

Things are looking good so far.. https://tbpl.mozilla.org/?tree=Try&rev=684a53489fa6 ..for a ~400kb patch.

This is an almost-finished rerfercter/rewrite of the frontend view. It's also a qfold on top of bug 794941, because most of the things changed there were cleaned up in this bug. Some other minor polish I'd like to do for the controller as well, because reasons. (especially carbonara callbacks, but also duplicating the sources contents cache store, awkward mess when trying to show a source, weird communication between the breakpoints controller vs. view etc.) To deal with ~4000 lines files, we now basically have: debugger-controller.js (protocol handling) debugger-view.js (unclassifiable view stuff (constructors, prompts, etc.)) debugger-panes.js (stackframes, breakpoints, variables, global search) debugger-toolbar.js (toolbar on top) VariableView.jsm (tree-view provider, not yet finished, see bug 794823) All tests pass on try. Rob, just let me know if the engineering in this patch make sense. Once the variables jsm is done, i'll put a patch here for full review. Any API changes required for bug 794823 and making other people's lives easier will be done there).

Comment on attachment 669999 [details] [diff] [review] v1 Review of attachment 669999 [details] [diff] [review]: ----------------------------------------------------------------- nice reorganization. I wonder if it's a tad too-granular. toolbar.js feels like it could still sit well in debugger-view.js, but since you've already done it, might as well keep the separation going. ::: browser/devtools/commandline/CmdBreak.jsm @@ +80,5 @@ > let dbg = win.DebuggerUI.getDebugger(); > let files = []; > if (dbg) { > + let sourcesView = dbg.contentWindow.DebuggerView.Sources; > + for each (let location in sourcesView.values) { you know me and for loops. Can this be a for..of loop? ::: browser/devtools/debugger/debugger-controller.js @@ +50,5 @@ > } > this._isInitialized = true; > window.removeEventListener("DOMContentLoaded", this._startupDebugger, true); > > + DebuggerView.initialize(function() { much nicer @@ +552,3 @@ > return set(); > } > + if (!aNoSwitch && DebuggerView.Sources.containsValue(aUrl)) { this Scripts -> Sources change gets a little verbose, doesn't it? I see the reasoning for it but it makes a lot of changes. @@ +1622,5 @@ > * Tells if you want to skip any breakpoint pane updates. > */ > removeBreakpoint: > function BP_removeBreakpoint(aBreakpoint, aCallback, aNoEditorUpdate, aNoPaneUpdate) { > + if (!aBreakpoint) { is this ever likely to be null? @@ +1906,5 @@ > + * > + * @param object target > + * @param object properties > + */ > +function create({ constructor, proto }, properties = {}) { is this used anywhere? ::: browser/devtools/debugger/debugger.xul @@ +18,5 @@ > <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/> > <script type="text/javascript" src="debugger-controller.js"/> > <script type="text/javascript" src="debugger-view.js"/> > + <script type="text/javascript" src="debugger-toolbar.js"/> > + <script type="text/javascript" src="debugger-panes.js"/> my one concern with this is additional loading overhead. While the breakdown is nicer, more well-contained, the one advantage (and the biggest reason) we have so many large JS files is to minimize startup pain. I still like what you've done here, so we'll see if anybody complains.

(In reply to Rob Campbell [:rc] (:robcee) from comment #8) > Comment on attachment 669999 [details] [diff] [review] > v1 > > Review of attachment 669999 [details] [diff] [review]: > ----------------------------------------------------------------- > Thanks for the feedback! > nice reorganization. I wonder if it's a tad too-granular. toolbar.js feels > like it could still sit well in debugger-view.js, but since you've already > done it, might as well keep the separation going. > I really, really dislike giant files. > ::: browser/devtools/commandline/CmdBreak.jsm > @@ +80,5 @@ > > let dbg = win.DebuggerUI.getDebugger(); > > let files = []; > > if (dbg) { > > + let sourcesView = dbg.contentWindow.DebuggerView.Sources; > > + for each (let location in sourcesView.values) { > > you know me and for loops. Can this be a for..of loop? > Okay. The changes here were only caused by different function names (it was a for each loop previously as well), but sure, I can change this now. > ::: browser/devtools/debugger/debugger-controller.js > @@ +50,5 @@ > > } > > this._isInitialized = true; > > window.removeEventListener("DOMContentLoaded", this._startupDebugger, true); > > > > + DebuggerView.initialize(function() { > > much nicer > :) > @@ +552,3 @@ > > return set(); > > } > > + if (!aNoSwitch && DebuggerView.Sources.containsValue(aUrl)) { > > this Scripts -> Sources change gets a little verbose, doesn't it? I see the > reasoning for it but it makes a lot of changes. > Yeah, unfortunately I had to do this because we're reusing the *exact* same functionality for chrome globals. So a constructor with things like "containsUrl", or "addScript" wouldn't make sense. It also wouldn't make sense to duplicate the implementation, hence the more abstracty methods. > @@ +1622,5 @@ > > * Tells if you want to skip any breakpoint pane updates. > > */ > > removeBreakpoint: > > function BP_removeBreakpoint(aBreakpoint, aCallback, aNoEditorUpdate, aNoPaneUpdate) { > > + if (!aBreakpoint) { > > is this ever likely to be null? Yeah. For example: the current way breakpoints are "disabled" is by removing them, but keeping info about them in the breakpoints pane. If I want to then remove a breakpoint from the pane, to make the implementation there use less conditionals, i just do a check here. > > @@ +1906,5 @@ > > + * > > + * @param object target > > + * @param object properties > > + */ > > +function create({ constructor, proto }, properties = {}) { > > is this used anywhere? > Everywhere actually! Every single menulist, pane, search results. > ::: browser/devtools/debugger/debugger.xul > @@ +18,5 @@ > > <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/> > > <script type="text/javascript" src="debugger-controller.js"/> > > <script type="text/javascript" src="debugger-view.js"/> > > + <script type="text/javascript" src="debugger-toolbar.js"/> > > + <script type="text/javascript" src="debugger-panes.js"/> > > my one concern with this is additional loading overhead. While the breakdown > is nicer, more well-contained, the one advantage (and the biggest reason) we > have so many large JS files is to minimize startup pain. > > I still like what you've done here, so we'll see if anybody complains. It's just 2 files. I hope it's alright.

(In reply to Victor Porof [:vp] from comment #9) > (In reply to Rob Campbell [:rc] (:robcee) from comment #8) > > ::: browser/devtools/debugger/debugger.xul > > @@ +18,5 @@ > > > <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/> > > > <script type="text/javascript" src="debugger-controller.js"/> > > > <script type="text/javascript" src="debugger-view.js"/> > > > + <script type="text/javascript" src="debugger-toolbar.js"/> > > > + <script type="text/javascript" src="debugger-panes.js"/> > > > > my one concern with this is additional loading overhead. While the breakdown > > is nicer, more well-contained, the one advantage (and the biggest reason) we > > have so many large JS files is to minimize startup pain. > > > > I still like what you've done here, so we'll see if anybody complains. > > It's just 2 files. I hope it's alright. It's disk access in the tool's startup path, can't we evaluate the perf hit with the profiler? Preprocessing is another option (like browser.js), but we lose easy debugging that way.

(In reply to Panos Astithas [:past] from comment #10) > (In reply to Victor Porof [:vp] from comment #9) > > (In reply to Rob Campbell [:rc] (:robcee) from comment #8) > > > ::: browser/devtools/debugger/debugger.xul > > > @@ +18,5 @@ > > > > <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/> > > > > <script type="text/javascript" src="debugger-controller.js"/> > > > > <script type="text/javascript" src="debugger-view.js"/> > > > > + <script type="text/javascript" src="debugger-toolbar.js"/> > > > > + <script type="text/javascript" src="debugger-panes.js"/> > > > > > > my one concern with this is additional loading overhead. While the breakdown > > > is nicer, more well-contained, the one advantage (and the biggest reason) we > > > have so many large JS files is to minimize startup pain. > > > > > > I still like what you've done here, so we'll see if anybody complains. > > > > It's just 2 files. I hope it's alright. > > It's disk access in the tool's startup path, can't we evaluate the perf hit > with the profiler? Preprocessing is another option (like browser.js), but we > lose easy debugging that way. That's a good point, a blocked disk pipe is pretty damn scary. Note that we also have lots of Cu.imports everywhere. It's also true that some of them may be already loaded. But when starting up the debugger first, there's (removing the obviously-already-loaded-jsms from the list for brevity): Cu.import("resource:///modules/source-editor.jsm"); Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); Cu.import("resource://gre/modules/devtools/dbg-client.jsm"); Cu.import("resource:///modules/devtools/LayoutHelpers.jsm"); Cu.import("resource:///modules/devtools/VariablesView.jsm"); This may actually be a counter-argument ("we already load so many files, adding 2 more is like pretending we don't have a problem"). Did *a bit* of profiling (didn't really know what to look for), but methods that have "file" or "io" in them take a cumulative 0.6 to 1.8%. However, my machine is a really bad for testing because ssd.

(In reply to Victor Porof [:vp] from comment #11) > (In reply to Panos Astithas [:past] from comment #10) > > (In reply to Victor Porof [:vp] from comment #9) > > > (In reply to Rob Campbell [:rc] (:robcee) from comment #8) > > > > ::: browser/devtools/debugger/debugger.xul > > > > @@ +18,5 @@ > > > > > <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/> > > > > > <script type="text/javascript" src="debugger-controller.js"/> > > > > > <script type="text/javascript" src="debugger-view.js"/> > > > > > + <script type="text/javascript" src="debugger-toolbar.js"/> > > > > > + <script type="text/javascript" src="debugger-panes.js"/> > > > > > > > > my one concern with this is additional loading overhead. While the breakdown > > > > is nicer, more well-contained, the one advantage (and the biggest reason) we > > > > have so many large JS files is to minimize startup pain. > > > > > > > > I still like what you've done here, so we'll see if anybody complains. > > > > > > It's just 2 files. I hope it's alright. > > > > It's disk access in the tool's startup path, can't we evaluate the perf hit > > with the profiler? Preprocessing is another option (like browser.js), but we > > lose easy debugging that way. > > That's a good point, a blocked disk pipe is pretty damn scary. > > Note that we also have lots of Cu.imports everywhere. It's also true that > some of them may be already loaded. But when starting up the debugger first, > there's (removing the obviously-already-loaded-jsms from the list for > brevity): > Cu.import("resource:///modules/source-editor.jsm"); > Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); > Cu.import("resource://gre/modules/devtools/dbg-client.jsm"); > Cu.import("resource:///modules/devtools/LayoutHelpers.jsm"); > Cu.import("resource:///modules/devtools/VariablesView.jsm"); > > This may actually be a counter-argument ("we already load so many files, > adding 2 more is like pretending we don't have a problem"). > > Did *a bit* of profiling (didn't really know what to look for), but methods > that have "file" or "io" in them take a cumulative 0.6 to 1.8%. However, my > machine is a really bad for testing because ssd. Good points. The most interesting question to me is whether this thing makes the performance improvements you make in this patch observably regress the startup time for the tool (of course the speedups after startup still matter). Can you count the time between toggleDebugger() and firing Debugger:Loaded with and without the patch to see what the difference is? If it's not that much, we can move on for now and reassess when it becomes a problem.

yeah, I didn't mean to derail progress. Was merely optimizing prematurely. Onward!

Rebased and halfway through implementing the VariablesView.jsm. Now also with a decent OptionsView constructor (soon to be turned into a 'gear menu', bug 796148).

(In reply to Rob Campbell [:rc] (:robcee) from comment #8) > Comment on attachment 669999 [details] [diff] [review] > v1 > > ::: browser/devtools/commandline/CmdBreak.jsm > @@ +80,5 @@ > > let dbg = win.DebuggerUI.getDebugger(); > > let files = []; > > if (dbg) { > > + let sourcesView = dbg.contentWindow.DebuggerView.Sources; > > + for each (let location in sourcesView.values) { > > you know me and for loops. Can this be a for..of loop? > Since there's a generator-iterator available for the DebuggerView.Sources (and all MenuContainer flavors), I just used a for in over the entire thing, because it's sexier.

Almost finished the VariablesView.jsm. Only thing left is variable flashing between pauses and delegating events for editing the value of a debuggee object's property.

Editing the value of a debuggee object's property works properly. Nonenumerable variables/properties added to their own separate container. [Empty text] automatically shown when no variables are present.

This is pretty much finished. One or two more tests fail and need some love, but this should be close to done. The good news is that the only thing needed to be done now in bug 794823 is move a file :)

All tests pass. Unfolding the patch into smaller chunks will follow.

'tis done. Splitting up the patch into chunks for reviewability may be a bit tricky, but here's the VariablesView.jsm for starters.

All the (importable) parts with context (cleanups/tests/css) in the correct order will arrive soon.

Awww yeah. https://tbpl.mozilla.org/?tree=Try&rev=9b388fb7e050

Did another separate run, also green. https://tbpl.mozilla.org/?tree=Try&rev=8e1dd40f8f87 Will start spamming the oths to be sure.

Comment on attachment 673157 [details] [diff] [review] part1 - variables view Review of attachment 673157 [details] [diff] [review]: ----------------------------------------------------------------- very pretty. My quibbles are minor. ::: browser/devtools/debugger/VariablesView.jsm @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +"use strict"; > + > +const Cc = Components.classes; > +const Ci = Components.interfaces; neither Cc nor Ci are used in this file. @@ +82,5 @@ > + > + /** > + * Specifies if enumerable properties and variables should be displayed. > + * @param boolean aFlag > + */ I think the comments for these two setters got exchanged. @@ +105,5 @@ > + > + /** > + * Creates and appends a label signaling that this container is empty. > + */ > + _appendEmptyNotice: function VV__createEmptyNotice() { mismatched method signatures. @@ +302,5 @@ > + /** > + * Toggles between the scope's collapsed and expanded state. > + */ > + toggle: function S_toggle() { > + this.expanded ^= 1; lol sadly, this violates the _isExpanded property's boolean type as-described in the @return part of get expanded()'s comment. @@ +993,5 @@ > +Property.prototype.__iterator__ = function VV_iterator() { > + for (let item of this._store) { > + yield item; > + } > +}; <3 @@ +1104,5 @@ > + let count = 0; > + return function(aName = "") { > + return aName.toLowerCase().trim().replace(/\s+/g, "-") + (++count); > + } > +})(); ermagherd! @@ +1121,5 @@ > + for (let name in properties) { > + propertiesObject[name] = Object.getOwnPropertyDescriptor(properties, name); > + } > + constructor.prototype = Object.create(proto, propertiesObject); > +} *squee* ahem. Is this pretty enough to live somewhere in shared? If it's going to end up getting pasted around we might want to do that.

Comment on attachment 673158 [details] [diff] [review] part2 - debugger view Review of attachment 673158 [details] [diff] [review]: ----------------------------------------------------------------- lovin' the deletions. A couple of minor typos, some wise-cracks about repeated messages and one serious question: Can we use WeakMaps instead of Maps in MenuContainer? ::: browser/devtools/debugger/debugger-view.js @@ +33,5 @@ > + this.Sources.initialize(); > + this.Filtering.initialize(); > + this.StackFrames.initialize(); > + this.Breakpoints.initialize(); > + this.GlobalSearch.initialize(); that is a whole lot of initialize(). @@ +38,5 @@ > > + this.Variables = new VariablesView(document.getElementById("variables")); > + this.Variables.emptyText = L10N.getStr("emptyVariablesText"); > + this.Variables.nonEnumVisible = Prefs.nonEnumVisible; > + this.Variables.eval = DebuggerController.StackFrames.evaluate; hunh! @@ +60,5 @@ > + this.Sources.destroy(); > + this.Filtering.destroy(); > + this.StackFrames.destroy(); > + this.Breakpoints.destroy(); > + this.GlobalSearch.destroy(); that's a whole lot of destroy()... @@ +452,5 @@ > + this.GlobalSearch.clearView(); > + this.GlobalSearch.clearCache(); > + this.StackFrames.empty(); > + this.Breakpoints.empty(); > + this.Variables.empty(); that's a lotta... I should stop. @@ +662,5 @@ > + if (!selectedValue) { > + return false; > + } > + > + let entangledLabel = this.getItemByValue(selectedValue).label; love it. @@ +700,5 @@ > + } > + > + this._itemsByLabel = new Map(); > + this._itemsByValue = new Map(); > + this._itemsByElement = new Map(); Should we be using WeakMaps here? We're not iterating any of these and we are mapping to nodes. No reason to make these hard references, afaict but am willing to entertain the notion. @@ +923,5 @@ > + * - 2: label OR value are different from all other items > + * - 3: only label is required to be different > + * - 4: only value is required to be different > + */ > + uniquenessQualifier: 1, I'm having an identity crisis. @@ +1055,5 @@ > + this._itemsByValue.set(aItem.value, aItem); > + this._itemsByElement.set(aElement, aItem); > + > + aItem._target = aElement; > + return aItem; why return this? For chaining? Some other nefarious purpose? More entanglement? @@ +1111,5 @@ > + * set itemFactory(aCallback:function) > + * > + * TODO: Use this in #796135 - "Provide some obvious UI for scripts filtering". > + * > + * @param nsIDOMNode aAssiciatedNode I bet this typo is going to appear repeatedly. @@ +1114,5 @@ > + * > + * @param nsIDOMNode aAssiciatedNode > + * The element associated with the displayed container. > + */ > +function StackList(aAssiciatedNode) { assiciate this. @@ +1117,5 @@ > + */ > +function StackList(aAssiciatedNode) { > + dumpn("StackList was instantiated with node: " + aAssiciatedNode); > + > + this._parent = aAssiciatedNode; yup. @@ +1424,2 @@ > /** > + * Shows the prompt and awaits for a remote host and port to connect to. s/awaits/waits/

Comment on attachment 673159 [details] [diff] [review] part3 - toolbar Review of attachment 673159 [details] [diff] [review]: ----------------------------------------------------------------- yup. ::: browser/devtools/debugger/debugger-toolbar.js @@ +402,5 @@ > + * The shortened url. > + */ > + trimUrlLength: function SU_trimUrlLength(aUrl, aMaxLength = SOURCE_URL_MAX_LENGTH) { > + if (aUrl.length > aMaxLength) { > + return aUrl.substring(0, aMaxLength) + L10N.ellipsis; a little pedantic, but are we counting aMaxLength to be the max length of the returned string? If so, adding the ellipsis is going to push us over. @@ +632,5 @@ > + ? tokenFlagIndex > + : rawLength; > + > + file = rawValue.slice(0, fileEnd); > + line = ~~(rawValue.slice(fileEnd + 1, lineEnd)) || -1; wha? Isn't this going to always return -1?

(In reply to Rob Campbell [:rc] (:robcee) from comment #35) > @@ +302,5 @@ > > + /** > > + * Toggles between the scope's collapsed and expanded state. > > + */ > > + toggle: function S_toggle() { > > + this.expanded ^= 1; > > lol > > sadly, this violates the _isExpanded property's boolean type as-described in > the @return part of get expanded()'s comment. Well, when I say boolean I mean truthy or falsy values. Doing ^= 1 is fine and will work, although I agree not 100% type consistent or human. > @@ +1121,5 @@ > > + for (let name in properties) { > > + propertiesObject[name] = Object.getOwnPropertyDescriptor(properties, name); > > + } > > + constructor.prototype = Object.create(proto, propertiesObject); > > +} > > *squee* > > ahem. > > Is this pretty enough to live somewhere in shared? If it's going to end up > getting pasted around we might want to do that. It may be a bit premature to do this, since the debugger and variables view are the only ones using this sugar for now. If it appeals to other folks, let's create a Comfort.jsm. (In reply to Rob Campbell [:rc] (:robcee) from comment #36) > @@ +700,5 @@ > > + } > > + > > + this._itemsByLabel = new Map(); > > + this._itemsByValue = new Map(); > > + this._itemsByElement = new Map(); > > Should we be using WeakMaps here? We're not iterating any of these and we > are mapping to nodes. No reason to make these hard references, afaict but am > willing to entertain the notion. I started replying with a comprehensive explanation on why we couldn't do that. After I finished writing it, I realized we can work around the trouble for itemsByElement (which is the major culprit here) (fwiw, we actually are iterating over itemsByElement in a few remote cases and tests). I'll try it out and see if it works. I wish I could say the same thing for itemsByLabel and itemsByValue. Both have string keys which will be garbage collected and (weirdly) would render all the values unreachable. WeakMaps + strings don't work for some reason. > @@ +1055,5 @@ > > + this._itemsByValue.set(aItem.value, aItem); > > + this._itemsByElement.set(aElement, aItem); > > + > > + aItem._target = aElement; > > + return aItem; > > why return this? For chaining? Some other nefarious purpose? More > entanglement? Needed/Sugar in DVMC__appendItem and DVMC__insertItemAt. > @@ +1111,5 @@ > > + * set itemFactory(aCallback:function) > > + * > > + * TODO: Use this in #796135 - "Provide some obvious UI for scripts filtering". > > + * > > + * @param nsIDOMNode aAssiciatedNode > > I bet this typo is going to appear repeatedly. Damn it! (In reply to Rob Campbell [:rc] (:robcee) from comment #37) > ::: browser/devtools/debugger/debugger-toolbar.js > @@ +402,5 @@ > > + * The shortened url. > > + */ > > + trimUrlLength: function SU_trimUrlLength(aUrl, aMaxLength = SOURCE_URL_MAX_LENGTH) { > > + if (aUrl.length > aMaxLength) { > > + return aUrl.substring(0, aMaxLength) + L10N.ellipsis; > > a little pedantic, but are we counting aMaxLength to be the max length of > the returned string? If so, adding the ellipsis is going to push us over. Short version: Tests were relying on this behavior and I was lazy. Also short verson but an engineresque explanation: No, the SOURCE_URL_MAX_LENGHT is for... the source url. Adding an ellipsis shouldn't be considered part of the url imho. > @@ +632,5 @@ > > + ? tokenFlagIndex > > + : rawLength; > > + > > + file = rawValue.slice(0, fileEnd); > > + line = ~~(rawValue.slice(fileEnd + 1, lineEnd)) || -1; > > wha? Isn't this going to always return -1? Jumping to line ":42px" or ":15canada" shouldn't work, thus we're not using parseInt. ~~ is a nice way of converting a string to an integer as long as the string is *actually* a number, which is a must in this case.

(Thanks for the superhero effort and reviewing 4 parts of this patch. That's half. That's awesome). Spammed some b-cs in try and we're all green. https://tbpl.mozilla.org/?tree=Try&rev=06d66a6df591 The only debugger thing that failed once is createRemote, but that's the OOM issue in bug 753225. For Linux there's bug 798849.

(In reply to Victor Porof [:vp] from comment #38) > (In reply to Rob Campbell [:rc] (:robcee) from comment #36) > > @@ +700,5 @@ > > > + } > > > + > > > + this._itemsByLabel = new Map(); > > > + this._itemsByValue = new Map(); > > > + this._itemsByElement = new Map(); > > > > Should we be using WeakMaps here? We're not iterating any of these and we > > are mapping to nodes. No reason to make these hard references, afaict but am > > willing to entertain the notion. > > I started replying with a comprehensive explanation on why we couldn't do > that. > After I finished writing it, I realized we can work around the trouble for > itemsByElement (which is the major culprit here) (fwiw, we actually are > iterating over itemsByElement in a few remote cases and tests). I'll try it > out and see if it works. Sigh, nope, we actually can't. Damn it and I deleted my comprehensive explanation. The short version: consider >> for (let [_, item] of this._itemsByElement) { >> this._untangleItem(item); >> } in DVMC__untangleItem, or >> for (let [element] of this._itemsByElement) { >> count += element.hidden ? 0 : 1; >> } in get visibleItems() You *could* iterate over itemsByValue instead and work with item.target, which is the element node in question. However, we're not going to reach all the nodes this way. Remember that "uniquenessQualifier"? By default, it doesn't allow dupe labels or values, but it's overridden in a few cases. When adding dupes like these, itemsByLabel will retain only one reference to the latest thing added in the container. itemsByElement will know all of them instead, hence the need to iterate on this map when needing to reach all the nodes. Different tl;dr argument: when the nodes disappear from the DOM we always remove them from the maps, or create new maps anyway.

ok, your explanation of why we can't use WeakMap is sufficient (and I hadn't gotten to the tests files yet). And I would also worry about getting the other maps out of sync with one WeakMap so no need to change it. Onwards.

Addressed comments and did some various bits of small cleanups/typo fixes.

Panos noticed that the stepping key shortcuts weren't working. Attached fix for a mismatched event method call in debugger.xul

Comment on attachment 673161 [details] [diff] [review] part4 - panes Review of attachment 673161 [details] [diff] [review]: ----------------------------------------------------------------- overall I'm super-happy with this code. It's super-pretty and beautiful. I would like the dumps to go away though. There are enough of them that I worry about them showing up in a profiler. Even in the off-state. Let us never refactor these again, ok? ::: browser/devtools/debugger/debugger-panes.js @@ +113,5 @@ > + /** > + * The scroll listener for the stackframes container. > + */ > + _onScroll: function DVSF__onScroll() { > + dumpn("The StackFramesView was scrolled"); do we need this level of debugging detail in here? I worry that if we ever turn this on for a try run, we're going to pollute the logs something terrible. @@ +263,5 @@ > + * - silent: pass true to not update the checkbox checked state; > + * this is usually necessary when the checked state will > + * be updated automatically (e.g: on a checkbox click). > + * - callback: function to invoke once the breakpoint is disabled > + * - id: a new id to be applied to the corresponding element node unclear why you'd want to change the id of an already-existent breakpoint. checking callers seems to indicate that this is done when creating the breakpoint for the first time. Almost feels like this could be further refactored but I'll accept that in a followup. @@ +277,5 @@ > + // Set a new id to the corresponding breakpoint element if required. > + if (aOptions.id) { > + breakpointItem.target.id = "breakpoint-" + aOptions.id; > + } > + // Don't update the checkbox state if not necessary. two negatives in a description is hard to understand. Maybe reverse that to "update the checkbox state if necessary". No need to change the if statement. @@ +334,5 @@ > + return false; > + }, > + > + /** > + * Highlights a breakpoint in this breakpoints container. What does highlighting a breakpoint mean in this context? Flashing the breakpoint in the view? @@ +451,5 @@ > + createMenuItem.call(this, "deleteOthers"); > + createMenuSeparator(); > + createMenuItem.call(this, "enableSelf", true); > + createMenuItem.call(this, "disableSelf"); > + createMenuItem.call(this, "deleteSelf"); why reorder all of these? I would think enable/disableSelf should be at the top of the menu. deleteall at the bottom. @@ +478,5 @@ > + let commandId = prefix + aName + "-" + breakpointId + "-command"; > + let menuitemId = prefix + aName + "-" + breakpointId + "-menuitem"; > + > + let label = L10N.getStr("breakpointMenuItem." + aName); > + let func = this["_on" + aName.charAt(0).toUpperCase() + aName.slice(1)]; not clear on why the ordering for these changed compared to the original file. Don't care, but it does make eyeball comparison harder. @@ +699,5 @@ > + }, > + > + _popupset: null, > + _cache: null > +}); whew! that was some menu code! @@ +895,5 @@ > + _onFetchSourcesFinished: function DVGS__onFetchSourcesFinished() { > + dumpn("Fetched: " + this._sourcesCount + " sources for the GlobalSearchView cache"); > + > + // All sources are fetched and stored in the cache, we can start searching. > + this._performGlobalSearch(); We do this when finished fetching sources? Always? _performGlobalSearch could probably just be included in this method since there are no other senders. @@ +899,5 @@ > + this._performGlobalSearch(); > + }, > + > + /** > + * Finds string matches in all the sources stored in the cache, and groups extra space between the and sources. @@ +1083,5 @@ > + > + sourceResultsItem.instance.expand(true); > + this._currentlyFocusedMatch = LineResults.indexOfElement(target); > + this._scrollMatchIntoViewIfNeeded(target); > + this._bounceMatch(target); bouncy! @@ +1130,5 @@ > + * > + * @param nsIDOMNode aMatch > + * The match to scroll into view. > + */ > + _scrollMatchIntoViewIfNeeded: function DVGS__scrollMatchIntoViewIfNeeded(aMatch) { I know we had one of these elsewhere. Have you tried using it? http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/LayoutHelpers.jsm#236 @@ +1274,5 @@ > + /** > + * Sets this element's expanded state. > + * @param boolean aFlag > + */ > + set expanded(aFlag) this[aFlag ? "expand" : "collapse"](), ergh!

Comment on attachment 673163 [details] [diff] [review] part6 - css Review of attachment 673163 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/gnomestripe/devtools/debugger.css @@ +11,5 @@ > /** > * Debugger content > */ > > +#chrome-globals, #sources { no longer need the padding: 0? @@ +70,5 @@ > -moz-padding-end: 4px; > -moz-border-end: 1px dotted #aaa; > text-align: end; > font: 8pt monospace; > + color: hsl(0,0%,55%); why these changes? @@ +280,5 @@ > background: url("chrome://browser/skin/identity-icons-https.png") no-repeat; > opacity: 0.5; > } > > +@media (min-resolution: 2dppx) { you're really liking that retina display, aren't you?

(In reply to Rob Campbell [:rc] (:robcee) from comment #44) > Comment on attachment 673161 [details] [diff] [review] > part4 - panes > > Review of attachment 673161 [details] [diff] [review]: > ----------------------------------------------------------------- > > overall I'm super-happy with this code. It's super-pretty and beautiful. I > would like the dumps to go away though. There are enough of them that I > worry about them showing up in a profiler. Even in the off-state. > Yeah, sure, these were added in there just for the development of the patch (making sure things are called in the right order etc.). Will remove them before landing. > Let us never refactor these again, ok? > That's the point ;) > ::: browser/devtools/debugger/debugger-panes.js > @@ +113,5 @@ > > + /** > > + * The scroll listener for the stackframes container. > > + */ > > + _onScroll: function DVSF__onScroll() { > > + dumpn("The StackFramesView was scrolled"); > > do we need this level of debugging detail in here? I worry that if we ever > turn this on for a try run, we're going to pollute the logs something > terrible. Nope. > > @@ +263,5 @@ > > + * - silent: pass true to not update the checkbox checked state; > > + * this is usually necessary when the checked state will > > + * be updated automatically (e.g: on a checkbox click). > > + * - callback: function to invoke once the breakpoint is disabled > > + * - id: a new id to be applied to the corresponding element node > > unclear why you'd want to change the id of an already-existent breakpoint. > checking callers seems to indicate that this is done when creating the > breakpoint for the first time. Almost feels like this could be further > refactored but I'll accept that in a followup. > Disabling a breakpoint currently means removing it both via client and the source editor. Enabling a breakpoint again is essentially re-adding it (hence the use in addBreakpoint). The old actor id shouldn't be preserved. This confused tests. > @@ +334,5 @@ > > + return false; > > + }, > > + > > + /** > > + * Highlights a breakpoint in this breakpoints container. > > What does highlighting a breakpoint mean in this context? Flashing the > breakpoint in the view? > Selecting it, just like in the stackframes view. We're reusing code here :) > @@ +451,5 @@ > > + createMenuItem.call(this, "deleteOthers"); > > + createMenuSeparator(); > > + createMenuItem.call(this, "enableSelf", true); > > + createMenuItem.call(this, "disableSelf"); > > + createMenuItem.call(this, "deleteSelf"); > > why reorder all of these? I would think enable/disableSelf should be at the > top of the menu. deleteall at the bottom. > Right click on breakpoint, the closest thing to the cursor is "DELETE ALL THE THINGS". I think that's bad, don't you? :/ Usually i just want to remove/disable one breakpoint. > > @@ +1130,5 @@ > > + * > > + * @param nsIDOMNode aMatch > > + * The match to scroll into view. > > + */ > > + _scrollMatchIntoViewIfNeeded: function DVGS__scrollMatchIntoViewIfNeeded(aMatch) { > > I know we had one of these elsewhere. Have you tried using it? > > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/ > LayoutHelpers.jsm#236 > Yup, we talked about it. Didn't work.

(In reply to Rob Campbell [:rc] (:robcee) from comment #45) > Comment on attachment 673163 [details] [diff] [review] > part6 - css > > Review of attachment 673163 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/gnomestripe/devtools/debugger.css > @@ +11,5 @@ > > /** > > * Debugger content > > */ > > > > +#chrome-globals, #sources { > > no longer need the padding: 0? > True story: the selector wasn't pointing at anything in the first place. > @@ +70,5 @@ > > -moz-padding-end: 4px; > > -moz-border-end: 1px dotted #aaa; > > text-align: end; > > font: 8pt monospace; > > + color: hsl(0,0%,55%); > > why these changes? > To match the source editor gutter. Taken from the editor theme css. > @@ +280,5 @@ > > background: url("chrome://browser/skin/identity-icons-https.png") no-repeat; > > opacity: 0.5; > > } > > > > +@media (min-resolution: 2dppx) { > > you're really liking that retina display, aren't you? Ermagherd yes.

Addressed comments and removed dumps.

Comment on attachment 673165 [details] [diff] [review] part7 - cmds + controller cleanup Review of attachment 673165 [details] [diff] [review]: ----------------------------------------------------------------- Really nice stuff. ::: browser/devtools/debugger/DebuggerUI.jsm @@ +565,5 @@ > */ > get height() { > if (this._height === undefined) { > this._height = Services.prefs.getIntPref("devtools.debugger.ui.height"); > } tidy. ::: browser/devtools/debugger/debugger-controller.js @@ +68,5 @@ > > + DebuggerView.destroy(function() { > + this.SourceScripts.disconnect(); > + this.StackFrames.disconnect(); > + this.ThreadState.disconnect(); hey look! all those destroys got tidied. @@ +463,5 @@ > + > + let self = this; > + let name = ""; > + > + do { ahh. good ol' do {. @@ +594,5 @@ > if (aVar.fetched) { > return; > } > > + this.activeThread.pauseGrip(aGrip).getPrototypeAndProperties(function(aResponse) { anonymizing this function won't necessarily make it easier to spot if something goes wrong. Losing the variable is virtuous though.

gulp.. https://hg.mozilla.org/integration/fx-team/rev/3380c067c0c6

http://hg.mozilla.org/mozilla-central/annotate/71b2ea41dff7/browser/devtools/debugger/DebuggerUI.jsm#l22 Shouldn't "resource:///modules/Services.jsm" be "resource://gre/modules/Services.jsm" ?

This bug fixed a number of intermittent test failures that also affect Aurora. Can we consider this for possible uplift?

(In reply to Ryan VanderMeulen from comment #56) > This bug fixed a number of intermittent test failures that also affect > Aurora. Can we consider this for possible uplift? It contains string changes and it's a hefty ~600KB patch. Is it that important now that we are one week away from the next merge?

We'll still be starring those failures for the next 6 weeks on beta (plus occasionally 6 weeks after that on beta), so it's not like they'll all just go away in a week when we merge. Ultimately, it's your call about whether it's worth requesting the uplift on (and obviously the string changes would be a no-go), and it's up to the release drivers to make the final call, but from my standpoint, if the patch is safe, it would be nice to have on Aurora, yes.

BTW, being on m-c for two weeks with seemingly no ill effects makes me think that the risk isn't horribly high on this. And I'm assuming that a string-free Aurora patch would include the fix for bug 807756 as well.

So we are talking about a version of this patch without the string changes, right? Victor, how much work would that be, and how confident are you that the resulting patch wouldn't be too risky for aurora?

(In reply to Ryan VanderMeulen from comment #59) (In reply to Panos Astithas [:past] from comment #60) > So we are talking about a version of this patch without the string changes, > right? Victor, how much work would that be, and how confident are you that > the resulting patch wouldn't be too risky for aurora? The idea of landing this on Aurora scares me a bit :) The number of bugs that landed before (and possibly affect) this patch is not entirely negligible. While rebasing shouldn't be hard, and the best case scenario is stopping a few oranges from occurring on aurora, I'm worried that the lack of testing without all those dependencies could result in more oranges. I'm not sure I'm comfortable with the tradeoff. Ryan, is the number of intermittent test failures that affect Aurora that large? If so and this patch would make a considerable difference, I could play with it a bit, rebase and see what happens.

Hooray for orange fixes! Can we just disable the tests on aurora? That incurs some risk (if an uplift breaks something the tests are testing), but I'd judge that risk lower than uplifting this patch.

(In reply to Dave Camp (:dcamp) from comment #62) > Hooray for orange fixes! > > Can we just disable the tests on aurora? That incurs some risk (if an > uplift breaks something the tests are testing), but I'd judge that risk > lower than uplifting this patch. we could. That's going to leave us blind on aurora though, as you suggest. This pretty much would require disabling *every* debugger test.