[webkit-reviews] review granted: [Bug 57435] Web Inspector: document CSS agent. : [Attachment 87561] [PATCH] Suggested solution

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 57435: Web Inspector: document CSS agent.
https://bugs.webkit.org/show_bug.cgi?id=57435

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

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