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.
Created attachment 58024 [details] [PATCH] Suggested solution
Attachment 58024 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3070210
Created attachment 58030 [details] [PATCH] Fixed compilability on mac
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.
Created attachment 58052 [details] [PATCH] Comments addressed
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.
Created attachment 58054 [details] [PATCH] Style fixed
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!
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).
(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?
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.
(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 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).
Created attachment 58242 [details] [PATCH] Comments addressed, property-wise persistence introduced
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.
(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.
> 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).
Created attachment 58461 [details] [PATCH] Revert to serializing the entire Settings stores (not property-wise)
Committed r61014: <http://trac.webkit.org/changeset/61014>