Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2773583002:
[DevTools] Introduce a sidebar with a drop-down
|
Created:
3 years, 9 months ago by eostroukhov Modified:
3 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Introduce a sidebar with a drop-down
Introduces a frames dropdown and a new sidebar implementation that uses
this drop-down. New sidebar is currently behind the experiment.
BUG=698027
Patch Set 1 #Patch Set 2 : [DevTools] Introduce a sidebar with a drop-down #
Total comments: 22
Patch Set 3 : [DevTools] Introduce a sidebar with a drop-down #
Total comments: 30
Patch Set 4 : [DevTools] Introduce a sidebar with a drop-down #
Total comments: 6
Patch Set 5 : [DevTools] Introduce a sidebar with a drop-down #
Total comments: 6
Patch Set 6 : [DevTools] Introduce a sidebar with a drop-down #Patch Set 7 : [DevTools] Introduce a sidebar with a drop-down #
Total comments: 5
Messages
Total messages: 41 (29 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
Please take a look. This is initial commit for this new functionality, I will be gradually porting individual features in followup commits.
https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/Main.js:102: Runtime.experiments.register('framesSelectorAppPanel', 'Show frames selector on the Application panel', true); Sort alphabetically by title. And by id at the same time :-) https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebarWithFramesSelector.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebarWithFramesSelector.js:5: Resources.ApplicationPanelSidebarWithFramesSelector = class extends UI.VBox { Let's come up with a shorter name please. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:5: Resources.FrameSelector = class extends Common.Object { Let's think about reusing this in Console right now (to not change a lot later). https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:59: function addListener(eventType, handler, target) { Just spell it out 3 times - makes it easier to search for common patterns. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:66: var mainTarget = SDK.targetManager.mainTarget(); Let's make this multitarget right away. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:67: var resourceTreeModel = mainTarget && mainTarget.hasDOMCapability() && SDK.ResourceTreeModel.fromTarget(mainTarget); You don't have to check capability anymore: mainTarget.model(SDK.ResourceTreeModel) does it for you. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:145: // anchorBox.height = childBox.y + childBox.height - anchorBox.y; Commented code. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:294: Resources._FrameTreeElement = class extends UI.TreeElement { Why do we need a treeoutline? Let's reuse FilteredListWidget - you get filtering and viewporting for free. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:309: get itemURL() { Unused. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/frameSelector.css (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/frameSelector.css:1: .frame-selector-popup { Missing copyright. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/resourcesPanel.css (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/resourcesPanel.css:166: .frame-selector { This should go to a separate applicationSidebar.css.
Please include a screenshot either here or in the bug.
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...
Redid the UX, moved to filtered list. Please take another look. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/Main.js:102: Runtime.experiments.register('framesSelectorAppPanel', 'Show frames selector on the Application panel', true); On 2017/03/24 17:06:54, dgozman wrote: > Sort alphabetically by title. And by id at the same time :-) Done :) https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebarWithFramesSelector.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/ApplicationPanelSidebarWithFramesSelector.js:5: Resources.ApplicationPanelSidebarWithFramesSelector = class extends UI.VBox { On 2017/03/24 17:06:54, dgozman wrote: > Let's come up with a shorter name please. Done. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:5: Resources.FrameSelector = class extends Common.Object { On 2017/03/24 17:06:54, dgozman wrote: > Let's think about reusing this in Console right now (to not change a lot later). Acknowledged. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:59: function addListener(eventType, handler, target) { On 2017/03/24 17:06:54, dgozman wrote: > Just spell it out 3 times - makes it easier to search for common patterns. Done, but I did that lambda so I don't have to repeat the "type cast" comment. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:66: var mainTarget = SDK.targetManager.mainTarget(); On 2017/03/24 17:06:54, dgozman wrote: > Let's make this multitarget right away. Please take a look. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:67: var resourceTreeModel = mainTarget && mainTarget.hasDOMCapability() && SDK.ResourceTreeModel.fromTarget(mainTarget); On 2017/03/24 17:06:54, dgozman wrote: > You don't have to check capability anymore: > mainTarget.model(SDK.ResourceTreeModel) does it for you. Done. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:145: // anchorBox.height = childBox.y + childBox.height - anchorBox.y; On 2017/03/24 17:06:54, dgozman wrote: > Commented code. Oops. Fixed! https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:294: Resources._FrameTreeElement = class extends UI.TreeElement { On 2017/03/24 17:06:54, dgozman wrote: > Why do we need a treeoutline? Let's reuse FilteredListWidget - you get filtering > and viewporting for free. Done. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameSelector.js:309: get itemURL() { On 2017/03/24 17:06:54, dgozman wrote: > Unused. Done. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/frameSelector.css (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/frameSelector.css:1: .frame-selector-popup { On 2017/03/24 17:06:54, dgozman wrote: > Missing copyright. Done. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/resourcesPanel.css (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/resourcesPanel.css:166: .frame-selector { On 2017/03/24 17:06:55, dgozman wrote: > This should go to a separate applicationSidebar.css. The whole frame-selector is gone now, replaced by the filtered list.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:195: var element = event.deepElementFromPoint(); Implement this similarly to click? var item = this._list.itemForNode(/** @type {?Node} */ (event.target)); ... https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:199: for (var child of this._list.element.getElementsByClassName('hovered')) Introduce |this._hoverElement| and remove the class in this._hover. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:203: listItemElement.classList.add('hovered'); This should go to _hover. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:310: this._hover(to); Why this? https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:316: selectItem(item) { setInitialSelection https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:320: this._selectedItemIndex = item; _initialSelectedItem https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:27: * @return {!Array<!Resources.FrameMenu._ListItem>} Let's not return since this function actually populates the |array|. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:30: var frames = framesTree.get(parentId); Looks like you call this with |null| parentId first time and immediately return empty array? https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:42: * @param {!Map<?string, !Array<!SDK.ResourceTreeFrame>>} framesTree Why frame id is nullable? I don't see where you use it. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:74: get element() { No getters. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:120: reset() { Inline this one. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:149: Resources.FrameMenu._ListProvider = class extends QuickOpen.FilteredListWidget.Provider { Why not merge with FrameMenu itself? https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:187: var domModel = frame && frame.target().model(SDK.DOMModel); How there could be no frame? https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js:25: this._frameMenu.selectRootFrame(); I'd move this logic to FrameMenu instead. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/frameMenu.css (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/frameMenu.css:1: .frame-selector-popup { Copyright!
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
Thank you for your review. A new version was uploaded, please take another look. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:195: var element = event.deepElementFromPoint(); On 2017/04/05 20:52:55, dgozman wrote: > Implement this similarly to click? > > var item = this._list.itemForNode(/** @type {?Node} */ (event.target)); > ... Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:199: for (var child of this._list.element.getElementsByClassName('hovered')) On 2017/04/05 20:52:55, dgozman wrote: > Introduce |this._hoverElement| and remove the class in this._hover. Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:203: listItemElement.classList.add('hovered'); On 2017/04/05 20:52:56, dgozman wrote: > This should go to _hover. Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:310: this._hover(to); On 2017/04/05 20:52:55, dgozman wrote: > Why this? For keyboard navigation. It is extremely confusing if highlighted frame is different from the one you are stepping onto with arrow keys. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:316: selectItem(item) { On 2017/04/05 20:52:55, dgozman wrote: > setInitialSelection Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:320: this._selectedItemIndex = item; On 2017/04/05 20:52:55, dgozman wrote: > _initialSelectedItem Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:27: * @return {!Array<!Resources.FrameMenu._ListItem>} On 2017/04/05 20:52:56, dgozman wrote: > Let's not return since this function actually populates the |array|. Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:30: var frames = framesTree.get(parentId); On 2017/04/05 20:52:56, dgozman wrote: > Looks like you call this with |null| parentId first time and immediately return > empty array? Not sure I understand. Can you elaborate? https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:42: * @param {!Map<?string, !Array<!SDK.ResourceTreeFrame>>} framesTree On 2017/04/05 20:52:56, dgozman wrote: > Why frame id is nullable? I don't see where you use it. It is for frames with null parent, e.g. for the root frame. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:74: get element() { On 2017/04/05 20:52:56, dgozman wrote: > No getters. Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:120: reset() { On 2017/04/05 20:52:56, dgozman wrote: > Inline this one. It was not used anywhere anyways :) https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:149: Resources.FrameMenu._ListProvider = class extends QuickOpen.FilteredListWidget.Provider { On 2017/04/05 20:52:56, dgozman wrote: > Why not merge with FrameMenu itself? I felt like it was more natural as it ties frames array lifetime to one of the popup. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:187: var domModel = frame && frame.target().model(SDK.DOMModel); On 2017/04/05 20:52:56, dgozman wrote: > How there could be no frame? Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js:25: this._frameMenu.selectRootFrame(); On 2017/04/05 20:52:56, dgozman wrote: > I'd move this logic to FrameMenu instead. Done. https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/frameMenu.css (right): https://codereview.chromium.org/2773583002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/frameMenu.css:1: .frame-selector-popup { On 2017/04/05 20:52:56, dgozman wrote: > Copyright! 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: This issue passed the CQ dry run.
dgozman@chromium.org changed reviewers: + einbinder@chromium.org
+Joel for FilteredListWidget, who is also working on similar thing for console. https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:310: this._hover(to); I don't think it's a good idea to mix up mouse accessibility and keyboard navigation. https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:572: this._hoveredListElement.classList.remove('hovered'); Let's do this with :hover css class, and remove ListControl.elementForItem. https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js (right): https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:109: Resources.FrameMenu._collectFrames(target, framesTree, visitedTargets); Let's move this to static ResourceTreeModel.frames() method.
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've uploaded a new version. Please take another look. https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:310: this._hover(to); On 2017/05/03 17:32:58, dgozman wrote: > I don't think it's a good idea to mix up mouse accessibility and keyboard > navigation. In the newer version it is now called "highlight" so I think it better reflects the intent. https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:572: this._hoveredListElement.classList.remove('hovered'); On 2017/05/03 17:32:58, dgozman wrote: > Let's do this with :hover css class, and remove ListControl.elementForItem. I was not sure if it was ok to use pseudoclasses :) I switched to pseudoclass and the code became radically simpler... https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js (right): https://codereview.chromium.org/2773583002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:109: Resources.FrameMenu._collectFrames(target, framesTree, visitedTargets); On 2017/05/03 17:32:58, dgozman wrote: > Let's move this to static ResourceTreeModel.frames() method. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:43: this._itemElementsContainer.addEventListener('mousemove', event => this._updateHover(event)); I think if you use 'mouseover' you can get away without storing the highlightedItem. https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:317: if (this._list.length() > 0) When can the list be non-empty here? https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ListControl.js (right): https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ListControl.js:230: elementForItem(item) { This is unused.
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. It is ready for another pass now :) https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js (right): https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:43: this._itemElementsContainer.addEventListener('mousemove', event => this._updateHover(event)); On 2017/05/04 07:05:09, einbinder wrote: > I think if you use 'mouseover' you can get away without storing the > highlightedItem. Done. https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js:317: if (this._list.length() > 0) On 2017/05/04 07:05:09, einbinder wrote: > When can the list be non-empty here? If this is called while the popup is visible. This method used to be called "setSelection" :) https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/ListControl.js (right): https://codereview.chromium.org/2773583002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/ListControl.js:230: elementForItem(item) { On 2017/05/04 07:05:09, einbinder wrote: > This is unused. Right. Removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== [DevTools] Introduce a sidebar with a drop-down Introduces a frames dropdown and a new sidebar implementation that uses this drop-down. New sidebar is currently behind the experiment. BUG=698027 ========== to ========== [DevTools] Introduce a sidebar with a drop-down Introduces a frames dropdown and a new sidebar implementation that uses this drop-down. New sidebar is currently behind the experiment. BUG=698027 ==========
eostroukhov@chromium.org changed reviewers: - caseq@chromium.org, dgozman@chromium.org, einbinder@chromium.org
The CQ bit was checked by eostroukhov@chromium.org to run a CQ dry run
eostroukhov@chromium.org changed reviewers: + caseq@chromium.org, dgozman@chromium.org
This CL is again up for consideration. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js (right): https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:66: var list = new QuickOpen.FilteredListWidget(provider); I think this should use a different control, preferably be consistent with console execution context selector. https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js:97: Resources.FrameMenu._ListItem = class { nit: convert to typedef? https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js (right): https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js:7: super(); Make it shadow, i.e. super(true)? https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js (right): https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:103: * @param {?SDK.ResourceTreeFrame} a Should it be nullable? https://codereview.chromium.org/2773583002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/sdk/ResourceTreeModel.js:108: var framesA = a ? SDK.ResourceTreeModel.framePath(a) : []; How about this: var framePathA = SDK.ResourceTreeModel.framePath(a).map(_ => _.id).join('\1'); var framePathB = SDK.ResourceTreeModel.framePath(b).map(_ => _.id).join('\1'); return framePathA.localeCompare(framePathB) |
