Bug 57435 - Web Inspector: document CSS agent.
Summary: Web Inspector: document CSS agent.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on: 57452
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-30 02:34 PDT by Alexander Pavlov (apavlov)
Modified: 2011-03-31 08:59 PDT (History)
12 users (show)

See Also:


Attachments
[PATCH] Suggested solution (17.66 KB, patch)
2011-03-30 10:19 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff
[PATCH] Comments addressed (19.21 KB, patch)
2011-03-31 06:05 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Fixed patch (18.48 KB, patch)
2011-03-31 06:35 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-03-30 02:34:06 PDT
Patch to follow.
Comment 1 Alexander Pavlov (apavlov) 2011-03-30 10:19:13 PDT
Created attachment 87561 [details]
[PATCH] Suggested solution
Comment 2 Pavel Feldman 2011-03-30 22:33:58 PDT
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.
Comment 3 Alexander Pavlov (apavlov) 2011-03-31 06:04:55 PDT
(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.
Comment 4 Alexander Pavlov (apavlov) 2011-03-31 06:05:25 PDT
Created attachment 87710 [details]
[PATCH] Comments addressed
Comment 5 Early Warning System Bot 2011-03-31 06:10:58 PDT
Attachment 87710 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8306317
Comment 6 WebKit Review Bot 2011-03-31 06:15:46 PDT
Attachment 87710 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8315138
Comment 7 Alexander Pavlov (apavlov) 2011-03-31 06:35:35 PDT
Created attachment 87715 [details]
[PATCH] Fixed patch
Comment 8 Alexander Pavlov (apavlov) 2011-03-31 08:58:52 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       Source/WebCore/ChangeLog
        M       Source/WebCore/inspector/Inspector.json
Committed r82583
Comment 9 Alexander Pavlov (apavlov) 2011-03-31 08:59:26 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       Source/WebCore/ChangeLog
        M       Source/WebCore/inspector/Inspector.json
Committed r82583