Last Comment Bug 698762 - Remove unmatched rules from style inspector
: Remove unmatched rules from style inspector
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Michael Ratcliffe [:miker] [:mratcliffe]
: J. Ryan Stinnett [:jryans] (use ni?)
Depends on: 692543 700036
Blocks: 689759 700061
  Show dependency treegraph
Reported: 2011-11-01 08:51 PDT by Michael Ratcliffe [:miker] [:mratcliffe]
Modified: 2011-11-15 14:16 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Unmatched selector extraction (41.82 KB, patch)
2011-11-02 06:08 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
mihai.sucan: review+
Details | Diff | Splinter Review
Rebased (47.37 KB, patch)
2011-11-05 04:39 PDT, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Splinter Review
minimal patch to disable unmatched selectors (9.33 KB, patch)
2011-11-05 14:22 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
minimal patch 2 (8.35 KB, patch)
2011-11-06 02:41 PST, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Splinter Review

Description Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-01 08:51:20 PDT
The calculations involved with unmatched rules are very costly so we need to drop them.
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-02 06:08:23 PDT
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.
Comment 2 Mihai Sucan [:msucan] 2011-11-02 11:12:45 PDT
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!
Comment 3 Michael Ratcliffe [:miker] [:mratcliffe] 2011-11-05 04:39:36 PDT
Created attachment 572188 [details] [diff] [review]
Comment 4 Rob Campbell [:rc] (:robcee) 2011-11-05 10:43:42 PDT
Comment 6 Dave Camp (:dcamp) 2011-11-05 13:43:18 PDT
Backed out due to rebase errors slipping in to the css files.
Comment 7 Mihai Sucan [:msucan] 2011-11-05 14:22:04 PDT
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.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-11-05 15:20:49 PDT
and further backing out:
Comment 9 Rob Campbell [:rc] (:robcee) 2011-11-05 15:23:06 PDT
and also:
Comment 10 Dave Camp (:dcamp) 2011-11-05 18:09:07 PDT
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
Comment 11 Mihai Sucan [:msucan] 2011-11-06 02:41:47 PST
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!)
Comment 12 Rob Campbell [:rc] (:robcee) 2011-11-06 13:31:33 PST
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.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-11-07 06:58:13 PST

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