Bug 57435 – Web Inspector: document CSS agent.

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

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 87561 [details]
[PATCH] Suggested solution

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

(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.

[Alexander Pavlov (apavlov)]

Created attachment 87710 [details]
[PATCH] Comments addressed

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[Alexander Pavlov (apavlov)]

Created attachment 87715 [details]
[PATCH] Fixed patch

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

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