Closed
Bug 707809
Opened 13 years ago
Closed 12 years ago
Refactor creation of registered sidebar tools iframes in InspectorUI
Categories
(DevTools :: Inspector, defect, P1)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 14
People
(Reporter: rcampbell, Assigned: dcamp)
References
Details
(Whiteboard: [firefox14-wanted])
Attachments
(1 file, 11 obsolete files)
155.52 KB,
patch
|
Details | Diff | Splinter Review |
Currently, addons or built-in sidebar tools for the Page Inspector have to manage the creation of their iframes within browser UI. This is not nice for a number of reasons and we should do that better. We should change the properties of the Registration Object to include a href for the iframe's source attribute that the tool will live in. We should also make a proper registerSidebarTools method to take care of the widget creation.
Assignee | ||
Comment 1•12 years ago
|
||
This takes the previous WIP and makes a few more changes: * isOpen isn't implemented by individual sidebar tools * Added a static InspectorUI.registerSidebar() that takes the static pieces of the tool registration (id/label/src/etc) and a factory for creating an actual Tool object after the iframe has been instantiated. Seems like that'll be useful for addons to register tools without having to listen for creating of new windows etc. * Took out 'dim', the highlight method for tools now takes extra information including whether the highlighter is locked; it can decide whether to update from that. * Moved rule view registration into StyleInspector.jsm, which now exists to register both the style inspector panels. * As a side-effect, included saving currently-open panels in a pref (which was the main motivation for starting this refactoring, wanted to make sure it served that need). I don't know if this is still relevant with the new plans. If this still makes any sense I'll finish cleaning it up and fix tests.
Assignee | ||
Comment 2•12 years ago
|
||
Also, the tools are now responsible for being able to respond to highlight/show/hide after their factory is called (something that falls sort of naturally out of separating the tool instantiation from registration). So bailing on isOpen has been removed from individual sidebar tool implementations (we know they're open, because those objects only exist after they're open). The highlighter is still responsible for only sending the highlight message to the current sidebar panel, but it's also the one that supplies the isOpen implementation for those panels, so that's not getting mixed up with initialization logic anymore.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #588601 -
Attachment is obsolete: true
Reporter | ||
Comment 4•12 years ago
|
||
would this be simpler if we separated the HTML view out of the registerTools API and used that solely for registering sidebar tools? There's at least one good reason for this: The HTML panel is integral to the Page Inspector and is kind of unique. The sidebar is intended to be the container for any number of "nodecentric" tools. Have a think and let that inform you. I don't want to derail your progress with a new direction here, but we might at some point decide to separate these.
Status: NEW → ASSIGNED
Comment 5•12 years ago
|
||
In an ideal world: Can we do that in a way that we don't have to edit inspector.jsm when we want to add a new tool? inspector.jsm should not have any code related to any specific tool. The registration mechanism should be totally generic. Does it make sense? Can this be part of this bug?
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #5) > In an ideal world: > > Can we do that in a way that we don't have to edit inspector.jsm when we > want to add a new tool? Yes - the way my patch is arranged, you can call registerTool at any point before the style inspector opens. > inspector.jsm should not have any code related to any specific tool. The > registration mechanism should be totally generic. Yeah, there's no real reason inspector.jsm needs to be calling that registration, can do that change easily. > Does it make sense? Can this be part of this bug? Yes and yes.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Rob Campbell [:rc] (robcee) from comment #4) > would this be simpler if we separated the HTML view out of the registerTools > API and used that solely for registering sidebar tools? Yeah, that would simplify things. > There's at least one good reason for this: The HTML panel is integral to the > Page Inspector and is kind of unique. The sidebar is intended to be the > container for any number of "nodecentric" tools. > > Have a think and let that inform you. I don't want to derail your progress > with a new direction here, but we might at some point decide to separate > these. Yeah, I'm happy to go that route.
Comment 8•12 years ago
|
||
Maybe this mechanism doesn't need to handle callbacks when a node is selected or locked. Maybe you just need to give access to the highlighter instance (IUI.highlighter), then it's the job of the tool to listen or not the "nodeselected" and "locked"/"unlocked" events.
Comment 9•12 years ago
|
||
Or maybe not. I am actually not sure we want to expose any inner object to these tools. Using the registration API to proxy these events might be better.
Assignee | ||
Comment 10•12 years ago
|
||
I was thinking we could hand them an object that the tool can decide to listen to events on, but not the internal highlighter object.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → dcamp
Assignee | ||
Comment 11•12 years ago
|
||
New version adds an Inspector object that's meant to be a per-tab representation of the inspector (rather than the InspectorUI, which is per-browser-window). It's not all the way there yet (see comments in the file), but it can be presented to the sidebars as such. Also factors sidebar management out into its own object.
Attachment #579143 -
Attachment is obsolete: true
Attachment #588602 -
Attachment is obsolete: true
Attachment #604561 -
Flags: feedback?(rcampbell)
Assignee | ||
Comment 12•12 years ago
|
||
Test updates were annoyingly intertwined with the real changes, so here are the test changes separately.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Paul Rouget [:paul] from comment #5) > In an ideal world: > > Can we do that in a way that we don't have to edit inspector.jsm when we > want to add a new tool? > inspector.jsm should not have any code related to any specific tool. The > registration mechanism should be totally generic. > > Does it make sense? Can this be part of this bug? Newest WIP leaves inspector.jsm without a clue that the style inspectors exist.
Assignee | ||
Comment 14•12 years ago
|
||
Patch also necessarily includes some small fixes to initialization of HTML and style inspectors. Slight changes to how they were loaded were causing problems.
Reporter | ||
Comment 16•12 years ago
|
||
Comment on attachment 604561 [details] [diff] [review] new WIP in TreePanel.jsm: Hey! You didn't add yourself to the Contributors section! You really should. I know you want to. - if (this.IUI.selection) - this.select(this.IUI.selection, true); + if (this.pendingSelect) { + this.select(this.pendingSelect.node, this.pendingSelect.scroll); + delete this.pendingSelect; + } I like this. Maybe change the name to pendingSelection or deferredSelection? Noun your verb thing. + if (this.ioBox) { + this.ioBox.destroy(); + delete this.ioBox; + } + guessing this was done too late in the tear-down before? If we wanted to keep it in the destroy method, we could probably move it above the treeIFrame deletion bits. But it can live here instead. I do think you should take it out of the destroy() method though. inspector.jsm (aka, the good bits): - // Fires when the CSS Rule View is opened and initialized. - RULEVIEWREADY: "inspector-ruleview-ready", do we need a sidebar ready notification instead? (nvm, found it) + markDirty: function ITA_markDirty() what does ITA stand for? My brain is trying to expand that to "InspectorTabActor". + /** + * Show the given CSS rule in the css editor. + * + * @param CSSRule aRule + */ + editCSSRule: function ITA_handleCSSLink(aDocument, aRule) + { this feels like a bit of an odd place for this, though it is reliant on the _chromeWindow getter. Presumably this is why it's not in the Computed view. + /** + * Set a single value in the store. Again, when this object replaces + * InspectorStore this can probably just be dropped in favor of decorating + * Inspector. + */ + _setStoreValue: function ITA_setStoreValue(aID, aValue) + { + this._IUI.store.setValue(this._winID, aID, aValue); + }, we'll want a follow-up bug and a TODO in this comment, I think. ... +InspectorUI.registerSidebar = function IUI_registerSidebar(aRegistration) +{ + // Only allow a given tool ID to be registered once. + if (InspectorUI._registeredSidebars.some(function(elt) elt.id == aRegistration.id)) + return false; + + InspectorUI._registeredSidebars.push(aRegistration); + + // XXX: Need to update any currently-running sidebars. is this still true? + + return true; +} + +// XXX: unregisterSidebar. + anything to be done for these comments? + _hide: function ISS__hide() + { + this._toggleButton.checked = false; + this._box.setAttribute("hidden", true); + this._splitter.setAttribute("hidden", true); + }, this could be rolled into hide(). Don't really need the extra method. + _showContent: function ISS_showContent(aTool) + { + // If the current tool is already loaded, notify that we're + // showing this sidebar. + if (aTool.loaded) { + this._inspector._emit("sidebaractivated", aTool.id); + this._inspector._emit("sidebaractivated-" + aTool.id); + return; + } ah, I see what you did there. In StyleInspector.jsm: -var EXPORTED_SYMBOLS = ["StyleInspector"]; +// This module doesn't currently export any symbols directly, it only +// registers inspector tools. cool. + selectFromPath: function CVT_selectFromPath(aNode) { - if (this.isOpen()) - this.close(); - if (this.cssHtmlTree) - this.cssHtmlTree.destroy(); - if (this.iframe) { - this.iframe.parentNode.removeChild(this.iframe); - delete this.iframe; + // XXX: Noop, I don't think this is used anymore. + }, cut it! + onSelect: function CVT_onSelect(aEvent) + { + let node = this.inspector.selection; + + // XXX: check locked state. a couple of todos in this file to get rid of. much tighter. Love this.
Attachment #604561 -
Flags: feedback?(rcampbell) → feedback+
Assignee | ||
Comment 17•12 years ago
|
||
Rebased, haven't responded to comments yet.
Attachment #604561 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #604563 -
Attachment is obsolete: true
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #610771 -
Flags: review?(rcampbell)
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #610754 -
Attachment is obsolete: true
Attachment #610772 -
Flags: review?(rcampbell)
Assignee | ||
Updated•12 years ago
|
Attachment #610752 -
Attachment is obsolete: true
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #16) > I like this. Maybe change the name to pendingSelection or deferredSelection? > Noun your verb thing. Done. > > + if (this.ioBox) { > + this.ioBox.destroy(); > + delete this.ioBox; > + } > + > > guessing this was done too late in the tear-down before? If we wanted to > keep it in the destroy method, we could probably move it above the > treeIFrame deletion bits. But it can live here instead. I do think you > should take it out of the destroy() method though. Removed from destroy(). It was needed because sometimes the tree panel isn't destroyed, it's just hidden. But the iobox is created in show, so we had a dangling ioBox while it was hidden. This patch tickles a lot of slightly different initialization paths in the various tools, so there are a few little tweaks like this to make initialization a bit more robust. > + markDirty: function ITA_markDirty() > > what does ITA stand for? At first I didn't have the hubris to claim that this was the "Inspector" object. I was timid and just wanted it to be "The wrapper over InspectorUI that we hand to tools". So it was called InspectorToolAPI. But then I decided to claim the Inspector name/identity. New patch updates function names appropriately. > this feels like a bit of an odd place for this, though it is reliant on the > _chromeWindow getter. Presumably this is why it's not in the Computed view. I didn't want tools to have to dig through _chromeWindow, so I gave the a way to get at the styleeditor without doing that. But addons dig through the chrome window all the time, so maybe it's not that bad. I compromised in this version of the patch - _chromeWindow is still marked private, but I just use it in the style tools to open the style editor. We can decide later whether we want to make that chromeWindow fair game for addons. (by the way, as a note for future inspector.jsm reviewers, Inspector *is* intended to be tweaked by addons, and we should be really careful with anything we dont mark _private). > > + /** > + * Set a single value in the store. Again, when this object replaces > + * InspectorStore this can probably just be dropped in favor of decorating > + * Inspector. > + */ > + _setStoreValue: function ITA_setStoreValue(aID, aValue) > we'll want a follow-up bug and a TODO in this comment, I think. Done. > +InspectorUI.registerSidebar = function IUI_registerSidebar(aRegistration) > + // XXX: Need to update any currently-running sidebars. > > is this still true? Yeah, but I moved the warning that running sidebars won't get newly-registered sidebars to the docs and followed a followup. > +// XXX: unregisterSidebar. > + > anything to be done for these comments? Added an unregisterSidebar with the same caveat. > + _hide: function ISS__hide() > + { > + this._toggleButton.checked = false; > + this._box.setAttribute("hidden", true); > + this._splitter.setAttribute("hidden", true); > + }, > > this could be rolled into hide(). Don't really need the extra method. Sometimes we want to twiddle the saved state (like when the user hides the sidebar) and sometimes we just want to hide the sidebar as an implementation detail (like when they switch tabs or close the inspector). I renamed _hide() to _teardown(). > + selectFromPath: function CVT_selectFromPath(aNode) > { > + // XXX: Noop, I don't think this is used anymore. > + }, > > cut it! Gone. > > + onSelect: function CVT_onSelect(aEvent) > + { > + let node = this.inspector.selection; > + > + // XXX: check locked state. > > a couple of todos in this file to get rid of. Gotten rid of.
Assignee | ||
Comment 22•12 years ago
|
||
Updated test patch, try run at: https://tbpl.mozilla.org/?tree=Try&rev=0ac327eb7422
Attachment #610772 -
Attachment is obsolete: true
Attachment #610772 -
Flags: review?(rcampbell)
Assignee | ||
Updated•12 years ago
|
Attachment #610772 -
Flags: review?(rcampbell)
Reporter | ||
Comment 23•12 years ago
|
||
Comment on attachment 610771 [details] [diff] [review] v4 XPCOMUtils.defineLazyGetter(this, "InspectorUI", function() { let tmp = {}; Cu.import("resource:///modules/inspector.jsm", tmp); + + // Let style inspector tools register themselves. + Cu.import("resource:///modules/devtools/StyleInspector.jsm", tmp); + return new tmp.InspectorUI(window); }); I'm a little mystified why this needs to be here. Can it be moved inside inspector.jsm? * update, I now think I understand why you put it here. StyleInspector.jsm no longer exports anything. However, can it still be moved inside inspector.jsm or does it have to be outside that file for StyleInspector.jsm to do what it needs to do? inspector.jsm @@ -535,37 +608,48 @@ InspectorUI.prototype = { Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.DESTROYED, winId); + }, nit: extra linebreak +/////////////////////////////////////////////////////////////////////////// +//// Style Sidebar + +/** + * Manages the UI and loading of registered sidebar tools. + */ nit: should document the aOptions parameter. + /** + * Called by InspectorUI to create the UI for a registered sidebar tool. + * Will create a toolbar button and an iframe for the tool. + * @param aRegObj object + * See the documentation for InspectorUI.registerSidebarTool(). + */ I believe that should now refer to InspectorUI.registerSidebar(). + /** + * Hides the sidebar UI elements. + */ + _teardown: function ISS_teardown() nit: method signature should have two _s. ISS__teardown. Here and other methods in the InspectorStyleSidebar. ok, first pass. done. Going to go over the next file attachment in this before final review and give this a bit of a playtest.
Attachment #610771 -
Flags: review?(rcampbell)
Reporter | ||
Comment 24•12 years ago
|
||
This works really well. Still curious about moving the StyleInspector.jsm out of browser.js, but if that's unavoidable, this is good to go (pending a browser peer review).
Assignee | ||
Comment 25•12 years ago
|
||
Tests didn't change.
Attachment #610771 -
Attachment is obsolete: true
Attachment #615511 -
Flags: review?(rcampbell)
Assignee | ||
Comment 26•12 years ago
|
||
(In reply to Rob Campbell [:rc] (:robcee) from comment #23) > > I'm a little mystified why this needs to be here. Can it be moved inside > inspector.jsm? > > * update, I now think I understand why you put it here. StyleInspector.jsm > no longer exports anything. However, can it still be moved inside > inspector.jsm or does it have to be outside that file for StyleInspector.jsm > to do what it needs to do? I moved this into InspectorUI's constructor. > inspector.jsm > > @@ -535,37 +608,48 @@ InspectorUI.prototype = { > > Services.obs.notifyObservers(null, INSPECTOR_NOTIFICATIONS.DESTROYED, > winId); > + > }, > > nit: extra linebreak Fixed. > +/////////////////////////////////////////////////////////////////////////// > +//// Style Sidebar > + > +/** > + * Manages the UI and loading of registered sidebar tools. > + */ > > nit: should document the aOptions parameter. > > + /** > + * Called by InspectorUI to create the UI for a registered sidebar tool. > + * Will create a toolbar button and an iframe for the tool. > + * @param aRegObj object > + * See the documentation for InspectorUI.registerSidebarTool(). > + */ > > I believe that should now refer to InspectorUI.registerSidebar(). Fixed. > + /** > + * Hides the sidebar UI elements. > + */ > + _teardown: function ISS_teardown() > > nit: method signature should have two _s. ISS__teardown. Here and other > methods in the InspectorStyleSidebar. Fixed everywhere. Try for the new patch is here: https://tbpl.mozilla.org/?tree=Try&rev=8be1a73b8728
Reporter | ||
Updated•12 years ago
|
Attachment #615511 -
Flags: review?(rcampbell) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #613735 -
Flags: review+
Assignee | ||
Comment 27•12 years ago
|
||
Rebased on top of some recent test changes. https://tbpl.mozilla.org/?tree=Try&rev=224fe8846e69
Attachment #613735 -
Attachment is obsolete: true
Attachment #615511 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Attachment #610772 -
Flags: review?(rcampbell)
Assignee | ||
Comment 28•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/92dbde201794
Whiteboard: [firefox14-wanted] → [firefox14-wanted][fixed-in-fx-team]
Comment 29•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92dbde201794
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [firefox14-wanted][fixed-in-fx-team] → [firefox14-wanted]
Target Milestone: --- → Firefox 14
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•