Bug 47339 - Web Inspector: Implement property toggling in InspectorCSSAgent
Summary: Web Inspector: Implement property toggling in InspectorCSSAgent
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:
Blocks:
 
Reported: 2010-10-07 04:49 PDT by Alexander Pavlov (apavlov)
Modified: 2010-10-22 08:57 PDT (History)
14 users (show)

See Also:


Attachments
[PATCH] Suggested solution (34.02 KB, patch)
2010-10-15 05:50 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Style fixed (34.01 KB, patch)
2010-10-15 06:04 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Implemented another approach suggested by pfeldman (35.17 KB, patch)
2010-10-19 05:52 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Removed mixing iterator (33.09 KB, patch)
2010-10-19 08:16 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Introduced InspectorStyle (40.55 KB, patch)
2010-10-19 10:52 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Style fixed (40.55 KB, patch)
2010-10-20 02:06 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Rewrote the API according to the latest chat with pfeldman (56.31 KB, patch)
2010-10-22 03:03 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed (92.03 KB, patch)
2010-10-22 06:22 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] The right fix for the comments (60.51 KB, patch)
2010-10-22 06:31 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
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) 2010-10-07 04:49:13 PDT
CSS style property disablement should be implemented in the CSS agent prior to enabling it.
Comment 1 Alexander Pavlov (apavlov) 2010-10-15 05:50:54 PDT
Created attachment 70858 [details]
[PATCH] Suggested solution
Comment 2 WebKit Review Bot 2010-10-15 05:52:11 PDT
Attachment 70858 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/InspectorStyleSheet.cpp:117:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
WebCore/inspector/InspectorStyleSheet.cpp:303:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexander Pavlov (apavlov) 2010-10-15 06:04:40 PDT
Created attachment 70860 [details]
[PATCH] Style fixed
Comment 4 Pavel Feldman 2010-10-17 01:02:58 PDT
Comment on attachment 70860 [details]
[PATCH] Style fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=70860&action=review

> WebCore/inspector/InspectorCSSAgent.cpp:93
> +//    parentStyleSheet : { id, href }

Nits:
- having sourceURL instead of href would be consistent with sourceLine
- we should either have parentStyleSheet id in all the rules or encode it as a part of ruleId, but not both.

> WebCore/inspector/InspectorCSSAgent.cpp:94
> +//    sourceLine       : <string>

Nit: should be <number>

> WebCore/inspector/InspectorCSSAgent.cpp:101
>  //    href           : <string>

sourceURL for consistency?

> WebCore/inspector/InspectorStyleSheet.cpp:125
> +FrontendPropertyIterator& FrontendPropertyIterator::operator++()

ditto

> WebCore/inspector/InspectorStyleSheet.cpp:166
> +    // A disabled property should be returned if:

This is very, very complex (proved with the long comment). Converting iterator into the Style object would probably simplify it a lot.

> WebCore/inspector/InspectorStyleSheet.cpp:297
> +    DisabledPropertyStore* store = disabledPropertyStoreForStyle(style, disable);

Disabled property store could live inside InspectorStyle object.

> WebCore/inspector/InspectorStyleSheet.cpp:395
> +    FrontendPropertyIterator it = allProperties(ruleOrStyleId(style));

I still find ruleOrStyleId notion wrong.  Can we be more explicit? I.e. keep style ids and rule ids separate?

> WebCore/inspector/InspectorStyleSheet.h:51
> +typedef std::pair<CSSPropertySourceData, bool> SourceAwareProperty;

So this bool is "hasSource"? What do we need this type for? Can we get away with FrontendPropertyData? Should we rename FrontendPropertyData to InspectorStyleProperty?

> WebCore/inspector/InspectorStyleSheet.h:108
> +    FrontendPropertyIterator allProperties(const String& styleId);

This interface gets too involved with the style content. Should we introduce InspectorStyle abstraction that would manage toggling and iterating for us?
Comment 5 Alexander Pavlov (apavlov) 2010-10-18 04:47:27 PDT
(In reply to comment #4)
> (From update of attachment 70860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70860&action=review

Since your comments involve changes to API structures (Web Inspector Protocol), I suggest that you also review the document, which I have changed to follow your comments.

> > WebCore/inspector/InspectorCSSAgent.cpp:93
> > +//    parentStyleSheet : { id, href }
> 
> Nits:
> - having sourceURL instead of href would be consistent with sourceLine
> - we should either have parentStyleSheet id in all the rules or encode it as a part of ruleId, but not both.

Done.

> > WebCore/inspector/InspectorCSSAgent.cpp:94
> > +//    sourceLine       : <string>
> 
> Nit: should be <number>

Done.

> > WebCore/inspector/InspectorCSSAgent.cpp:101
> >  //    href           : <string>
> 
> sourceURL for consistency?

Removed according to the first comment.

> > WebCore/inspector/InspectorStyleSheet.cpp:125
> > +FrontendPropertyIterator& FrontendPropertyIterator::operator++()
> 
> ditto

What is this related to?

> > WebCore/inspector/InspectorStyleSheet.cpp:166
> > +    // A disabled property should be returned if:
> 
> This is very, very complex (proved with the long comment). Converting iterator into the Style object would probably simplify it a lot.

No it wouldn't. This algorithm joins disabled properties of a single style with live properties of the same style in order to produce the final "cssProperties" InspectorObject for the frontend. Is there any difference with regard to the location of this code (also see a minor comment at the end)?

This complexity is due to the loose constraints that we impose on the property disablement and style editing, to keep the disabled properties as close to their original positions in the aggregated style as possible, meaning that we need to keep this order when producing the final property object vector (array). This complexity is encapsulated into the iterator, so I do not see how extracting InspectorStyle will help reduce this complexity.

> > WebCore/inspector/InspectorStyleSheet.cpp:297
> > +    DisabledPropertyStore* store = disabledPropertyStoreForStyle(style, disable);
> 
> Disabled property store could live inside InspectorStyle object.

Do you suggest holding a map CSSStyleDeclaration* -> InspectorStyle?

> > WebCore/inspector/InspectorStyleSheet.cpp:395
> > +    FrontendPropertyIterator it = allProperties(ruleOrStyleId(style));
> 
> I still find ruleOrStyleId notion wrong.  Can we be more explicit? I.e. keep style ids and rule ids separate?

It is an implementation detail, as you can see. We never address styles using ruleId or rules using styleId. We also never store them in the backend. What is your particular concern?

> > WebCore/inspector/InspectorStyleSheet.h:51
> > +typedef std::pair<CSSPropertySourceData, bool> SourceAwareProperty;
> 
> So this bool is "hasSource"? What do we need this type for? Can we get away with FrontendPropertyData?

Done.

> Should we rename FrontendPropertyData to InspectorStyleProperty?

Done.

> > WebCore/inspector/InspectorStyleSheet.h:108
> > +    FrontendPropertyIterator allProperties(const String& styleId);
> 
> This interface gets too involved with the style content. Should we introduce InspectorStyle abstraction that would manage toggling and iterating for us?

Iterating is done by the InspectorFrontendPropertyIterator. This is a non-trivial operation (akin to a "mixing iterator"), and I do not see why you don't like it, given that we never need to have a distinct vector of all the style properties at hand.

Will submit the patch once the remaining issues are resolved.
Comment 6 Pavel Feldman 2010-10-18 05:03:36 PDT
> > This is very, very complex (proved with the long comment). Converting iterator into the Style object would probably simplify it a lot.
> 
> No it wouldn't. This algorithm joins disabled properties of a single style with live properties of the same style in order to produce the final "cssProperties" InspectorObject for the frontend. Is there any difference with regard to the location of this code (also see a minor comment at the end)?
> 
> This complexity is due to the loose constraints that we impose on the property disablement and style editing, to keep the disabled properties as close to their original positions in the aggregated style as possible, meaning that we need to keep this order when producing the final property object vector (array). This complexity is encapsulated into the iterator, so I do not see how extracting InspectorStyle will help reduce this complexity.
> 

I think that stateful style abstraction will help you in getting rid of this complexity. Also, "InspectorFrontendPropertyIterator" suggests that it is a part of front-end, while it is not.
Comment 7 Alexander Pavlov (apavlov) 2010-10-19 05:52:05 PDT
Created attachment 71158 [details]
[PATCH] Implemented another approach suggested by pfeldman
Comment 8 Early Warning System Bot 2010-10-19 06:41:38 PDT
Attachment 71158 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4500019
Comment 9 Pavel Feldman 2010-10-19 06:55:07 PDT
Comment on attachment 71158 [details]
[PATCH] Implemented another approach suggested by pfeldman

View in context: https://bugs.webkit.org/attachment.cgi?id=71158&action=review

> WebCore/inspector/InspectorStyleSheet.h:78
> +    const Vector<InspectorStyleProperty> m_liveProperties;

I still don't get the iterator thing. What is it actually used for? Wouldn't it be simpler to have a class that would build front-end objects for merged properties in the loop?
Comment 10 Eric Seidel (no email) 2010-10-19 07:17:21 PDT
Attachment 71158 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4489038
Comment 11 WebKit Review Bot 2010-10-19 07:21:14 PDT
Attachment 71158 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4447076
Comment 12 Alexander Pavlov (apavlov) 2010-10-19 08:16:22 PDT
Created attachment 71173 [details]
[PATCH] Removed mixing iterator
Comment 13 Pavel Feldman 2010-10-19 08:42:50 PDT
Comment on attachment 71173 [details]
[PATCH] Removed mixing iterator

View in context: https://bugs.webkit.org/attachment.cgi?id=71173&action=review

> WebCore/inspector/InspectorCSSAgent.cpp:180
> +void InspectorCSSAgent::populateInspectorFrontendProperties(CSSStyleDeclaration* style, Vector<CSSPropertySourceData>* sourcePropertyData, DisabledPropertyStore disabledProperties, Vector<InspectorStyleProperty>* result)

ditto.

> WebCore/inspector/InspectorCSSAgent.cpp:363
> +void InspectorCSSAgent::setStyleData2(const String& fullStyleId, RefPtr<InspectorObject> styleDataObject, RefPtr<InspectorValue>* result)

I think we should pass text and disabled property list in separate parameters. Disabled property list is optional.

> WebCore/inspector/InspectorCSSAgent.h:59
> +    static PassRefPtr<InspectorObject> buildObjectForStyle(CSSStyleDeclaration*, const String& fullStyleId, Vector<InspectorStyleProperty>& properties, CSSStyleSourceData* = 0);

I am now lost what this method does: you pass live CSSStyleDeclaration to it, style id, a set of properties and style source data and expect a front-end object to be built. But I don't know where to take all of these items. Should there be a clean InspectorStyle abstraction that would contain what it should and be able to build front-end object for itself? It should either be private or have friendly interface.

> WebCore/inspector/InspectorStyleSheet.h:63
> +    CSSPropertySourceData property;

InspectorStyleProperty::property looks weird. Should it be called "sourceData"?

> WebCore/inspector/InspectorStyleSheet.h:68
> +typedef RefPtr<InspectorArray> DisabledPropertyStore;

You should not use messaging classes for storing data.
Comment 14 Alexander Pavlov (apavlov) 2010-10-19 10:52:41 PDT
Created attachment 71184 [details]
[PATCH] Introduced InspectorStyle

Let's stick to the implemented message format and change it later if need be. The code is not used at the moment.
Comment 15 WebKit Review Bot 2010-10-19 10:54:41 PDT
Attachment 71184 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/InspectorStyleSheet.cpp:101:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Alexander Pavlov (apavlov) 2010-10-20 02:06:00 PDT
Created attachment 71264 [details]
[PATCH] Style fixed
Comment 17 Pavel Feldman 2010-10-20 03:22:29 PDT
Comment on attachment 71264 [details]
[PATCH] Style fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=71264&action=review

> WebCore/inspector/InspectorStyleSheet.h:63
> +    CSSPropertySourceData property;

should this be called sourceData? property in property sounds confusing.

> WebCore/inspector/InspectorStyleSheet.h:68
> +struct DisabledProperty {

As we agreed, lets try using InspectorStyleProperty in place of this one and hence save the markup.

> WebCore/inspector/InspectorStyleSheet.h:86
> +        : m_id(fullStyleId)

Nit: you sometimes use the fullStyleId and sometimes id. In this case it is clear they are the same thing.

> WebCore/inspector/InspectorStyleSheet.h:96
> +    InspectorStyleProperty disabledInspectorStyleProperty(unsigned disabledIndex) const;

This one will become especially trivial once you store disabled properties in this class.

> WebCore/inspector/InspectorStyleSheet.h:129
> +    bool setStyle(const String& styleId, InspectorObject* styleObject);

We should somehow ease understanding of these methods. Like in this case, it is not clear from what kind of object style is being set. But I guess we won't need it anyways.

> WebCore/inspector/InspectorStyleSheet.h:140
> +    virtual void setDisabledPropertiesForStyle(CSSStyleDeclaration*, PassRefPtr<InspectorArray> disabledProperties);

I think you should access these via InspectorStyle

> WebCore/inspector/InspectorStyleSheet.h:141
> +    virtual DisabledPropertyStore* disabledPropertyStoreForStyle(CSSStyleDeclaration*);

ditto

> WebCore/inspector/InspectorStyleSheet.h:184
> +    virtual void setDisabledPropertiesForStyle(CSSStyleDeclaration* style, PassRefPtr<InspectorArray> disabledProperties);

I think you should access these via InspectorStyle

> WebCore/inspector/InspectorStyleSheet.h:185
> +    virtual DisabledPropertyStore* disabledPropertyStoreForStyle(CSSStyleDeclaration* style, bool) { ASSERT_UNUSED(style, style == inlineStyle()); return &m_disabledProperties; }

ditto
Comment 18 Alexander Pavlov (apavlov) 2010-10-22 03:03:51 PDT
Created attachment 71539 [details]
[PATCH] Rewrote the API according to the latest chat with pfeldman
Comment 19 Alexander Pavlov (apavlov) 2010-10-22 03:05:43 PDT
(In reply to comment #17)
> (From update of attachment 71264 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71264&action=review
> 
> > WebCore/inspector/InspectorStyleSheet.h:63
> > +    CSSPropertySourceData property;
> 
> should this be called sourceData? property in property sounds confusing.

Correct, fixed.

> > WebCore/inspector/InspectorStyleSheet.h:68
> > +struct DisabledProperty {
> 
> As we agreed, lets try using InspectorStyleProperty in place of this one and hence save the markup.

Done.

> > WebCore/inspector/InspectorStyleSheet.h:86
> > +        : m_id(fullStyleId)
> 
> Nit: you sometimes use the fullStyleId and sometimes id. In this case it is clear they are the same thing.

Changed to m_fullId.

The rest of comments are obsolete due to the changes. The entire API has become much cleaner and more efficient wrt the disabled properties position retention.
Comment 20 Pavel Feldman 2010-10-22 03:58:04 PDT
Comment on attachment 71539 [details]
[PATCH] Rewrote the API according to the latest chat with pfeldman

View in context: https://bugs.webkit.org/attachment.cgi?id=71539&action=review

Looks much better now, almost ready for landing.

> WebCore/inspector/InspectorStyleSheet.cpp:662
> +        rememberInspectorStyle(styleForId(styleId), inspectorStyle.release());

You should release the style once it stores no toggled properties.

> WebCore/inspector/InspectorStyleSheet.h:52
> +class InspectorCompoundId {

Now that you have "parsed" id, class, I think you should operate it instead of strings in the InspectorStyleSheet and such. You should quickly parse the string that you get from the front-end and carry this thing since then.

> WebCore/inspector/InspectorStyleSheet.h:98
> +    static PassRefPtr<InspectorStyle> create(const String& fullStyleId, InspectorStyleSheet* parentStyleSheet)

Constructing this class in two different ways does not make sense to my. We should always assign to m_style, styleId / parentStylSheet can be undefined for r/o unbound styles.

> WebCore/inspector/InspectorStyleSheet.h:166
> +    void populateAllProperties(const String& styleId, Vector<InspectorStyleProperty>* result);

Please remove this.

> WebCore/inspector/InspectorStyleSheet.h:231
> +    virtual void rememberInspectorStyle(CSSStyleDeclaration* style, PassRefPtr<InspectorStyle> inspectorStyle) { }

It seems strange that you are passing both model and inspector styles. One should be sufficient.
Comment 21 Alexander Pavlov (apavlov) 2010-10-22 06:22:09 PDT
Created attachment 71558 [details]
[PATCH] Comments addressed
Comment 22 Alexander Pavlov (apavlov) 2010-10-22 06:27:27 PDT
Comment on attachment 71558 [details]
[PATCH] Comments addressed

Wrong augmented patch
Comment 23 Alexander Pavlov (apavlov) 2010-10-22 06:31:03 PDT
Created attachment 71560 [details]
[PATCH] The right fix for the comments
Comment 24 Pavel Feldman 2010-10-22 07:20:36 PDT
Comment on attachment 71560 [details]
[PATCH] The right fix for the comments

This now looks good. See single comment below. I'd suggest that we land it after this review and do a follow-up inspection in the source. The change has been up in the air for too long and it is fairly large.

> WebCore/inspector/InspectorStyleSheet.h:52
> +class InspectorCompoundId {

How about we rename it to InspectorStyleId and hardcode the style id structure? (I.e. stylesheet id and style id parts).
Comment 25 Alexander Pavlov (apavlov) 2010-10-22 08:07:11 PDT
Landed with the changes suggested by pfeldman.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       WebCore/ChangeLog
        M       WebCore/inspector/InspectorCSSAgent.cpp
        M       WebCore/inspector/InspectorCSSAgent.h
        M       WebCore/inspector/InspectorStyleSheet.cpp
        M       WebCore/inspector/InspectorStyleSheet.h
Committed r70306
Comment 26 WebKit Review Bot 2010-10-22 08:57:45 PDT
http://trac.webkit.org/changeset/70306 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
mathml/presentation/fenced-mi.xhtml
mathml/presentation/fenced.xhtml
mathml/presentation/fractions-vertical-alignment.xhtml
mathml/presentation/fractions.xhtml
mathml/presentation/mo.xhtml
mathml/presentation/over.xhtml
mathml/presentation/roots.xhtml
mathml/presentation/row-alignment.xhtml