Bug 45974 – Web Inspector: refactoring. getBackendSettings was renamed to getInspectorState.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

We have a lot of flags inside Inspector.
Some of them are settings, the others are the runtime state of inspector.

It'd be better to transfer these flags to frontend side and persist/restore to/from browser (chromium specific) together as the Inspector state.
As result we can drop separate functions for persisting/restoring the state of resourceTracking and timelineProfiler and other inspector state flags in the future.

[Ilya Tikhonovsky]

Created attachment 67932 [details]
[patch] initial version.

[Pavel Feldman]

Comment on attachment 67932 [details]
[patch] initial version.

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.

[Ilya Tikhonovsky]

Created attachment 68077 [details]
[patch] second iteration

>> 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.

[Pavel Feldman]

Comment on attachment 68077 [details]
[patch] second iteration

View in context: https://bugs.webkit.org/attachment.cgi?id=68077&action=review

r+ with nits.

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

Can we extract these into a separate class?

> WebCore/inspector/InspectorController.cpp:261
> +void InspectorController::saveInspectorState()

updateInspectorStateCookie

> WebCore/inspector/InspectorController.cpp:267
> +    m_client->saveInspectorState(state->toJSONString());

Save is a bad name for this method. m_client->updateInspectorStateCookie?

> WebCore/inspector/InspectorController.cpp:270
> +void InspectorController::restoreInspectorState(const String& inspectorStateString)

restoreInspectorStateFromCookie ?

> WebCore/inspector/InspectorController.cpp:503
> +void InspectorController::setMonitoringXHREnabled(bool enable, bool* newState)

nit: enabled

[Ilya Tikhonovsky]

Committed r67852
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
	M	WebKit/chromium/src/InspectorClientImpl.h
	M	WebKit/chromium/src/WebDevToolsAgentImpl.h
	M	WebKit/chromium/src/InspectorClientImpl.cpp
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/InspectorClient.h
	M	WebCore/inspector/front-end/ResourcesPanel.js
	M	WebCore/inspector/front-end/ConsoleView.js
	M	WebCore/inspector/front-end/Settings.js
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/InspectorController.h
	M	LayoutTests/http/tests/inspector/console-xhr-logging.html
	M	LayoutTests/inspector/report-API-errors-expected.txt
	M	LayoutTests/inspector/report-API-errors.html
	M	LayoutTests/ChangeLog
r67852 = 9777aacd90a8a72436471a522711fe871b390d1b (refs/remotes/trunk)