DevTools: Split SoftDropdown out of ConsoleContextSelector

DevTools: Split SoftDropdown out of ConsoleContextSelector

BUG=none

Review-Url: https://codereview.chromium.org/2911363002
Cr-Commit-Position: refs/heads/master@{#478854}
Committed: https://chromium.googlesource.com/chromium/src/+/ac01d0ced61e8512ec144b6689bdfb633732d618

[einbinder, 2017-05-30 22:30:40]

einbinder@chromium.org changed reviewers:
+ caseq@chromium.org

[einbinder, 2017-05-30 22:30:41]

ptal

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css (right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:56: color:
white;
I am not sure where this CSS should go. The FilteredListWidget passes the title
and subtitle to directly to the delegate. Or each delegate could inject their
own stylesheet?

[caseq, 2017-05-30 23:40:08]

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:51:
resourceTreeModel.domModel().overlayModel().highlightFrame(item.frameId);
why not directly get OverlayModel for target?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:1: // Copyright
2015 The Chromium Authors. All rights reserved.
2017 :-)

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:28:
this._list.element.tabIndex = -1;
This belongs to UI.ListControl, I'm already moving it there
(https://chromium-review.googlesource.com/c/517295)

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:29:
this._rowHeight = 36;
Can I have a different rowHeight in my control?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:43:
event.consume(true);
nit: this could be written shorter as event => void event.consume(true);

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:56:
shadowRoot.appendChild(dropdownArrowIcon);
please move this up to where we setup this.element for better readability.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:70:
this._list.scrollItemIntoView(this._selectedItem, true);
This should not be necessary, selectItem() above should take care of this.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:72:
this.element.focus();
Do we need focus on the button after _show()?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:79:
this._glassPane.setMaxContentSize(new UI.Size(315, maxHeight));
Why 315? Also, some use cases may call for larger width. Perhaps, extract
constant and make it configurable?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:99: case
'ArrowUp':
This and the one below are actually already handled in the UI.ListControl.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:105: case
'ArrowRight':
The very notion of depth is rather specific to the usage in
ConsoleContextSelector. One does not generally have a notion of 'depth' in the
list -- can we keep this logic in the context selector?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:126: case
'PageUp':
This and the one below are also handled in UI.ListControl.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:132: case
'Home':
Should we add Home/End to UI.ListControl as well?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:204: if
(this._list.indexOfItem(item) !== -1)
Do we really need this check?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:217:
this._selectedItem = item;
Do we need our own copy of the selected item? Perhaps just always get it from
the underlying UI.ListControl?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:228:
element.style.paddingLeft = (8 + this._delegate.depthFor(item) * 15) + 'px';
This is too specific, let's keep in in the console context selector.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:229:
element.addEventListener('mousemove', e => {
Let's have one mousemove listener for the entire conrtrol instead.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css (right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:56: color:
white;
On 2017/05/30 22:30:41, einbinder wrote:
> I am not sure where this CSS should go. The FilteredListWidget passes the
title
> and subtitle to directly to the delegate. Or each delegate could inject their
> own stylesheet?

I think each item should have its own shadow DOM and be styled by the delegate.

[einbinder, 2017-05-31 21:14:27]

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:51:
resourceTreeModel.domModel().overlayModel().highlightFrame(item.frameId);
On 2017/05/30 at 23:40:07, caseq wrote:
> why not directly get OverlayModel for target?

I didn't realize you could just do that.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:28:
this._list.element.tabIndex = -1;
On 2017/05/30 at 23:40:07, caseq wrote:
> This belongs to UI.ListControl, I'm already moving it there
(https://chromium-review.googlesource.com/c/517295)

I guess that is ok. It removes the ability to have a list inside something
focusable but I don't think we do that anywhere.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:29:
this._rowHeight = 36;
On 2017/05/30 at 23:40:08, caseq wrote:
> Can I have a different rowHeight in my control?

Yeah, but we will need to figure out who owns what css.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:43:
event.consume(true);
On 2017/05/30 at 23:40:08, caseq wrote:
> nit: this could be written shorter as event => void event.consume(true);

Done.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:56:
shadowRoot.appendChild(dropdownArrowIcon);
On 2017/05/30 at 23:40:08, caseq wrote:
> please move this up to where we setup this.element for better readability.

Done.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:70:
this._list.scrollItemIntoView(this._selectedItem, true);
On 2017/05/30 at 23:40:07, caseq wrote:
> This should not be necessary, selectItem() above should take care of this.

Done.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:72:
this.element.focus();
On 2017/05/30 at 23:40:08, caseq wrote:
> Do we need focus on the button after _show()?

Yes, because we cancel the mousedown.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:79:
this._glassPane.setMaxContentSize(new UI.Size(315, maxHeight));
On 2017/05/30 at 23:40:08, caseq wrote:
> Why 315? Also, some use cases may call for larger width. Perhaps, extract
constant and make it configurable?

Yep.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:99: case
'ArrowUp':
On 2017/05/30 at 23:40:07, caseq wrote:
> This and the one below are actually already handled in the UI.ListControl.

The soft drop down needs to know if the key was handled in order to update the
selection.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:105: case
'ArrowRight':
On 2017/05/30 at 23:40:08, caseq wrote:
> The very notion of depth is rather specific to the usage in
ConsoleContextSelector. One does not generally have a notion of 'depth' in the
list -- can we keep this logic in the context selector?

Putting it back into the ConsoleContextSelector would involve making a lot of
things public on the dropdown. The frames selector will want depth as well, so
it makes sense to me to leave it.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:132: case
'Home':
On 2017/05/30 at 23:40:08, caseq wrote:
> Should we add Home/End to UI.ListControl as well?

We can, but that seems like something for a different patch.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:204: if
(this._list.indexOfItem(item) !== -1)
On 2017/05/30 at 23:40:08, caseq wrote:
> Do we really need this check?

The list asserts that the item exists before it can be removed. The dropdown
doesn't expose its contents at all, which makes it hard to use removeItem
safely.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:217:
this._selectedItem = item;
On 2017/05/30 at 23:40:08, caseq wrote:
> Do we need our own copy of the selected item? Perhaps just always get it from
the underlying UI.ListControl?

The selection in the dropdown is separate from that in the list. The
selectedItem in the list corresponds to the highlighted item in the dropdown.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:228:
element.style.paddingLeft = (8 + this._delegate.depthFor(item) * 15) + 'px';
On 2017/05/30 at 23:40:08, caseq wrote:
> This is too specific, let's keep in in the console context selector.

If we go with the SoftDropDown handling depth, it made sense to me to show it in
some standard way.

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:229:
element.addEventListener('mousemove', e => {
On 2017/05/30 at 23:40:08, caseq wrote:
> Let's have one mousemove listener for the entire conrtrol instead.

Why?

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css (right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:56: color:
white;
On 2017/05/30 at 23:40:08, caseq wrote:
> On 2017/05/30 22:30:41, einbinder wrote:
> > I am not sure where this CSS should go. The FilteredListWidget passes the
title
> > and subtitle to directly to the delegate. Or each delegate could inject
their
> > own stylesheet?
> 
> I think each item should have its own shadow DOM and be styled by the
delegate.

Done.

[einbinder, 2017-06-05 21:03:39]

ptal, rebased on ListModel patch

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:228:
element.style.paddingLeft = (8 + this._delegate.depthFor(item) * 15) + 'px';
On 2017/05/31 at 21:14:27, einbinder wrote:
> On 2017/05/30 at 23:40:08, caseq wrote:
> > This is too specific, let's keep in in the console context selector.
> 
> If we go with the SoftDropDown handling depth, it made sense to me to show it
in some standard way.

Removed depth-aware keyboard shortcuts, and moved depth into console console
context selector.

[caseq, 2017-06-06 18:06:00]

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:97: case
'ArrowUp':
Can you drop this and UI.ListControl do its job here?

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:108: handled =
this._list.selectItemNextPage(false);
ditto.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:131: case 'Tab':
Should we also consume the event in all these cases?

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:174: this._width
= width;
_updateGlasspaneSize() perhaps?

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:256:
_selectHighlightedItem() {
this.selectItem(this._list.selectedItem()), then inline _updateSelectedItem

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:293:
renderElementForItem(element, item) {
Why not have it consistent with UI.ListDelegate?

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css
(right):

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/inspectorCommon.css:11:
min-width: 0;
wrong indent! actually, please revert the entire file.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/textButton.css (right):

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/textButton.css:16: min-height:
2em !important;
We avoid using 'em'.

[dgozman, 2017-06-06 19:09:21]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2017-06-06 19:09:22]

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:180:
setRowHeight(rowHeight) {
Why not go to delegate? Looks like artificial restriction with no benefits.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:218:
this._delegate.renderElementForItem(element, item);
You should first ask delegate for an item, and then wrap it if needed. Do not
let them render your own elements.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:306:
itemSelected(item) {
Is this like 'onchange' event?

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:312:
itemHighlighted(item) {
Should we be consistent with list?
highlightedItemChanged(from, to)

[einbinder, 2017-06-08 23:43:27]

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:97: case
'ArrowUp':
On 2017/06/06 at 18:06:00, caseq wrote:
> Can you drop this and UI.ListControl do its job here?

No. List control doesn't return whether or not the key was handled, so the
selection won't be properly updated.
The list also doesn't have focus.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:131: case 'Tab':
On 2017/06/06 at 18:06:00, caseq wrote:
> Should we also consume the event in all these cases?

This gets consumed by the hide.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:174: this._width
= width;
On 2017/06/06 at 18:06:00, caseq wrote:
> _updateGlasspaneSize() perhaps?

Done.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:180:
setRowHeight(rowHeight) {
On 2017/06/06 at 19:09:21, dgozman wrote:
> Why not go to delegate? Looks like artificial restriction with no benefits.

This gets really messy with getting the height back out of the ListControl for
sizing the GlassPane.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:218:
this._delegate.renderElementForItem(element, item);
On 2017/06/06 at 19:09:21, dgozman wrote:
> You should first ask delegate for an item, and then wrap it if needed. Do not
let them render your own elements.

Done.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:256:
_selectHighlightedItem() {
On 2017/06/06 at 18:06:00, caseq wrote:
> this.selectItem(this._list.selectedItem()), then inline _updateSelectedItem

Done.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:293:
renderElementForItem(element, item) {
On 2017/06/06 at 18:06:00, caseq wrote:
> Why not have it consistent with UI.ListDelegate?

Done.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:306:
itemSelected(item) {
On 2017/06/06 at 19:09:22, dgozman wrote:
> Is this like 'onchange' event?

Yes.

https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:312:
itemHighlighted(item) {
On 2017/06/06 at 19:09:21, dgozman wrote:
> Should we be consistent with list?
> highlightedItemChanged(from, to)

The list does that to encourage you setting your own css classes on highlight.
The dropdown already sets the highlight class, so it doesn't seem necessary.

[caseq, 2017-06-09 18:53:17]

On 2017/06/08 23:43:27, einbinder wrote:
>
https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):
> 
>
https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:97: case
> 'ArrowUp':
> On 2017/06/06 at 18:06:00, caseq wrote:
> > Can you drop this and UI.ListControl do its job here?
> 
> No. List control doesn't return whether or not the key was handled, so the
> selection won't be properly updated.
> The list also doesn't have focus.

You're still handling selectedItemChanged() from the ListControl, is there a
reason to not update the current selection based on selection in the list?
It's also focusable now, so why not focus it?

[einbinder, 2017-06-09 21:17:51]

On 2017/06/09 at 18:53:17, caseq wrote:
> On 2017/06/08 23:43:27, einbinder wrote:
> >
https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js
(right):
> > 
> >
https://codereview.chromium.org/2911363002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:97: case
> > 'ArrowUp':
> > On 2017/06/06 at 18:06:00, caseq wrote:
> > > Can you drop this and UI.ListControl do its job here?
> > 
> > No. List control doesn't return whether or not the key was handled, so the
> > selection won't be properly updated.
> > The list also doesn't have focus.
> 
> You're still handling selectedItemChanged() from the ListControl, is there a
reason to not update the current selection based on selection in the list?
> It's also focusable now, so why not focus it?

You can just the arrow keys to change selection even when the list isn't open.
The native control doesn't move focus onto its list.
Selection in the list changes on mouse hover. The native control doesn't update
on hover so that escape cancels the current selection.

[caseq, 2017-06-10 00:52:54]

lgtm

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:247:
* @return {!Element} element
drop 'element'

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:327:
this._dropDown.refreshItem(executionContext);
refresh on model instead?

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:343:
this._dropDown.refreshItem(executionContext);
ditto.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js (right):

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SoftDropDown.js:269:
refreshItem(item) {
Drop it?

[dgozman, 2017-06-12 17:44:43]

lgtm

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:250:
var element = createElementWithClass('div', 'context');
'context' is not used in css.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:327:
this._dropDown.refreshItem(executionContext);
On 2017/06/10 00:52:54, caseq wrote:
> refresh on model instead?

You mean replace it by itself? I find that refreshing in UI when some attribute
you return through delegate changes is reasonable.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css (right):

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:7: padding: 2px
1px 2px 2px;
Remove.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:8: white-space:
nowrap;
Remove.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:10:
flex-direction: column;
Remove.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:11: height:
36px;
Remove.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:12:
justify-content: center;
stretch instead?

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:19: .title {
Remove this - drop down does not add any titles.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:25: .badge {
Remove this - drop down does not add any badges.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:32: .subtitle {
Remove this - drop down does not add any subtitles.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:55:
.item.highlighted .subtitle {
Remove this - drop down does not add any subtitles.

[einbinder, 2017-06-12 22:23:22]

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js
(right):

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:247:
* @return {!Element} element
On 2017/06/10 at 00:52:54, caseq wrote:
> drop 'element'

Done.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/console/ConsoleContextSelector.js:250:
var element = createElementWithClass('div', 'context');
On 2017/06/12 at 17:44:42, dgozman wrote:
> 'context' is not used in css.

Done.

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css (right):

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:7: padding: 2px
1px 2px 2px;
On 2017/06/12 at 17:44:43, dgozman wrote:
> Remove.

This whole rule should have been in consoleContextSelector.css. Moved

https://codereview.chromium.org/2911363002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/softDropDown.css:55:
.item.highlighted .subtitle {
On 2017/06/12 at 17:44:43, dgozman wrote:
> Remove this - drop down does not add any subtitles.

Done.

[einbinder, 2017-06-12 22:23:24]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2017-06-12 22:23:25]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2911363002/#ps160001
(title: "fixes")

[commit-bot: I haz the power, 2017-06-12 22:24:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 01:16:49]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497306204629810,
"parent_rev": "0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67", "commit_rev":
"ac01d0ced61e8512ec144b6689bdfb633732d618"}

[commit-bot: I haz the power, 2017-06-13 01:17:05]

Description was changed from

==========
DevTools: Split SoftDropdown out of ConsoleContextSelector

BUG=none
==========

to

==========
DevTools: Split SoftDropdown out of ConsoleContextSelector

BUG=none

Review-Url: https://codereview.chromium.org/2911363002
Cr-Commit-Position: refs/heads/master@{#478854}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac01d0ced61e8512ec144b6689bd...
==========

[commit-bot: I haz the power, 2017-06-13 01:17:07]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ac01d0ced61e8512ec144b6689bd...