Closed
Bug 692543
Opened 13 years ago
Closed 13 years ago
Make the Style Inspector UI faster
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: msucan, Assigned: msucan)
References
Details
(Whiteboard: [styleinspector][fixed-in-fx-team])
Attachments
(1 file, 10 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Building on top of the work being done in bug 685902 and in bug 672748, we need to make the whole Style Inspector UI faster. For this purpose we need changes both in CssLogic and CssHtmlTree.
Assignee | ||
Comment 1•13 years ago
|
||
Updated patch to address review comments from bug 685902 comment 11.
I also made changes to build on top of bug 672748.
(In reply to Dave Camp (:dcamp) from bug 685902 comment #11)
> Comment on attachment 563811 [details] [diff] [review] [diff] [details] [review]
> more performance work (wip)
>
> Review of attachment 563811 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
>
> r- mostly for the removed timeouts. If you think we can guarantee 50ms
> response without those, feel free to renom.
>
> ::: browser/devtools/styleinspector/CssHtmlTree.jsm
> @@ -192,5 @@
> >
> > - // We use a setTimeout loop to display the properties in batches of 15 at a
> > - // time. This results in a perceptibly more responsive UI.
> > - let i = 0;
> > - let batchSize = 15;
>
> You removed the timeouts for testing, right? Do you intend to land this?
>
> We're generally trying to avoid letting chrome go more than 50ms without
> hitting the event loop (see
> https://wiki.mozilla.org/Firefox/Features/50ms_ASSERT). We need to take that
> into account here.
Good point. No longer removing timeouts, just making *some* adjustments. Personally I am "70% happy" with the performance on Ubuntu.com and Github.
(even removing the timers wouldn't be too bad)
With this patch things do feel much faster on my system.
> ::: browser/devtools/styleinspector/CssLogic.jsm
> @@ +588,5 @@
> > * the domRules for the element are passed to the callback function.
> > *
> > * @param {function} [aCallback] Simple callback method
> > */
> > + hasMatchedSelectors: function CL_hasMatchedSelectors(aProperties)
>
> This method no longer matches its description.
Fixed. It was only a WIP. :)
> @@ +605,5 @@
> > + if (this._matchedRules) {
> > + if (aProperties.length > 0) {
> > + this._matchedRules.some(function(aRule) {
> > + aProperties = aProperties.filter(propertiesFilter.bind(this, aRule));
> > + return aProperties.length == 0;
>
> The logic for building up |result| is just complicated enough that
> duplicating it (one way when _matchedRules already exists, another when
> building _matchedRules) that it took some time to verify both paths. Might
> be nicer to just have:
>
> if (!this._matchedRules) {
> // regen matched rules, maybe split up into its own method
> }
>
> this._matchedRules.some(aProperties.filter(// etc
>
> Splitting out the matched-rule-regeneration into its own method would make
> all the various callsites that have bare hasMatchedSelectors() calls less
> weird too.
Good points. Fixed!
> @@ +660,3 @@
> > }
> > + } while ((element = element.parentNode) &&
> > + element.nodeType === Ci.nsIDOMNode.ELEMENT_NODE);
>
> Err, this parent node stuff seems incorrect. It should take into account
> the fact that some properties (like text alignment) are inherited from
> parent nodes, but others (like borders) aren't.
>
> This patch didn't introduce this problem, but ick. I filed bug 691978.
True. Thanks for the report!
> @@ +669,5 @@
> > * match the highlighted element or its parent elements.
> > *
> > * @param {String} aProperty The CSS property to check against
> > */
> > + hasUnmatchedSelectors: function CL_hasUnmatchedSelectors(aProperties)
>
> This comment needs updating too.
>
> Since on the surface they have such similar APIs, it's probably worth
> mentioning in the comments that hasUnmatchedSelectors() and
> hasMatchedSelectors() have vastly different performance characteristics.
Fixed.
> @@ +718,5 @@
> > + result[aProperty] = unmatched;
> > +
> > + // Keep this property if the rule matched. We need to find if a rule
> > + // has this property while it doesn't match the viewedElement.
> > + return !unmatched;
>
> If unmatched was false, we would have bailed out above the filter() call,
> right? Will this ever return true?
Hah, good catch! I wrote the filter function before I wrote the logic that skips rules entirely based on their matched/unmatched state, and I forgot to update the filter accordingly. Thanks for pointing this out.
Thank you Dave for your review! Looking forward for more comments!
Comment 2•13 years ago
|
||
Comment on attachment 565497 [details] [diff] [review]
updated patch
Review of attachment 565497 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good!
Attachment #565497 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 3•13 years ago
|
||
Thank you Dave for the r+!
A note on the expected performance with this patch: first open is slowest, it takes up to a second on github.org and ubuntu.com. Subsequent highlights of elements, expanding of matched selectors and so on... are (close to) instant.
There are edge cases where subsequent highlights of elements, or expansions of matched selectors that take up to a second - similar to how long a first open takes. These "edge cases" depend on which element is highlighted, which rules match, which don't, and how the styles of the page are structured. Some cause more code to run.
Switching between showing UA styles or not is also (close to) instant (in all cases).
Comment 4•13 years ago
|
||
Rebased to work on top of the current style inspector patch set
Attachment #565497 -
Attachment is obsolete: true
Assignee | ||
Comment 6•13 years ago
|
||
Thanks for your patch rebase.
Removed an obsolete comment in the rebase you did. :)
Attachment #566817 -
Attachment is obsolete: true
Comment 7•13 years ago
|
||
Attachment #567426 -
Attachment is obsolete: true
Updated•13 years ago
|
Whiteboard: [styleinspector] → [styleinspector][r+]
Comment 8•13 years ago
|
||
Attachment #569639 -
Attachment is obsolete: true
Comment 9•13 years ago
|
||
Attachment #569936 -
Attachment is obsolete: true
Updated•13 years ago
|
Priority: -- → P1
Assignee | ||
Comment 10•13 years ago
|
||
Another patch rebase, now with the fix from bug 692400 included.
Attachment #569946 -
Attachment is obsolete: true
Comment 11•13 years ago
|
||
Attachment #570067 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #571287 -
Attachment is obsolete: true
Comment 13•13 years ago
|
||
Attachment #571380 -
Attachment is obsolete: true
Updated•13 years ago
|
Whiteboard: [styleinspector][r+] → [styleinspector][land-in-fx-team]
Assignee | ||
Comment 14•13 years ago
|
||
Comment on attachment 571792 [details] [diff] [review]
Rebase
Rob: this patch is missing the CssLogic.shortSource() fix I have in attachment 571380 [details] [diff] [review].
Attachment #571792 -
Attachment is obsolete: true
Assignee | ||
Comment 15•13 years ago
|
||
Comment on attachment 571380 [details] [diff] [review]
another rebase
This patch still applies cleanly.
Attachment #571380 -
Attachment is obsolete: false
Comment 16•13 years ago
|
||
Whiteboard: [styleinspector][land-in-fx-team] → [styleinspector][fixed-in-fx-team]
Comment 17•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•