Bug 77865 – Web Inspector: Closed computed style sidebar pane rebuilds, resulting in slowness

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Patch to follow

[Alexander Pavlov (apavlov)]

Created attachment 125633 [details]
Patch

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

Created attachment 125657 [details]
Patch

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

Created attachment 125665 [details]
Patch

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

Created attachment 125807 [details]
Patch

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

Created attachment 125844 [details]
Patch

[Alexander Pavlov (apavlov)]

Committed r107055: <http://trac.webkit.org/changeset/107055>