Summary: | Web Inspector: Closed computed style sidebar pane rebuilds, resulting in slowness | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2012-02-06 04:47:42 PST
Created attachment 125633 [details]
Patch
Comment on attachment 125633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125633&action=review > Source/WebCore/inspector/front-end/StylesSidebarPane.js:258 > + resultStyles.computedStyle = new WebInspector.CSSStyleDeclaration({ cssProperties: [], shorthandEntries: {} }); Why not null? > Source/WebCore/inspector/front-end/StylesSidebarPane.js:-493 > - _markUsedProperties: function(styleRules, usedProperties, disabledComputedProperties) I'd suggest that you remove disabledComputedProperties as a separate change. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:854 > + // Overriding expand() rather than onexpand() to eliminate visual slowness due to a possible backend trip. Why exactly is this needed? > LayoutTests/http/tests/inspector/elements-test.js:86 > +InspectorTest.selectNodeAndWaitForStyles = function(idValue, expandComputedStyles, callback) Not the best change to the harness given that all the clients now need to think about the computed style. I'd suggest that you introduce a separate command that expands it separately. > LayoutTests/http/tests/inspector/elements-test.js:107 > + InspectorTest.runAfterPendingDispatches(function() { We should not use runAfterPendingDispatches if at all possible. Created attachment 125657 [details]
Patch
Comment on attachment 125657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125657&action=review > Source/WebCore/inspector/front-end/StylesSidebarPane.js:864 > + if (WebInspector.panels.elements.sidebarPanes.styles.sections[0][0].propertiesElement.children.length) You should never access styles via the panel. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:870 > + _expandedForTest: function(node) You could sniff instead. > LayoutTests/inspector/elements/elements-panel-styles.html:11 > + InspectorTest.selectNodeAndWaitForStylesWithComputed("foo", step1); It would be great to have a test that checks that computed style is not loaded initially, but is loaded on demand. Created attachment 125665 [details]
Patch
Comment on attachment 125665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125665&action=review > Source/WebCore/inspector/front-end/StylesSidebarPane.js:860 > + WebInspector.SidebarPane.prototype.expand.call(this, userCallback); You could sniff this in a test. > Source/WebCore/inspector/front-end/StylesSidebarPane.js:866 > + WebInspector.panels.elements.sidebarPanes.styles._refreshComputedStyleSection(callback.bind(this, userCallback)); I would leave this as the only path for the sake of the code simplicity. Created attachment 125807 [details]
Patch
Comment on attachment 125807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125807&action=review > Source/WebCore/inspector/front-end/StylesSidebarPane.js:429 > + if (nodeComputedStyle || nodeComputedStyle === null) Why did this change? > Source/WebCore/inspector/front-end/StylesSidebarPane.js:618 > + if (computedStyle || computedStyle === null) Why did this change? > Source/WebCore/inspector/front-end/StylesSidebarPane.js:863 > + WebInspector.panels.elements.sidebarPanes.styles._refreshComputedStyleSection(callback.bind(this)); You should not reach out for this via the elements panel. Created attachment 125844 [details]
Patch
Committed r107055: <http://trac.webkit.org/changeset/107055> |