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

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has granted 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 70079: [patch] initial version.
https://bugs.webkit.org/attachment.cgi?id=70079&action=review

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

> WebCore/inspector/InspectorState.cpp:60
> +    REGISTER_INSPECTOR_PROPERTY(monitoringXHR, false, "monitoringXHR",
"xhrMonitor");
> +    REGISTER_INSPECTOR_PROPERTY(resourceTrackingEnabled, false,
"resourceTrackingEnabled", "");
> +    REGISTER_INSPECTOR_PROPERTY(resourceTrackingAlwaysEnabled, false, "",
"resourceTrackingEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(timelineProfilerEnabled, false,
"timelineProfilerEnabled", "");
> +    REGISTER_INSPECTOR_PROPERTY(searchingForNode, false,
"searchingForNodeEnabled", "");
> +    REGISTER_INSPECTOR_PROPERTY(profilerAlwaysEnabled, false, "",
"profilerEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(frontendSettings, "", "",
"frontendSettings");
> +    REGISTER_INSPECTOR_PROPERTY(debuggerAlwaysEnabled, false, "",
"debuggerEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(lastActivePanel,
InspectorController::LastActivePanel, "", "lastActivePanel");
> +    REGISTER_INSPECTOR_PROPERTY(inspectorStartsAttached, true, "",
"InspectorStartsAttached");
> +    REGISTER_INSPECTOR_PROPERTY(inspectorAttachedHeight,
InspectorController::defaultAttachedHeight, "", "inspectorAttachedHeight");

I am still convinced that we'd better have one list of the property
descriptions and use it both in the header and here.

> WebCore/inspector/InspectorState.cpp:82
> +	   if ((*i)->m_stateName.length())

simply	if ((*i)->m_stateName)

> WebCore/inspector/InspectorState.cpp:90
> +	   if ((*i)->m_preferenceName.length()) {

simply	if ((*i)->m_preferenceName) {

> WebCore/inspector/InspectorState.cpp:102
> +	   if ((*i)->m_stateName.length())

ditto

> WebCore/inspector/InspectorState.cpp:106
> +    if (property->m_preferenceName.length())

ditto

> WebCore/inspector/InspectorState.h:3
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Fix copyright.

> WebCore/inspector/InspectorState.h:105
> +    friend class InspectorProperty<bool>;

This is not needed anymore, please remove.

> WebCore/inspector/InspectorState.h:129
> +    void propertyUpdated(InspectorPropertyBase* property);

why not make it private?

> WebCore/inspector/InspectorValues.h:182
> +    void set(const String& name, bool value) { setBoolean(name, value); }
> +    void set(const String& name, const String& value) { setString(name,
value); }
> +    void set(const String& name, double value) { setNumber(name, value); }
> +    void set(const String& name, long value) { setNumber(name, value); }
> +
> +    void get(const String& name, bool* value) const { getBoolean(name,
value); }
> +    void get(const String& name, String* value) const { getString(name,
value); }
> +    void get(const String& name, double* value) const { getNumber(name,
value); }
> +    void get(const String& name, long* value) const { getNumber(name,
value); }

I think we should have either overloaded setters/getters or separate ones with
method names including types.