[webkit-reviews] review denied: [Bug 45974] Web Inspector: refactoring. getBackendSettings was renamed to getInspectorState. : [Attachment 67932] [patch] initial version.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 45974: Web Inspector: refactoring. getBackendSettings was renamed to
getInspectorState.
https://bugs.webkit.org/show_bug.cgi?id=45974

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67932&action=prettypatch

> WebCore/inspector/Inspector.idl:101
> +	   [handler=Controller] void getInspectorState(out Object state);

I think we should have domain-specific states. Like Debugging will have
'breakpointsActive', DOM will have searching for node and such.

> WebCore/inspector/InspectorController.cpp:124
> +static const char* const timelineProfilerEnabledSettingName =
"timelineProfilerEnabled";

This is not a setting name.

> WebCore/inspector/InspectorController.cpp:274
> +    inspectorState->getBoolean(resourceTrackingEnabledSettingName,
&m_resourceTrackingEnabled);

You should not mix settings with state.

> WebCore/inspector/InspectorController.cpp:1156
> +void InspectorController::setResourceTracking(bool enable)

setResourceTrackingEnabled

> WebKit/chromium/src/WebDevToolsAgentImpl.h:96
> +    virtual void persistInspectorState(const WTF::String&);

This will break canaries.