Last Comment Bug 707809 - Refactor creation of registered sidebar tools iframes in InspectorUI
: Refactor creation of registered sidebar tools iframes in InspectorUI
Status: RESOLVED FIXED
[firefox14-wanted]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 14
Assigned To: Dave Camp (:dcamp)
:
: Patrick Brosset <:pbro>
Mentors:
Depends on: 740615 747550
Blocks: 683954 697983 702411 717929 719867 721745 740662
  Show dependency treegraph
 
Reported: 2011-12-05 13:11 PST by Rob Campbell [:rc] (:robcee)
Modified: 2012-04-20 15:01 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
registerToolsRedux [WIP] (12.83 KB, patch)
2011-12-05 13:11 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
another WIP (42.60 KB, patch)
2012-01-13 22:20 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
another WIP (with small fix) (42.61 KB, patch)
2012-01-13 22:29 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
new WIP (69.42 KB, patch)
2012-03-09 16:30 PST, Dave Camp (:dcamp)
rcampbell: feedback+
Details | Diff | Splinter Review
test updates (63.20 KB, patch)
2012-03-09 16:31 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
wip 3 (70.44 KB, patch)
2012-03-29 16:43 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
wip 3 test updates (63.20 KB, patch)
2012-03-29 16:44 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v4 (75.55 KB, patch)
2012-03-29 17:23 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v4 tests (77.03 KB, patch)
2012-03-29 17:23 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v4.1 tests (77.30 KB, patch)
2012-04-10 13:23 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
v5 (75.65 KB, patch)
2012-04-16 15:46 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
combined and rebased (155.52 KB, patch)
2012-04-17 12:13 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-12-05 13:11:22 PST
Created attachment 579143 [details] [diff] [review]
registerToolsRedux [WIP]

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.
Comment 1 Dave Camp (:dcamp) 2012-01-13 22:20:35 PST
Created attachment 588601 [details] [diff] [review]
another WIP

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.
Comment 2 Dave Camp (:dcamp) 2012-01-13 22:24:36 PST
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.
Comment 3 Dave Camp (:dcamp) 2012-01-13 22:29:49 PST
Created attachment 588602 [details] [diff] [review]
another WIP (with small fix)
Comment 4 Rob Campbell [:rc] (:robcee) 2012-01-16 08:41:18 PST
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.
Comment 5 Paul Rouget [:paul] 2012-01-16 10:04:18 PST
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?
Comment 6 Dave Camp (:dcamp) 2012-01-17 09:44:19 PST
(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.
Comment 7 Dave Camp (:dcamp) 2012-01-17 09:45:04 PST
(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 Paul Rouget [:paul] 2012-01-21 07:47:18 PST
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 Paul Rouget [:paul] 2012-01-21 08:06:41 PST
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.
Comment 10 Dave Camp (:dcamp) 2012-01-21 08:55:01 PST
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.
Comment 11 Dave Camp (:dcamp) 2012-03-09 16:30:33 PST
Created attachment 604561 [details] [diff] [review]
new WIP

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.
Comment 12 Dave Camp (:dcamp) 2012-03-09 16:31:18 PST
Created attachment 604563 [details] [diff] [review]
test updates

Test updates were annoyingly intertwined with the real changes, so here are the test changes separately.
Comment 13 Dave Camp (:dcamp) 2012-03-09 16:32:09 PST
(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.
Comment 14 Dave Camp (:dcamp) 2012-03-09 16:33:13 PST
Patch also necessarily includes some small fixes to initialization of HTML and style inspectors.  Slight changes to how they were loaded were causing problems.
Comment 15 Paul Rouget [:paul] 2012-03-14 09:14:23 PDT
triage, filter on centaur
Comment 16 Rob Campbell [:rc] (:robcee) 2012-03-29 08:27:31 PDT
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.
Comment 17 Dave Camp (:dcamp) 2012-03-29 16:43:43 PDT
Created attachment 610752 [details] [diff] [review]
wip 3

Rebased, haven't responded to comments yet.
Comment 18 Dave Camp (:dcamp) 2012-03-29 16:44:40 PDT
Created attachment 610754 [details] [diff] [review]
wip 3 test updates
Comment 19 Dave Camp (:dcamp) 2012-03-29 17:23:13 PDT
Created attachment 610771 [details] [diff] [review]
v4
Comment 20 Dave Camp (:dcamp) 2012-03-29 17:23:46 PDT
Created attachment 610772 [details] [diff] [review]
v4 tests
Comment 21 Dave Camp (:dcamp) 2012-03-29 17:33:40 PDT
(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.
Comment 22 Dave Camp (:dcamp) 2012-04-10 13:23:20 PDT
Created attachment 613735 [details] [diff] [review]
v4.1 tests

Updated test patch, try run at:

https://tbpl.mozilla.org/?tree=Try&rev=0ac327eb7422
Comment 23 Rob Campbell [:rc] (:robcee) 2012-04-11 13:12:36 PDT
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.
Comment 24 Rob Campbell [:rc] (:robcee) 2012-04-12 09:11:15 PDT
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).
Comment 25 Dave Camp (:dcamp) 2012-04-16 15:46:19 PDT
Created attachment 615511 [details] [diff] [review]
v5

Tests didn't change.
Comment 26 Dave Camp (:dcamp) 2012-04-16 15:50:31 PDT
(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
Comment 27 Dave Camp (:dcamp) 2012-04-17 12:13:41 PDT
Created attachment 615812 [details] [diff] [review]
combined and rebased

Rebased on top of some recent test changes.
https://tbpl.mozilla.org/?tree=Try&rev=224fe8846e69
Comment 29 Tim Taubert [:ttaubert] 2012-04-20 01:59:29 PDT
https://hg.mozilla.org/mozilla-central/rev/92dbde201794

Note You need to log in before you can comment on or make changes to this bug.