Chromium Code Reviews
Help | Chromium Project | Sign in
(8)

Issue 2773583002: [DevTools] Introduce a sidebar with a drop-down

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by eostroukhov
Modified:
3 weeks ago
Reviewers:
caseq, dgozman
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/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: 15
Unified diffs Side-by-side diffs Delta from patch set Stats (+432 lines, -9 lines) Patch
M third_party/WebKit/Source/devtools/BUILD.gn View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/main/Main.js View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/quick_open/FilteredListWidget.js View 1 2 11 chunks +78 lines, -5 lines 6 comments Download
M third_party/WebKit/Source/devtools/front_end/quick_open/filteredListWidget.css View 1 2 2 chunks +20 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/devtools/front_end/resources/FrameMenu.js View 1 2 1 chunk +239 lines, -0 lines 7 comments Download
A third_party/WebKit/Source/devtools/front_end/resources/FrameMenuSidebar.js View 1 2 1 chunk +38 lines, -0 lines 1 comment Download
M third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js View 1 2 1 chunk +4 lines, -1 line 0 comments Download
A third_party/WebKit/Source/devtools/front_end/resources/frameMenu.css View 1 2 1 chunk +9 lines, -0 lines 1 comment Download
A third_party/WebKit/Source/devtools/front_end/resources/frameMenuSidebar.css View 1 2 1 chunk +33 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/devtools/front_end/resources/module.json View 1 2 3 chunks +6 lines, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 14 (9 generated)
eostroukhov
Please take a look. This is initial commit for this new functionality, I will be ...
1 month ago (2017-03-23 23:35:12 UTC) #6
dgozman
https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Source/devtools/front_end/main/Main.js File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Source/devtools/front_end/main/Main.js#newcode102 third_party/WebKit/Source/devtools/front_end/main/Main.js:102: Runtime.experiments.register('framesSelectorAppPanel', 'Show frames selector on the Application panel', true); ...
1 month ago (2017-03-24 17:06:55 UTC) #7
dgozman
Please include a screenshot either here or in the bug.
1 month ago (2017-03-24 17:07:38 UTC) #8
eostroukhov
Redid the UX, moved to filtered list. Please take another look. https://codereview.chromium.org/2773583002/diff/20001/third_party/WebKit/Source/devtools/front_end/main/Main.js File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): ...
3 weeks, 1 day ago (2017-04-04 23:47:17 UTC) #11
dgozman
3 weeks ago (2017-04-05 20:52:56 UTC) #14
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!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46