[webkit-reviews] review denied: [Bug 47339] Web Inspector: Implement property toggling in InspectorCSSAgent : [Attachment 71539] [PATCH] Rewrote the API according to the latest chat with pfeldman

[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 71539: [PATCH] Rewrote the API according to the latest chat with
pfeldman
https://bugs.webkit.org/attachment.cgi?id=71539&action=review

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