Last Comment Bug 672743 - Remove category view from style inspector
: Remove category view from style inspector
Status: RESOLVED FIXED
[styleinspector][minotaur][best: 4h; ...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 All
: P2 normal (vote)
: Firefox 9
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
:
Mentors:
Depends on: 582596 680111
Blocks: 672744 672746 672748 674482 674485 674856
  Show dependency treegraph
 
Reported: 2011-07-20 01:42 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2011-08-31 05:50 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Wireframe (208.32 KB, image/png)
2011-07-20 02:01 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details
Update style inspector UI patch (25.86 KB, patch)
2011-07-26 07:30 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Style Inspector with categories removed (59.55 KB, image/png)
2011-07-26 07:40 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details
Remove categories (39.88 KB, patch)
2011-07-27 02:19 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Remove categories patch 2 (42.64 KB, patch)
2011-07-28 12:32 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Remove categories patch 3 (50.41 KB, patch)
2011-08-09 09:41 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review-
Details | Diff | Splinter Review
Remove categories patch 4 (54.63 KB, patch)
2011-08-11 10:01 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review
Remove categories patch 5 (56.10 KB, patch)
2011-08-12 13:25 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Remove categories patch 6 (55.81 KB, patch)
2011-08-13 04:58 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Remove categories patch 7 (55.98 KB, patch)
2011-08-14 10:46 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Remove categories patch 8 (55.61 KB, patch)
2011-08-19 04:11 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
Remove categories patch 9 (55.61 KB, patch)
2011-08-25 02:47 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
[checked-in] Remove categories patch 10 (54.26 KB, patch)
2011-08-30 04:00 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-20 01:42:43 PDT
We need to remove the category view from the style inspector.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-20 02:01:30 PDT
Created attachment 547013 [details]
Wireframe
Comment 2 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-21 03:30:43 PDT
Bug 672748 depends on this so this also needs the [minotaur] keyword
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-26 07:30:21 PDT
Created attachment 548445 [details] [diff] [review]
Update style inspector UI patch

This patch is not yet complete. We need to find the cause of the long pause.
Comment 4 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-26 07:40:05 PDT
Created attachment 548451 [details]
Style Inspector with categories removed
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-27 02:19:59 PDT
Created attachment 548724 [details] [diff] [review]
Remove categories
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2011-07-28 12:32:11 PDT
Created attachment 549197 [details] [diff] [review]
Remove categories patch 2
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-09 09:41:45 PDT
Created attachment 551797 [details] [diff] [review]
Remove categories patch 3

Rebased
Comment 8 Mihai Sucan [:msucan] 2011-08-09 13:15:18 PDT
Comment on attachment 551797 [details] [diff] [review]
Remove categories patch 3

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

Patch looks great, it's close to r+.

General comments:

- I did try inspectstyle(document.body) on some random site and in the Error Console I got:

Error: data is undefined
Source File: resource:///modules/templater.jsm
Line: 80

- When I run the style inspector tests I get:

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Console message: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIDOMCSSStyleDeclaration.getPropertyValue]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource:///modules/csslogic.jsm :: <TOP_LEVEL> :: line 1214"  data: no]
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.js | Console message: Error reading computed style for overflow

... for every property.

Please fix these issues and address the comments below. Looking forward for the updated patch. Thank you!

::: browser/base/content/inspector.js
@@ +946,5 @@
> +
> +    // We need a 300ms delay before selecting the current node in attached tools to prevent them from slowing down the
> +    // highlighter (nodes will only be selected in tools when the user pauses over it with the mouse for 300ms). This
> +    // number could be lower if we were not using css transitions.
> +    if (this.select.toolTimeout) {

Please use this.highlightNodeTimeout or something like that. Mainly, do not add a new property to the this.select() method - add it to InspectorUI.

@@ +957,5 @@
> +        if (aTool.panel.state == "open") {
> +          aTool.onSelect.apply(aTool.context, [aNode]);
> +        }
> +      });
> +    }, 300);

This is a magic number. Please add a new constant that tells the delay, and describe its purpose in a comment.

Also, you can do something like:
- get rid of the let self = this.
- setTimeout(this.toolsDo.bind(this, function (aTool) { ... }), 300);

(no need for self, you can use bind)

::: browser/devtools/styleinspector/csshtmltree.jsm
@@ +111,5 @@
>   * processed nodes will be displayed.
>   * @param {object} aData the data to pass to the template.
>   */
> +CssHtmlTree.processTemplate = function CssHtmlTree_processTemplate(aTemplate, aDestination,
> +                                                     aData, aPreserveDestination)

Please update the jsdoc to include a description of the new aPreserveDestination param.

@@ +143,5 @@
>     */
>    highlight: function CssHtmlTree_highlight(aElement)
>    {
> +    if (this.viewedElement != aElement) {
> +      this.viewedElement = aElement;

Do an early return so you don't nest the entire function body inside this if.

if (this.viewedElement == aElement) {
  return;
}
// ...

@@ +174,5 @@
> +            self.win.setTimeout(displayProperties, 0);
> +          }
> +        }
> +      }
> +      this.win.setTimeout(displayProperties, 0);

You can use .bind(this) for displayProperties to get rid of self = this.

Also, why not put the call to new PropertyView() inside displayProperties()?. i goes from 0 to propertyViews.length -1 anyway.

@@ +220,2 @@
>    {
> +    if (!CssHtmlTree.CSSPropertyNames) {

You can do an early return here. if (CssHtmlTree.CSSPropertyNames) { return; }

Also, perhaps a better array name would be just propertyNames? I am open to better ideas.

@@ +234,5 @@
> +      for (let len = arr.length; i < len; i++) {
> +          if (arr[i].charAt(0) != "-")
> +              break;
> +      }
> +      CssHtmlTree.CSSPropertyNames = Array.concat(arr, arr.splice(0, i));

It might be more efficient to:

- in the first loop where you read each styles.item() push to two arrays. One that holds the "-" vendor prefixed props, and another array where you store the other properties.

- then Array.concat(arr1, arr2).sort().

In this way you walk each item fewer times.

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +92,4 @@
>    </div>
>  
>    <!--
> +  TemplateProperties lists the properties themselves. Each needs data like this:

I think you mean templateProperty.

Below you renamed templateProperties to templateProperty.

@@ +107,4 @@
>          </span>
> +        <span class="property-value" dir="ltr">${value}</span>
> +        <div class="link" dir="${getRTLAttr}">
> +          <div class="expander"></div>${ruleTitle(__element)}

You can use <div class="expander"/>.

Please note that this markup mixes inline with block level elements in the same parent node, which is not really good. Please make the code use block level elements.

@@ +141,5 @@
>      </loop>
>      <tr if="${showUnmatchedLink}">
>        <td colspan="4">
> +        <div class="unmatched-rule-expander"></div><a href="#" onclick="${showUnmatchedLinkClick}"
> +            class="unmatchedlink">${showUnmatchedLinkText}</a>

Similar to above: mixing of inline with block level. Also, you can use <div/>.

::: browser/themes/gnomestripe/browser/csshtmltree.css
@@ +46,5 @@
> +  margin-left: 32px;
> +}
> +td {
> +  padding-left: 10px;
> +}

I don't generally like the idea of changing the style of tags. Too general. You might want to have a more specific selector.

Additionally, margin/padding-left might need to be changed to -start for RTL users. Please check.

Anyhow, for style changes I suggest you ask for review from Dão as well.
Comment 9 Mihai Sucan [:msucan] 2011-08-10 05:30:40 PDT
Comment on attachment 551797 [details] [diff] [review]
Remove categories patch 3

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

Dependency issue. See below.

::: browser/base/content/inspector.js
@@ +946,5 @@
> +
> +    // We need a 300ms delay before selecting the current node in attached tools to prevent them from slowing down the
> +    // highlighter (nodes will only be selected in tools when the user pauses over it with the mouse for 300ms). This
> +    // number could be lower if we were not using css transitions.
> +    if (this.select.toolTimeout) {

Is the style inspector registered as a tool in Inspector? I can't find it.

I found bug 663831 which should probably be marked as a dependency in this bug.
Comment 10 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-11 10:01:55 PDT
Created attachment 552413 [details] [diff] [review]
Remove categories patch 4

Dão: Mihai asked if you could check the css files included in this patch. If you have time can you take a look to see if there is anything that we can do better?

(In reply to Mihai Sucan [:msucan] from comment #8)
> Comment on attachment 551797 [details] [diff] [review]
> Remove categories patch 3
> 
> Review of attachment 551797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks great, it's close to r+.
> 
> General comments:
> 
> - I did try inspectstyle(document.body) on some random site and in the Error
> Console I got:
> 
> Error: data is undefined
> Source File: resource:///modules/templater.jsm
> Line: 80
> 
> - When I run the style inspector tests I get:
> 
> TEST-INFO |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_styleinspector_webconsole.js | Console message:
> [Exception... "Component returned failure code: 0x80040111
> (NS_ERROR_NOT_AVAILABLE) [nsIDOMCSSStyleDeclaration.getPropertyValue]" 
> nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame ::
> resource:///modules/csslogic.jsm :: <TOP_LEVEL> :: line 1214"  data: no]
> TEST-INFO |
> chrome://mochitests/content/browser/browser/devtools/styleinspector/test/
> browser/browser_styleinspector_webconsole.js | Console message: Error
> reading computed style for overflow
> 
> ... for every property.
> 
> Please fix these issues and address the comments below. Looking forward for
> the updated patch. Thank you!
> 

Fixed

> ::: browser/base/content/inspector.js
> @@ +946,5 @@
> > +
> > +    // We need a 300ms delay before selecting the current node in attached tools to prevent them from slowing down the
> > +    // highlighter (nodes will only be selected in tools when the user pauses over it with the mouse for 300ms). This
> > +    // number could be lower if we were not using css transitions.
> > +    if (this.select.toolTimeout) {
> 
> Please use this.highlightNodeTimeout or something like that. Mainly, do not
> add a new property to the this.select() method - add it to InspectorUI.
> 

Okay, Done.

> @@ +957,5 @@
> > +        if (aTool.panel.state == "open") {
> > +          aTool.onSelect.apply(aTool.context, [aNode]);
> > +        }
> > +      });
> > +    }, 300);
> 
> This is a magic number. Please add a new constant that tells the delay, and
> describe its purpose in a comment.
> 

Done

> Also, you can do something like:
> - get rid of the let self = this.
> - setTimeout(this.toolsDo.bind(this, function (aTool) { ... }), 300);
> 
> (no need for self, you can use bind)
> 

Done ... or at least something like it, IUI_highlightNodeTimeout was the method that needed to be bound.

> ::: browser/devtools/styleinspector/csshtmltree.jsm
> @@ +111,5 @@
> >   * processed nodes will be displayed.
> >   * @param {object} aData the data to pass to the template.
> >   */
> > +CssHtmlTree.processTemplate = function CssHtmlTree_processTemplate(aTemplate, aDestination,
> > +                                                     aData, aPreserveDestination)
> 
> Please update the jsdoc to include a description of the new
> aPreserveDestination param.
> 

Nicely spotted ... done.

> @@ +143,5 @@
> >     */
> >    highlight: function CssHtmlTree_highlight(aElement)
> >    {
> > +    if (this.viewedElement != aElement) {
> > +      this.viewedElement = aElement;
> 
> Do an early return so you don't nest the entire function body inside this if.
> 
> if (this.viewedElement == aElement) {
>   return;
> }
> // ...
> 

Seems like this is the preferred style in fx code ... noted and done.

> @@ +174,5 @@
> > +            self.win.setTimeout(displayProperties, 0);
> > +          }
> > +        }
> > +      }
> > +      this.win.setTimeout(displayProperties, 0);
> 
> You can use .bind(this) for displayProperties to get rid of self = this.
> 

Done

> Also, why not put the call to new PropertyView() inside
> displayProperties()?. i goes from 0 to propertyViews.length -1 anyway.
> 

I left it separate to simplify investigation of the long pause but you are right that it can go back inside the loop now. Done.

> @@ +220,2 @@
> >    {
> > +    if (!CssHtmlTree.CSSPropertyNames) {
> 
> You can do an early return here. if (CssHtmlTree.CSSPropertyNames) { return;
> }
> 
> Also, perhaps a better array name would be just propertyNames? I am open to
> better ideas.
> 

I guess it is obvious here that we are talking about CSS properties. Changed.

> @@ +234,5 @@
> > +      for (let len = arr.length; i < len; i++) {
> > +          if (arr[i].charAt(0) != "-")
> > +              break;
> > +      }
> > +      CssHtmlTree.CSSPropertyNames = Array.concat(arr, arr.splice(0, i));
> 
> It might be more efficient to:
> 
> - in the first loop where you read each styles.item() push to two arrays.
> One that holds the "-" vendor prefixed props, and another array where you
> store the other properties.
> 
> - then Array.concat(arr1, arr2).sort().
> 
> In this way you walk each item fewer times.
> 

You are right and the code is more maintainable this way ... done.

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +92,4 @@
> >    </div>
> >  
> >    <!--
> > +  TemplateProperties lists the properties themselves. Each needs data like this:
> 
> I think you mean templateProperty.
> 
> Below you renamed templateProperties to templateProperty.
> 

Changed

> @@ +107,4 @@
> >          </span>
> > +        <span class="property-value" dir="ltr">${value}</span>
> > +        <div class="link" dir="${getRTLAttr}">
> > +          <div class="expander"></div>${ruleTitle(__element)}
> 
> You can use <div class="expander"/>.
> 

Because this is an XHTML document you are correct but I prefer to go with the premise that tags that can contain content e.g. p, strong, div, blockquote, textarea, script etc. should not be self closed, only tags that cannot contain content should be self closed.

> Please note that this markup mixes inline with block level elements in the
> same parent node, which is not really good. Please make the code use block
> level elements.
> 
> @@ +141,5 @@
> >      </loop>
> >      <tr if="${showUnmatchedLink}">
> >        <td colspan="4">
> > +        <div class="unmatched-rule-expander"></div><a href="#" onclick="${showUnmatchedLinkClick}"
> > +            class="unmatchedlink">${showUnmatchedLinkText}</a>
> 
> Similar to above: mixing of inline with block level. Also, you can use
> <div/>.
> 

Done

> ::: browser/themes/gnomestripe/browser/csshtmltree.css
> @@ +46,5 @@
> > +  margin-left: 32px;
> > +}
> > +td {
> > +  padding-left: 10px;
> > +}
> 
> I don't generally like the idea of changing the style of tags. Too general.
> You might want to have a more specific selector.
> 

Apart from styling the body tag I would agree. I have now moved these styles into existing classes.

> Additionally, margin/padding-left might need to be changed to -start for RTL
> users. Please check.
> 

Of course ... done

> Anyhow, for style changes I suggest you ask for review from Dão as well.

Done
Comment 11 Dão Gottwald [:dao] 2011-08-11 11:11:53 PDT
Comment on attachment 552413 [details] [diff] [review]
Remove categories patch 4

>+        <div class="inline-block">

>+.inline-block {
>+  display: inline-block;
>+}

Bad non-descriptive class name, just a convoluted way of writing <div style="display:inline-block">.

>+.link, .unmatchedlink { color: #55A;  }

There should be a line break after each comma, opening curly bracket and semicolon:

.link,
.unmatchedlink {
  color: #55A;
}

>+.rule-count .expander,
>+.rule-unmatched .expander,

Can those rules not use the descendant selector?
https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector
Comment 12 Mihai Sucan [:msucan] 2011-08-11 12:43:50 PDT
Comment on attachment 552413 [details] [diff] [review]
Remove categories patch 4

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

Nice! All tests pass. Giving the patch r+ with the comments below addressed. Thanks for your update!

Also, please answer the questions below.

::: browser/base/content/inspector.js
@@ +958,5 @@
> +          if (aTool.panel.state == "open") {
> +            aTool.onSelect.apply(aTool.context, [aNode]);
> +          }
> +        });
> +      }.bind(this), toolDelay);

Maybe I wasn't clear. In my previous review comment I asked for:

this.highlightNodeTimeout = setTimeout(this.toolsDo.bind(this,
  function IUI_toolsOnSelect(aTool) {
    if (aTool.panel.state == "open") {
      aTool.onSelect.call(aTool.context, aNode);
    }
  }), toolDelay);

Notice the toolsDo.bind() trick, and also aTool.onSelect.call() instead of .apply(). You do not need .apply here.

::: browser/devtools/styleinspector/csshtmltree.jsm
@@ +110,5 @@
>   * @param {nsIDOMElement} aDestination the destination node where the
>   * processed nodes will be displayed.
>   * @param {object} aData the data to pass to the template.
> + * @param {Boolean} aPreserveDestination If true then the template will be appended to aDestination's content else$
> + * aDestination.innerHTML will be cleared before the template is appended.

Please properly wrap this comment at 80 characters and remove the $.

@@ +168,5 @@
> +      if (this.viewedElement == aElement && this.panel.isOpen()) {
> +        for (let step = i + stepSize; i < step && i <= max; i++) {
> +          let propView = new PropertyView(this, CssHtmlTree.propertyNames[i]);
> +          CssHtmlTree.processTemplate(this.templateProperty, this.propertyContainer, propView, true);
> +        }

Is this loop supposed to add only one property view per timeout? (per call to displayProperties()) Or is it supposed to add 25 property views at once for every displayProperties() call in a timeout?

(code is a bit confusing)

@@ +236,5 @@
> +      }
> +    }
> +    CssHtmlTree.propertyNames.sort();
> +    mozProps.sort();
> +    CssHtmlTree.propertyNames = Array.concat(CssHtmlTree.propertyNames, mozProps);

Seeing this, it might be fancier to just do:

CssHtmlTree.propertyNames.sort();
CssHtmlTree.propertyNames.push.apply(CssHtmlTree.propertyNames, mozProps.sort());

::: browser/devtools/styleinspector/csshtmltree.xhtml
@@ +55,5 @@
>  <html xmlns="http://www.w3.org/1999/xhtml"
>    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>  <head>
> +  <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" />
> +  <link rel="stylesheet" type="text/css" href="chrome://browser/skin/csshtmltree.css" />

Is this change needed?

@@ +104,3 @@
>          </span>
> +        <span class="property-value" dir="ltr">${value}</span>
> +        <div class="link" dir="${getRTLAttr}">

You still mix inline with block level elements here (span and div). :)

@@ +138,5 @@
>      </loop>
>      <tr if="${showUnmatchedLink}">
>        <td colspan="4">
> +        <div class="unmatched-rule-expander"></div>
> +        <div class="inline-block">

Agreed with Dão here:  this needs a better class name.

::: browser/devtools/styleinspector/test/browser/browser_styleinspector_webconsole.htm
@@ -169,5 @@
> -
> -    html::selection {
> -      background-color: #f00;
> -      font-family: fantasy;
> -    }

Why this change?
Comment 13 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-12 13:25:35 PDT
Created attachment 552748 [details] [diff] [review]
Remove categories patch 5

Updated according to comments above
Comment 14 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-12 13:26:48 PDT
Forgot to add my feedback:
(In reply to Dão Gottwald [:dao] from comment #11)
> Comment on attachment 552413 [details] [diff] [review]
> Remove categories patch 4
> 
> >+        <div class="inline-block">
> 
> >+.inline-block {
> >+  display: inline-block;
> >+}
> 
> Bad non-descriptive class name, just a convoluted way of writing <div
> style="display:inline-block">.
> 

True, moved it into a.unmatchedlink

> >+.link, .unmatchedlink { color: #55A;  }
> 
> There should be a line break after each comma, opening curly bracket and
> semicolon:
> 

I am glad you said that, fixed.

> .link,
> .unmatchedlink {
>   color: #55A;
> }
> 
> >+.rule-count .expander,
> >+.rule-unmatched .expander,
> 
> Can those rules not use the descendant selector?
> https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector

To be honest I think the best we can do is:
.rule-count > .expander,
.rule-unmatched > .expander,

There is not much choice here.

> ::: browser/base/content/inspector.js
> @@ +958,5 @@
> > +          if (aTool.panel.state == "open") {
> > +            aTool.onSelect.apply(aTool.context, [aNode]);
> > +          }
> > +        });
> > +      }.bind(this), toolDelay);
> 
> Maybe I wasn't clear. In my previous review comment I asked for:
> 
> this.highlightNodeTimeout = setTimeout(this.toolsDo.bind(this,
>   function IUI_toolsOnSelect(aTool) {
>     if (aTool.panel.state == "open") {
>       aTool.onSelect.call(aTool.context, aNode);
>     }
>   }), toolDelay);
> 
> Notice the toolsDo.bind() trick, and also aTool.onSelect.call() instead of
> .apply(). You do not need .apply here.
> 

Okay, done

> ::: browser/devtools/styleinspector/csshtmltree.jsm
> @@ +110,5 @@
> >   * @param {nsIDOMElement} aDestination the destination node where the
> >   * processed nodes will be displayed.
> >   * @param {object} aData the data to pass to the template.
> > + * @param {Boolean} aPreserveDestination If true then the template will be appended to aDestination's content else$
> > + * aDestination.innerHTML will be cleared before the template is appended.
> 
> Please properly wrap this comment at 80 characters and remove the $.
> 

Done ... my editor was not properly configured.

> @@ +168,5 @@
> > +      if (this.viewedElement == aElement && this.panel.isOpen()) {
> > +        for (let step = i + stepSize; i < step && i <= max; i++) {
> > +          let propView = new PropertyView(this, CssHtmlTree.propertyNames[i]);
> > +          CssHtmlTree.processTemplate(this.templateProperty, this.propertyContainer, propView, true);
> > +        }
> 
> Is this loop supposed to add only one property view per timeout? (per call
> to displayProperties()) Or is it supposed to add 25 property views at once
> for every displayProperties() call in a timeout?
> 
> (code is a bit confusing)
> 

I have added comments to make it easier to follow.

> @@ +236,5 @@
> > +      }
> > +    }
> > +    CssHtmlTree.propertyNames.sort();
> > +    mozProps.sort();
> > +    CssHtmlTree.propertyNames = Array.concat(CssHtmlTree.propertyNames, mozProps);
> 
> Seeing this, it might be fancier to just do:
> 
> CssHtmlTree.propertyNames.sort();
> CssHtmlTree.propertyNames.push.apply(CssHtmlTree.propertyNames,
> mozProps.sort());
> 

Wow ... you can extend an array this way? And it is around 2.5 x faster than Array.concat()? Done!

> ::: browser/devtools/styleinspector/csshtmltree.xhtml
> @@ +55,5 @@
> >  <html xmlns="http://www.w3.org/1999/xhtml"
> >    xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> >  <head>
> > +  <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=UTF-8" />
> > +  <link rel="stylesheet" type="text/css" href="chrome://browser/skin/csshtmltree.css" />
> 
> Is this change needed?
> 

No ... my visible margin was not set to 80 characters. I thought it was set correctly.

> @@ +104,3 @@
> >          </span>
> > +        <span class="property-value" dir="ltr">${value}</span>
> > +        <div class="link" dir="${getRTLAttr}">
> 
> You still mix inline with block level elements here (span and div). :)
> 

Done, but I should say that I agree that having a block level element inside an inline element would be bad, but I see no problems with mixing them in the way I had.

> @@ +138,5 @@
> >      </loop>
> >      <tr if="${showUnmatchedLink}">
> >        <td colspan="4">
> > +        <div class="unmatched-rule-expander"></div>
> > +        <div class="inline-block">
> 
> Agreed with Dão here:  this needs a better class name.
> 

Done

> :::
> browser/devtools/styleinspector/test/browser/
> browser_styleinspector_webconsole.htm
> @@ -169,5 @@
> > -
> > -    html::selection {
> > -      background-color: #f00;
> > -      font-family: fantasy;
> > -    }
> 
> Why this change?

Because in tests it was throwing an option strict warning (unsupported pseudo element). I did not know that we have ::-moz-selection so I have now changed it to that.
Comment 15 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-13 04:58:15 PDT
Created attachment 552858 [details] [diff] [review]
Remove categories patch 6

> > .link,
> > .unmatchedlink {
> >   color: #55A;
> > }
> > 
> > >+.rule-count .expander,
> > >+.rule-unmatched .expander,
> > 
> > Can those rules not use the descendant selector?
> > https://developer.mozilla.org/en/Writing_Efficient_CSS#Avoid_the_descendant_selector
> 
> To be honest I think the best we can do is:
> .rule-count > .expander,
> .rule-unmatched > .expander,
> 
> There is not much choice here.

After some thought of course I can remove the descendant selector.
Comment 16 Mihai Sucan [:msucan] 2011-08-13 07:11:09 PDT
Rechecked the patch. Good work! Thanks for your updates Mike!
Comment 17 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-14 10:46:13 PDT
Created attachment 552994 [details] [diff] [review]
Remove categories patch 7

Now works with new devtools folders
Comment 18 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-19 04:11:50 PDT
Created attachment 554360 [details] [diff] [review]
Remove categories patch 8

Rebased
Comment 19 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-25 02:47:56 PDT
Created attachment 555683 [details] [diff] [review]
Remove categories patch 9

Rebased
Comment 20 Michael Ratcliffe [:miker] [:mratcliffe] 2011-08-30 04:00:39 PDT
Created attachment 556794 [details] [diff] [review]
[checked-in] Remove categories patch 10
Comment 21 Rob Campbell [:rc] (:robcee) 2011-08-30 09:57:48 PDT
Comment on attachment 556794 [details] [diff] [review]
[checked-in] Remove categories patch 10

http://hg.mozilla.org/integration/fx-team/rev/c6309c9aa79a
Comment 22 Rob Campbell [:rc] (:robcee) 2011-08-31 05:49:13 PDT
Comment on attachment 556794 [details] [diff] [review]
[checked-in] Remove categories patch 10

http://hg.mozilla.org/mozilla-central/rev/c6309c9aa79a

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