Closed
Bug 698762
Opened 13 years ago
Closed 13 years ago
Remove unmatched rules from style inspector
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: miker, Assigned: miker)
References
Details
(Whiteboard: [style-inspector][fixed-in-fx-team])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
The calculations involved with unmatched rules are very costly so we need to drop them.
Assignee | ||
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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!
Attachment #571316 -
Flags: review?(mihai.sucan) → review+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [styleinspector] → [styleinspector][land-in-fx-team]
Comment 4•13 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2967e93cdb44
Whiteboard: [styleinspector][land-in-fx-team] → [styleinspector][fixed-in-fx-team]
Comment 5•13 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f0e736dbc7bc https://hg.mozilla.org/integration/fx-team/rev/1b6acffa7a94
Status: NEW → ASSIGNED
Whiteboard: [styleinspector][fixed-in-fx-team] → [style-inspector][backed-out]
Comment 7•13 years ago
|
||
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)
Comment 8•13 years ago
|
||
and further backing out: https://hg.mozilla.org/integration/fx-team/rev/b5d68f1763ed
Comment 10•13 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
Comment 11•13 years ago
|
||
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)
Updated•13 years ago
|
Priority: -- → P1
Comment 12•13 years ago
|
||
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•13 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/74b8d49b12af
Whiteboard: [style-inspector][backed-out] → [style-inspector][fixed-in-fx-team]
Comment 14•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/74b8d49b12af
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•13 years ago
|
Attachment #572278 -
Flags: review?(dcamp)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•