Remove unmatched rules from style inspector

RESOLVED FIXED in Firefox 10



Developer Tools
6 years ago
6 years ago


(Reporter: miker, Assigned: miker)


Firefox 10
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [style-inspector][fixed-in-fx-team])


(2 attachments, 2 obsolete attachments)

The calculations involved with unmatched rules are very costly so we need to drop them.
Depends on: 692543
Created attachment 571316 [details] [diff] [review]
Unmatched selector extraction

Now even fast on Github! I tried removing the timeouts but then the interface became far less responsive. Works fine with breadcrumbs etc.
Attachment #571316 - Flags: review?(mihai.sucan)
Comment on attachment 571316 [details] [diff] [review]
Unmatched selector extraction

Review of attachment 571316 [details] [diff] [review]:

Patch looks good, r+. Thank you!

Non-review comment: I would have expected a minimal patch that simply removes the calls to code dealing with unmatched selectors - basically just remove them from the UI. This would have made the review quicker and it would have made easier to later revive unmatched selectors. Since the work is already done in this patch, no point in going back.

Performance is instant for me on and Yay!
Attachment #571316 - Flags: review?(mihai.sucan) → review+
Whiteboard: [styleinspector] → [styleinspector][land-in-fx-team]
Created attachment 572188 [details] [diff] [review]
Attachment #571316 - Attachment is obsolete: true
Blocks: 689759
Whiteboard: [styleinspector][land-in-fx-team] → [styleinspector][fixed-in-fx-team]
Whiteboard: [styleinspector][fixed-in-fx-team] → [style-inspector][backed-out]

Comment 6

6 years ago
Backed out due to rebase errors slipping in to the css files.
Created attachment 572232 [details] [diff] [review]
minimal patch to disable unmatched selectors

As agreed with dcamp, I have made a minimal patch to disable the unmatched selectors.

This patch now depends on bug 700036.
Attachment #572232 - Flags: review?(rcampbell)


6 years ago
Depends on: 700036


6 years ago
Blocks: 700061
and further backing out:
and also:

Comment 10

6 years ago
Comment on attachment 572232 [details] [diff] [review]
minimal patch to disable unmatched selectors

Review of attachment 572232 [details] [diff] [review]:

We should either:

a) Make sure Every Commented-out-section has a comment with the followup
b) Replace comments with if (kUnmatchedEnabled) { ... }
c) Just remove the minimal UI glue code (but not the bulk of the unmatchedSelector support in CssLogic) and make sure the followup bug includes this patch in case someone wants to reference when they put stuff back in.

I slightly prefer c), commented out code tends to bit-rot.  Anyone resurrecting this support can put back in the minimal glue code manually.  But only slightly.

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +138,5 @@
>  CssHtmlTree.prototype = {
>    // Cache the list of properties that have matched and unmatched properties.
>    _matchedProperties: null,
> +
> +  // TODO: bug 700061 - re-enalbe unmatched selectors or entirely remove them

This typo is propagated through the patch
Created attachment 572278 [details] [diff] [review]
minimal patch 2

Updated minimal patch. I have removed the code instead of commenting out, as requested. I did not remove code that doesn't need to be removed for the purpose of disabling unmatched selectors. I am trying to keep the patch *minimal*, with minimal rebase churn. If we grow the patch we'll have more rebase fun.

(asking for review from any of you dcamp/robcee - who gets to this first is fine. thank you!)
Attachment #572232 - Attachment is obsolete: true
Attachment #572232 - Flags: review?(rcampbell)
Attachment #572278 - Flags: review?(rcampbell)
Attachment #572278 - Flags: review?(dcamp)


6 years ago
Priority: -- → P1
Comment on attachment 572278 [details] [diff] [review]
minimal patch 2

looks ok, still not keen on the comments. Should maybe reference future remove unmatched selectors bug in preceding comment.

R+ with that.
Attachment #572278 - Flags: review?(rcampbell) → review+

Comment 13

6 years ago
Whiteboard: [style-inspector][backed-out] → [style-inspector][fixed-in-fx-team]
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10


6 years ago
Attachment #572278 - Flags: review?(dcamp)
You need to log in before you can comment on or make changes to this bug.