Bug 62294 – Web Inspector: Implement content history tracking for inline CSS stylesheets (including "via inspector" ones)

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

[Alexander Pavlov (apavlov)]

Currently we can only track history of external CSS stylesheet modifications, but the same is desirable for inline stylesheets and stylesheets constructed by Inspector while adding new style rules.

[Alexander Pavlov (apavlov)]

Created attachment 100495 [details]
[PATCH] Suggested solution

The suggested solution does not update line numbers in CSSStyleRules added into inline stylesheets (since they were not parsed from the stylesheet text), forcing them to always be 0. We could always reparse the stylesheet text but this would cancel all the existing rule data (including disabled properties).

[WebKit Review Bot]

Comment on attachment 100495 [details]
[PATCH] Suggested solution

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

New failing tests:
http/tests/misc/slow-loading-mask.html
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-rl-selection.html
svg/transforms/text-with-mask-with-svg-transform.svg
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
inspector/styles/styles-computed-trace.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
inspector/styles/styles-source-lines-inline.html
inspector/styles/styles-iframe.html
inspector/styles/styles-history.html
fast/blockflow/japanese-lr-selection.html
inspector/styles/styles-source-lines.html

[WebKit Review Bot]

Created attachment 100498 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Sorry for the bot spam.  I understand the issue now.  These are the real failures:

Regressions: Unexpected text diff mismatch : (4)
  inspector/styles/styles-computed-trace.html = TEXT
  inspector/styles/styles-iframe.html = TEXT
  inspector/styles/styles-source-lines-inline.html = TEXT
  inspector/styles/styles-source-lines.html = TEXT

Regressions: Unexpected tests timed out : (1)
  inspector/styles/styles-history.html = TIMEOUT

[Pavel Feldman]

Comment on attachment 100495 [details]
[PATCH] Suggested solution

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

> Source/WebCore/inspector/Inspector.json:1176
> +                    { "name": "frameId", "type": "string", "optional": true, "description": "The stylesheet owner frame identifier."},

Why is frameId optional? Can this really happen?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:704
> +    if (!m_origin.length() || m_origin == "inspector")

Is there a constant for "inspector" ?

> Source/WebCore/inspector/front-end/CSSStyleModel.js:448
> +        return WebInspector.cssModel.modelResourceBinding.styleSheetURL(this.sourceURL, this.id.styleSheetId);

How can resourceURL change? This sounds counterintuitive.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:759
> +                        resource = new WebInspector.Resource(null, styleSheetURL, frame.loaderId);

You should use ResourceTreeModel's factory for resources here.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:774
> +                    resource.setInitialContent(event.data.content, true);

You should have done it where you created the resource, you don't need justCreated flag.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:-202
> -    _addResourceToFrame: function(resource)

I'd rather introduce public "createArtificialResource" that would capture creation and the code under majorChange && justCreated.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> (From update of attachment 100495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100495&action=review
> 
> > Source/WebCore/inspector/Inspector.json:1176
> > +                    { "name": "frameId", "type": "string", "optional": true, "description": "The stylesheet owner frame identifier."},
> 
> Why is frameId optional? Can this really happen?

I believe that a frame can be removed from a host document while the stylesheet request is in flight. As such, the stylesheet can still be around (e.g. due to being held by InspectorStyleSheet) but its document will have no owner frame.

> > Source/WebCore/inspector/InspectorStyleSheet.cpp:704
> > +    if (!m_origin.length() || m_origin == "inspector")
> 
> Is there a constant for "inspector" ?

There is none. I don't know how to express this idea in code best, since InspectorStyleSheet should be origin-agnostic (origins are computed by InspectorCSSAgent).

> > Source/WebCore/inspector/front-end/CSSStyleModel.js:448
> > +        return WebInspector.cssModel.modelResourceBinding.styleSheetURL(this.sourceURL, this.id.styleSheetId);
> 
> How can resourceURL change? This sounds counterintuitive.

Agree, the resourceURL name is slightly against the rules - it should be something like "displayURL"

> > Source/WebCore/inspector/front-end/CSSStyleModel.js:759
> > +                        resource = new WebInspector.Resource(null, styleSheetURL, frame.loaderId);
> 
> You should use ResourceTreeModel's factory for resources here.

Fair point, except that it didn't fully initialize the state. I will fix it.

> > Source/WebCore/inspector/front-end/CSSStyleModel.js:774
> > +                    resource.setInitialContent(event.data.content, true);
> 
> You should have done it where you created the resource, you don't need justCreated flag.

Could be. I wanted to aggregate the actions under a common if (majorChange) condition.

> > Source/WebCore/inspector/front-end/ResourceTreeModel.js:-202
> > -    _addResourceToFrame: function(resource)
> 
> I'd rather introduce public "createArtificialResource" that would capture creation and the code under majorChange && justCreated.

Good point. This is just a proof-of-concept.

[Alexander Pavlov (apavlov)]

Created attachment 100820 [details]
[PATCH] Comments addressed

[WebKit Review Bot]

Comment on attachment 100820 [details]
[PATCH] Comments addressed

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

New failing tests:
inspector/styles/css-live-edit.html
inspector/styles/styles-computed-trace.html
inspector/styles/styles-source-lines-inline.html
inspector/styles/styles-iframe.html
inspector/styles/styles-history.html
inspector/styles/styles-source-lines.html

[WebKit Review Bot]

Created attachment 100847 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Alexander Pavlov (apavlov)]

Created attachment 101142 [details]
[PATCH] Fixed ChangeLog entry

[WebKit Review Bot]

Comment on attachment 101142 [details]
[PATCH] Fixed ChangeLog entry

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

New failing tests:
inspector/styles/css-live-edit.html
inspector/styles/styles-computed-trace.html
inspector/styles/styles-source-lines-inline.html
inspector/styles/styles-iframe.html
inspector/styles/styles-history.html
inspector/styles/styles-source-lines.html

[Yury Semikhatsky]

Alternative solution discussed offline would be to generate virtual resources for each inline style sheet. This way we could edit them as regular linked resources and that approach doesn't suffer the line number problem.

[Nikita Vasilyev]

(In reply to comment #12)
> Alternative solution discussed offline would be to generate virtual resources for each inline style sheet. This way we could edit them as regular linked resources and that approach doesn't suffer the line number problem.

What event.type and event.url will fire onResourceContentCommitted on editing inline stylesheets and adding new style rules by Inspector?