Bug 62294

Summary: Web Inspector: Implement content history tracking for inline CSS stylesheets (including "via inspector" ones)
Product: WebKit Reporter: Alexander Pavlov (apavlov) <apavlov>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, burg, bweinstein, dglazkov, joepeck, keishi, loislo, me, pfeldman, pmuellr, rik, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Suggested solution
pfeldman: review-, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
[PATCH] Comments addressed
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02
none
[PATCH] Fixed ChangeLog entry webkit.review.bot: commit-queue-

Description Alexander Pavlov (apavlov) 2011-06-08 09:39:25 PDT
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.
Comment 1 Alexander Pavlov (apavlov) 2011-07-12 09:09:16 PDT
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).
Comment 2 WebKit Review Bot 2011-07-12 09:27:06 PDT
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
Comment 3 WebKit Review Bot 2011-07-12 09:27:10 PDT
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
Comment 4 Adam Barth 2011-07-12 10:16:50 PDT
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
Comment 5 Pavel Feldman 2011-07-13 23:41:39 PDT
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.
Comment 6 Alexander Pavlov (apavlov) 2011-07-14 09:27:14 PDT
(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.
Comment 7 Alexander Pavlov (apavlov) 2011-07-14 10:02:03 PDT
Created attachment 100820 [details]
[PATCH] Comments addressed
Comment 8 WebKit Review Bot 2011-07-14 12:58:17 PDT
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
Comment 9 WebKit Review Bot 2011-07-14 12:58:30 PDT
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
Comment 10 Alexander Pavlov (apavlov) 2011-07-18 03:04:22 PDT
Created attachment 101142 [details]
[PATCH] Fixed ChangeLog entry
Comment 11 WebKit Review Bot 2011-07-18 03:30:04 PDT
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
Comment 12 Yury Semikhatsky 2012-01-19 05:34:44 PST
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.
Comment 13 Nikita Vasilyev 2012-03-22 03:18:14 PDT
(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?