Bug 40228 – Web Inspector: Enable serialization/deserialization of the frontend state

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

[Alexander Pavlov (apavlov)]

In order to introduce CSS source patching on style modification via the Web Inspector, the changes made should be persisted by the frontend and restored next time the frontend is opened for the same inspector session (i.e., unless the user has navigated away from the inspected page). I believe, we are likely to expand the scope of data persisted/loaded this way.

[Alexander Pavlov (apavlov)]

Created attachment 58024 [details]
[PATCH] Suggested solution

[Eric Seidel (no email)]

Attachment 58024 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3070210

[Alexander Pavlov (apavlov)]

Created attachment 58030 [details]
[PATCH] Fixed compilability on mac

[Pavel Feldman]

Comment on attachment 58030 [details]
[PATCH] Fixed compilability on mac

WebCore/inspector/InspectorBackend.idl:38
 +          void saveFrontendSession(in DOMString sessionData);
I would introduce a setSessionProperty (with corresponding push method on front-end) instead. You can use new InspectorValue class as a dictionary that can be JSON-stringified.

[Alexander Pavlov (apavlov)]

Created attachment 58052 [details]
[PATCH] Comments addressed

[WebKit Review Bot]

Attachment 58052 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/inspector/front-end/Settings.js:71:  Line contains tab character.  [whitespace/tab] [5]
WebCore/inspector/front-end/Settings.js:111:  Line contains tab character.  [whitespace/tab] [5]
WebCore/inspector/front-end/Settings.js:113:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Created attachment 58054 [details]
[PATCH] Style fixed

[Pavel Feldman]

Comment on attachment 58054 [details]
[PATCH] Style fixed

We are using same control flow / data structures for the "FrontendSettings" and "SessionProperties". Furthermore, data structure is called Settings on the front-end side. I think we should align the naming better. I realize that 'settings' is not the best name for the session store, but I don't like the way you name the new thing. Rest of the patch seems to be fine. Clearing r? flag while I am thinking of the better names. Please think about it too and suggest things!

[Alexander Pavlov (apavlov)]

Created attachment 58138 [details]
[PATCH] Implemented suggested naming convention

Settled on "applicationSettings" for persistence across launches, "sessionSettings" for persistence across frontends in a single session (until didCommitLoad() is called).

[Joseph Pecoraro]

(In reply to comment #9)
> Created an attachment (id=58138) [details]
> [PATCH] Implemented suggested naming convention
> 
> Settled on "applicationSettings" for persistence across launches,
> "sessionSettings" for persistence across frontends in a single
> session (until didCommitLoad() is called).'

This last patch looks great to me. I'll let Pavel take a final
look at it though, as he knows more about this.

What type of sessionSettings do you envision?

[Alexander Pavlov (apavlov)]

The upcoming use is going to be the changes to the CSS source a user makes via the Styles pane (all source deltas are going to be computed/stored in the frontend and, as such, will be lost when the inspector window is closed). This patch is aimed to prevent this.

[Joseph Pecoraro]

(In reply to comment #11)
> The upcoming use is going to be the changes to the CSS source a
user makes via the Styles pane (all source deltas are going to be
computed/stored in the frontend and, as such, will be lost when the
inspector window is closed). This patch is aimed to prevent this.

Sounds good! Thats what it sounded like from the original comment,
but I thought there might be some other ideas as well.

[Pavel Feldman]

Comment on attachment 58138 [details]
[PATCH] Implemented suggested naming convention

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).

[Alexander Pavlov (apavlov)]

Created attachment 58242 [details]
[PATCH] Comments addressed, property-wise persistence introduced

[Pavel Feldman]

Comment on attachment 58242 [details]
[PATCH] Comments addressed, property-wise persistence introduced

WebCore/inspector/InspectorController.cpp:234
 +  void InspectorController::setFrontendProperty(const String& name, const String& valueJSON, bool session)
valueJSON -> value

I don't think we need to require value to be JSON-stringified object.

[Alexander Pavlov (apavlov)]

(In reply to comment #15)
> (From update of attachment 58242 [details])
> WebCore/inspector/InspectorController.cpp:234
>  +  void InspectorController::setFrontendProperty(const String& name, const String& valueJSON, bool session)
> valueJSON -> value
> 
> I don't think we need to require value to be JSON-stringified object.

Making InspectorController::setFrontendProperty type-agnostic (i.e. it should save plain strings rather than parse valueJSON into InspectorValues). This breaks the format of the "frontendSettings" setting in the backend store, which is not acceptable (and this case is also not detectable by the backend code, as the property values are interpreted in the frontend by WebInspector.Settings).

Persisting separate application-wide frontend properties into the backing store by their own [prefixed] names is also not an option from the design standpoint; those properties are pushed into the frontend at once (currently, as a single object), and a list of them should be added to InspectorController and kept in sync with that found in Settings.js.

This issue should be resolved before we can move forward.

[Alexander Pavlov (apavlov)]

> Making InspectorController::setFrontendProperty type-agnostic (i.e. it should save plain strings rather than parse valueJSON into InspectorValues). This breaks the format of the "frontendSettings" setting in the backend store, which is not acceptable (and this case is also not detectable by the backend code, as the property values are interpreted in the frontend by WebInspector.Settings).

I have attempted a migration of the frontendSettings property format from stringified
store: [key -> objectValue]
to
store: [key -> JSON.stringify(objectValue)]

but the Safari preferences backend fails to properly store value strings like
{"resources-large-rows":"false"}
turning them into
{"resources-large-rows":"\"\\\"\\\\\\\"\\\\\\\\\\\\\\\"\\\\\\\\\\\\\\\\\.....

hence I'm reverting to the patch 58138 with comments addressed, but without property-wise persistence (the stores contain objects, and are sent to InspectorBackend as a single stringified object).

[Alexander Pavlov (apavlov)]

Created attachment 58461 [details]
[PATCH] Revert to serializing the entire Settings stores (not property-wise)

[Alexander Pavlov (apavlov)]

Committed r61014: <http://trac.webkit.org/changeset/61014>