Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2649923006:
[DevTools] Add filtering to DOM storages (Closed)
|
Created:
3 years, 11 months ago by eostroukhov Modified:
3 years, 10 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, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Add filtering to DOM storages
1. Extracted toolbar setup code to share the implementation between
cookies and DOM storage panes.
2. Wired DOM storage pane to support filtering.
BUG=684204, 685314
Review-Url: https://codereview.chromium.org/2649923006
Cr-Commit-Position: refs/heads/master@{#447343}
Committed: https://chromium.googlesource.com/chromium/src/+/2ed48364ede3c10466f4d854fe6684bef63e645a
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Refactored, introduced a superclass #Patch Set 4 : [DevTools] Add filtering to DOM storages #Patch Set 5 : Moved the toolbar to the bottom for non-ItemViews #Patch Set 6 : Fixed a typo #Patch Set 7 : No more UI.SimpleView #
Total comments: 35
Patch Set 8 : Reverted change to CSS. #Patch Set 9 : Fixed a merge issue #Patch Set 10 : Review comments were addressed. #Patch Set 11 : Rebased #
Total comments: 12
Patch Set 12 : [DevTools] Add filtering to DOM storages #
Total comments: 7
Patch Set 13 : [DevTools] Add filtering to DOM storages #Patch Set 14 : [DevTools] Add filtering to DOM storages #Messages
Total messages: 68 (52 generated)
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...
eostroukhov@chromium.org changed reviewers: + dgozman@chromium.org
Please take a look. Note that there are plans to redo the search UI (merge the two toolbars together, etc) but it is out of scope for this CL. This CL lays out the foundation by sharing the implementation between 2 panels (another task will be to add same support to other panels).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 eostroukhov@chromium.org
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Thank you for the review. I refactored to replace a toolbar class with a superclass for panels. Please take another look.
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.
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...
Moved the toolbar to the bottom for the classes that don't extends the ItemView.
Description was changed from ========== [DevTools] Add filtering to DOM storages 1. Extracted toolbar setup code to share the implementation between cookies and DOM storage panes. 2. Wired DOM storage pane to support filtering. BUG=684204 ========== to ========== [DevTools] Add filtering to DOM storages 1. Extracted toolbar setup code to share the implementation between cookies and DOM storage panes. 2. Wired DOM storage pane to support filtering. BUG=684204,685314 ==========
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: 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...)
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 change the ItemsView to no longer extend SimpleView. Please take a look at the patch.
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 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...
Mostly looks good. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:67: _update() { Merge this with refresh? https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:34: this.domStorage = domStorage; While you are here, mind renaming to _domStorage? https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:42: var dataGrid = new DataGrid.DataGrid(columns, this._editingCallback.bind(this), this._deleteCallback.bind(this)); this._dataGrid https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:46: this.showFilterBar(); Why show filter by default? https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:175: for (var item of this.filter(items, item => `${item[0]} ${item[1]}`)) { Mind extracting |filteredItems| variable? https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:178: var node = new DataGrid.DataGridNode({key, value}, false); Don't use new language features without discussion with a team first. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:181: if (!selectedNode || key === selectedKey) { if (key === selectedKey) selectedNode = node; .... selectedNode = selectedNode || rootNode.children[0]; if (selectedNode) selectedNode.selected = true; https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:193: this.deleteButton.setEnabled(enabled); Inline this. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: * @protected We usually do protected getters in this case. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:69: showFilterBar() { Pass boolean |supportsFiltering| in constructor instead? https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:85: onClearAll() { @protected ? https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:87: onDelete() { style: empty line between functions https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:89: onRefresh() { deleteAllItems, deleteSelectedItem, refreshItem https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/module.json (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/module.json:30: "ItemsView.js", Add this file to devtools/BUILD.gn as well.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
Please include the screenshot into the bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/26 22:27:19, pfeldman wrote: > Please include the screenshot into the bug. I did that. Note that I am currently working on a separate CL (based on this one) that will merge the toolbar with filter bar and will remove the filter button and add a clear filter button.
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Thanks for the comments! I addressed them - please take a look. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:67: _update() { On 2017/01/26 22:21:09, dgozman wrote: > Merge this with refresh? Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:34: this.domStorage = domStorage; On 2017/01/26 22:21:09, dgozman wrote: > While you are here, mind renaming to _domStorage? Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:42: var dataGrid = new DataGrid.DataGrid(columns, this._editingCallback.bind(this), this._deleteCallback.bind(this)); On 2017/01/26 22:21:09, dgozman wrote: > this._dataGrid Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:46: this.showFilterBar(); On 2017/01/26 22:21:09, dgozman wrote: > Why show filter by default? I will rename to "showFilterUI" - what it does is adding a filter button and another toolbar, that is hidden by default. It needs to be called here so the filter toolbar appears above the grid when shown. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:175: for (var item of this.filter(items, item => `${item[0]} ${item[1]}`)) { On 2017/01/26 22:21:09, dgozman wrote: > Mind extracting |filteredItems| variable? Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:178: var node = new DataGrid.DataGridNode({key, value}, false); On 2017/01/26 22:21:09, dgozman wrote: > Don't use new language features without discussion with a team first. Assumed it was ok to use ES6 - I removed this feature. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:181: if (!selectedNode || key === selectedKey) { On 2017/01/26 22:21:09, dgozman wrote: > if (key === selectedKey) > selectedNode = node; > > > .... > > selectedNode = selectedNode || rootNode.children[0]; > if (selectedNode) > selectedNode.selected = true; Done. Note that I kept "!selectedNode" to select the top node if there's no node with the selectedKey https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:193: this.deleteButton.setEnabled(enabled); On 2017/01/26 22:21:09, dgozman wrote: > Inline this. Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/26 22:21:09, dgozman wrote: > 2017 Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: * @protected On 2017/01/26 22:21:09, dgozman wrote: > We usually do protected getters in this case. There does not seem to be a need for accessing the buttons other then to toggle enablement - I added protected setCan{DeleteSelected,DeleteAll,Refresh,Filter} https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:69: showFilterBar() { On 2017/01/26 22:21:09, dgozman wrote: > Pass boolean |supportsFiltering| in constructor instead? For now, this can be unconditional https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:85: onClearAll() { On 2017/01/26 22:21:09, dgozman wrote: > @protected ? ResourcesPanel calls "deleteAll" - so I'd like to keep all of them public. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:87: onDelete() { On 2017/01/26 22:21:09, dgozman wrote: > style: empty line between functions Done. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:89: onRefresh() { On 2017/01/26 22:21:09, dgozman wrote: > deleteAllItems, deleteSelectedItem, refreshItem I'm suggesting dropping the "item" - deleteAll/deleteSelected/refresh. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/module.json (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/module.json:30: "ItemsView.js", On 2017/01/26 22:21:09, dgozman wrote: > Add this file to devtools/BUILD.gn as well. Done.
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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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.
https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: * @protected On 2017/01/26 23:35:46, eostroukhov wrote: > On 2017/01/26 22:21:09, dgozman wrote: > > We usually do protected getters in this case. > > There does not seem to be a need for accessing the buttons other then to toggle > enablement - I added protected setCan{DeleteSelected,DeleteAll,Refresh,Filter} Sounds like extra indirection. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:89: onRefresh() { On 2017/01/26 23:35:46, eostroukhov wrote: > On 2017/01/26 22:21:09, dgozman wrote: > > deleteAllItems, deleteSelectedItem, refreshItem > > I'm suggesting dropping the "item" - deleteAll/deleteSelected/refresh. I'm suggesting to leave the "item" - deleteAllItems/deleteSelectedItem/refreshItem. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:27: constructor(domStorage) { JSDoc please. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:4: Resources.ItemsView = class extends UI.VBox { nit: empty line before this? https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:5: constructor(title, filterName) { JSDoc please https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:79: this.setCanDeleteSelected(false); Why this?
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...
Thanks for the review. Comments were addressed, please take another look. https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: * @protected On 2017/01/27 22:48:45, dgozman wrote: > On 2017/01/26 23:35:46, eostroukhov wrote: > > On 2017/01/26 22:21:09, dgozman wrote: > > > We usually do protected getters in this case. > > > > There does not seem to be a need for accessing the buttons other then to > toggle > > enablement - I added protected setCan{DeleteSelected,DeleteAll,Refresh,Filter} > > Sounds like extra indirection. It was either a getter or this indirection. I actually prefer this - as it abstracts the subclasses from the implemention (e.g. in the CL I'm working on "setCanFilter" will gray out the text field as there will no longer be a toolbar button) https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:89: onRefresh() { On 2017/01/27 22:48:45, dgozman wrote: > On 2017/01/26 23:35:46, eostroukhov wrote: > > On 2017/01/26 22:21:09, dgozman wrote: > > > deleteAllItems, deleteSelectedItem, refreshItem > > > > I'm suggesting dropping the "item" - deleteAll/deleteSelected/refresh. > > I'm suggesting to leave the "item" - > deleteAllItems/deleteSelectedItem/refreshItem. Done. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:27: constructor(domStorage) { On 2017/01/27 22:48:45, dgozman wrote: > JSDoc please. Done. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:4: Resources.ItemsView = class extends UI.VBox { On 2017/01/27 22:48:45, dgozman wrote: > nit: empty line before this? Done. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:5: constructor(title, filterName) { On 2017/01/27 22:48:45, dgozman wrote: > JSDoc please Done. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:79: this.setCanDeleteSelected(false); On 2017/01/27 22:48:45, dgozman wrote: > Why this? That's how it worked in those panels, I pushed up code that was duplicated in both subclasses.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: * @protected On 2017/01/28 00:03:29, eostroukhov wrote: > On 2017/01/27 22:48:45, dgozman wrote: > > On 2017/01/26 23:35:46, eostroukhov wrote: > > > On 2017/01/26 22:21:09, dgozman wrote: > > > > We usually do protected getters in this case. > > > > > > There does not seem to be a need for accessing the buttons other then to > > toggle > > > enablement - I added protected > setCan{DeleteSelected,DeleteAll,Refresh,Filter} > > > > Sounds like extra indirection. > > It was either a getter or this indirection. I actually prefer this - as it > abstracts the subclasses from the implemention (e.g. in the CL I'm working on > "setCanFilter" will gray out the text field as there will no longer be a toolbar > button) Sounds good. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:79: this.setCanDeleteSelected(false); On 2017/01/28 00:03:29, eostroukhov wrote: > On 2017/01/27 22:48:45, dgozman wrote: > > Why this? > > That's how it worked in those panels, I pushed up code that was duplicated in > both subclasses. Let's either understand why it's here (using blame) or remove it. https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/BUILD.gn (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/BUILD.gn:397: "front_end/resources/ItemsView.js", Let's rename to StorageItemsView or something. Our rule is "generic names only for generic components, like ui/ stuff". https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:119: contextMenu.appendItem(Common.UIString('Refresh'), this.refreshItems.bind(this)); I'm curious whether this should go to base class. https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: nit: extra blank line. https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:588: * @param {!SDK.Target} cookieFrameTarget Nice :-)
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 addressed the comments - had to rename a file so a new LGTM is now needed. Please take another look. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:79: this.setCanDeleteSelected(false); On 2017/01/30 21:53:48, dgozman wrote: > On 2017/01/28 00:03:29, eostroukhov wrote: > > On 2017/01/27 22:48:45, dgozman wrote: > > > Why this? > > > > That's how it worked in those panels, I pushed up code that was duplicated in > > both subclasses. > > Let's either understand why it's here (using blame) or remove it. Looks like "it had always been this way" - I traced it back to https://bugs.webkit.org/show_bug.cgi?id=49864 Should I remove it? https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/BUILD.gn (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/BUILD.gn:397: "front_end/resources/ItemsView.js", On 2017/01/30 21:53:48, dgozman wrote: > Let's rename to StorageItemsView or something. Our rule is "generic names only > for generic components, like ui/ stuff". Done. https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:119: contextMenu.appendItem(Common.UIString('Refresh'), this.refreshItems.bind(this)); On 2017/01/30 21:53:48, dgozman wrote: > I'm curious whether this should go to base class. Done. I changed the menu item to show even when there's no items. https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:14: On 2017/01/30 21:53:48, dgozman wrote: > nit: extra blank line. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:79: this.setCanDeleteSelected(false); On 2017/01/31 01:17:34, eostroukhov wrote: > On 2017/01/30 21:53:48, dgozman wrote: > > On 2017/01/28 00:03:29, eostroukhov wrote: > > > On 2017/01/27 22:48:45, dgozman wrote: > > > > Why this? > > > > > > That's how it worked in those panels, I pushed up code that was duplicated > in > > > both subclasses. > > > > Let's either understand why it's here (using blame) or remove it. > > Looks like "it had always been this way" - I traced it back to > https://bugs.webkit.org/show_bug.cgi?id=49864 > > Should I remove it? Yes, please.
Thanks for the review. Submitting. https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js (right): https://codereview.chromium.org/2649923006/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/ItemsView.js:79: this.setCanDeleteSelected(false); On 2017/01/31 03:51:17, dgozman_slow wrote: > On 2017/01/31 01:17:34, eostroukhov wrote: > > On 2017/01/30 21:53:48, dgozman wrote: > > > On 2017/01/28 00:03:29, eostroukhov wrote: > > > > On 2017/01/27 22:48:45, dgozman wrote: > > > > > Why this? > > > > > > > > That's how it worked in those panels, I pushed up code that was duplicated > > in > > > > both subclasses. > > > > > > Let's either understand why it's here (using blame) or remove it. > > > > Looks like "it had always been this way" - I traced it back to > > https://bugs.webkit.org/show_bug.cgi?id=49864 > > > > Should I remove it? > > Yes, please. Done.
The CQ bit was checked by eostroukhov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2649923006/#ps260001 (title: "[DevTools] Add filtering to DOM storages")
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": 260001, "attempt_start_ts": 1485887956539420,
"parent_rev": "893d5184aad6e4a88fa7e46b11202d1a4a7f3009", "commit_rev":
"2ed48364ede3c10466f4d854fe6684bef63e645a"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Add filtering to DOM storages 1. Extracted toolbar setup code to share the implementation between cookies and DOM storage panes. 2. Wired DOM storage pane to support filtering. BUG=684204,685314 ========== to ========== [DevTools] Add filtering to DOM storages 1. Extracted toolbar setup code to share the implementation between cookies and DOM storage panes. 2. Wired DOM storage pane to support filtering. BUG=684204,685314 Review-Url: https://codereview.chromium.org/2649923006 Cr-Commit-Position: refs/heads/master@{#447343} Committed: https://chromium.googlesource.com/chromium/src/+/2ed48364ede3c10466f4d854fe66... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/2ed48364ede3c10466f4d854fe66... |
