[webkit-reviews] review denied: [Bug 40228] Web Inspector: Enable serialization/deserialization of the frontend state : [Attachment 58138] [PATCH] Implemented suggested naming convention

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 40228: Web Inspector: Enable serialization/deserialization of the frontend
state
https://bugs.webkit.org/show_bug.cgi?id=40228

Attachment 58138: [PATCH] Implemented suggested naming convention
https://bugs.webkit.org/attachment.cgi?id=58138&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
I'm going to be a bit more picky than Joe.

WebCore/inspector/InspectorBackend.cpp:90
 +	m_inspectorController->setSessionSettings(settings);
Just as above, you should always test for inspector's availability prior to
calling into it. InspectorBackend is refcounted due to being the binding. It
can exist after the page + inspector controller are removed. r- for this one.


WebCore/inspector/InspectorController.cpp:283
 +  void InspectorController::setSessionSettings(const String& settings)
I'd call parameter settingsJSON


WebCore/inspector/InspectorController.cpp:283
 +  void InspectorController::setSessionSettings(const String& settings)
Also, I think this API should be setSetting(name, value) + we probably should
do the same for application settings. I am sure I mentioned it in one of the
previous reviews.


WebCore/inspector/InspectorController.h:128
 +	String setting(const String& key) const;
These are now worth renaming in case they are internal (not sure they are
though - could you pleaes check the clients first? I remember some WebKit code
using these).


WebCore/inspector/front-end/Settings.js:64
 +	WebInspector.sessionSettings._load(settingsString);
Will this work without installing settings explicitly as above?


WebCore/inspector/front-end/Settings.js:108
 +		var store = JSON.stringify(this._store);
So making it setSettung(name, value) gets tricky due to this code. But I think
we should do the same to the application settings. Something like
InspectorBackend.saveSetting(name, value, isSession).