Bug 47275 – Web Inspector: inspector settings/properties/states management should be extracted into separate class.

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

[Ilya Tikhonovsky]

We have a lot of flags/values in the InspectorController.
Some flags are persisting into profile.
Others are the part of inspector state for frontend.
All these flags should keep their values after navigation.

It'd be better to extract these flags/values into separate class which will care about their lifetime.

[Ilya Tikhonovsky]

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

[Yury Semikhatsky]

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

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

> WebCore/inspector/InspectorState.cpp:61
> +#define REGISTER_INSPECTOR_PROPERTY(name, value, stateName, preferencesName)\
> +m_##name.init(this, value);\
> +m_inspectorCookie.set(&m_##name, "name");\
> +if (stateName)\
> +m_inspectorState.set(&m_##name, "stateName");\
> +if (preferencesName)\
> +m_inspectorPreferences.set(&m_##name, "preferenceName");
> +
> +InspectorState::InspectorState(InspectorClient* client)
> +    : m_client(client)
> +{
> +    REGISTER_INSPECTOR_PROPERTY(monitoringXHR, false, "monitoringXHR", "xhrMonitor");
> +    REGISTER_INSPECTOR_PROPERTY(resourceTrackingEnabled, false, "resourceTrackingEnabled", 0);
> +    REGISTER_INSPECTOR_PROPERTY(resourceTrackingAlwaysEnabled, false, 0, "resourceTrackingEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(timelineProfilerEnabled, false, "timelineProfilerEnabled", 0);
> +    REGISTER_INSPECTOR_PROPERTY(searchingForNode, false, "searchingForNodeEnabled", 0);
> +    REGISTER_INSPECTOR_PROPERTY(profilerAlwaysEnabled, false, 0, "profilerEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(frontendSettings, "", 0, "frontendSettings");
> +    REGISTER_INSPECTOR_PROPERTY(debuggerAlwaysEnabled, false, 0, "debuggerEnabled");
> +    REGISTER_INSPECTOR_PROPERTY(lastActivePanel, InspectorController::LastActivePanel, 0, "lastActivePanel");
> +    REGISTER_INSPECTOR_PROPERTY(inspectorStartsAttached, true, 0, "InspectorStartsAttached");
> +    REGISTER_INSPECTOR_PROPERTY(inspectorAttachedHeight, InspectorController::defaultAttachedHeight, 0, "inspectorAttachedHeight");

Consider declaring the list of properties as a macros accepting another macros. This way you could generate getter/setter declarations and implementations that would pass stateName and preferenceName directly into propertyUpdeted and get rid of InspectorPropertyBase completely.

> WebCore/inspector/InspectorState.h:67
> +    static void save(InspectorObject* container, const String& name, bool value) { container->setBoolean(name, value); }
> +    static void save(InspectorObject* container, const String& name, const String& value) { container->setString(name, value); }
> +    static void save(InspectorObject* container, const String& name, long value) { container->setNumber(name, value); }
> +    static void restore(const InspectorObject* const container, const String& name, bool* value) { container->getBoolean(name, value); }
> +    static void restore(const InspectorObject* const container, const String& name, String* value) { container->getString(name, value); }
> +    static void restore(const InspectorObject* const container, const String& name, long* value) { container->getNumber(name, value); }
> +
> +    static String asString(bool value) { return value ? "true" : "false"; }
> +    static String asString(const String& value) { return value; }
> +    static String asString(const long value) { return String::number(value); }
> +    static void fromString(const String& stringValue, bool* value) { *value = stringValue == "true"; }
> +    static void fromString(const String& stringValue, String* value) { *value = stringValue; }
> +    static void fromString(const String& stringValue, long* value) { *value = stringValue.toInt(); }

Consider moving these methods into property type-specific descendants.

> WebCore/inspector/InspectorState.h:70
> +template <typename ValueType, typename OwnerType>

Please remove OwnerType parameter since it's always InspectorState.

> WebCore/inspector/InspectorState.h:115
> +    INSPECTOR_PROPERTY(bool, monitoringXHR, setMonitoringXHR);

Would be better to combine this declarations with those in the constructor.

> WebCore/inspector/InspectorState.h:127
> +    PassRefPtr<InspectorObject> getState() { return propertyMapToInspectorObject(m_inspectorState); }

Should be buildXXX as in other places

> WebCore/inspector/InspectorState.h:128
> +    void restoreFromJSONString(const String& json);

restoreFromCookie

> WebKit/win/WebCoreSupport/WebInspectorClient.cpp:367
> +        shouldAttach = m_inspectedWebView->page()->inspectorController()->inspectorStartsAttached();

I think this can be handled on WebKit side without calls to WebCore.

[Ilya Tikhonovsky]

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

[Yury Semikhatsky]

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

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.

[Pavel Feldman]

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

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

> WebCore/inspector/InspectorController.cpp:123
> +const char* const InspectorController::LastActivePanel = "lastActivePanel";

Const names start with lower case.

> WebCore/inspector/InspectorState.h:18
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY

Should be Google :)

> WebCore/inspector/InspectorState.h:51
> +    virtual void saveToContainerAs(InspectorObject* container, const String& name) const = 0;

Should be PassRefPtr<InspectorValue> toInspectorValue() and fromInspectorValue.

> WebCore/inspector/InspectorState.h:111
> +    INSPECTOR_PROPERTY(bool, monitoringXHR, setMonitoringXHR);

Could we please use a simple approach where we register and set properties using classical OOP? (think chromium's PrefsService).

> WebCore/inspector/InspectorValues.h:174
> +    void set(const String& name, bool value) { setBoolean(name, value); }

set("foo", long_long) will result in setBoolean. I am not sure we are ready to this. Should we use traits instead?

> WebKit/win/WebCoreSupport/WebInspectorClient.cpp:276
> +    m_inspectedWebView->page()->inspectorController()->setInspectorStartsAttached(true);

We should store this setting from within inspector controller.

[Ilya Tikhonovsky]

Created attachment 70213 [details]
[patch] third iteration.


(In reply to comment #5)
> (From update of attachment 70079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70079&action=review
> 
> > WebCore/inspector/InspectorController.cpp:123
> > +const char* const InspectorController::LastActivePanel = "lastActivePanel";
> 
> Const names start with lower case.

it'd be better to do that in a separate patch.


> 
> > WebCore/inspector/InspectorState.h:18
> > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> 
> Should be Google :)

done.


> 
> > WebCore/inspector/InspectorState.h:51
> > +    virtual void saveToContainerAs(InspectorObject* container, const String& name) const = 0;
> 
> Should be PassRefPtr<InspectorValue> toInspectorValue() and fromInspectorValue.

done.

> 
> > WebCore/inspector/InspectorState.h:111
> > +    INSPECTOR_PROPERTY(bool, monitoringXHR, setMonitoringXHR);
> 
> Could we please use a simple approach where we register and set properties using classical OOP? (think chromium's PrefsService).

It looks like Java style. In that case we will have a lot of code like 
m_state->setBoolean(someInteresingPropertyName, true); instead of 
m_state->setSomeInteresingProperty(true);

My variant a bit shorter and type safer.


> > WebCore/inspector/InspectorValues.h:174
> > +    void set(const String& name, bool value) { setBoolean(name, value); }
> 
> set("foo", long_long) will result in setBoolean. I am not sure we are ready to this. Should we use traits instead?

done.


> > WebKit/win/WebCoreSupport/WebInspectorClient.cpp:276
> > +    m_inspectedWebView->page()->inspectorController()->setInspectorStartsAttached(true);
> 
> We should store this setting from within inspector controller.

will do that as a separate patch

[Ilya Tikhonovsky]

proposed patch allows us to have strong type checking for set operations at compile time.

If you call setSomeBooleanProperty(1.0) then you will get compile time error like this:


WebCore/inspector/InspectorState.h: In member function 'void WebCore::InspectorProperty<ValueType>::set(ArgumentType) [with ArgumentType = int, ValueType = long int]':
WebCore/inspector/InspectorState.h:153:   instantiated from 'void WebCore::InspectorState::setInspectorAttachedHeight(const argType&) [with argType = int]'
WebCore/inspector/InspectorController.cpp:210:   instantiated from here
WebCore/inspector/InspectorState.h:97: error: 'Implicit_conversion_guard__Please_use_exact_type_for_set_operation' was not declared in this scope
distcc[59007] ERROR: compile WebCore/inspector/InspectorController.cpp on localhost failed

[Yury Semikhatsky]

Comment on attachment 70213 [details]
[patch] third iteration.

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

> WebCore/inspector/InspectorState.h:113
> +struct property_type_traits<String> {

Stick to style guide when naming the structs.

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

Should be private.

[Pavel Feldman]

This is amazing how overly complex solution for a simple problem could be. Macro, templates, virtual methods, ugliness all over for a primitive registry sounds like over-engineering to me.

[Pavel Feldman]

Comment on attachment 70213 [details]
[patch] third iteration.

My colleagues are suggesting that I should r- the patch in case I have concerns of the kind.

[Ilya Tikhonovsky]

Created attachment 70347 [details]
[patch] polished but very complex solution :) without InspectorProperty* classes and theirs virtual functions. Just for try bots.

has no virtual functions
has strong type checking
has no overhead on get* operations.
debugger friendly :)

[Ilya Tikhonovsky]

Created attachment 70348 [details]
[patch] OOP version for review.

[Early Warning System Bot]

Attachment 70348 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4291005

[Eric Seidel (no email)]

Attachment 70348 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4336005

[WebKit Review Bot]

Attachment 70348 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4334004

[Ilya Tikhonovsky]

Comment on attachment 70348 [details]
[patch] OOP version for review.

Just missed the InspectorState files :(

[Ilya Tikhonovsky]

Created attachment 70359 [details]
[patch] OOP version for review. Now with new files.

[Pavel Feldman]

Comment on attachment 70359 [details]
[patch] OOP version for review. Now with new files.

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

Looks good. Few nits and we can land this.

> WebCore/inspector/InspectorState.cpp:86
> +void InspectorState::restoreFromSettings()

loadFromSettings

> WebCore/inspector/InspectorState.h:87
> +    void registerBoolean(InspectorPropertyId propertyId, bool value, const String& stateName, const String& preferenceName) { m_properties.set(propertyId, Property::create(InspectorBasicValue::create(value), stateName, preferenceName));}

Could you move implementation into the .cpp file?

> WebCore/inspector/InspectorState.h:94
> +inline InspectorState::Property InspectorState::Property::create(PassRefPtr<InspectorValue> value, const String& stateName, const String& preferenceName)

Should also be in cpp

> WebCore/inspector/InspectorValues.cpp:656
> +bool InspectorObject::getNumber(const String& name, long* output) const

Do you need this change?

> WebCore/inspector/InspectorValues.h:109
> +    static PassRefPtr<InspectorBasicValue> create(long value)

Where is this one used? Why is overloaded double value not used?

> WebCore/inspector/InspectorValues.h:130
> +    explicit InspectorBasicValue(long value) : InspectorValue(TypeNumber), m_doubleValue((double)value) { }

ditto

> WebKit/mac/WebCoreSupport/WebInspectorClient.mm:361
> +    _shouldAttach = [_inspectedWebView page]->inspectorController()->inspectorStartsAttached();

Please add FIXME here.

> WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-397
> -    [_inspectedWebView page]->inspectorController()->setSetting(InspectorController::inspectorStartsAttachedSettingName(), "true");

ditto

> WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-408
> -    [_inspectedWebView page]->inspectorController()->setSetting(InspectorController::inspectorStartsAttachedSettingName(), "false");

ditto

> WebKit/win/WebCoreSupport/WebInspectorClient.cpp:-276
> -    m_inspectedWebView->page()->inspectorController()->setSetting(InspectorController::inspectorStartsAttachedSettingName(), "true");

ditto

> WebKit/win/WebCoreSupport/WebInspectorClient.cpp:-287
> -    m_inspectedWebView->page()->inspectorController()->setSetting(InspectorController::inspectorStartsAttachedSettingName(), "false");

ditto

> WebKit/win/WebCoreSupport/WebInspectorClient.cpp:367
> +        shouldAttach = m_inspectedWebView->page()->inspectorController()->inspectorStartsAttached();

ditto

[Ilya Tikhonovsky]

Created attachment 70720 [details]
[patch] third iteration. OOP

[Pavel Feldman]

Comment on attachment 70720 [details]
[patch] third iteration. OOP

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

> WebCore/inspector/InspectorController.cpp:242
> +        m_state->setBoolean(InspectorState::pauseOnExceptionsState, m_debuggerAgent->pauseOnExceptionsState());

This should be setNumber.

> WebCore/inspector/InspectorController.cpp:244
> +    *state = m_state->getStateObjectForFrontend();

Nit: "toInspectorObject" ?

> WebCore/inspector/InspectorController.cpp:394
> +    m_state->setString(InspectorState::lastActivePanel, panelName);

Please add // FIXME: move last panel setting to the front-end?

[Ilya Tikhonovsky]

Created attachment 70732 [details]
[patch] fourth iteration. OOP

(In reply to comment #20)
> (From update of attachment 70720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70720&action=review
> 
> > WebCore/inspector/InspectorController.cpp:242
> > +        m_state->setBoolean(InspectorState::pauseOnExceptionsState, m_debuggerAgent->pauseOnExceptionsState());
> 
> This should be setNumber.

My fault. 
Did you notice that such kind of mistakes would be caught by template version at compile time?

> 
> > WebCore/inspector/InspectorController.cpp:244
> > +    *state = m_state->getStateObjectForFrontend();
> 
> Nit: "toInspectorObject" ?

I think it'd be better to call it generateStateObjectForFrontend.

> 
> > WebCore/inspector/InspectorController.cpp:394
> > +    m_state->setString(InspectorState::lastActivePanel, panelName);
> 
> Please add // FIXME: move last panel setting to the front-end?

done.

[Ilya Tikhonovsky]

Created attachment 70735 [details]
[patch] next iteration. rebaselined.

[Eric Seidel (no email)]

Attachment 70735 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4412024

[Early Warning System Bot]

Attachment 70735 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4436030

[WebKit Review Bot]

Attachment 70735 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4406037

[WebKit Review Bot]

Attachment 70735 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4409031

[Ilya Tikhonovsky]

Created attachment 70737 [details]
[patch] next iteration.

[Pavel Feldman]

Comment on attachment 70737 [details]
[patch] next iteration.

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

> WebCore/inspector/InspectorController.cpp:400
> +    if (!enabled() || !m_state->getBoolean(InspectorState::searchingForNode))

Use !searchingForNodeInPage() here?

> WebCore/inspector/InspectorController.cpp:415
> +    ASSERT(m_state->getBoolean(InspectorState::searchingForNode));

ditto

> WebCore/inspector/InspectorController.cpp:454
> +    if (m_state->getBoolean(InspectorState::searchingForNode) == enabled)

ditto

> WebCore/inspector/InspectorController.cpp:1111
> +    if (!enabled() || !m_state->getBoolean(InspectorState::resourceTrackingEnabled))

resourceTrackingEnabled() ?

> WebCore/inspector/InspectorController.cpp:1140
> +    if (m_state->getBoolean(InspectorState::resourceTrackingEnabled) == enable)

ditto

> WebCore/inspector/InspectorState.cpp:44
> +    registerBoolean(resourceTrackingAlwaysEnabled, false, (const char*)0, "resourceTrackingEnabled");

This is likely to add the confusion (resourceTrackingAlwaysEnabled is persisted as "resourceTrackingEnabled".

> WebCore/inspector/InspectorState.cpp:170
> +InspectorState::Property InspectorState::Property::create(PassRefPtr<InspectorValue> value, const String& stateName, const String& preferenceName)

stateName is not used anywhere other than in the front-end. InspectorController itself uses enum literals. Should we rename "stateName" to "frontendAlias"?

[Ilya Tikhonovsky]

Committed r69844
	M	WebKit/win/ChangeLog
	M	WebKit/win/WebCoreSupport/WebInspectorClient.cpp
	M	WebKit/mac/WebCoreSupport/WebInspectorClient.mm
	M	WebKit/mac/ChangeLog
	M	WebCore/WebCore.exp.in
	M	WebCore/WebCore.pro
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	A	WebCore/inspector/InspectorState.cpp
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorFrontendClientLocal.cpp
	A	WebCore/inspector/InspectorState.h
	M	WebCore/inspector/InspectorController.h
	M	WebCore/CMakeLists.txt
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r69844 = 47c617dabd41d8f7c7ae430a14bf4fa300196946 (refs/remotes/trunk)

[Eric Seidel (no email)]

I think you'll find "webkit-patch land" or "webkit-patch land-from-bug" takes care of much of this patch-maintenance for you.  If you have webkit-patch troubles, let Adam or I know (by filing a bug), we'd be happy to fix them.