Bug 40228 - Web Inspector: Enable serialization/deserialization of the frontend state
Summary: Web Inspector: Enable serialization/deserialization of the frontend state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-07 04:36 PDT by Alexander Pavlov (apavlov)
Modified: 2010-07-05 05:28 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Suggested solution (8.88 KB, patch)
2010-06-07 06:48 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Fixed compilability on mac (8.67 KB, patch)
2010-06-07 08:05 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Comments addressed (12.42 KB, patch)
2010-06-07 11:24 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Style fixed (12.43 KB, patch)
2010-06-07 11:30 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Implemented suggested naming convention (31.09 KB, patch)
2010-06-08 07:59 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comments addressed, property-wise persistence introduced (42.28 KB, patch)
2010-06-09 07:21 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Revert to serializing the entire Settings stores (not property-wise) (31.19 KB, patch)
2010-06-11 04:17 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2010-06-07 04:36:07 PDT
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.
Comment 1 Alexander Pavlov (apavlov) 2010-06-07 06:48:43 PDT
Created attachment 58024 [details]
[PATCH] Suggested solution
Comment 2 Eric Seidel (no email) 2010-06-07 06:58:37 PDT
Attachment 58024 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3070210
Comment 3 Alexander Pavlov (apavlov) 2010-06-07 08:05:27 PDT
Created attachment 58030 [details]
[PATCH] Fixed compilability on mac
Comment 4 Pavel Feldman 2010-06-07 08:35:44 PDT
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.
Comment 5 Alexander Pavlov (apavlov) 2010-06-07 11:24:51 PDT
Created attachment 58052 [details]
[PATCH] Comments addressed
Comment 6 WebKit Review Bot 2010-06-07 11:26:27 PDT
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.
Comment 7 Alexander Pavlov (apavlov) 2010-06-07 11:30:47 PDT
Created attachment 58054 [details]
[PATCH] Style fixed
Comment 8 Pavel Feldman 2010-06-07 11:52:10 PDT
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!
Comment 9 Alexander Pavlov (apavlov) 2010-06-08 07:59:04 PDT
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).
Comment 10 Joseph Pecoraro 2010-06-08 11:05:33 PDT
(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?
Comment 11 Alexander Pavlov (apavlov) 2010-06-08 11:13:09 PDT
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.
Comment 12 Joseph Pecoraro 2010-06-08 11:21:29 PDT
(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.
Comment 13 Pavel Feldman 2010-06-08 12:41:30 PDT
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).
Comment 14 Alexander Pavlov (apavlov) 2010-06-09 07:21:31 PDT
Created attachment 58242 [details]
[PATCH] Comments addressed, property-wise persistence introduced
Comment 15 Pavel Feldman 2010-06-09 07:54:51 PDT
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.
Comment 16 Alexander Pavlov (apavlov) 2010-06-09 08:46:17 PDT
(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.
Comment 17 Alexander Pavlov (apavlov) 2010-06-11 04:16:06 PDT
> 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).
Comment 18 Alexander Pavlov (apavlov) 2010-06-11 04:17:56 PDT
Created attachment 58461 [details]
[PATCH] Revert to serializing the entire Settings stores (not property-wise)
Comment 19 Alexander Pavlov (apavlov) 2010-07-05 05:28:55 PDT
Committed r61014: <http://trac.webkit.org/changeset/61014>