Bug 105828 – Web Inspector: Implement tracking of active stylesheets in the frontend

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

[Alexander Pavlov (apavlov)]

This is necessary to implement the stylesheet disablement feature.

[Alexander Pavlov (apavlov)]

Created attachment 180883 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 180883 [details]
Patch

Attachment 180883 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15576009

[Build Bot]

Comment on attachment 180883 [details]
Patch

Attachment 180883 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15551835

New failing tests:
http/tests/inspector/resource-har-pages.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html
fast/sub-pixel/transformed-iframe-copy-on-scroll.html

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 180883 [details])
> Attachment 180883 [details] did not pass cr-android-ews (chromium-android):
> Output: http://queues.webkit.org/results/15576009

Sounds like a buildsystem flaw or something (a stale generated file out/Release/gen/webkit/InspectorFrontend.h has got stuck in the way).

[Alexander Pavlov (apavlov)]

Created attachment 180915 [details]
Patch for EWS

[Peter Beverloo (cr-android ews)]

Comment on attachment 180915 [details]
Patch for EWS

Attachment 180915 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15558973

[Build Bot]

Comment on attachment 180915 [details]
Patch for EWS

Attachment 180915 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15592023

New failing tests:
http/tests/inspector/resource-har-pages.html

[Alexander Pavlov (apavlov)]

Created attachment 180923 [details]
Fix JsDoc/remove unused param

[Build Bot]

Comment on attachment 180923 [details]
Fix JsDoc/remove unused param

Attachment 180923 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15584190

New failing tests:
http/tests/inspector/resource-har-pages.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html
fast/sub-pixel/transformed-iframe-copy-on-scroll.html

[Vsevolod Vlasov]

Comment on attachment 180923 [details]
Fix JsDoc/remove unused param

View in context: https://bugs.webkit.org/attachment.cgi?id=180923&action=review

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1250
> +        this._loadStyleSheetHeaders();

I thought we want to push headers, so this should not be needed anymore (and hence _loadStyleSheetHeaders can be removed.)?

[Alexander Pavlov (apavlov)]

Created attachment 181920 [details]
Get rid of _loadStyleSheetHeaders(), as it's not needed

[WebKit Review Bot]

Comment on attachment 181920 [details]
Get rid of _loadStyleSheetHeaders(), as it's not needed

Attachment 181920 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15755609

[Vsevolod Vlasov]

Comment on attachment 181920 [details]
Get rid of _loadStyleSheetHeaders(), as it's not needed

View in context: https://bugs.webkit.org/attachment.cgi?id=181920&action=review

> Source/WebCore/inspector/front-end/CSSStyleModel.js:387
> +        if (header.origin === "inspector")

I would keep this in resource binding so that this check is not spread all over the code.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:480
> +        return this._resourceBinding._getOrCreateInspectorResource(rule.id.styleSheetId);

I think inspector resource should be already created by this moment, so it's better to use a simple getter here. Then we can simplify the method that creates inspector resource.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1195
> +    headerForStyleSheetId: function(styleSheetId)

Looks like this one is unused

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1247
>       */

Could you please annotate this._frameAndURLToStyleSheetId and this._styleSheetIdToHeader below?

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1259
> +    _getOrCreateInspectorResource: function(styleSheetId)

This should be called from _setHeaderForStyleSheetId only now so you can pass header in it.

[Alexander Pavlov (apavlov)]

Created attachment 182125 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182125 [details]
Patch

Attachment 182125 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15771972

[Peter Beverloo (cr-android ews)]

Comment on attachment 182125 [details]
Patch

Attachment 182125 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15760901

[Vsevolod Vlasov]

Comment on attachment 182125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182125&action=review

Front end changes look good to me.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:474
> +    getViaInspectorResourceForRule: function(rule)

Nit: please remove get prefix, we only use get for asynchronous getters.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1267
>          var inspectorResource = frame.resourceForURL(viaInspectorURL);

I would replace this with console.assert(!frame.resourceForURL(viaInspectorURL));

> Source/WebCore/inspector/front-end/CSSStyleModel.js:1276
>          inspectorResource = new WebInspector.Resource(null, viaInspectorURL, resource.documentURL, resource.frameId, resource.loaderId, WebInspector.resourceTypes.Stylesheet, "text/css", true);

var ..

[Alexander Pavlov (apavlov)]

Created attachment 182332 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 184736 [details]
Rebased patch

[Alexander Pavlov (apavlov)]

Created attachment 186222 [details]
Rebased patch

[Build Bot]

Comment on attachment 186222 [details]
Rebased patch

Attachment 186222 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16343718

[Pavel Feldman]

Comment on attachment 186222 [details]
Rebased patch

Given the scope of the change and lack of tests, I am afraid it will regress something. Could we move in a more incremental manner? I.e. splitting the change into several ones and covering each with the tests?

[Pavel Feldman]

Comment on attachment 186222 [details]
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186222&action=review

Backend looks good.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:685
> +            continue;

When does this happen?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:800
>  void InspectorCSSAgent::getAllStyleSheets(ErrorString*, RefPtr<TypeBuilder::Array<TypeBuilder::CSS::CSSStyleSheetHeader> >& styleInfos)

We should not need this method any longer.

> Source/WebCore/inspector/InspectorCSSAgent.h:144
> +        CreatingViaInspectorStyleSheetScope(InspectorCSSAgent* agent)

Just skip this.

[Alexander Pavlov (apavlov)]

Created attachment 187074 [details]
Patch

[Build Bot]

Comment on attachment 187074 [details]
Patch

Attachment 187074 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16454004

[Alexander Pavlov (apavlov)]

Created attachment 188342 [details]
Test added

[Alexander Pavlov (apavlov)]

Committed r142975: <http://trac.webkit.org/changeset/142975>

[WebKit Review Bot]

Re-opened since this is blocked by bug 110225

[Alexander Pavlov (apavlov)]

Created attachment 189783 [details]
Patch

[Build Bot]

Comment on attachment 189783 [details]
Patch

Attachment 189783 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16723036

[Build Bot]

Comment on attachment 189783 [details]
Patch

Attachment 189783 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16713820

[Alexander Pavlov (apavlov)]

Created attachment 190002 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 190002 [details]
Patch

Cleared review? from attachment 190002 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Joseph Pecoraro]

I'm looking into this.

[Joseph Pecoraro]

Reopening, as this is still useful for some cases like refreshing the sidebar when a StyleSheet is disabled/enabled, added/removed.

That said, it will also be useful for the frontend to have stylesheet header information (such as offset line/column) to accurately generate source code location links for things like a CSSRule inside a <style> in the main document.

[Joseph Pecoraro]

Adjusting status.

[Joseph Pecoraro]

<rdar://problem/13213680>

[Joseph Pecoraro]

Created attachment 259803 [details]
[PATCH] Proposed Fix

I think I need to write one more test for handling of removing a sub-frame/document, but this is a good start building off of the previous patches.

[Build Bot]

Comment on attachment 259803 [details]
[PATCH] Proposed Fix

Attachment 259803 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/101864

New failing tests:
inspector/css/stylesheet-events-imports.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/css/stylesheet-events-basic.html

[Build Bot]

Created attachment 259808 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 259803 [details]
[PATCH] Proposed Fix

Attachment 259803 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/101867

New failing tests:
inspector/css/stylesheet-events-imports.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/css/stylesheet-events-basic.html

[Build Bot]

Created attachment 259809 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Joseph Pecoraro]

Comment on attachment 259803 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259803&action=review

That said I don't understand the other test failures at the moment.

> LayoutTests/inspector/css/stylesheet-events-basic.html:168
> +        extraEnableDisableTests: [
> +            {title: "DisableExternalStyleSheetViaMediaAttribute", expression: "disableExternalStyleSheetViaMediaAttribute()"},
> +            {title: "EnableExternalStyleSheetViaMediaAttribute", expression: "enableExternalStyleSheetViaMediaAttribute()"},
> +        ],

These depend on a bug not yet fixed (patch is out for review):
<https://webkit.org/b/148392> Page does not update when <link> media attribute changes to no longer apply to page

> LayoutTests/inspector/css/stylesheet-events-inspector-stylesheet-expected.txt:9
> +LOG: CSS.styleSheetAdded {"styleSheetId":"1","origin":"inspector","disabled":false,"sourceURL":"file:///Users/pecoraro/Code/safari/OpenSource/LayoutTests/inspector/css/stylesheet-events-inspector-stylesheet.html","title":"","frameId":"0.1","isInline":false,"startLine":0,"startColumn":0}

Oops, this should not be here.

[Joseph Pecoraro]

(In reply to comment #43)
> Comment on attachment 259803 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259803&action=review
> 
> That said I don't understand the other test failures at the moment.

Ahh, this appears to be a late change I made.

[Joseph Pecoraro]

Created attachment 259819 [details]
[PATCH] Proposed Fix

Now with tests for subframes.

[Timothy Hatcher]

Comment on attachment 259819 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259819&action=review

> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:301
> +                let origin = "origin" in styleSheetInfo ? styleSheetInfo.origin : "regular";

Should this be an enum? Is it exposing a protocol enum as origin on WebInspector.CSSStyleSheet?

[Joseph Pecoraro]

Comment on attachment 259819 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259819&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/CSSStyleManager.js:301
>> +                let origin = "origin" in styleSheetInfo ? styleSheetInfo.origin : "regular";
> 
> Should this be an enum? Is it exposing a protocol enum as origin on WebInspector.CSSStyleSheet?

Yeah, I can make a CSSStyleSheet.Type enum (probably replacing WebInspector.CSSRule.Type, since they are the same).

[Blaze Burg]

Comment on attachment 259819 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259819&action=review

r=me on tests, just some readability suggestions.

> LayoutTests/inspector/css/stylesheet-events-basic.html:26
> +function addInlineStyle() {

I would have expected this to be style="...", do you test that case somewhere?

> LayoutTests/inspector/css/stylesheet-events-basic.html:51
> +    InspectorTest.dumpActivityToSystemConsole = true;

Oops

> LayoutTests/inspector/css/stylesheet-events-basic.html:65
> +        let {title, addExpression, removeExpression, checkStyleSheetAddedCallback, checkStyleSheetRemovedCallback, extraEnableDisableTests} = args;

I wish keyword args were a thing.. :)

> LayoutTests/inspector/css/stylesheet-events-basic.html:76
> +                    if (checkStyleSheetRemovedCallback) checkStyleSheetAddedCallback(addedStyleSheet);

Newline

> LayoutTests/inspector/css/stylesheet-events-basic.html:88
> +                    InspectorTest.assert(event.data.styleSheet instanceof WebInspector.CSSStyleSheet);

(Multiple places) Needs a message, else it will log ASSERT: (nothing)

> LayoutTests/inspector/css/stylesheet-events-basic.html:145
> +                    if (checkStyleSheetRemovedCallback) checkStyleSheetRemovedCallback(event.data.styleSheet);

Newline.

> LayoutTests/inspector/css/stylesheet-events-basic.html:163
> +        checkStyleSheetAddedCallback: (styleSheet) => { InspectorTest.log(sanitizeURL(styleSheet.url)); },

I recommend describing what it is that's being dumped and used as reference output, so it's obvious whether rebaselining is okay or not.

> LayoutTests/inspector/css/stylesheet-events-imports.html:61
> +        description: "Disable the top stylesheet containing imports will remove all.",

Disabling ... remove all stylesheets

> LayoutTests/inspector/css/stylesheet-events-imports.html:64
> +                events.map((e) => sanitizeURL(e.data.styleSheet.url)).sort().forEach((url) => { InspectorTest.log("StyleSheetRemoved: " + url); });

(Multiple places)

Wow, this is cool. However, I would wrap at the chained .sort() to avoid super long lines, as is common practice in d3 code:

events.map((e) => sanitizeURL(e.data.styleSheet.url))
    .sort()
    .forEach((url) => { InspectorTest.log("StyleSheetRemoved: " + url); });

> LayoutTests/inspector/css/stylesheet-events-imports.html:74
> +        description: "Enable the top stylesheet containing imports will add all.",

Enabling ... add all stylesheets

> LayoutTests/inspector/css/stylesheet-events-inspector-stylesheet.html:10
> +    let suite = InspectorTest.createAsyncSuite("CSS.styleSheetAdded for Inspector stylesheets");

I try to make the suite names camel case too (often based on filename), so it's easy to grep for them.

> LayoutTests/inspector/css/stylesheet-events-inspector-stylesheet.html:15
> +        name: "CheckStyleSheets",

"EnsureNoStylesheets" or "CheckNoStylesheets"

> LayoutTests/inspector/css/stylesheet-events-inspector-stylesheet.html:30
> +                InspectorTest.expectThat(event.data.styleSheet.origin === CSSAgent.StyleSheetOrigin.Inspector, "StyleSheet origin is inspector.");

is -> should be, I tend to put values like 'inspector' in quotes

[Joseph Pecoraro]

Comment on attachment 259819 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=259819&action=review

>> LayoutTests/inspector/css/stylesheet-events-basic.html:26
>> +function addInlineStyle() {
> 
> I would have expected this to be style="...", do you test that case somewhere?

Inline style="..." doesn't actually create a StyleSheet in this sense. The backend and frontend masquerade that as a InlineInspectorStyleSheet / CSSStyleSheet, but there is no frontend<->backend communication in this regard.

I'll rename this to be clearer, "addStyleTag".

>> LayoutTests/inspector/css/stylesheet-events-basic.html:51
>> +    InspectorTest.dumpActivityToSystemConsole = true;
> 
> Oops

Yes, sorry =), I had removed these from all the tests locally.

>> LayoutTests/inspector/css/stylesheet-events-imports.html:64
>> +                events.map((e) => sanitizeURL(e.data.styleSheet.url)).sort().forEach((url) => { InspectorTest.log("StyleSheetRemoved: " + url); });
> 
> (Multiple places)
> 
> Wow, this is cool. However, I would wrap at the chained .sort() to avoid super long lines, as is common practice in d3 code:
> 
> events.map((e) => sanitizeURL(e.data.styleSheet.url))
>     .sort()
>     .forEach((url) => { InspectorTest.log("StyleSheetRemoved: " + url); });

Yeah, i dislike that style. Perhaps I should start using it more often though, so I'll start.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/189002