Bug 47339 – Web Inspector: Implement property toggling in InspectorCSSAgent

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

[Alexander Pavlov (apavlov)]

CSS style property disablement should be implemented in the CSS agent prior to enabling it.

[Alexander Pavlov (apavlov)]

Created attachment 70858 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

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.

[Alexander Pavlov (apavlov)]

Created attachment 70860 [details]
[PATCH] Style fixed

[Pavel Feldman]

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?

[Alexander Pavlov (apavlov)]

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

[Pavel Feldman]

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

[Alexander Pavlov (apavlov)]

Created attachment 71158 [details]
[PATCH] Implemented another approach suggested by pfeldman

[Early Warning System Bot]

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

[Pavel Feldman]

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?

[Eric Seidel (no email)]

Attachment 71158 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4489038

[WebKit Review Bot]

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

[Alexander Pavlov (apavlov)]

Created attachment 71173 [details]
[PATCH] Removed mixing iterator

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

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.

[WebKit Review Bot]

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.

[Alexander Pavlov (apavlov)]

Created attachment 71264 [details]
[PATCH] Style fixed

[Pavel Feldman]

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

[Alexander Pavlov (apavlov)]

Created attachment 71539 [details]
[PATCH] Rewrote the API according to the latest chat with pfeldman

[Alexander Pavlov (apavlov)]

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

[Pavel Feldman]

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.

[Alexander Pavlov (apavlov)]

Created attachment 71558 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

Comment on attachment 71558 [details]
[PATCH] Comments addressed

Wrong augmented patch

[Alexander Pavlov (apavlov)]

Created attachment 71560 [details]
[PATCH] The right fix for the comments

[Pavel Feldman]

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

[Alexander Pavlov (apavlov)]

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

[WebKit Review Bot]

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