Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632553002:
[DevTools] Clear local storage (Closed)
|
Created:
3 years, 11 months ago by eostroukhov Modified:
3 years, 11 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Clear local storage
Add a Clear pop-up menu item and toolbar button to the local storage UI.
BUG=681054
Review-Url: https://codereview.chromium.org/2632553002
Cr-Commit-Position: refs/heads/master@{#445222}
Committed: https://chromium.googlesource.com/chromium/src/+/7f604c4d2d191d8b4f1ccbb15b69e4a3ff2e5a58
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments, moved the toolbar. #
Total comments: 14
Patch Set 3 : [DevTools] Clear local storage #Patch Set 4 : Switched to using DOM storage APIs for clearing local and session storages. #
Total comments: 2
Patch Set 5 : [DevTools] Clear local storage #Messages
Total messages: 43 (27 generated)
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
eostroukhov@chromium.org changed reviewers: + dgozman@chromium.org
Please take a look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This needs a bug with a screenshot!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [DevTools] Clear local storage Add a Clear pop-up menu item and toolbar button to the local storage UI. BUG=none ========== to ========== [DevTools] Clear local storage Add a Clear pop-up menu item and toolbar button to the local storage UI. BUG=681054 ==========
On 2017/01/13 00:36:44, pfeldman wrote: > This needs a bug with a screenshot! Created a bug & added number to this CL description.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:42: this.clearButton = new UI.ToolbarButton(Common.UIString('Clear All'), 'largeicon-clear'); this._clearButton - it should be private. While you are here, you could fix the other ones as well. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:63: return [this.refreshButton, this.clearButton, this.deleteButton]; We are consistently using delete button to close things, so we need another (trash?) icon for deleting. Could you talk to Chris about it? https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:262: clear() { inline this. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:92: clear() { When clearing the model, it should dispatch events so that views could clear themselves. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1925: this.listItemElement.addEventListener('contextmenu', this._handleContextMenuEvent.bind(this), true); Drop context menu.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I moved the toolbar (and filter field) to the top. I don't like the filter toolbar - so I will see how hard it would be to retrofit local/session storage views to have the filter and will see if I can move the filter to the toolbar and get rid of the filter button. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:42: this.clearButton = new UI.ToolbarButton(Common.UIString('Clear All'), 'largeicon-clear'); On 2017/01/13 18:51:25, pfeldman wrote: > this._clearButton - it should be private. While you are here, you could fix the > other ones as well. Done. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:63: return [this.refreshButton, this.clearButton, this.deleteButton]; On 2017/01/13 18:51:25, pfeldman wrote: > We are consistently using delete button to close things, so we need another > (trash?) icon for deleting. Could you talk to Chris about it? Acknowledged (no change for now, pending new icons). https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:262: clear() { On 2017/01/13 18:51:25, pfeldman wrote: > inline this. Done. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:92: clear() { On 2017/01/13 18:51:25, pfeldman wrote: > When clearing the model, it should dispatch events so that views could clear > themselves. Done. https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2632553002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1925: this.listItemElement.addEventListener('contextmenu', this._handleContextMenuEvent.bind(this), true); On 2017/01/13 18:51:25, pfeldman wrote: > Drop context menu. I do not see any harm in having this menu item - since cookies view has it already.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:62: if (this.domStorage.isLocalStorage) Why different toolbars? https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:93: if (!this.isLocalStorage) Why? https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:585: _clearDOMStorage(domStorage) { Why do you need a method that calls a method on the parameter? https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:586: if (!domStorage) You just declared it as not nullable. https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1927: contextMenu.appendItem(Common.UIString('Clear'), this._clearDOMStorage.bind(this)); () => this._domStorage.clear() https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1931: _clearDOMStorage() { You don't need it.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for the review. I updated the CL, please take another look. https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:62: if (this.domStorage.isLocalStorage) On 2017/01/19 23:38:45, pfeldman wrote: > Why different toolbars? We do not have a way to wipe session storage - https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c... https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:93: if (!this.isLocalStorage) On 2017/01/19 23:38:45, pfeldman wrote: > Why? Can't wipe session storage. https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:585: _clearDOMStorage(domStorage) { On 2017/01/19 23:38:46, pfeldman wrote: > Why do you need a method that calls a method on the parameter? Fixed. Vestige of an older implementation... https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:586: if (!domStorage) On 2017/01/19 23:38:46, pfeldman wrote: > You just declared it as not nullable. Acknowledged. https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1927: contextMenu.appendItem(Common.UIString('Clear'), this._clearDOMStorage.bind(this)); On 2017/01/19 23:38:46, pfeldman wrote: > () => this._domStorage.clear() Done. https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1931: _clearDOMStorage() { On 2017/01/19 23:38:46, pfeldman wrote: > You don't need it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:93: if (!this.isLocalStorage) On 2017/01/20 19:24:00, eostroukhov wrote: > On 2017/01/19 23:38:45, pfeldman wrote: > > Why? > > Can't wipe session storage. sessionStorage.clear() works just fine for me.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js (right): https://codereview.chromium.org/2632553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageModel.js:93: if (!this.isLocalStorage) On 2017/01/20 19:27:31, pfeldman wrote: > On 2017/01/20 19:24:00, eostroukhov wrote: > > On 2017/01/19 23:38:45, pfeldman wrote: > > > Why? > > > > Can't wipe session storage. > > sessionStorage.clear() works just fine for me. Not sure what you are talking about. Open the Verge, then open "Clear Storage" and try wiping "Local and session storage" - I see local being cleaned up while session storage remains untouched.
> Not sure what you are talking about. Open the Verge, then open "Clear Storage" > and try wiping "Local and session storage" - I see local being cleaned up while > session storage remains untouched. I'm talking about clearing session storage via typing sessionStorage.clear().
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/20 19:50:42, pfeldman wrote: > > Not sure what you are talking about. Open the Verge, then open "Clear Storage" > > and try wiping "Local and session storage" - I see local being cleaned up > while > > session storage remains untouched. > > I'm talking about clearing session storage via typing sessionStorage.clear(). Thanks. I changed the code to use those APIs.
Now storage is cleared via InspectorDOMStorageAgent on the renderer side. Please take another look.
lgtm https://codereview.chromium.org/2632553002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2632553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1916: if (this._domStorage.isLocalStorage) Why the check?
Thanks for the review! I'm submitting now. https://codereview.chromium.org/2632553002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2632553002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:1916: if (this._domStorage.isLocalStorage) On 2017/01/20 21:32:06, pfeldman wrote: > Why the check? Nice catch. I removed the check.
The CQ bit was checked by eostroukhov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2632553002/#ps80001 (title: "[DevTools] Clear local storage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484948821112670,
"parent_rev": "c2ff1efb3496708d94fb12f79b3aeac6b1846b4b", "commit_rev":
"7f604c4d2d191d8b4f1ccbb15b69e4a3ff2e5a58"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Clear local storage Add a Clear pop-up menu item and toolbar button to the local storage UI. BUG=681054 ========== to ========== [DevTools] Clear local storage Add a Clear pop-up menu item and toolbar button to the local storage UI. BUG=681054 Review-Url: https://codereview.chromium.org/2632553002 Cr-Commit-Position: refs/heads/master@{#445222} Committed: https://chromium.googlesource.com/chromium/src/+/7f604c4d2d191d8b4f1ccbb15b69... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7f604c4d2d191d8b4f1ccbb15b69... |
