[webkit-reviews] review denied: [Bug 47339] Web Inspector: Implement property toggling in InspectorCSSAgent : [Attachment 71173] [PATCH] Removed mixing iterator

[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 property toggling in InspectorCSSAgent
https://bugs.webkit.org/show_bug.cgi?id=47339

Attachment 71173: [PATCH] Removed mixing iterator
https://bugs.webkit.org/attachment.cgi?id=71173&action=review

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