Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2662403002:
[DevTools] Merge filter bar with the main toolbar (Closed)
|
Created:
3 years, 10 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] Merge filter bar with the main toolbar
BUG=684204
Review-Url: https://codereview.chromium.org/2662403002
Cr-Commit-Position: refs/heads/master@{#448903}
Committed: https://chromium.googlesource.com/chromium/src/+/db69128064ff7433c7243e4387294b70dc052de1
Patch Set 1 #
Total comments: 8
Patch Set 2 : [DevTools] Merge filter bar with the main toolbar #Patch Set 3 : [DevTools] Merge filter bar with the main toolbar #
Total comments: 12
Patch Set 4 : [DevTools] Merge filter bar with the main toolbar #
Total comments: 12
Patch Set 5 : [DevTools] Merge filter bar with the main toolbar #
Total comments: 4
Patch Set 6 : [DevTools] Merge filter bar with the main toolbar #
Total comments: 4
Patch Set 7 : [DevTools] Merge filter bar with the main toolbar #
Messages
Total messages: 39 (25 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, pfeldman@chromium.org, phulce@chromium.org
Please take a look. I also attached a screenshot to the bug.
https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:239: if (!root || root.children.length === 0) There is always a rootNode. https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/FilterToolbarItem.js (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/FilterToolbarItem.js:5: Resources.FilterToolbarItem = class extends UI.ToolbarItem { - Should inherit from UI controls - only compose them. - Why not just use ToolbarInput? - I'd just inline it into StorageItemsView. https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:123: focusGrid() { Why not just focus? https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/filterToolbarItem.css (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/filterToolbarItem.css:1: .filter-input-body { Missing copyright.
> - Should inherit from UI controls - only compose them. Ops, I meant should _not_ inherit.
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...
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 redid the patch, this time enabling the UI.ToolbarInput to act as a filter field. I manually verified other places where the field is used to check if there's no regressions. Please take another look. https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:239: if (!root || root.children.length === 0) On 2017/02/01 00:28:35, dgozman_slow wrote: > There is always a rootNode. Done. https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/FilterToolbarItem.js (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/FilterToolbarItem.js:5: Resources.FilterToolbarItem = class extends UI.ToolbarItem { On 2017/02/01 00:28:35, dgozman_slow wrote: > - Should inherit from UI controls - only compose them. > - Why not just use ToolbarInput? > - I'd just inline it into StorageItemsView. Not relevant any more - I removed this class. https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:123: focusGrid() { On 2017/02/01 00:28:35, dgozman_slow wrote: > Why not just focus? I feel like "focus" would be ambiguous as the pane at least has a toolbar. https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/resources/filterToolbarItem.css (right): https://codereview.chromium.org/2662403002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/resources/filterToolbarItem.css:1: .filter-input-body { On 2017/02/01 00:28:35, dgozman_slow wrote: > Missing copyright. File's gone
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:169: if (!this._dataGrid || this._dataGrid.rootNode().children.length === 0) nit: !.....length https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:239: if (root.children.length === 0) Make it similar to another place: if (!this._dataGrid || !this._dataGrid.rootNode().children.length) return; https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:587: * @param {boolean=} notify Don't add parameters to public methods if they are not intended to be used by clients. If you need this, add a private function. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:602: _onKeydownCallback(event) { JSDoc please. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:604: this.dispatchEventToListeners(UI.ToolbarInput.Event.FocusOnResultsRequested, this); I don't think this is too generic to be in ToolbarInput. Let's expose KeyDown event (with data == |event|) instead, and let clients handle ArrowDown as they want. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:605: if (event.code !== 'Escape' || !this._input.value) Escape is good though.
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 comments. Addressed. Please take another look. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:169: if (!this._dataGrid || this._dataGrid.rootNode().children.length === 0) On 2017/02/03 20:54:53, dgozman (OOO till Feb 13) wrote: > nit: !.....length Done. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/DOMStorageItemsView.js:239: if (root.children.length === 0) On 2017/02/03 20:54:53, dgozman (OOO till Feb 13) wrote: > Make it similar to another place: > > if (!this._dataGrid || !this._dataGrid.rootNode().children.length) > return; Done. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:587: * @param {boolean=} notify On 2017/02/03 20:54:53, dgozman (OOO till Feb 13) wrote: > Don't add parameters to public methods if they are not intended to be used by > clients. If you need this, add a private function. Done. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:602: _onKeydownCallback(event) { On 2017/02/03 20:54:53, dgozman (OOO till Feb 13) wrote: > JSDoc please. Done. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:604: this.dispatchEventToListeners(UI.ToolbarInput.Event.FocusOnResultsRequested, this); On 2017/02/03 20:54:53, dgozman (OOO till Feb 13) wrote: > I don't think this is too generic to be in ToolbarInput. Let's expose KeyDown > event (with data == |event|) instead, and let clients handle ArrowDown as they > want. Done. https://codereview.chromium.org/2662403002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:605: if (event.code !== 'Escape' || !this._input.value) On 2017/02/03 20:54:53, dgozman (OOO till Feb 13) wrote: > Escape is good though. Acknowledged.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:177: focus() { No need to override, I think dataGrid will get focus method called. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:118: this._cookiesTable.selectFirstCookie(); You are a view, there already is a way to override 'focus()' method if need be. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:56: _onFilterKeyDown(event) { Drop this, can be added later. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:67: var text = event.data; Cast to string. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:615: if (this.isSearchField || event.code !== 'Escape' || !this._input.value) isEscKey(event) https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:628: KeyDown: Symbol('KeyDown') Don't expose it - it needs to be a part of traversal policy.
Fixed what I could :/ Please take another look. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:177: focus() { On 2017/02/06 19:37:42, pfeldman wrote: > No need to override, I think dataGrid will get focus method called. No it does not... https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:118: this._cookiesTable.selectFirstCookie(); On 2017/02/06 19:37:42, pfeldman wrote: > You are a view, there already is a way to override 'focus()' method if need be. This is for moving focus within the view. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:56: _onFilterKeyDown(event) { On 2017/02/06 19:37:42, pfeldman wrote: > Drop this, can be added later. Why? I added it because as a user, I tried this and it didn't work. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:67: var text = event.data; On 2017/02/06 19:37:42, pfeldman wrote: > Cast to string. Done. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:615: if (this.isSearchField || event.code !== 'Escape' || !this._input.value) On 2017/02/06 19:37:42, pfeldman wrote: > isEscKey(event) Done. https://codereview.chromium.org/2662403002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:628: KeyDown: Symbol('KeyDown') On 2017/02/06 19:37:42, pfeldman wrote: > Don't expose it - it needs to be a part of traversal policy. Done. (I've exposed the input element so the pane can set a listener directly)
https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:178: if (this._dataGrid) this.setDefaultFocusedChild(this._dataGrid) in the constructor please. https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:133: focusGrid() { Wait, we don't want to do 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...
Please take another look. https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js (right): https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/cookie_table/CookiesTable.js:178: if (this._dataGrid) On 2017/02/07 23:15:35, pfeldman wrote: > this.setDefaultFocusedChild(this._dataGrid) in the constructor please. I reverted all the changes made to CookiesTable as they are no longer needed. https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js (right): https://codereview.chromium.org/2662403002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/StorageItemsView.js:133: focusGrid() { On 2017/02/07 23:15:35, pfeldman wrote: > Wait, we don't want to do this! I removed the handler for the down arrow.
lgtm % comments https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:564: this.input.addEventListener('blur', () => this.element.classList.toggle('focused', false)); you can say classList.remove here and .add below https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right): https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:212: .toolbar-input:focus, this never happend how, right?
Thank you for the review. Uploading & submitting the new version. https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js (right): https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/Toolbar.js:564: this.input.addEventListener('blur', () => this.element.classList.toggle('focused', false)); On 2017/02/08 01:59:29, pfeldman wrote: > you can say classList.remove here and .add below Done. https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/toolbar.css (right): https://codereview.chromium.org/2662403002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/toolbar.css:212: .toolbar-input:focus, On 2017/02/08 01:59:29, pfeldman wrote: > this never happend how, right? Done.
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/2662403002/#ps120001 (title: "[DevTools] Merge filter bar with the main toolbar")
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": 120001, "attempt_start_ts": 1486519853415530,
"parent_rev": "ddc1157e707c7d3a00a43e648a0f99b98d03f085", "commit_rev":
"db69128064ff7433c7243e4387294b70dc052de1"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Merge filter bar with the main toolbar BUG=684204 ========== to ========== [DevTools] Merge filter bar with the main toolbar BUG=684204 Review-Url: https://codereview.chromium.org/2662403002 Cr-Commit-Position: refs/heads/master@{#448903} Committed: https://chromium.googlesource.com/chromium/src/+/db69128064ff7433c7243e438729... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/db69128064ff7433c7243e438729... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
