698762 - Remove unmatched rules from style inspector

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

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

The calculations involved with unmatched rules are very costly so we need to drop them.

Comment 1

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

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

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 ubuntu.com and github.com. Yay!

Comment 3

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

Attachment: Rebased

Comment 4

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

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

Comment 5

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

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

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

Comment 6

[Dave Camp (:dcamp)]

Backed out due to rebase errors slipping in to the css files.

Comment 7

[Mihai Sucan [:msucan]]

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

and further backing out:

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

Comment 9

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

and also:

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

Comment 10

[Dave Camp (:dcamp)]

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

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

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 13

[Dave Camp (:dcamp)]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/74b8d49b12af