[webkit-reviews] review denied: [Bug 62294] Web Inspector: Implement content history tracking for inline CSS stylesheets (including "via inspector" ones) : [Attachment 100495] [PATCH] Suggested solution

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 62294: Web Inspector: Implement content history tracking for inline CSS
stylesheets (including "via inspector" ones)
https://bugs.webkit.org/show_bug.cgi?id=62294

Attachment 100495: [PATCH] Suggested solution
https://bugs.webkit.org/attachment.cgi?id=100495&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.