WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
57435
Web Inspector: document CSS agent.
https://bugs.webkit.org/show_bug.cgi?id=57435
Summary
Web Inspector: document CSS agent.
Alexander Pavlov (apavlov)
Reported
2011-03-30 02:34:06 PDT
Patch to follow.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alexander Pavlov (apavlov)
Comment 1
2011-03-30 10:19:13 PDT
Created
attachment 87561
[details]
[PATCH] Suggested solution
Pavel Feldman
Comment 2
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.
Alexander Pavlov (apavlov)
Comment 3
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.
Alexander Pavlov (apavlov)
Comment 4
2011-03-31 06:05:25 PDT
Created
attachment 87710
[details]
[PATCH] Comments addressed
Early Warning System Bot
Comment 5
2011-03-31 06:10:58 PDT
Attachment 87710
[details]
did not build on qt: Build output:
http://queues.webkit.org/results/8306317
WebKit Review Bot
Comment 6
2011-03-31 06:15:46 PDT
Attachment 87710
[details]
did not build on chromium: Build output:
http://queues.webkit.org/results/8315138
Alexander Pavlov (apavlov)
Comment 7
2011-03-31 06:35:35 PDT
Created
attachment 87715
[details]
[PATCH] Fixed patch
Alexander Pavlov (apavlov)
Comment 8
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
Alexander Pavlov (apavlov)
Comment 9
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug