[webkit-reviews] review requested: [Bug 45974] Web Inspector: refactoring. getBackendSettings was renamed to getInspectorState. : [Attachment 68077] [patch] second iteration

[bugzilla-daemon at webkit.org]

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

Attachment 68077: [patch] second iteration
https://bugs.webkit.org/attachment.cgi?id=68077&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
>> 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.

Right now we have very small set of flags whose lifetime is not matched with
lifetime of the frontend.
As far as we are using JSON Object as the container for these flags it will be
quite simple to extend the set of flags with new properties in the future.

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

>This is not a setting name.

done

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

> You should not mix settings with state.

done.

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

>setResourceTrackingEnabled

done.

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

This will break canaries.

That change is affecting the interface between WebCore and WebKit.
Chromium part is stay untouched.
Try bot is fine with these changes.