Bug 45974 - Web Inspector: refactoring. getBackendSettings was renamed to getInspectorState.
Summary: Web Inspector: refactoring. getBackendSettings was renamed to getInspectorState.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-17 10:48 PDT by Ilya Tikhonovsky
Modified: 2010-09-20 08:48 PDT (History)
9 users (show)

See Also:


Attachments
[patch] initial version. (19.53 KB, patch)
2010-09-17 12:25 PDT, Ilya Tikhonovsky
pfeldman: review-
Details | Formatted Diff | Diff
[patch] second iteration (30.84 KB, patch)
2010-09-20 07:04 PDT, Ilya Tikhonovsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2010-09-17 10:48:02 PDT
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.
Comment 1 Ilya Tikhonovsky 2010-09-17 12:25:39 PDT
Created attachment 67932 [details]
[patch] initial version.
Comment 2 Pavel Feldman 2010-09-20 04:09:06 PDT
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.
Comment 3 Ilya Tikhonovsky 2010-09-20 07:04:27 PDT
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.
Comment 4 Pavel Feldman 2010-09-20 07:33:13 PDT
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
Comment 5 Ilya Tikhonovsky 2010-09-20 08:48:06 PDT
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)