[webkit-reviews] review denied: [Bug 47339] Web Inspector: Implement InspectorCSSAgent::toggleProperty2() : [Attachment 70860] [PATCH] Style fixed

[bugzilla-daemon at webkit.org]

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

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

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