685893 - style inspector properties and methods to be moved out of the panel

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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]]

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)]

taking.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: style panelectomy WIP

functionality is there, but some of the tests still need to be dealt with.

Comment 4

[Rob Campbell [:rc] (:robcee)]

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]]

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)]

Attachment: 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]]

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)]

Attachment: 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]]

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)]

geez, not sure how I missed comment 7. Updating...

Comment 11

[Mihai Sucan [:msucan]]

(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)]

(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)]

Attachment: style panelectomy 4

fourth try.

Comment 14

[Mihai Sucan [:msucan]]

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)]

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)]

Attachment: style panelectomy 5

Comment 17

[Rob Campbell [:rc] (:robcee)]

Attachment: 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]]

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)]

gah! good catches. Thanks for the reviews.

Comment 20

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/a0abf1681c3d

Comment 21

[Rob Campbell [:rc] (:robcee)]

test failure bug fix 007f57853973

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a0abf1681c3d

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/007f57853973