Closed
Bug 650794
Opened 13 years ago
Closed 12 years ago
Disable HTML panel and make it use registerTools API
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 9
People
(Reporter: rcampbell, Assigned: rcampbell)
References
Details
(Whiteboard: [highlighter][minotaur][best: 3h; likely: 1d; worst: 3d])
Attachments
(1 file, 21 obsolete files)
The current HTML view implementation is going to be made obsolete and should be largely unnecessary with the next version of the highlighter (bug 642471).
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → rcampbell
Summary: Strip HTML panel and support code from browser → Disable HTML panel and make it use registerTools API
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
backed out due to testfailures. iframe removal patch landed on central. lol. http://tinderbox.mozilla.org/showlog.cgi?log=Devtools/1310599510.1310600924.985.gz&fulltext=1
Assignee | ||
Comment 2•12 years ago
|
||
Morphing this bug. Rather than remove the HTML panel, we want to make it disabled by default and use the registerTools API in the Inspector code to show and hide the Panel.
Updated•12 years ago
|
Whiteboard: [highlighter] → [highlighter][minotaur]
Assignee | ||
Comment 3•12 years ago
|
||
lots of entanglement with the HTML panel currently. Disconnecting it is going to require brain surgery. Also, lots of tests will need to be updated.
Status: NEW → ASSIGNED
Whiteboard: [highlighter][minotaur] → [highlighter][minotaur][best: 3h; likely: 1d; worst: 3d]
Assignee | ||
Updated•12 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 4•12 years ago
|
||
WIP patch. Behavior is working well, but need to patch up the existing unittests and create a new one to verify this is working as intended.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #550183 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 years ago
|
||
ok, this is ready for feedback. Still need to write a test to enable the HTML panel and verify it's working. Might need another notification to let tests know when the HTML panel is "up".
Attachment #551074 -
Attachment is obsolete: true
Attachment #551777 -
Flags: feedback?(mihai.sucan)
Comment 7•12 years ago
|
||
Comment on attachment 551777 [details] [diff] [review] disable html panel v3 Review of attachment 551777 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks fine. Please address the comments below. f+ with that! ::: browser/base/content/inspector.js @@ +780,5 @@ > + if (!this.treePanel) { > + this.treePanel = document.getElementById("inspector-tree-panel"); > + this.treePanel.hidden = false; > + } > + this.registerTool({ General comment here: I would like to see the tree panel more decoupled than this. inspector.js will become a JSM, so I would like the TreePanel object to be independent. The openTreePanel(), closeTreePanel() and all tree panel methods and properties should move there, in the new object. The tree panel code in this patch is still intertwined with the main inspector UI code. (albeit, at a lesser extent) @@ +900,5 @@ > this.selection = null; > this.treeLoaded = false; > + this.closing = false; > + delete this.treePanel; > + // delete this.toolbar; Why is this line commented? Also, please note that the Highlighter.destroy() method has a delete this.toolbar line that was added in the patch of bug 666650. That should've been in here, IIANM. (There is no toolbar property in the Highlighter.) See line 225 in inspector.js. ::: browser/base/content/test/inspector/browser_inspector_initialization.js @@ +52,5 @@ > Services.obs.addObserver(finishInspectorTests, > INSPECTOR_NOTIFICATIONS.CLOSED, false); > > + // let iframe = document.getElementById("inspector-tree-iframe"); > + // is(InspectorUI.treeIFrame, iframe, "Inspector IFrame matches"); Please remove these lines if they are no longer need, to avoid confusion in the future. ::: browser/base/content/test/inspector/browser_inspector_treePanel_output.js @@ +73,5 @@ > Services.obs.removeObserver(inspectorOpened, > INSPECTOR_NOTIFICATIONS.OPENED, false); > > ok(InspectorUI.inspecting, "Inspector is highlighting"); > +// ok(InspectorUI.isTreePanelOpen, "Inspector Tree Panel is open"); This should be fixed. Commenting out is not a fix. :) @@ +93,5 @@ > > +// ok(InspectorUI.treePanelDiv, "InspectorUI.treePanelDiv is available"); > +// is(InspectorUI.treePanelDiv.innerHTML.replace(/^\s+|\s+$/mg, ''), > +// expectedResult, "treePanelDiv.innerHTML is correct"); > +// expectedResult = null; Same here please.
Attachment #551777 -
Flags: feedback?(mihai.sucan) → feedback+
Comment 8•12 years ago
|
||
Also, please note that I get test failures: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_initialization.js | Inspector Tree Panel is not open TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/inspector/browser_inspector_tab_switch.js | Inspector Tree Panel is not open
Comment 9•12 years ago
|
||
Comment on attachment 551777 [details] [diff] [review] disable html panel v3 Review of attachment 551777 [details] [diff] [review]: ----------------------------------------------------------------- Forgot something yesterday. See below. ::: browser/base/content/inspector.js @@ +784,5 @@ > + this.registerTool({ > + id: "treepanel", > + label: "HTML", > + tooltiptext: "HTML panel", > + accesskey: "H", These strings need to be in inspector.properties.
Assignee | ||
Comment 10•12 years ago
|
||
WIP patch migrating tree panel code to a module. Also moved insideOutBox.js to InsideOutBox.jsm. Patch is currently working, but I still have to address the commented out tests mentioned in the review comments, make sure TreePanel cleans up properly after itself and investigate an annoying error message coming from the registerTools method. ... And verify that the unittests are working properly again.
Attachment #551777 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Updated patch, tests passing and augmented to check for activation of html panel. inspector.properties readded.
Attachment #552227 -
Attachment is obsolete: true
Attachment #552873 -
Flags: review?(mihai.sucan)
Comment 12•12 years ago
|
||
Comment on attachment 552873 [details] [diff] [review] disable and modularize html panel v5 Review of attachment 552873 [details] [diff] [review]: ----------------------------------------------------------------- Great improvements! All tests pass locally. Thanks for the updated patch. Giving the patch r- only because it still has some problems, hehe. Please see the comments below. Once addressed, we can go forward. Thank you! ::: browser/base/content/insideOutBox.js @@ +134,5 @@ > this.box = aBox; > > this.rootObject = null; > > this.rootObjectBox = null; As a measure of safety, to avoid memory leaks we should have an InsideOutBox.destroy() method. This object holds to a number of DOM objects that we should delete. @@ +453,5 @@ > try { > this.box.removeChild(this.rootObjectBox); > } catch (exc) { > + //InspectorUI._log("this.box.removeChild(this.rootObjectBox) FAILS " + > + // this.box + " must not contain " + this.rootObjectBox); Cu.reportError() or remove this? ::: browser/base/content/TreePanel.jsm @@ +65,5 @@ > + > + /** > + * Initialization function for the TreePanel. > + */ > + initialize: function TP_initialize() Maybe rename this method to initializeIframe(). @@ +109,5 @@ > + this.treeIFrame = this.document.createElement("iframe"); > + this.treeIFrame.setAttribute("id", "inspector-tree-iframe"); > + this.treeIFrame.setAttribute("flex", "1"); > + this.treeIFrame.setAttribute("type", "content"); > + this.treeIFrame.setAttribute("onclick", "InspectorUI.TreePanel.onTreeClick(event)"); Why doesn't this use addEventListener()? @@ +232,5 @@ > + child = this.getNextSibling(previousSibling); > + else > + child = this.getFirstChild(node); > + > + if (this.showTextNodesWithWhitespace) this.IUI.showTextNodesWithWhitespace @@ +285,5 @@ > + if (hitTwisty) { > + this.ioBox.toggleObject(node); > + } else { > + if (this.inspecting) { > + this.stopInspecting(true); stopInspecting() doesn't exist here. I think this needs to be changed to this.IUI.stopInspecting(). The same for this.inspecting. @@ +287,5 @@ > + } else { > + if (this.inspecting) { > + this.stopInspecting(true); > + } else { > + this.select(node, true, false); this.select() exists, but in the previous code iteration it meant that IUI.select() is called (not only the tree panel select code). Please change this line to call this.IUI.select(). @@ +311,5 @@ > + * @param aClass > + * The class string. > + * @returns boolean > + */ > + hasClass: function IUI_hasClass(aNode, aClass) Please update the function name (IUI_hasClass to TP_hasClass). The same goes for the rest of the functions that still have the IUI_ prefix. @@ +382,5 @@ > + > + if (this.treeIFrame) > + delete this.treeIFrame; > + > + delete this.ioBox; You still keep some DOM refs: treePanelDiv, treeBrowserDocument and treeWalker. The first two are removed in the close() method, but there's no guarantee that close() was called. Either call it here, or just remove the DOM refs. I would also do this.treeLoaded = false. ::: browser/base/content/inspector.js @@ +571,5 @@ > * @returns boolean > */ > get isTreePanelOpen() > { > + return this.TreePanel && this.TreePanel.container.state == "open"; Why not have return this.TreePanel.isOpen() ? I would suggest adding an isOpen() method (or getter) to the TreePanel.jsm code. @@ +609,5 @@ > + if (!this.TreePanel) { > + Cu.import("resource:///modules/TreePanel.jsm", this); > + this.TreePanel.document = document; > + this.TreePanel.window = window; > + this.TreePanel.IUI = this; Why not this.TreePanel.init(this) ? IUI has sufficient objects that you can derive both the chrome document and the chrome window objects. Also, document can be derived from window (and vice-versa). No need to pass both. (note that there's a TreePanel.initialize() method that actually deals with the first initialization of the iframe. that's a different method, and we should have a clearer naming distinction, to avoid confusion.) @@ +623,5 @@ > + context: this, > + onShow: this.openTreePanel, > + onHide: this.closeTreePanel, > + onSelect: this.treePanelSelect, > + panel: this.TreePanel.container I would expect that context: this.TreePanel, onShow: this.TreePanel.onShow, onHide: this.TreePanel.onHide, onSelect: this.TreePanel.onSelect, In the case of "context" I understand you already pass IUI to the TreePanel. If the code there needs to access IUI, it can do so. And actually, why shouldn't the treepanel register itself? Move this call into the new proposed init(). @@ +668,5 @@ > }, > > + openTreePanel: function IUI_openTreePanel() > + { > + this.TreePanel.open(this); Why is this needed to be passed to open()? The initialization part already did this. @@ +730,5 @@ > this.winID = null; > this.selection = null; > this.treeLoaded = false; > + this.closing = false; > + delete this.treePanel; TreePanel.destroy() is never called. I believe we'll have memleaks. The JSM continues to hold to DOM nodes. @@ +798,4 @@ > } > this.toolsDo(function IUI_toolsOnSelect(aTool) { > if (aTool.panel.state == "open") { > + aTool.onSelect.apply(aTool.context, [aNode, aScroll]); .apply() is not needed here. You can use aTool.onSelect.call(aTool.context, aNode, aScroll). @@ +802,5 @@ > } > }); > }, > > + treePanelSelect: function IUI_treePanelSelect(aNode, aScroll) I would expect this method to be only in the TreePanel code, not in IUI. @@ +1072,1 @@ > aRegObj.onSelect.apply(aRegObj.context, [InspectorUI.selection]); Same as above. .apply() is not needed here. You can use .call(). ::: browser/base/content/test/inspector/Makefile.in @@ +57,5 @@ > browser_inspector_registertools.js \ > browser_inspector_bug_665880.js \ > $(NULL) > > +# browser_inspector_treePanel_click.js \ Why is this test disabled? ::: browser/base/content/test/inspector/browser_inspector_tab_switch.js @@ +166,5 @@ > + > + // Switch back to tab 1. > + Services.obs.addObserver(inspectorSecondFocusTab1, > + INSPECTOR_NOTIFICATIONS.TREEPANELREADY, false); > + gBrowser.selectedTab = tab1; Why do you wait for TREEPANELREADY here? It fires after IUI OPENED? Once we have more panels, we'll want to have a mechanism that tells us "all panels are ready". (worth a follow up bug report?)
Attachment #552873 -
Flags: review?(mihai.sucan) → review-
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #12) > Comment on attachment 552873 [details] [diff] [review] > disable and modularize html panel v5 > > Review of attachment 552873 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great improvements! All tests pass locally. Thanks for the updated patch. > > Giving the patch r- only because it still has some problems, hehe. Please > see the comments below. Once addressed, we can go forward. Thank you! Well, if there were no problems, it would've been an r+! :) > ::: browser/base/content/insideOutBox.js > @@ +134,5 @@ > > this.box = aBox; > > > > this.rootObject = null; > > > > this.rootObjectBox = null; > > As a measure of safety, to avoid memory leaks we should have an > InsideOutBox.destroy() method. This object holds to a number of DOM objects > that we should delete. Ah, good call. Added. > @@ +453,5 @@ > > try { > > this.box.removeChild(this.rootObjectBox); > > } catch (exc) { > > + //InspectorUI._log("this.box.removeChild(this.rootObjectBox) FAILS " + > > + // this.box + " must not contain " + this.rootObjectBox); > > Cu.reportError() or remove this? Switched that to this.view._log which is equivalent to calling InspectorUI._log. > ::: browser/base/content/TreePanel.jsm > @@ +65,5 @@ > > + > > + /** > > + * Initialization function for the TreePanel. > > + */ > > + initialize: function TP_initialize() > > Maybe rename this method to initializeIframe(). I'll disagree here. It's not like we have more than one initialization function. Makes the method signature longer. > @@ +109,5 @@ > > + this.treeIFrame = this.document.createElement("iframe"); > > + this.treeIFrame.setAttribute("id", "inspector-tree-iframe"); > > + this.treeIFrame.setAttribute("flex", "1"); > > + this.treeIFrame.setAttribute("type", "content"); > > + this.treeIFrame.setAttribute("onclick", "InspectorUI.TreePanel.onTreeClick(event)"); > > Why doesn't this use addEventListener()? Historical reasons, I think. When we first landed this, I wasn't getting a listener to fire on the iframe itself. Probably have to add it to the document. I will do so. > @@ +232,5 @@ > > + child = this.getNextSibling(previousSibling); > > + else > > + child = this.getFirstChild(node); > > + > > + if (this.showTextNodesWithWhitespace) > > this.IUI.showTextNodesWithWhitespace oops. Wonder if that needs to be in TreePanel.jsm as well? Probably since it's the only thing that refers to it. > @@ +285,5 @@ > > + if (hitTwisty) { > > + this.ioBox.toggleObject(node); > > + } else { > > + if (this.inspecting) { > > + this.stopInspecting(true); > > stopInspecting() doesn't exist here. I think this needs to be changed to > this.IUI.stopInspecting(). ah yes. > The same for this.inspecting. Same for this.inspecting. > @@ +287,5 @@ > > + } else { > > + if (this.inspecting) { > > + this.stopInspecting(true); > > + } else { > > + this.select(node, true, false); > > this.select() exists, but in the previous code iteration it meant that > IUI.select() is called (not only the tree panel select code). > > Please change this line to call this.IUI.select(). OK! > @@ +311,5 @@ > > + * @param aClass > > + * The class string. > > + * @returns boolean > > + */ > > + hasClass: function IUI_hasClass(aNode, aClass) > > Please update the function name (IUI_hasClass to TP_hasClass). > > The same goes for the rest of the functions that still have the IUI_ prefix. > > @@ +382,5 @@ > > + > > + if (this.treeIFrame) > > + delete this.treeIFrame; > > + > > + delete this.ioBox; > > You still keep some DOM refs: treePanelDiv, treeBrowserDocument and > treeWalker. The first two are removed in the close() method, but there's no > guarantee that close() was called. Either call it here, or just remove the > DOM refs. done! > I would also do this.treeLoaded = false. also done. > ::: browser/base/content/inspector.js > @@ +571,5 @@ > > * @returns boolean > > */ > > get isTreePanelOpen() > > { > > + return this.TreePanel && this.TreePanel.container.state == "open"; > > Why not have return this.TreePanel.isOpen() ? I guess that's a thing we could do. I will do that! > I would suggest adding an isOpen() method (or getter) to the TreePanel.jsm > code. done. > @@ +609,5 @@ > > + if (!this.TreePanel) { > > + Cu.import("resource:///modules/TreePanel.jsm", this); > > + this.TreePanel.document = document; > > + this.TreePanel.window = window; > > + this.TreePanel.IUI = this; > > Why not this.TreePanel.init(this) ? true true. > IUI has sufficient objects that you can derive both the chrome document and > the chrome window objects. Also, document can be derived from window (and > vice-versa). No need to pass both. true. > (note that there's a TreePanel.initialize() method that actually deals with > the first initialization of the iframe. that's a different method, and we > should have a clearer naming distinction, to avoid confusion.) well, ok. I guess you sold me on the initializeIFrame method renaming. Though I still hate having "IFrame" in method names. It capitalizes poorly. > @@ +623,5 @@ > > + context: this, > > + onShow: this.openTreePanel, > > + onHide: this.closeTreePanel, > > + onSelect: this.treePanelSelect, > > + panel: this.TreePanel.container > > I would expect that context: this.TreePanel, > onShow: this.TreePanel.onShow, > onHide: this.TreePanel.onHide, > onSelect: this.TreePanel.onSelect, I think the API is somewhat misleading. Those aren't "onShow" and "onHide" events. They're "show" and "hide" events. How does the tool show and hide itself? > In the case of "context" I understand you already pass IUI to the TreePanel. > If the code there needs to access IUI, it can do so. > > And actually, why shouldn't the treepanel register itself? Move this call > into the new proposed init(). done. > > @@ +668,5 @@ > > }, > > > > + openTreePanel: function IUI_openTreePanel() > > + { > > + this.TreePanel.open(this); > > Why is this needed to be passed to open()? The initialization part already > did this. That was a leftover. Removed... the WHOLE METHOD. > @@ +730,5 @@ > > this.winID = null; > > this.selection = null; > > this.treeLoaded = false; > > + this.closing = false; > > + delete this.treePanel; > > TreePanel.destroy() is never called. I believe we'll have memleaks. The JSM > continues to hold to DOM nodes. Eek! Lemme fix that. > @@ +798,4 @@ > > } > > this.toolsDo(function IUI_toolsOnSelect(aTool) { > > if (aTool.panel.state == "open") { > > + aTool.onSelect.apply(aTool.context, [aNode, aScroll]); > > .apply() is not needed here. You can use aTool.onSelect.call(aTool.context, > aNode, aScroll). um, same thing, only different? > @@ +802,5 @@ > > } > > }); > > }, > > > > + treePanelSelect: function IUI_treePanelSelect(aNode, aScroll) > > I would expect this method to be only in the TreePanel code, not in IUI. OK. > @@ +1072,1 @@ > > aRegObj.onSelect.apply(aRegObj.context, [InspectorUI.selection]); > > Same as above. .apply() is not needed here. You can use .call(). > > ::: browser/base/content/test/inspector/Makefile.in > @@ +57,5 @@ > > browser_inspector_registertools.js \ > > browser_inspector_bug_665880.js \ > > $(NULL) > > > > +# browser_inspector_treePanel_click.js \ > > Why is this test disabled? It was disabled previously when paul landed another test, I think. afaict, it was never enabled and never worked. Needs work. > ::: browser/base/content/test/inspector/browser_inspector_tab_switch.js > @@ +166,5 @@ > > + > > + // Switch back to tab 1. > > + Services.obs.addObserver(inspectorSecondFocusTab1, > > + INSPECTOR_NOTIFICATIONS.TREEPANELREADY, false); > > + gBrowser.selectedTab = tab1; > > Why do you wait for TREEPANELREADY here? It fires after IUI OPENED? Yes. > Once we have more panels, we'll want to have a mechanism that tells us "all > panels are ready". (worth a follow up bug report?) not sure how we'll do that. The panels could be opening in any given order and will certainly be ready in a non-deterministic order. Maybe we could register a listener in IUI that waits for all of the panel ready notifications to arrive and then fires a "ALLREADY!" notice to its observers. But yes, definitely worth a followup. Bug 679711.
Assignee | ||
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Comment 14•12 years ago
|
||
the registerTools api provided no way to deregister. Worse, event listeners weren't getting cleaned up from the various XUL appendages (toolbar buttons, panels). Fixed that. Created a real instance of TreePanel to address multiple window problems. need to update the tests.
Attachment #552873 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
passing all but iframeTests and tab_switch. I'll get these tomorrow.
Assignee | ||
Updated•12 years ago
|
Attachment #555192 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 16•12 years ago
|
||
tests passing.
Attachment #555211 -
Attachment is obsolete: true
Attachment #555374 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #555192 -
Attachment is obsolete: true
Attachment #555374 -
Attachment is obsolete: true
Attachment #555192 -
Flags: review?(mihai.sucan)
Attachment #555374 -
Flags: review?(mihai.sucan)
Attachment #555383 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 18•12 years ago
|
||
patch moves the tree panel and friends into browser/devtools/highlighter
Attachment #555393 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 19•12 years ago
|
||
Comment on attachment 555393 [details] [diff] [review] move treepanel code to devtools moved this patch to bug 681651.
Attachment #555393 -
Flags: review?(mihai.sucan)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 20•12 years ago
|
||
updated based on moved registerTools API bug 681653.
Attachment #555383 -
Attachment is obsolete: true
Attachment #555393 -
Attachment is obsolete: true
Attachment #555383 -
Flags: review?(mihai.sucan)
Attachment #555419 -
Flags: review?(mihai.sucan)
Comment 21•12 years ago
|
||
Comment on attachment 555419 [details] [diff] [review] disable and modularize html panel v12 Review of attachment 555419 [details] [diff] [review]: ----------------------------------------------------------------- All tests pass, really good progress! Giving the patch r+, but please address the comments below. This patch has a lot of trailing white space, please fix this. (I'll take a quick pass through the updated patch, when you submit it.) ::: browser/base/content/TreePanel.jsm @@ +54,5 @@ > + * @param aContext nsIDOMWindow (xulwindow) > + * @param aIUI global InspectorUI object > + */ > + > +function TreePanel(aContext, aIUI) { Nit: there's a new line before the function and the comment that describes it. Please remove it. (same comment applies for TreePanel.destroy()) @@ +74,5 @@ > + * originating context and the InspectorUI global. > + * @param aContext nsIDOMWindow (xulwindow) > + * @param aIUI global InspectorUI object > + */ > + _init: function TP_init(aContext, aIUI) Nit: function TP__init(aContext, aIUI) @@ +76,5 @@ > + * @param aIUI global InspectorUI object > + */ > + _init: function TP_init(aContext, aIUI) > + { > + this.showTextNodesWithWhitespace = false; You could put this and the id properties on the prototype directly, no need to set them in _init(). @@ +77,5 @@ > + */ > + _init: function TP_init(aContext, aIUI) > + { > + this.showTextNodesWithWhitespace = false; > + this.id = "treepanel"; // DO NOT LOCALIZE Why is the "DO NOT LOCALIZE" comment needed here? Do localizers check the source code? @@ +81,5 @@ > + this.id = "treepanel"; // DO NOT LOCALIZE > + this.IUI = aIUI; > + this.window = aContext; > + this.document = this.window.document; > + this.editingContext = null; Is this needed here? @@ +85,5 @@ > + this.editingContext = null; > + > + if (!this.domplate) { > + Cu.import("resource:///modules/domplate.jsm", this); > + this.domplateUtils.setDOM(this.window); We shouldn't do Cu.import() for every init() call. We should have the Cu.import() at the top of the file, only once. We are now in a JSM, more freedom. (maybe not worth doing this right now, maybe a follow up?) @@ +126,5 @@ > + delete this.initializingTreePanel; > + Services.obs.notifyObservers(null, > + this.window.INSPECTOR_NOTIFICATIONS.TREEPANELREADY, null); > + if (this.IUI.selection) > + this.select(this.IUI.selection, true); Please keep the coding style consistent through the whole file: curly braces for one liners or not. This method (and the whole file) mixes both styles. (I, for one, prefer enforcing braces for one liners, but it really doesn't matter whatever we chose.) @@ +132,5 @@ > + > + /** > + * Open the inspector's tree panel and initialize it. > + */ > + open: function TP_openTreePanel() open: function TP_open() ::: browser/base/content/inspector.js @@ +537,5 @@ > var InspectorUI = { > browser: null, > tools: {}, > + toolButtonEvents: {}, > + toolPanelEvents: {}, These should be in bug 681653. @@ +618,1 @@ > } Similar to the comment I had for the domplate Cu.import() from the TreePanel.jsm: I wish this would be at the top of the inspector.jsm (when we switch to jsm). Also, this.treePanel versus this.TreePanel is quite confusing. We should aim here for something better. @@ +670,5 @@ > { > // if currently editing an attribute value, closing the > // highlighter/HTML panel dismisses the editor > + if (this.treePanel.editingContext) > + this.treePanel.closeEditor(); I see this check in multiple places. This check fails (it throws) when you have the tree panel disabled from the prefs (when treePanelEnabled is false), because this.treePanel is null (it doesn't initialize, see openInspectorUI()). Please update this to fix the failure, in all places where it exists. @@ +721,5 @@ > > + delete this.treePanel; > + delete this.toolbar; > + delete this.TreePanel; > + Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.CLOSED, null); We need a way to announce registered tools, at once, to destroy() themselves. Here you call destroy() manually for the treePanel. The common way to register should be to just add a Cu.import() at the top of this inspector.jsm (when it becomes a jsm). To unregister, maybe we could add a notification, or a callback to registerTool objects, like destroy() which we invoke here when destroy() happens. This would avoid the need to do aTool.panel.hidePopup() above. (maybe file a follow up bug?) @@ +1158,5 @@ > + this.toolPanelEvents[aPanel.id + "popupshowing"], false); > + this.toolPanelEvents[aPanel.id + "popupshowing"] = null; > + delete this.toolPanelEvents[aPanel.id + "popupshowing"] > + } > + }, The (un)bindTool*Events() and getToolbarButtonId() methods come from bug 681653 as well. These shouldn't be here, right? ::: browser/base/content/test/inspector/browser_inspector_initialization.js @@ +57,5 @@ > + ok(InspectorUI.inspecting, "Inspector is inspecting"); > + ok(!InspectorUI.isTreePanelOpen, "Inspector Tree Panel is not open"); > + ok(InspectorUI.highlighter, "Highlighter is up"); > + > + InspectorUI.treePanel.open(); What happens here if the tree panel is disabled by the pref? (and in the other test files)
Attachment #555419 -
Flags: review?(mihai.sucan) → review+
Assignee | ||
Comment 22•12 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #21) > Comment on attachment 555419 [details] [diff] [review] > disable and modularize html panel v12 > > Review of attachment 555419 [details] [diff] [review]: > ----------------------------------------------------------------- > > All tests pass, really good progress! Giving the patch r+, but please > address the comments below. > > This patch has a lot of trailing white space, please fix this. it does? that's ... odd. /me scans. Oh. Wow. How'd I miss that? *fix* Think I got them all. > (I'll take a quick pass through the updated patch, when you submit it.) Thanks. I can usually catch those when eyeballing my patch through qdiff as it highlights them. I think I forgot to this time. They weren't mine. :| > ::: browser/base/content/TreePanel.jsm > @@ +54,5 @@ > > + * @param aContext nsIDOMWindow (xulwindow) > > + * @param aIUI global InspectorUI object > > + */ > > + > > +function TreePanel(aContext, aIUI) { > > Nit: there's a new line before the function and the comment that describes > it. Please remove it. > > (same comment applies for TreePanel.destroy()) AUGH! :) > > @@ +74,5 @@ > > + * originating context and the InspectorUI global. > > + * @param aContext nsIDOMWindow (xulwindow) > > + * @param aIUI global InspectorUI object > > + */ > > + _init: function TP_init(aContext, aIUI) > > Nit: function TP__init(aContext, aIUI) > > @@ +76,5 @@ > > + * @param aIUI global InspectorUI object > > + */ > > + _init: function TP_init(aContext, aIUI) > > + { > > + this.showTextNodesWithWhitespace = false; > > You could put this and the id properties on the prototype directly, no need > to set them in _init(). ah yes. > > @@ +77,5 @@ > > + */ > > + _init: function TP_init(aContext, aIUI) > > + { > > + this.showTextNodesWithWhitespace = false; > > + this.id = "treepanel"; // DO NOT LOCALIZE > > Why is the "DO NOT LOCALIZE" comment needed here? Do localizers check the > source code? Mostly in case there's an l10n review on this and I'm told to stick it in a stringBundle. > @@ +81,5 @@ > > + this.id = "treepanel"; // DO NOT LOCALIZE > > + this.IUI = aIUI; > > + this.window = aContext; > > + this.document = this.window.document; > > + this.editingContext = null; > > Is this needed here? probably not. I figure it was added during the editor patch to "declare" it ahead of use and to match a potential clean up in the destroy function. I'll remove it. > @@ +85,5 @@ > > + this.editingContext = null; > > + > > + if (!this.domplate) { > > + Cu.import("resource:///modules/domplate.jsm", this); > > + this.domplateUtils.setDOM(this.window); > > We shouldn't do Cu.import() for every init() call. We should have the > Cu.import() at the top of the file, only once. We are now in a JSM, more > freedom. good point. > (maybe not worth doing this right now, maybe a follow up?) Nah, might as well move it now. > @@ +126,5 @@ > > + delete this.initializingTreePanel; > > + Services.obs.notifyObservers(null, > > + this.window.INSPECTOR_NOTIFICATIONS.TREEPANELREADY, null); > > + if (this.IUI.selection) > > + this.select(this.IUI.selection, true); > > Please keep the coding style consistent through the whole file: curly braces > for one liners or not. This method (and the whole file) mixes both styles. my file. I get to call it! (this is a silly nit). Seriously though, most of this code was copied over straight from inspector.js. It was good enough for there, it's good enough for this. > (I, for one, prefer enforcing braces for one liners, but it really doesn't > matter whatever we chose.) I'm a little less strict on requiring unnecessary brackets. I don't. > @@ +132,5 @@ > > + > > + /** > > + * Open the inspector's tree panel and initialize it. > > + */ > > + open: function TP_openTreePanel() > > open: function TP_open() good. > ::: browser/base/content/inspector.js > @@ +537,5 @@ > > var InspectorUI = { > > browser: null, > > tools: {}, > > + toolButtonEvents: {}, > > + toolPanelEvents: {}, > > These should be in bug 681653. ah yes. Moved! > @@ +618,1 @@ > > } > > Similar to the comment I had for the domplate Cu.import() from the > TreePanel.jsm: I wish this would be at the top of the inspector.jsm (when we > switch to jsm). > > Also, this.treePanel versus this.TreePanel is quite confusing. We should aim > here for something better. TreePanel's the class, treePanel the instance. It's pretty common in object oriented programming. > @@ +670,5 @@ > > { > > // if currently editing an attribute value, closing the > > // highlighter/HTML panel dismisses the editor > > + if (this.treePanel.editingContext) > > + this.treePanel.closeEditor(); > > I see this check in multiple places. This check fails (it throws) when you > have the tree panel disabled from the prefs (when treePanelEnabled is > false), because this.treePanel is null (it doesn't initialize, see > openInspectorUI()). ehh. > Please update this to fix the failure, in all places where it exists. ok. > @@ +721,5 @@ > > > > + delete this.treePanel; > > + delete this.toolbar; > > + delete this.TreePanel; > > + Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.CLOSED, null); > > We need a way to announce registered tools, at once, to destroy() > themselves. Here you call destroy() manually for the treePanel. > > The common way to register should be to just add a Cu.import() at the top of > this inspector.jsm (when it becomes a jsm). To unregister, maybe we could > add a notification, or a callback to registerTool objects, like destroy() > which we invoke here when destroy() happens. This would avoid the need to do > aTool.panel.hidePopup() above. > > (maybe file a follow up bug?) Sure. Bug 681975. > @@ +1158,5 @@ > > + this.toolPanelEvents[aPanel.id + "popupshowing"], false); > > + this.toolPanelEvents[aPanel.id + "popupshowing"] = null; > > + delete this.toolPanelEvents[aPanel.id + "popupshowing"] > > + } > > + }, > > The (un)bindTool*Events() and getToolbarButtonId() methods come from bug > 681653 as well. These shouldn't be here, right? Right. Oops. > ::: browser/base/content/test/inspector/browser_inspector_initialization.js > @@ +57,5 @@ > > + ok(InspectorUI.inspecting, "Inspector is inspecting"); > > + ok(!InspectorUI.isTreePanelOpen, "Inspector Tree Panel is not open"); > > + ok(InspectorUI.highlighter, "Highlighter is up"); > > + > > + InspectorUI.treePanel.open(); > > What happens here if the tree panel is disabled by the pref? (and in the > other test files) *boom* Is it worth changing the tests for now? I think if we ever disable it by default, we should update the tests then. It's hard-coded to true.
Assignee | ||
Comment 23•12 years ago
|
||
updated post review.
Attachment #555419 -
Attachment is obsolete: true
Attachment #555749 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 24•12 years ago
|
||
rebased
Attachment #555749 -
Attachment is obsolete: true
Attachment #555749 -
Flags: review?(gavin.sharp)
Attachment #556054 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 25•12 years ago
|
||
missed a reject in the inspector.properties file.
Attachment #556054 -
Attachment is obsolete: true
Attachment #556054 -
Flags: review?(gavin.sharp)
Attachment #556064 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 26•12 years ago
|
||
changes pending based on blocking bug 681653. Patch forth-coming...
Assignee | ||
Comment 27•12 years ago
|
||
Comment on attachment 556064 [details] [diff] [review] disable and modularize html panel v15 (rebased 08/26) canceling review pending updated patch.
Attachment #556064 -
Flags: review?(gavin.sharp)
Comment 28•12 years ago
|
||
Since today is a vacation for US (and Canada?) I took the liberty to rebase this patch so I can work on bug 562168. Attached the work I did: - rebased on top of the latest fxteam. - found that the TreePanel.jsm didn't get the new code from the previous rebase. in saveEditor() we need this.IUI.isDirty = true, to prevent users from loosing page changes. - the InspectorProgressListener was not updated to check isInspectorOpen (it still had the check for isTreePanelOpen, which broke the code). - the page navigation test file browser_inspector_bug_566084_location_changed.js failed. Fixed it. - the InspectorUI.isDirty flag wasn't properly remembered across tab switches, nor was it cleared on page navigation. Fixed. - there was some code confusion over InspectorUI.inspectCmd. The menuitem checkbox needs to be enabled/disabled when the inspector opens/closes. The Inspect toolbutton needs to be checked/unchecked when the inspecting (highlighting) mode is active/inactive. These were not working properly and this patch did touch related code. Fixed. - removed the getToolbarButtonId(), bindToolButtonEvent(), unbindToolButtonEvent(), bindToolPanelEvents() and unbindToolPanelEvents() methods from IUI, as agreed in comment 22. I hope my patch update makes it easier for you to update it again when bug 681653 changes.
Assignee | ||
Comment 29•12 years ago
|
||
new version based on updated register style and augmented registerTools API.
Assignee | ||
Updated•12 years ago
|
Attachment #558604 -
Attachment description: disable and modularize html panel v16 (rebased 08/26) → disable and modularize html panel v17 (rebased 09/06)
Assignee | ||
Comment 30•12 years ago
|
||
updated based on new registerTools API. All tests are passing.
Attachment #556064 -
Attachment is obsolete: true
Attachment #558309 -
Attachment is obsolete: true
Attachment #558604 -
Attachment is obsolete: true
Attachment #558944 -
Flags: review?(gavin.sharp)
Updated•12 years ago
|
Attachment #558944 -
Attachment is patch: true
Comment 31•12 years ago
|
||
This patch actually depends on the patch from bug 681653, which in turn depends on bug 681653. (updating to make it easier for others to follow the deps tree)
Assignee | ||
Comment 32•12 years ago
|
||
Folded contents of bug 681651 into this patch as requested.
Attachment #558944 -
Attachment is obsolete: true
Attachment #558944 -
Flags: review?(gavin.sharp)
Attachment #561175 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 33•12 years ago
|
||
also, there's a scratchpad makefile change included in this patch. I can leave it out, but included it as this set because I was "in there".
Assignee | ||
Comment 34•12 years ago
|
||
cleaned and polished.
Attachment #561175 -
Attachment is obsolete: true
Attachment #561175 -
Flags: review?(gavin.sharp)
Attachment #561451 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #561463 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 36•12 years ago
|
||
rebased onto context menu patch.
Attachment #561463 -
Attachment is obsolete: true
Attachment #561463 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•12 years ago
|
Attachment #561710 -
Attachment is patch: true
Assignee | ||
Updated•12 years ago
|
Attachment #561710 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 37•12 years ago
|
||
fixes for re-entrant inspect context menu.
Attachment #561710 -
Attachment is obsolete: true
Attachment #561710 -
Flags: review?(gavin.sharp)
Updated•12 years ago
|
Attachment #561451 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•12 years ago
|
Attachment #561451 -
Attachment is obsolete: true
Assignee | ||
Comment 38•12 years ago
|
||
Comment on attachment 561818 [details] [diff] [review] [in-fx-team] disable and modularize html panel v20 (requires patch from bugs 687854, 668558) https://hg.mozilla.org/integration/fx-team/rev/121518f3df44
Attachment #561818 -
Attachment description: disable and modularize html panel v20 (requires patch from bugs 687854, 668558) → [in-fx-team] disable and modularize html panel v20 (requires patch from bugs 687854, 668558)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [highlighter][minotaur][best: 3h; likely: 1d; worst: 3d] → [highlighter][minotaur][best: 3h; likely: 1d; worst: 3d][fixed-in-fx-team]
Assignee | ||
Comment 39•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/121518f3df44
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 40•12 years ago
|
||
Backed out because of various new mochitest-browser-chrome leaks. Unfortunately, this landed with a bunch of other interweaved patches. I only felt comfortable ruling out f297a626dd13 and 7d5311c92e04.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 41•12 years ago
|
||
relanded: https://hg.mozilla.org/integration/fx-team/rev/c3cc5ae0e2a9
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 9 → ---
Comment 42•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c3cc5ae0e2a9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [highlighter][minotaur][best: 3h; likely: 1d; worst: 3d][fixed-in-fx-team] → [highlighter][minotaur][best: 3h; likely: 1d; worst: 3d]
Target Milestone: --- → Firefox 9
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•