[webkit-reviews] review denied: [Bug 47275] Web Inspector: inspector settings/properties/states management should be extracted into separate class. : [Attachment 69952] [patch] initial version.

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 47275: Web Inspector: inspector settings/properties/states management
should be extracted into separate class.
https://bugs.webkit.org/show_bug.cgi?id=47275

Attachment 69952: [patch] initial version.
https://bugs.webkit.org/attachment.cgi?id=69952&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69952&action=review

> WebCore/inspector/InspectorState.cpp:61
> +#define REGISTER_INSPECTOR_PROPERTY(name, value, stateName,
preferencesName)\
> +m_##name.init(this, value);\
> +m_inspectorCookie.set(&m_##name, "name");\
> +if (stateName)\
> +m_inspectorState.set(&m_##name, "stateName");\
> +if (preferencesName)\
> +m_inspectorPreferences.set(&m_##name, "preferenceName");
> +
> +InspectorState::InspectorState(InspectorClient* client)
> +    : m_client(client)
> +{
> +    REGISTER_INSPECTOR_PROPERTY(monitoringXHR, false, "monitoringXHR",
"xhrMonitor");
> +    REGISTER_INSPECTOR_PROPERTY(resourceTrackingEnabled, false,
"resourceTrackingEnabled", 0);
> +    REGISTER_INSPECTOR_PROPERTY(resourceTrackingAlwaysEnabled, false, 0,
"resourceTrackingEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(timelineProfilerEnabled, false,
"timelineProfilerEnabled", 0);
> +    REGISTER_INSPECTOR_PROPERTY(searchingForNode, false,
"searchingForNodeEnabled", 0);
> +    REGISTER_INSPECTOR_PROPERTY(profilerAlwaysEnabled, false, 0,
"profilerEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(frontendSettings, "", 0,
"frontendSettings");
> +    REGISTER_INSPECTOR_PROPERTY(debuggerAlwaysEnabled, false, 0,
"debuggerEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(lastActivePanel,
InspectorController::LastActivePanel, 0, "lastActivePanel");
> +    REGISTER_INSPECTOR_PROPERTY(inspectorStartsAttached, true, 0,
"InspectorStartsAttached");
> +    REGISTER_INSPECTOR_PROPERTY(inspectorAttachedHeight,
InspectorController::defaultAttachedHeight, 0, "inspectorAttachedHeight");

Consider declaring the list of properties as a macros accepting another macros.
This way you could generate getter/setter declarations and implementations that
would pass stateName and preferenceName directly into propertyUpdeted and get
rid of InspectorPropertyBase completely.

> WebCore/inspector/InspectorState.h:67
> +    static void save(InspectorObject* container, const String& name, bool
value) { container->setBoolean(name, value); }
> +    static void save(InspectorObject* container, const String& name, const
String& value) { container->setString(name, value); }
> +    static void save(InspectorObject* container, const String& name, long
value) { container->setNumber(name, value); }
> +    static void restore(const InspectorObject* const container, const
String& name, bool* value) { container->getBoolean(name, value); }
> +    static void restore(const InspectorObject* const container, const
String& name, String* value) { container->getString(name, value); }
> +    static void restore(const InspectorObject* const container, const
String& name, long* value) { container->getNumber(name, value); }
> +
> +    static String asString(bool value) { return value ? "true" : "false"; }
> +    static String asString(const String& value) { return value; }
> +    static String asString(const long value) { return String::number(value);
}
> +    static void fromString(const String& stringValue, bool* value) { *value
= stringValue == "true"; }
> +    static void fromString(const String& stringValue, String* value) {
*value = stringValue; }
> +    static void fromString(const String& stringValue, long* value) { *value
= stringValue.toInt(); }

Consider moving these methods into property type-specific descendants.

> WebCore/inspector/InspectorState.h:70
> +template <typename ValueType, typename OwnerType>

Please remove OwnerType parameter since it's always InspectorState.

> WebCore/inspector/InspectorState.h:115
> +    INSPECTOR_PROPERTY(bool, monitoringXHR, setMonitoringXHR);

Would be better to combine this declarations with those in the constructor.

> WebCore/inspector/InspectorState.h:127
> +    PassRefPtr<InspectorObject> getState() { return
propertyMapToInspectorObject(m_inspectorState); }

Should be buildXXX as in other places

> WebCore/inspector/InspectorState.h:128
> +    void restoreFromJSONString(const String& json);

restoreFromCookie

> WebKit/win/WebCoreSupport/WebInspectorClient.cpp:367
> +	   shouldAttach =
m_inspectedWebView->page()->inspectorController()->inspectorStartsAttached();

I think this can be handled on WebKit side without calls to WebCore.