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

[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 70732: [patch] fourth iteration. OOP
https://bugs.webkit.org/attachment.cgi?id=70732&action=review

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