Closed Bug 672743 Opened 13 years ago Closed 13 years ago

Remove category view from style inspector

Categories

(DevTools :: General, defect, P2)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 9

People

(Reporter: miker, Assigned: miker)

References

Details

(Whiteboard: [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d][has-patch][review+][fixed-in-fx-team])

Attachments

(1 file, 12 obsolete files)

54.26 KB, patch
Details | Diff | Splinter Review
We need to remove the category view from the style inspector.
Attached image Wireframe (obsolete) —
Whiteboard: [hydra]
Bug 672748 depends on this so this also needs the [minotaur] keyword
Whiteboard: [hydra] → [hydra][minotaur]
Whiteboard: [hydra][minotaur] → [styleinspector][minotaur]
Assignee: nobody → mratcliffe
Priority: -- → P2
Whiteboard: [styleinspector][minotaur] → [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d]
Attached patch Update style inspector UI patch (obsolete) — Splinter Review
This patch is not yet complete. We need to find the cause of the long pause.
No longer blocks: 672748
Attached image Style Inspector with categories removed (obsolete) —
Attachment #547013 - Attachment is obsolete: true
Blocks: 672748
No longer depends on: 672748
Attached patch Remove categories (obsolete) — Splinter Review
Attachment #548445 - Attachment is obsolete: true
Attachment #548451 - Attachment is obsolete: true
Whiteboard: [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d] → [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d][has-patch]
Status: NEW → ASSIGNED
Attached patch Remove categories patch 2 (obsolete) — Splinter Review
Attachment #548724 - Attachment is obsolete: true
Attachment #549197 - Flags: review?(mihai.sucan)
Attached patch Remove categories patch 3 (obsolete) — Splinter Review
Rebased
Attachment #549197 - Attachment is obsolete: true
Attachment #551797 - Flags: review?(mihai.sucan)
Attachment #549197 - Flags: review?(mihai.sucan)
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.
Attachment #551797 - Flags: review?(mihai.sucan) → review-
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.
Attached patch Remove categories patch 4 (obsolete) — Splinter Review
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
Attachment #551797 - Attachment is obsolete: true
Attachment #552413 - Flags: review?(mihai.sucan)
Attachment #552413 - Flags: feedback?(dao)
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
Attachment #552413 - Flags: feedback?(dao)
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?
Attachment #552413 - Flags: review?(mihai.sucan) → review+
Attached patch Remove categories patch 5 (obsolete) — Splinter Review
Updated according to comments above
Attachment #552413 - Attachment is obsolete: true
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.
Attached patch Remove categories patch 6 (obsolete) — Splinter Review
> > .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.
Attachment #552748 - Attachment is obsolete: true
Whiteboard: [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d][has-patch] → [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d][has-patch][review+]
Rechecked the patch. Good work! Thanks for your updates Mike!
Attached patch Remove categories patch 7 (obsolete) — Splinter Review
Now works with new devtools folders
Attachment #552858 - Attachment is obsolete: true
Attached patch Remove categories patch 8 (obsolete) — Splinter Review
Rebased
Attachment #552994 - Attachment is obsolete: true
Attached patch Remove categories patch 9 (obsolete) — Splinter Review
Rebased
Attachment #554360 - Attachment is obsolete: true
No longer blocks: 680111
Depends on: 680111
No longer depends on: 663831
Whiteboard: [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d][has-patch][review+] → [styleinspector][minotaur][best: 4h; likely: 1d; worst: 3d][has-patch][review+][fixed-in-fx-team]
Comment on attachment 556794 [details] [diff] [review]
[checked-in] Remove categories patch 10

http://hg.mozilla.org/integration/fx-team/rev/c6309c9aa79a
Attachment #556794 - Attachment description: Remove categories patch 10 → [in-fx-team] Remove categories patch 10
Comment on attachment 556794 [details] [diff] [review]
[checked-in] Remove categories patch 10

http://hg.mozilla.org/mozilla-central/rev/c6309c9aa79a
Attachment #556794 - Attachment description: [in-fx-team] Remove categories patch 10 → [checked-in] Remove categories patch 10
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: