Bug 105828

Created attachment 180883 [details] Patch

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

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

(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).

Created attachment 180915 [details] Patch for 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

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

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

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

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.)?

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

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

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.

Created attachment 182125 [details] Patch

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

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

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 ..

Created attachment 182332 [details] Patch

Created attachment 184736 [details] Rebased patch

Created attachment 186222 [details] Rebased patch

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

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?

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.

Created attachment 187074 [details] Patch

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

Created attachment 188342 [details] Test added

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

Re-opened since this is blocked by bug 110225

Created attachment 189783 [details] Patch

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

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

Created attachment 190002 [details] Patch

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).

I'm looking into this.

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.

Adjusting status.

<rdar://problem/13213680>

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.

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

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

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

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

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.

(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.

Created attachment 259819 [details] [PATCH] Proposed Fix Now with tests for subframes.

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?

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).

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

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.