[webkit-reviews] review canceled: [Bug 47339] Web Inspector: Implement property toggling in InspectorCSSAgent : [Attachment 71264] [PATCH] Style fixed

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has canceled Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 47339: Web Inspector: Implement property toggling in InspectorCSSAgent
https://bugs.webkit.org/show_bug.cgi?id=47339

Attachment 71264: [PATCH] Style fixed
https://bugs.webkit.org/attachment.cgi?id=71264&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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