[webkit-reviews] review requested: [Bug 47275] Web Inspector: inspector settings/properties/states management should be extracted into separate class. : [Attachment 70213] [patch] third iteration.

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  for review:
Bug 47275: Web Inspector: inspector settings/properties/states management
should be extracted into separate class.
https://bugs.webkit.org/show_bug.cgi?id=47275

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

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>

(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(t
rue);
> 
> We should store this setting from within inspector controller.

will do that as a separate patch