Last Comment Bug 685893 - style inspector properties and methods to be moved out of the panel
: style inspector properties and methods to be moved out of the panel
Status: RESOLVED FIXED
[styleinspector]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-09 09:12 PDT by Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
Modified: 2011-10-22 12:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
style panelectomy WIP (21.14 KB, patch)
2011-10-06 14:47 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: feedback+
Details | Diff | Review
style panelectomy 2 WIP (25.25 KB, patch)
2011-10-13 13:44 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: feedback+
Details | Diff | Review
style panelectomy 3 (35.26 KB, patch)
2011-10-17 11:40 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review-
Details | Diff | Review
style panelectomy 4 (38.59 KB, patch)
2011-10-19 13:02 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review-
Details | Diff | Review
style panelectomy 5 (40.37 KB, patch)
2011-10-20 12:51 PDT, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Review
style panelectomy 6 (39.81 KB, patch)
2011-10-20 13:50 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Review

Description Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2011-09-09 09:12:18 PDT
The style inspector currently stores all properties and methods in the panel itself. The factory method createPanel should be changed to produce an object containing the panel, methods and properties.
Comment 1 Mihai Sucan [:msucan] 2011-09-23 01:24:18 PDT
Also see bug 687854 comment 18: we need to store a reference to IUI, or at least a reference to the opening chrome window, such that we avoid the use of getMostRecentWindow() in the Style Inspector code.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-10-06 05:59:24 PDT
taking.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-10-06 14:47:20 PDT
Created attachment 565362 [details] [diff] [review]
style panelectomy WIP

functionality is there, but some of the tests still need to be dealt with.
Comment 4 Rob Campbell [:rc] (:robcee) 2011-10-06 14:49:09 PDT
Comment on attachment 565362 [details] [diff] [review]
style panelectomy WIP

feedback on the approach might be good. I copied the pattern we used in TreePanel.jsm pretty closely so you should be familiar with it.
Comment 5 Mihai Sucan [:msucan] 2011-10-07 03:07:57 PDT
Comment on attachment 565362 [details] [diff] [review]
style panelectomy WIP

Review of attachment 565362 [details] [diff] [review]:
-----------------------------------------------------------------

Direction is exactly what we want. f+!

Some comments below. I'm sure you've already fixed some, but I noted them so we don't forget stuff.

::: browser/devtools/highlighter/inspector.jsm
@@ +895,1 @@
>        this.registerTool({

Registration should be moved into the StyleInspector._init() method. Just like the TreePanel does.

In _init() you can check if an IUI instance was passed. If yes, then do the registerTool() dance.

(basically we are making the StyleInspector work nicely both with the Inspector and Web Console, which is quite fancy)

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +62,3 @@
>   * @constructor
>   */
> +function CssHtmlTree(aIframe, aStyleInspector)

Comment above doesn't match the function signature.

Also, do we need to pass aIframe? Since we already pass the StyleInspector instance...

@@ +194,5 @@
>        let i = 0;
>        let batchSize = 15;
>        let max = CssHtmlTree.propertyNames.length - 1;
>        function displayProperties() {
> +        if (this.viewedElement == aElement && this.styleInspector.isOpen()) {

There are also some references to getMostRecentWindow() and to InspectorUI inside this file. Please update those accordingly. Thanks!

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +45,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  var EXPORTED_SYMBOLS = ["StyleInspector"];
>  
> +function StyleInspector(aContext)

This should also take an optional IUI instance, so we can use it inside CssHtmlTree.

@@ +72,2 @@
>     */
> +  createPanel: function SI_createPanel(aPreserveOnHide, aHudId, aCallback)

Do we need to pass aHudId from the HUDService?

@@ +87,4 @@
>      panel.setAttribute("width", 350);
> +    panel.setAttribute("height", this.window.screen.height / 2);
> +    if (aHudId)
> +      panel.setAttribute("hudToolId", aHudId);

This is something the HUDService can handle in its own callback.

@@ +98,5 @@
> +    {
> +      this.iframe.removeEventListener("load", boundIframeOnLoad, true);
> +      this.iframeReady = true;
> +      if (aCallback)
> +        aCallback();

Should pass |this| to the callback.

@@ +132,5 @@
> +
> +    return panel;
> +  },
> +
> +  popupShown: function SI_popupShown()

Some methods are missing jsdoc comments.

::: browser/devtools/webconsole/HUDService.jsm
@@ +4467,5 @@
>      if (!errstr) {
> +      let chromeWin = HUDService.getHudReferenceById(aJSTerm.hudId).chromeWindow;
> +      let styleInspector = new StyleInspector(chromeWin);
> +      styleInspector.createPanel(false, aJSTerm.hudId, function() {
> +        styleInspector.showTool(aNode);

Here you can also do styleInspector.panel.setAttribute("hudId", aJSTerm.hudId).

I'd say the StyleInspector should "know" as little as possible about the Web Console. In this sense, createPanel() shouldn't take an optional hudId, because we can handle this from the callback.

@@ +5378,5 @@
>    {
>      this.inputNode.removeEventListener("keypress", this._keyPress, false);
>      this.inputNode.removeEventListener("input", this._inputEventHandler, false);
>      this.inputNode.removeEventListener("keyup", this._inputEventHandler, false);
> +    if (this.styleInspector) {

Is this ever true?

I don't see jsterm.styleInspector = ... anywhere in the patch.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-10-13 13:44:49 PDT
Created attachment 566926 [details] [diff] [review]
style panelectomy 2 WIP

I think I managed to address most of your comments with this. Still need to update the tests which are likely all failing now.
Comment 7 Mihai Sucan [:msucan] 2011-10-17 05:08:04 PDT
Comment on attachment 566926 [details] [diff] [review]
style panelectomy 2 WIP

Review of attachment 566926 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch. This looks really good. F+!

I did some user testing and when I switch tabs, coming back opens the Style Inspector, but it's empty.

Looking forward for the test fixes.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +64,5 @@
> +  this.styleWin = aStyleInspector.iframe;
> +  this.styleInspector = aStyleInspector;
> +  this.cssLogic = aStyleInspector.cssLogic;
> +  this.doc = aStyleInspector.document;
> +  CssHtmlTree.win = this.win = aStyleInspector.window;

CssHtmlTree.win is shared across all instances of CssHtmlTree (a static property).

I see why you did this (for getRTLAttr), but I think we can have a better solution: merge isRTLAttr/getRTLAttr() (we don't need both, just one of them is sufficient), then make the method static, no need for making it a lazy getter, and just change the signature to accept the chrome window object.

@@ +69,1 @@
>    this.getRTLAttr = CssHtmlTree.getRTLAttr;

So we don't need this line...

@@ +91,5 @@
> + * @return {String} "ltr" or "rtl"
> + */
> +XPCOMUtils.defineLazyGetter(CssHtmlTree, "getRTLAttr", function() {
> +  return CssHtmlTree.win.getComputedStyle(CssHtmlTree.win.gBrowser).direction;
> +});

return aWin.getComputedStyle(aWin.gBrowser).direction;

(and no lazy getter. I don't see why a lazy getter should be used here)

@@ +243,3 @@
>            let elt = aEvent.target.pathElement;
> +          this.styleInspector.IUI.inspectNode(elt);
> +          // this.styleInspector.selectNode(elt);

I believe here we should aim for something a bit different: always pass the pathElement to this.styleInspector.selectNode(). That method should check if IUI is available (and thus call IUI.inspectNode()) or not (when the Style Inspector was opened from the Web Console). In the latter case it would only call CssLogic.highlight() and CssHtmlTree.highlight().

The reasoning is that we shouldn't make the CssHtmlTree "aware" of IUI, unless really needed: to keep it separate, to have less code to check when we want to make IUI changes. We know that it's all in StyleInspector.jsm.

@@ +354,4 @@
>      delete this.styleWin;
>      delete this.cssLogic;
>      delete this.doc;
>      delete this.win;

Given there's CssHtmlTree.win, we need delete for it as well, to avoid memleaks.

@@ +718,5 @@
>      }
>    },
>  
>    text: function SelectorView_text(aElement) {
> +    let result = this.selectorInfo.selector.text; // is this needed?

Yes. That gives us the display of the selector.

(see csshtmltree.xhtml)

@@ +731,5 @@
> +            addEventListener("click", function(aEvent) {
> +              this.styleInspector.IUI.inspectNode(this.selectorInfo.sourceElement);
> +              aEvent.preventDefault();
> +            }, false);
> +        }

The event handler needs to be added for both cases, even when sourceElement == IUI.selection. The is is needed so we call aEvent.preventDefault() when the user clicks on "element" source link for the "this" element.

Also, the event handler needs .bind(this) for it to work.

Also, you need a reference to the Style Inspector instance in SelectorView (see the constructor). this.styleInspector will throw in the event handler because it is undefined.

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +111,2 @@
>     */
> +  createPanel: function SI_createPanel(aPreserveOnHide, aCallback)

aHudId is still described in the comment above.

Also "optional" is missing the "l".

@@ +200,5 @@
> +
> +  /**
> +   * Check if the style inspector is open
> +   */
> +  isOpen: function SI_isOpen()

needs a @return
Comment 8 Rob Campbell [:rc] (:robcee) 2011-10-17 11:40:04 PDT
Created attachment 567507 [details] [diff] [review]
style panelectomy 3

all tests passing.

I did have to comment out a section in the webconsole test. It should probably be moved elsewhere as it's going to be hard to get that out of the jsterm now.

I also removed the StylePanel.isEnabled getter as it's only really used in tests and not really useful.
Comment 9 Mihai Sucan [:msucan] 2011-10-18 12:32:52 PDT
Comment on attachment 567507 [details] [diff] [review]
style panelectomy 3

Review of attachment 567507 [details] [diff] [review]:
-----------------------------------------------------------------

General comments:

- Feedback from comment 7 has not been addressed. (no code changes, according to interdiff)

- Test failure:

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_initialization.js | Console message: [JavaScript Error: "InspectorUI.stylePanel.showTool is not a function" {file: "chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_initialization.js" line: 96}]
... then a timeout.

- The rest of the test fixes look good.

Giving r- for the first two general comments. Looking forward for the patch update. Thank you!

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js
@@ +121,1 @@
>      let htmlTree = stylePanels[i].cssHtmlTree;

Agreed, but commenting out these tests here and now shouldn't be an option. Maybe a follow up bug report to move them out? Or just move the code now into a separate test file?
Comment 10 Rob Campbell [:rc] (:robcee) 2011-10-19 08:07:20 PDT
geez, not sure how I missed comment 7. Updating...
Comment 11 Mihai Sucan [:msucan] 2011-10-19 09:07:34 PDT
(In reply to Mihai Sucan [:msucan] from comment #7)
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +64,5 @@
> > +  this.styleWin = aStyleInspector.iframe;
> > +  this.styleInspector = aStyleInspector;
> > +  this.cssLogic = aStyleInspector.cssLogic;
> > +  this.doc = aStyleInspector.document;
> > +  CssHtmlTree.win = this.win = aStyleInspector.window;
> 
> CssHtmlTree.win is shared across all instances of CssHtmlTree (a static
> property).
> 
> I see why you did this (for getRTLAttr), but I think we can have a better
> solution: merge isRTLAttr/getRTLAttr() (we don't need both, just one of them
> is sufficient), then make the method static, no need for making it a lazy
> getter, and just change the signature to accept the chrome window object.

Just a quick note: csshtmltree.xhtml makes use of getRTLAttr() - so keeping this one seems to be the best choice.
Comment 12 Rob Campbell [:rc] (:robcee) 2011-10-19 12:59:01 PDT
(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 566926 [details] [diff] [review] [diff] [details] [review]
> style panelectomy 2 WIP
> 
> Review of attachment 566926 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updated patch. This looks really good. F+!
> 
> I did some user testing and when I switch tabs, coming back opens the Style
> Inspector, but it's empty.

talked about this a bit in IRC to explain the situation. It's not good, I'm afraid.

To Recap: The popup for the StyleInspector is opening before the iframe is loaded. This can be fixed by adding a setTimeout around the tools restore state code, but this causes timing problems with other tests. I may be able to figure out a way to defer the panelShown code if the iframe is not ready, but I'm reluctant to do much to the Inspector's startup code sequence as it's pretty tight right now.

> Looking forward for the test fixes.
> 
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ +64,5 @@
> > +  this.styleWin = aStyleInspector.iframe;
> > +  this.styleInspector = aStyleInspector;
> > +  this.cssLogic = aStyleInspector.cssLogic;
> > +  this.doc = aStyleInspector.document;
> > +  CssHtmlTree.win = this.win = aStyleInspector.window;
> 
> CssHtmlTree.win is shared across all instances of CssHtmlTree (a static
> property).

That's true, but we really only need *any* browser xul window since this is only used for getRTLAttr.

> I see why you did this (for getRTLAttr), but I think we can have a better
> solution: merge isRTLAttr/getRTLAttr() (we don't need both, just one of them
> is sufficient), then make the method static, no need for making it a lazy
> getter, and just change the signature to accept the chrome window object.

I've compromised and just referenced the static CssHtmlTree.win variable from inside getRTLAttr. No need for the argument if that's set, and it should be.

> @@ +91,5 @@
> > + * @return {String} "ltr" or "rtl"
> > + */
> > +XPCOMUtils.defineLazyGetter(CssHtmlTree, "getRTLAttr", function() {
> > +  return CssHtmlTree.win.getComputedStyle(CssHtmlTree.win.gBrowser).direction;
> > +});
> 
> return aWin.getComputedStyle(aWin.gBrowser).direction;
> 
> (and no lazy getter. I don't see why a lazy getter should be used here)

The lazy getter made sense when this was using getMostRecentWindow.

> @@ +243,3 @@
> >            let elt = aEvent.target.pathElement;
> > +          this.styleInspector.IUI.inspectNode(elt);
> > +          // this.styleInspector.selectNode(elt);
> 
> I believe here we should aim for something a bit different: always pass the
> pathElement to this.styleInspector.selectNode(). That method should check if
> IUI is available (and thus call IUI.inspectNode()) or not (when the Style
> Inspector was opened from the Web Console). In the latter case it would only
> call CssLogic.highlight() and CssHtmlTree.highlight().
> 
> The reasoning is that we shouldn't make the CssHtmlTree "aware" of IUI,
> unless really needed: to keep it separate, to have less code to check when
> we want to make IUI changes. We know that it's all in StyleInspector.jsm.

I understand why you want this, but selectNode is used by IUI in its onSelect method. So we really need two methods here.

> @@ +354,4 @@
> >      delete this.styleWin;
> >      delete this.cssLogic;
> >      delete this.doc;
> >      delete this.win;
> 
> Given there's CssHtmlTree.win, we need delete for it as well, to avoid
> memleaks.

Yes. Thanks!

> 
> @@ +718,5 @@
> >      }
> >    },
> >  
> >    text: function SelectorView_text(aElement) {
> > +    let result = this.selectorInfo.selector.text; // is this needed?
> 
> Yes. That gives us the display of the selector.
> 
> (see csshtmltree.xhtml)
> 
> @@ +731,5 @@
> > +            addEventListener("click", function(aEvent) {
> > +              this.styleInspector.IUI.inspectNode(this.selectorInfo.sourceElement);
> > +              aEvent.preventDefault();
> > +            }, false);
> > +        }
> 
> The event handler needs to be added for both cases, even when sourceElement
> == IUI.selection. The is is needed so we call aEvent.preventDefault() when
> the user clicks on "element" source link for the "this" element.
> 
> Also, the event handler needs .bind(this) for it to work.
> 
> Also, you need a reference to the Style Inspector instance in SelectorView
> (see the constructor). this.styleInspector will throw in the event handler
> because it is undefined.

gah. OK, fixed.

> ::: browser/devtools/styleinspector/StyleInspector.jsm
> @@ +111,2 @@
> >     */
> > +  createPanel: function SI_createPanel(aPreserveOnHide, aCallback)
> 
> aHudId is still described in the comment above.
> 
> Also "optional" is missing the "l".
> 
> @@ +200,5 @@
> > +
> > +  /**
> > +   * Check if the style inspector is open
> > +   */
> > +  isOpen: function SI_isOpen()
> 
> needs a @return
Comment 13 Rob Campbell [:rc] (:robcee) 2011-10-19 13:02:22 PDT
Created attachment 568162 [details] [diff] [review]
style panelectomy 4

fourth try.
Comment 14 Mihai Sucan [:msucan] 2011-10-20 04:39:32 PDT
Comment on attachment 568162 [details] [diff] [review]
style panelectomy 4

Review of attachment 568162 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is 99% there, but the little buggers... See comments below.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +64,5 @@
> +  this.styleWin = aStyleInspector.iframe;
> +  this.styleInspector = aStyleInspector;
> +  this.cssLogic = aStyleInspector.cssLogic;
> +  this.doc = aStyleInspector.document;
> +  CssHtmlTree.win = this.win = aStyleInspector.window;

Given the compromise to put CssHtmlTree.getRTLAttr as a static, you also need to do this.getRTLAttr = CssHtmlTree.getRTLAttr. (thanks to how csshtmltree.xhtml can only access properties from the CssHtmlTree object instance ...)

I get a big number of console messages with errors because getRTLAttr is not available.

(no need to do delete this.getRTLAttr later in the code, because it won't cause memleaks)

@@ +229,5 @@
>    {
>      aEvent.preventDefault();
>      if (aEvent.target && this.viewedElement != aEvent.target.pathElement) {
> +      let elt = aEvent.target.pathElement;
> +      this.styleInspector.selectFromPath(elt);

Why not give it pathElement directly?

@@ -373,1 @@
>  

You need to keep this here to allow csshtmltree.xhtml access to the method.

@@ +655,5 @@
>     * A localized Get localized human readable info
>     */
>    humanReadableText: function SelectorView_humanReadableText(aElement)
>    {
> +    if (CssHtmlTree.getRTLAttr()) {

if (CssHtmlTree.getRTLAttr() == "rtl") {

@@ +673,5 @@
> +        } else {
> +          result = CssLogic.getShortName(this.selectorInfo.sourceElement);
> +          aElement.parentNode.querySelector(".rule-link > a").
> +            addEventListener("click", function(aEvent) {
> +              this.styleInspector.selectPath(this.selectorInfo.sourceElement);

selectPath is undefined.
this.styleInspector is also undefined.

You need to pass a reference to the Style Inspector down into SelectorView.

@@ +681,2 @@
>        }
>  

Please add the event handler even when styleInspector.IUI is not available, so users can click "element" in the Web Console use-case. (it should work in both cases)

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js
@@ +121,1 @@
>      let htmlTree = stylePanels[i].cssHtmlTree;

Please file a follow up bug report about this.
Comment 15 Rob Campbell [:rc] (:robcee) 2011-10-20 12:50:44 PDT
so, after gnashing my teeth on this for an afternoon, I am unable to get rid of the "ReferenceError: getRTLAttr is not defined" errors on the console.

I've tried a few variations, including reverting to using the defineLazyGetter mechanism that got us here in the first place.

At a bit of a loss, but attaching my patch in case anything seems obvious to someone who hasn't spent far too much time on this bug.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-10-20 12:51:59 PDT
Created attachment 568484 [details] [diff] [review]
style panelectomy 5
Comment 17 Rob Campbell [:rc] (:robcee) 2011-10-20 13:50:55 PDT
Created attachment 568504 [details] [diff] [review]
style panelectomy 6

Dave noticed I'd removed getRTLAttr from PropertyView. That was the cause of the failures. Ditched CssHtmlTree.getRTLAttr as it wasn't used.
Comment 18 Mihai Sucan [:msucan] 2011-10-21 05:38:25 PDT
Comment on attachment 568504 [details] [diff] [review]
style panelectomy 6

Review of attachment 568504 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. r+ with the comments below addressed. (there's still some minor breakage)

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +63,2 @@
>  {
> +  dump("Creating CssHtmlTree\n");

There are multiple dump() calls through out the file, please remove them.

@@ +653,5 @@
>     * A localized Get localized human readable info
>     */
>    humanReadableText: function SelectorView_humanReadableText(aElement)
>    {
> +    if (aTree.getRTLAttr == "rtl") {

aTree is undefined.

s/aTree/this.tree/

@@ +674,4 @@
>        }
> +      aElement.parentNode.querySelector(".rule-link > a").
> +        addEventListener("click", function(aEvent) {
> +          this.tree.styleInspector.selectPath(this.selectorInfo.sourceElement);

selectPath is undefined

s/selectPath/selectFromPath/

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +217,3 @@
>        }
> +    } else {
> +      this.selectNode(aEvent.target.pathElement);

aEvent is undefined

You mean this.selectNode(aNode).
Comment 19 Rob Campbell [:rc] (:robcee) 2011-10-21 10:46:16 PDT
gah! good catches. Thanks for the reviews.
Comment 20 Rob Campbell [:rc] (:robcee) 2011-10-21 10:56:27 PDT
https://hg.mozilla.org/integration/fx-team/rev/a0abf1681c3d
Comment 21 Rob Campbell [:rc] (:robcee) 2011-10-21 13:04:55 PDT
test failure bug fix 007f57853973
Comment 22 Tim Taubert [:ttaubert] 2011-10-22 12:15:52 PDT
https://hg.mozilla.org/mozilla-central/rev/a0abf1681c3d
Comment 23 Tim Taubert [:ttaubert] 2011-10-22 12:19:46 PDT
https://hg.mozilla.org/mozilla-central/rev/007f57853973

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