Bug 57435

Created attachment 87561 [details] [PATCH] Suggested solution

Comment on attachment 87561 [details] [PATCH] Suggested solution View in context: https://bugs.webkit.org/attachment.cgi?id=87561&action=review Great job, thanks. A bunch of nits that can be addressed in different bugs. > Source/WebCore/inspector/Inspector.json:917 > "domain": "CSS", Is this the wrong diff? It changes description that is not there on ToT. > Source/WebCore/inspector/Inspector.json:979 > + "id": "CSSStyleSheetInfo", CSSStyleSheetHeader ? > Source/WebCore/inspector/Inspector.json:990 > + "id": "CSSStyleSheet", CSSStyleSheetBody ? > Source/WebCore/inspector/Inspector.json:1007 > + "origin": { "type": "string", "description": "The parent stylesheet type: \"user\" for user stylesheets, \"user-agent\" for user-agent stylesheets, \"inspector\" for stylesheets created by the inspector (i.e. those holding new rules created with <code>addRule()</code>), \"\" for regular stylesheets."}, Please put // FIXME right into the description - I do that to remember that we need to define enumerable types in the documentation. > Source/WebCore/inspector/Inspector.json:1040 > + "startOffset": { "type": "integer", "optional": true, "description": "The style declaration start offset in the enclosing stylesheet (if available), inclusive." }, I think this one can be inlined (offsets should be a part of the CSSStyle description in a "range" property). Dimensions can also be there for now. > Source/WebCore/inspector/Inspector.json:1054 > + "status": { "type": "string", "optional": true, "description": "The property status: \"active\" (implied if absent) if the property is effective in the style, \"inactive\" if the property is overridden by a same-named property in this style later on, \"disabled\" if the property is disabled by the user, \"style\" if the property is reported by the browser rather than by the CSS source parser." }, FIXME > Source/WebCore/inspector/Inspector.json:1056 > + "startOffset": { "type": "integer", "optional": true, "description": "The entire property start offset in the enclosing style declaration (if available), inclusive." }, You can make everything have "range": SourceRance instead.

(In reply to comment #2) > (From update of attachment 87561 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87561&action=review > > Great job, thanks. A bunch of nits that can be addressed in different bugs. I'm going to land a variation of this patch with all the nits fixed after bug 57538 patch is landed (which is intended to address the "range" thing and inline the width/height properties in the CSSStyle object). > > Source/WebCore/inspector/Inspector.json:917 > > "domain": "CSS", > > Is this the wrong diff? It changes description that is not there on ToT. Yes, sorry, I inadvertently had two commits in my branch, of which only the topmost one got into the patch. I'm attaching the correct fixed patch now. > > Source/WebCore/inspector/Inspector.json:979 > > + "id": "CSSStyleSheetInfo", > > CSSStyleSheetHeader ? Fixed. > > Source/WebCore/inspector/Inspector.json:990 > > + "id": "CSSStyleSheet", > > CSSStyleSheetBody ? Fixed. > > Source/WebCore/inspector/Inspector.json:1007 > > + "origin": { "type": "string", "description": "The parent stylesheet type: \"user\" for user stylesheets, \"user-agent\" for user-agent stylesheets, \"inspector\" for stylesheets created by the inspector (i.e. those holding new rules created with <code>addRule()</code>), \"\" for regular stylesheets."}, > > Please put // FIXME right into the description - I do that to remember that we need to define enumerable types in the documentation. Fixed. > > Source/WebCore/inspector/Inspector.json:1040 > > + "startOffset": { "type": "integer", "optional": true, "description": "The style declaration start offset in the enclosing stylesheet (if available), inclusive." }, > > I think this one can be inlined (offsets should be a part of the CSSStyle description in a "range" property). Dimensions can also be there for now. Fixed. Dimensions are not used by our code, yet I will retain them for now. > > Source/WebCore/inspector/Inspector.json:1054 > > + "status": { "type": "string", "optional": true, "description": "The property status: \"active\" (implied if absent) if the property is effective in the style, \"inactive\" if the property is overridden by a same-named property in this style later on, \"disabled\" if the property is disabled by the user, \"style\" if the property is reported by the browser rather than by the CSS source parser." }, > > FIXME Fixed. > > Source/WebCore/inspector/Inspector.json:1056 > > + "startOffset": { "type": "integer", "optional": true, "description": "The entire property start offset in the enclosing style declaration (if available), inclusive." }, > > You can make everything have "range": SourceRance instead. Fixed. Needs bug 57538 patch landed to reflect the reality.

Created attachment 87710 [details] [PATCH] Comments addressed

Attachment 87710 [details] did not build on qt: Build output: http://queues.webkit.org/results/8306317

Attachment 87710 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8315138

Created attachment 87715 [details] [PATCH] Fixed patch

Committing to http://svn.webkit.org/repository/webkit/trunk ... M Source/WebCore/ChangeLog M Source/WebCore/inspector/Inspector.json Committed r82583

Committing to http://svn.webkit.org/repository/webkit/trunk ... M Source/WebCore/ChangeLog M Source/WebCore/inspector/Inspector.json Committed r82583