Enable keyboard navigation for Desktopcapture window selection dialog

Keyboard navigation on desktop capture screen selection dialog didn't work.

This issue was shown up in multi-platform include Windows.

The focus of the dialog was missed. I added the code for enable and request focus for the list view under the dialog.

When Desktop video capture is launching. A Desktop Media Picker Dialog is popped up to let user select source. After fixed, user should be able to use keyboard to select source. No item will be selected right after dialog launch. Click up, down, left and right keys first time will allow the first item to be selected.

BUG=527950
Committed: https://crrev.com/1998bee9f96031dec220b9b7b9e50c68de5d995d
Cr-Commit-Position: refs/heads/master@{#352701}

[GeorgeZ, 2015-09-03 20:56:21]

gyzhou@chromium.org changed reviewers:
+ pkasting@google.com

[gyzhou, 2015-09-03 22:42:27]

gyzhou@google.com changed reviewers:
+ gyzhou@google.com

[Peter Kasting, 2015-09-03 23:03:43]

(1) I'm the wrong primary reviewer for this.  Find the person who wrote this
code via git blame.
(2) This seems questionable.  Showing the dialog should already give it focus. 
I would suspect something else is wrong, especially if this is Linux-only (the
bug doesn't say what happens on Windows, which is the gold standard for UI and
what all bugs should be tested against to see what happens).
(3) Please see other code reviews for the proper structure of your CL
description, including how to notate applicable bugs.

[gyzhou, 2015-09-03 23:56:31]

gyzhou@google.com changed reviewers:
+ sergeyu@chromium.org
- gyzhou@google.com, pkasting@google.com

[gyzhou, 2015-09-03 23:57:05]

gyzhou@google.com changed reviewers:
+ gyzhou@google.com

[gyzhou, 2015-09-03 23:57:05]

Could you have a code review for me.

Thanks,

George

[Sergey Ulanov, 2015-09-14 20:59:24]

lgtm

https://codereview.chromium.org/1313843003/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_media_picker_views.cc:424: // Enable and request
keyboard focus
Please add . at the end of the sentence.

[gyzhou, 2015-09-14 22:51:28]

gyzhou@google.com changed reviewers:
+ pkasting@google.com
- sergeyu@chromium.org

[gyzhou, 2015-09-14 22:51:29]

Please Rubber stamp for me. The code had been reviewed by the author.

Thanks.

https://codereview.chromium.org/1313843003/diff/1/chrome/browser/ui/views/des...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/1/chrome/browser/ui/views/des...
chrome/browser/ui/views/desktop_media_picker_views.cc:424: // Enable and request
keyboard focus
On 2015/09/14 20:59:24, Sergey Ulanov wrote:
> Please add . at the end of the sentence.

Done.

[gyzhou, 2015-09-14 22:55:06]

Please Rubber stamp for me. The code had been reviewed by the author.

Thanks,

[Peter Kasting, 2015-09-14 23:14:49]

On 2015/09/14 22:51:29, gyzhou wrote:
> Please Rubber stamp for me. The code had been reviewed by the author.

Are you talking to me?  If there are multiple reviewers on a change, always be
clear which reviewer you're asking to do what, so your requests don't get
unintentionally ignored.

Sergey, did you see my concerns in my first comment here?  I'd like to make sure
they're unfounded before approving this.

[gyzhou, 2015-09-14 23:19:56]

On 2015/09/14 23:14:49, Peter Kasting wrote:
> On 2015/09/14 22:51:29, gyzhou wrote:
> > Please Rubber stamp for me. The code had been reviewed by the author.
> 
> Are you talking to me?  If there are multiple reviewers on a change, always be
> clear which reviewer you're asking to do what, so your requests don't get
> unintentionally ignored.
> 
> Sergey, did you see my concerns in my first comment here?  I'd like to make
sure
> they're unfounded before approving this.

I need an owner to approve. Sergey is the author but not the owner.

[Niklas Enbom, 2015-09-22 20:52:00]

niklase@chromium.org changed reviewers:
+ annacc@chromium.org

[gyzhou, 2015-09-22 21:05:20]

On 2015/09/14 23:19:56, gyzhou wrote:
> On 2015/09/14 23:14:49, Peter Kasting wrote:
> > On 2015/09/14 22:51:29, gyzhou wrote:
> > > Please Rubber stamp for me. The code had been reviewed by the author.
> > 
> > Are you talking to me?  If there are multiple reviewers on a change, always
be
> > clear which reviewer you're asking to do what, so your requests don't get
> > unintentionally ignored.
> > 
> > Sergey, did you see my concerns in my first comment here?  I'd like to make
> sure
> > they're unfounded before approving this.
> 
> I need an owner to approve. Sergey is the author but not the owner.

pkasting,

I probably didn't address your comment (2). The issue is reported cross the
platform (linux, windows, ..). dialogview class is derived from view class. By
default, view class disables the focus.

If you have any more concern, please let me know.

[Peter Kasting, 2015-09-22 21:07:56]

On 2015/09/22 21:05:20, gyzhou wrote:
> On 2015/09/14 23:19:56, gyzhou wrote:
> > On 2015/09/14 23:14:49, Peter Kasting wrote:
> > > On 2015/09/14 22:51:29, gyzhou wrote:
> > > > Please Rubber stamp for me. The code had been reviewed by the author.
> > > 
> > > Are you talking to me?  If there are multiple reviewers on a change,
always
> be
> > > clear which reviewer you're asking to do what, so your requests don't get
> > > unintentionally ignored.
> > > 
> > > Sergey, did you see my concerns in my first comment here?  I'd like to
make
> > sure
> > > they're unfounded before approving this.
> > 
> > I need an owner to approve. Sergey is the author but not the owner.
> 
> pkasting,
> 
> I probably didn't address your comment (2). The issue is reported cross the
> platform (linux, windows, ..). dialogview class is derived from view class. By
> default, view class disables the focus.

Where?  Why?  Is that correct?  Is it maybe incorrect for all dialog views?

[chromium-reviews, 2015-09-22 21:47:55]

pkasting,


In desktop_media_picker_views.h
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...>

class DesktopMediaPickerDialogView
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...>
: public views
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/di...>
{


in dialog_delegate.h

class VIEWS_EXPORT
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/views_exp...>
DialogDelegateView
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/di...>
: public DialogDelegate
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/di...>,
                                       public View
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.h&cl...>


This class implementation doesn't mention focus


in view.h

// Sets whether this view is capable of taking focus. It will clear
focus if  // the focused view is set to be non-focusable.  // Note
that this is false by default so that a view used as a container does
// not get the focus.  void SetFocusable
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.h&cl...
<https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/third_pa...>
focusable
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.h&cl...>);


From the derive chain above, it seems that DialogDelegateView
<https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/di...>
by default is non-focusable.


If you have different opinion or source, please let me know.


Thanks,


George


On Tue, Sep 22, 2015 at 2:07 PM, <pkasting@chromium.org> wrote:

> On 2015/09/22 21:05:20, gyzhou wrote:
>
>> On 2015/09/14 23:19:56, gyzhou wrote:
>> > On 2015/09/14 23:14:49, Peter Kasting wrote:
>> > > On 2015/09/14 22:51:29, gyzhou wrote:
>> > > > Please Rubber stamp for me. The code had been reviewed by the
>> author.
>> > >
>> > > Are you talking to me?  If there are multiple reviewers on a change,
>>
> always
>
>> be
>> > > clear which reviewer you're asking to do what, so your requests don't
>> get
>> > > unintentionally ignored.
>> > >
>> > > Sergey, did you see my concerns in my first comment here?  I'd like to
>>
> make
>
>> > sure
>> > > they're unfounded before approving this.
>> >
>> > I need an owner to approve. Sergey is the author but not the owner.
>>
>
> pkasting,
>>
>
> I probably didn't address your comment (2). The issue is reported cross the
>> platform (linux, windows, ..). dialogview class is derived from view
>> class. By
>> default, view class disables the focus.
>>
>
> Where?  Why?  Is that correct?  Is it maybe incorrect for all dialog views?
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gyzhou, 2015-09-22 21:56:23]

On 2015/09/22 21:07:56, Peter Kasting wrote:
> On 2015/09/22 21:05:20, gyzhou wrote:
> > On 2015/09/14 23:19:56, gyzhou wrote:
> > > On 2015/09/14 23:14:49, Peter Kasting wrote:
> > > > On 2015/09/14 22:51:29, gyzhou wrote:
> > > > > Please Rubber stamp for me. The code had been reviewed by the author.
> > > > 
> > > > Are you talking to me?  If there are multiple reviewers on a change,
> always
> > be
> > > > clear which reviewer you're asking to do what, so your requests don't
get
> > > > unintentionally ignored.
> > > > 
> > > > Sergey, did you see my concerns in my first comment here?  I'd like to
> make
> > > sure
> > > > they're unfounded before approving this.
> > > 
> > > I need an owner to approve. Sergey is the author but not the owner.
> > 
> > pkasting,
> > 
> > I probably didn't address your comment (2). The issue is reported cross the
> > platform (linux, windows, ..). dialogview class is derived from view class.
By
> > default, view class disables the focus.
> 
> Where?  Why?  Is that correct?  Is it maybe incorrect for all dialog views?

pkasting,

Replying from gmail lead to a very confused format. I do it again here in case
you have 
difficult to read my reply.  

In desktop_media_picker_views.h
class DesktopMediaPickerDialogView : public views::DialogDelegateView {

in dialog_delegate.h
class VIEWS_EXPORT DialogDelegateView : public DialogDelegate,
                                        public View 

This class implementation doesn't mention focus

in view.h
// Sets whether this view is capable of taking focus. It will clear focus if
  // the focused view is set to be non-focusable.
  // Note that this is false by default so that a view used as a container does
  // not get the focus.
  void SetFocusable(bool focusable);

From the derive chain above, it seems that DialogDelegateView by default is
non-focusable.

If you have different opinion or source, please let me know.

Thanks,

George

[Peter Kasting, 2015-09-22 22:04:32]

Can you give a screenshot of the dialog you're modifying?  It's not obvious to
me whether we actually want the dialog delegate view here to have focus, or
whether we want something inside it to have focus.

If we want the dialog to have focus, what about other dialogs?  Who else uses
DialogDelegateView?  Should those dialogs be getting keyboard focus?

The underlying theme here is: don't just answer my direct questions; think
holistically about the entire problem of keyboard accessibility in Chrome and
figure out what level we should really be changing to solve not just this bug
but all such bugs.

[chromium-reviews, 2015-09-22 22:33:08]

pkasting,

Thanks for your timely reply. The screen shot is attached.

Inside DesktopMediaPickerDaiogview, there is a DesktopMediaPickerListView.
Inside DesktopMediaPickerListView, there are several
DesktopMediaPickerSourceViews. Eventually, the focus will pass to one of
DesktopMediaPickerSourceViews through  DesktopMediaPickerListView, or to
buttons "cancel" or "share".

I appreciate that you pointed me the underlying theme that your concern.
This will benefit me in my future work.

When I created the CL, I was with google 3 weeks and my background is not
on UI. I probably cannot give you  good answers of your other questions.

Thanks,

George



On Tue, Sep 22, 2015 at 3:04 PM, <pkasting@chromium.org> wrote:

> Can you give a screenshot of the dialog you're modifying?  It's not
> obvious to
> me whether we actually want the dialog delegate view here to have focus, or
> whether we want something inside it to have focus.
>
> If we want the dialog to have focus, what about other dialogs?  Who else
> uses
> DialogDelegateView?  Should those dialogs be getting keyboard focus?
>
> The underlying theme here is: don't just answer my direct questions; think
> holistically about the entire problem of keyboard accessibility in Chrome
> and
> figure out what level we should really be changing to solve not just this
> bug
> but all such bugs.
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-09-22 22:45:29]

On 2015/09/22 22:33:08, chromium-reviews wrote:
> pkasting,
> 
> Thanks for your timely reply. The screen shot is attached.

(For future reference, post screenshots on bugs or else put them on an image
host and include URLs, so that people reading a codereview in the future can
access the screenshot.)

> Inside DesktopMediaPickerDaiogview, there is a DesktopMediaPickerListView.

Should that be given focus instead then?

> I appreciate that you pointed me the underlying theme that your concern.
> This will benefit me in my future work.
> 
> When I created the CL, I was with google 3 weeks and my background is not
> on UI. I probably cannot give you  good answers of your other questions.

That's perfectly fine, but the answer there is that you probably need to do some
research and consult with more knowledgeable views OWNERS like sky or msw in
order to gain the necessary background here :)

[msw, 2015-09-23 00:10:15]

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

[msw, 2015-09-23 00:10:16]

As per the general question; views::View and views::DialogDelegateView are
intentionally not focusable by default.

The proper question here is "what element should get initial focus in this
dialog?". DialogDelegate::GetInitiallyFocusedView() determines the initial focus
within the dialog (as a WidgetDelegate). The dialog implementation tries to
focus the default/ok/cancel buttons. Perhaps you need to set a default button on
your dialog, or override GetInitiallyFocusedView() if the logic doesn't suit
your dialog's needs.

In this particular example, I suspect that DesktopMediaListView or it's elements
should be made focusable, so that [Tab] traversal of the dialog allows keyboard
users to actually pick something. Perhaps the picker should actually be a
views::TreeView or views::TableView subclass to inherit lots of base control
functionality, but that's a much larger change. Also one of the following fixes
would likely be appropriate:
(1) set initial focus to the DesktopMediaPickerListView or the first element
therein.
(2) set initial focus to the default button ("ok" or whatever), and ensure that
DesktopMediaListView highlights its first/default element without any direct
interaction.

[msw, 2015-09-23 00:11:58]

Also, posting screenshots and detailed steps to reproduce this defect on the
associated bug would be highly appreciated, thanks!

[gyzhou, 2015-09-23 20:30:52]

gyzhou@google.com changed reviewers:
+ sergeyu@chromium.org
- annacc@chromium.org, msw@chromium.org

[gyzhou, 2015-09-23 20:30:52]

Peter and Sergey,

Desktop media source picker is implemented in DesktopMediaPickerDialogView
class. Inside DesktopMediaPickerDialogView, there is a DesktopMedialistview
instance. Several DesktopMediaSourceview instances will be added to
DesktopMediaPickerListView after dialog launched. Therefore, either
DesktopMediaPickerDialogView or DesktopMediaListview need get focus after dialog
launched. I think let DesktopMedialistview get focus is better than the other
way.

Please have a review for me for this code change.

[Peter Kasting, 2015-09-23 20:54:18]

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

[Peter Kasting, 2015-09-23 20:54:20]

Also consider what should happen when the user opens the dialog and then simply
hits enter.  Probably this should accept the current choice.  I worry this CL
might break that.  If so, you'd probably need to ensure the parent view handles
OnKeyPressed() for the enter key.

You should also test whether this change allows tab traversal among the list
view/buttons to work correctly.

https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:448:
list_view_->SetFocusable(true);
I would move this to the list view's constructor.

https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:449:
list_view_->RequestFocus();
I would instead implement this by overriding GetInitiallyFocusedView() to return
the list view.

[chromium-reviews, 2015-09-23 21:13:51]

Peter,

Right after dialog launches, no item/sourceview is added to listview.
Therefore, no item is selected. when the user opens the dialog and then
simply hits enter, nothing is going to happen. This behave hasn't been
changed before and after this CL.

I have tested that Tab will let focus navigates among list view and the two
buttons in the dialog.

You comments on list_view_->SetFocusable(true); and list_view_->RequestFocus();
make sense. I will modify the CL.

Thanks,

Geroge



On Wed, Sep 23, 2015 at 1:54 PM, <pkasting@chromium.org> wrote:

> Also consider what should happen when the user opens the dialog and then
> simply
> hits enter.  Probably this should accept the current choice.  I worry this
> CL
> might break that.  If so, you'd probably need to ensure the parent view
> handles
> OnKeyPressed() for the enter key.
>
> You should also test whether this change allows tab traversal among the
> list
> view/buttons to work correctly.
>
>
>
>
https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
>
>
>
https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/desktop_media_picker_views.cc:448:
> list_view_->SetFocusable(true);
> I would move this to the list view's constructor.
>
>
>
https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/desktop_media_picker_views.cc:449:
> list_view_->RequestFocus();
> I would instead implement this by overriding GetInitiallyFocusedView()
> to return the list view.
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-09-23 21:33:08]

On 2015/09/23 21:13:51, chromium-reviews wrote:
> Peter,
> 
> Right after dialog launches, no item/sourceview is added to listview.
> Therefore, no item is selected. when the user opens the dialog and then
> simply hits enter, nothing is going to happen. This behave hasn't been
> changed before and after this CL.

When does the content appear in the list view then?  Why should the list view be
given initial focus if it has nothing in it to select?  How do we get into the
populated state shown in your screenshot to me?

[chromium-reviews, 2015-09-23 21:54:39]

Peter,

Last line of code in constructor of DesktopMediaPickerDialogView used to be
 list_view_->StartUpdating(dialog_window_id);

Then a thread in other place will  collect all information of available
windows for capture and capture a snapshot for each windows. Each snapshot
will be given to a source view and then added to listview. This process is
repeated every 1 seconds. I have no control when this process is done.

Hope this answer your question.

George



On Wed, Sep 23, 2015 at 2:33 PM, <pkasting@chromium.org> wrote:

> On 2015/09/23 21:13:51, chromium-reviews wrote:
>
>> Peter,
>>
>
> Right after dialog launches, no item/sourceview is added to listview.
>> Therefore, no item is selected. when the user opens the dialog and then
>> simply hits enter, nothing is going to happen. This behave hasn't been
>> changed before and after this CL.
>>
>
> When does the content appear in the list view then?  Why should the list
> view be
> given initial focus if it has nothing in it to select?  How do we get into
> the
> populated state shown in your screenshot to me?
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-09-23 21:56:09]

On 2015/09/23 21:54:39, chromium-reviews wrote:
> Peter,
> 
> Last line of code in constructor of DesktopMediaPickerDialogView used to be
>  list_view_->StartUpdating(dialog_window_id);
> 
> Then a thread in other place will  collect all information of available
> windows for capture and capture a snapshot for each windows. Each snapshot
> will be given to a source view and then added to listview. This process is
> repeated every 1 seconds. I have no control when this process is done.

Then I go back to my earlier set of questions: ignoring the initial milliseconds
when the list view is empty, please consider hitting enter in the context of a
populates list view, which should happen very quickly after the window is
opened.

[chromium-reviews, 2015-09-23 22:02:31]

By design, if user didn't do anything, then no item will be selected
automatically even the listview is populated. I am not asked to change this
dialog behavior.

Thanks for quick response.


On Wed, Sep 23, 2015 at 2:56 PM, <pkasting@chromium.org> wrote:

> On 2015/09/23 21:54:39, chromium-reviews wrote:
>
>> Peter,
>>
>
> Last line of code in constructor of DesktopMediaPickerDialogView used to be
>>   list_view_->StartUpdating(dialog_window_id);
>>
>
> Then a thread in other place will  collect all information of available
>> windows for capture and capture a snapshot for each windows. Each snapshot
>> will be given to a source view and then added to listview. This process is
>> repeated every 1 seconds. I have no control when this process is done.
>>
>
> Then I go back to my earlier set of questions: ignoring the initial
> milliseconds
> when the list view is empty, please consider hitting enter in the context
> of a
> populates list view, which should happen very quickly after the window is
> opened.
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-09-23 22:12:44]

On 2015/09/23 22:02:31, chromium-reviews wrote:
> By design, if user didn't do anything, then no item will be selected
> automatically even the listview is populated. I am not asked to change this
> dialog behavior.

That doesn't answer my question.

You're changing the dialog to focus the list view instead of the buttons that
would have previously been focused.  What would have happened if the user waited
for the list view to be populated before, then hit enter, or escape?  Wouldn't
we have closed the dialog with at least one of those?  Does that still happen?

Even ignoring this, what will happen after your change if a user _does_ select
something, then hits enter?  Does it work as expected (the dialog closes and
takes action on the selected item)?

Again, this goes back to: don't focus on answering the questions that I ask,
focus on the bigger picture I'm trying to get at when I ask these questions, and
consider the whole user experience of interacting with this dialog.

[chromium-reviews, 2015-09-23 22:33:15]

Peter,

The issue that I was asked to fix is that after DesktopMediaPicker dialog
is launched, user cannot use the keyboard to select a item inside the view.
Neither list view nor buttons in the dialog get focus and can be selected.
I didn't change the behavior of dialog but only bring focus to the dialog.

For the dialog, when nothing is selected. Enter key will do nothing and
escape will close the dialog just like cancel button is clicked. When a
iterm is selcted, enter key works like share button is clicked and escape
keys works like cancel button is clicked.

George



On Wed, Sep 23, 2015 at 3:12 PM, <pkasting@chromium.org> wrote:

> On 2015/09/23 22:02:31, chromium-reviews wrote:
>
>> By design, if user didn't do anything, then no item will be selected
>> automatically even the listview is populated. I am not asked to change
>> this
>> dialog behavior.
>>
>
> That doesn't answer my question.
>
> You're changing the dialog to focus the list view instead of the buttons
> that
> would have previously been focused.  What would have happened if the user
> waited
> for the list view to be populated before, then hit enter, or escape?
> Wouldn't
> we have closed the dialog with at least one of those?  Does that still
> happen?
>
> Even ignoring this, what will happen after your change if a user _does_
> select
> something, then hits enter?  Does it work as expected (the dialog closes
> and
> takes action on the selected item)?
>
> Again, this goes back to: don't focus on answering the questions that I
> ask,
> focus on the bigger picture I'm trying to get at when I ask these
> questions, and
> consider the whole user experience of interacting with this dialog.
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gyzhou, 2015-09-23 22:37:25]

gyzhou@google.com changed reviewers:
- pkasting@chromium.org, sergeyu@chromium.org

[gyzhou, 2015-09-23 22:37:26]

Peter,

I just updated the code based on your review comments.

Thanks,

George

https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:448:
list_view_->SetFocusable(true);
On 2015/09/23 20:54:20, Peter Kasting wrote:
> I would move this to the list view's constructor.

Done.

https://codereview.chromium.org/1313843003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:449:
list_view_->RequestFocus();
On 2015/09/23 20:54:20, Peter Kasting wrote:
> I would instead implement this by overriding GetInitiallyFocusedView() to
return
> the list view.

Done.

[Peter Kasting, 2015-09-23 22:43:56]

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

[Peter Kasting, 2015-09-23 22:43:57]

On 2015/09/23 22:33:15, chromium-reviews wrote:
> For the dialog, when nothing is selected. Enter key will do nothing and
> escape will close the dialog just like cancel button is clicked. When a
> iterm is selcted, enter key works like share button is clicked and escape
> keys works like cancel button is clicked.

OK, so sounds like your changes won't have any effect on what happens if a user
hits enter/escape after the list view populates.  That's what I wanted to know.

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:210: // Enable keyboard
focus
This comment adds nothing to the code; remove it.

Once that's done you can probably remove the blank line above this too.

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:450: // Request keyboard
focus for initial view.
This block should not be necessary because the widget code should already be
doing this (that was the entire point of overriding GetInitiallyFocusedView(),
after all).  Why is it here?

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:460: if (list_view_)
Why null-check this when it can't ever be null?

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.h (right):

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.h:121: views::View*
GetInitiallyFocusedView() override;
This should be in the list of DialogDelegateView overrides below, ordered the
same as these functions are ordered in the base DialogDelegateView declaration.

Keep the definition in the .cc file in this order as well.

[gyzhou, 2015-09-23 23:43:39]

gyzhou@google.com changed reviewers:
- pkasting@chromium.org

[gyzhou, 2015-09-23 23:43:40]

Peter,

Codes were updated based on your comments.

Thanks for your help.

George

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:210: // Enable keyboard
focus
On 2015/09/23 22:43:57, Peter Kasting wrote:
> This comment adds nothing to the code; remove it.
> 
> Once that's done you can probably remove the blank line above this too.

Done.

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:450: // Request keyboard
focus for initial view.
On 2015/09/23 22:43:57, Peter Kasting wrote:
> This block should not be necessary because the widget code should already be
> doing this (that was the entire point of overriding GetInitiallyFocusedView(),
> after all).  Why is it here?

Done.

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.cc:460: if (list_view_)
On 2015/09/23 22:43:57, Peter Kasting wrote:
> Why null-check this when it can't ever be null?

Done.

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/desktop_media_picker_views.h (right):

https://codereview.chromium.org/1313843003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/desktop_media_picker_views.h:121: views::View*
GetInitiallyFocusedView() override;
On 2015/09/23 22:43:57, Peter Kasting wrote:
> This should be in the list of DialogDelegateView overrides below, ordered the
> same as these functions are ordered in the base DialogDelegateView
declaration.
> 
> Keep the definition in the .cc file in this order as well.

This is an override for DialogDelegate

Done.

[Peter Kasting, 2015-09-24 04:10:24]

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

[Peter Kasting, 2015-09-24 04:10:25]

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views.cc:448: views::View* initView
= GetInitiallyFocusedView();
This block is still here.  Why is it not removed?

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views.h (right):

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views.h:133: views::View*
GetInitiallyFocusedView() override;
You don't directly subclass DialogDelegate; you get this function through
DialogDelegateView, and that's the only override section you should have.  (The
existing separate section for View shouldn't exist either.)

Or to put it differently, this class doesn't care about how DialogDelegateView
is itself implemented, and if we were to change that someday to move this method
from one base class to another, we shouldn't need to check all subclasses (of
any depth) of all these classes to update the comments and organization of their
overrides.

[gyzhou, 2015-09-24 15:51:25]

On 2015/09/24 04:10:25, Peter Kasting wrote:
>
https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views.cc:448: views::View*
initView
> = GetInitiallyFocusedView();
> This block is still here.  Why is it not removed?
> 
>
https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
> File chrome/browser/ui/views/desktop_media_picker_views.h (right):
> 
>
https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views.h:133: views::View*
> GetInitiallyFocusedView() override;
> You don't directly subclass DialogDelegate; you get this function through
> DialogDelegateView, and that's the only override section you should have. 
(The
> existing separate section for View shouldn't exist either.)
> 
> Or to put it differently, this class doesn't care about how DialogDelegateView
> is itself implemented, and if we were to change that someday to move this
method
> from one base class to another, we shouldn't need to check all subclasses (of
> any depth) of all these classes to update the comments and organization of
their
> overrides.

Peter,

Fixed based on your comments

Thanks,

George

[Peter Kasting, 2015-09-24 20:46:56]

I'm feeling frustrated about the communication and the quality of implementation
here.  I realize that you didn't come into this bug with knowledge of UI
implementation, but I think even given that, this has taken more time, more
round-trips, and more repeating the same things than it should have.  Please try
to be careful, conscientious, and detailed in both your coding and your replies.

https://codereview.chromium.org/1313843003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views.cc:448:
GetInitiallyFocusedView()->RequestFocus();
I've asked several times why this code is still here and you repeatedly haven't
answered and haven't removed it.  I don't understand why this is happening.

To reiterate what has already been explained: the whole point of using
GetInitiallyFocusedView() is to let the widget machinery focus the view for you.
 You should not be doing this yourself.  If you need to, something else is
broken elsewhere and this still isn't the right fix.  If you still don't
understand something, ask more questions about it, but don't let my feedback
simply go ignored.

[Peter Kasting, 2015-09-24 20:47:18]

pkasting@chromium.org changed reviewers:
+ sergeyu@chromium.org
- gyzhou@google.com, pkasting@google.com

[gyzhou, 2015-09-24 21:22:41]

On 2015/09/24 20:46:56, Peter Kasting wrote:
> I'm feeling frustrated about the communication and the quality of
implementation
> here.  I realize that you didn't come into this bug with knowledge of UI
> implementation, but I think even given that, this has taken more time, more
> round-trips, and more repeating the same things than it should have.  Please
try
> to be careful, conscientious, and detailed in both your coding and your
replies.
> 
>
https://codereview.chromium.org/1313843003/diff/140001/chrome/browser/ui/view...
> File chrome/browser/ui/views/desktop_media_picker_views.cc (right):
> 
>
https://codereview.chromium.org/1313843003/diff/140001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views.cc:448:
> GetInitiallyFocusedView()->RequestFocus();
> I've asked several times why this code is still here and you repeatedly
haven't
> answered and haven't removed it.  I don't understand why this is happening.
> 
> To reiterate what has already been explained: the whole point of using
> GetInitiallyFocusedView() is to let the widget machinery focus the view for
you.
>  You should not be doing this yourself.  If you need to, something else is
> broken elsewhere and this still isn't the right fix.  If you still don't
> understand something, ask more questions about it, but don't let my feedback
> simply go ignored.


I guess I eventually got your point. When a windows is active, one of its
component will get focus automatically.

I deleted the line of GetInitiallyFocusedView()->RequestFocus();

[Peter Kasting, 2015-09-24 21:34:37]

On 2015/09/24 21:22:41, gyzhou wrote:
> I guess I eventually got your point. When a windows is active, one of its
> component will get focus automatically.

Or at least it should -- make sure this still actually addresses the bug.

Now that the implementation work is done: is there a way to test this?  Some
sort of test that could open a relevant dialog, wait for the list to be
populated, then hit various keys (arrows, escape, enter) and make sure they all
have the desired effect?  If at all possible, we should always have tests for
code changes.

sergeyu (code author) and phajdan.jr (test expert) might have more knowledge on
how to accomplish this.

[chromium-reviews, 2015-09-24 21:42:28]

Peter

Javascript Test code is in
/src/chrome/common/extensions/docs/examples/api/desktopCapture

I did several test and it looks fine.

George

On Thu, Sep 24, 2015 at 2:34 PM, <pkasting@chromium.org> wrote:

> On 2015/09/24 21:22:41, gyzhou wrote:
>
>> I guess I eventually got your point. When a windows is active, one of its
>> component will get focus automatically.
>>
>
> Or at least it should -- make sure this still actually addresses the bug.
>
> Now that the implementation work is done: is there a way to test this?
> Some
> sort of test that could open a relevant dialog, wait for the list to be
> populated, then hit various keys (arrows, escape, enter) and make sure
> they all
> have the desired effect?  If at all possible, we should always have tests
> for
> code changes.
>
> sergeyu (code author) and phajdan.jr (test expert) might have more
> knowledge on
> how to accomplish this.
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-09-24 22:45:50]

On 2015/09/24 21:42:28, chromium-reviews wrote:
> Peter
> 
> Javascript Test code is in
> /src/chrome/common/extensions/docs/examples/api/desktopCapture

Examples that can be tested manually are not what I'm looking for.  Code changes
in Chromium should have automated tests added whenever possible.  Ideally these
are unit tests, but in a case like this we likely need to do higher-level
testing.

[gyzhou, 2015-09-28 16:19:32]

Sergey,

I just added a unit test function as required to check the medial list view get
focus after the dialog is launched.

Could you have a code review for me.

Thanks,

George

[Sergey Ulanov, 2015-10-01 18:23:11]

lgtm

https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:188: // Verifies
that the MediaListView get the initial focus
nit: add. at the end

https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:191:
GetMediaListViewForTesting()->HasFocus());
nit: don't need to wrap this line - it can all fit in one line.

[gyzhou, 2015-10-01 20:44:41]

On 2015/10/01 18:23:11, Sergey Ulanov wrote:
> lgtm
> 
>
https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
> File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):
> 
>
https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:188: //
Verifies
> that the MediaListView get the initial focus
> nit: add. at the end
> 
>
https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:191:
> GetMediaListViewForTesting()->HasFocus());
> nit: don't need to wrap this line - it can all fit in one line.

Peter,

Would you please review the unit test.

Thanks,

George

[gyzhou, 2015-10-02 18:51:25]

gyzhou@google.com changed reviewers:
+ gyzhou@google.com
- pkasting@chromium.org, sergeyu@chromium.org

[gyzhou, 2015-10-02 18:51:26]

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views.cc:448: views::View* initView
= GetInitiallyFocusedView();
On 2015/09/24 04:10:25, Peter Kasting wrote:
> This block is still here.  Why is it not removed?

Done.

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views.h (right):

https://codereview.chromium.org/1313843003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views.h:133: views::View*
GetInitiallyFocusedView() override;
On 2015/09/24 04:10:25, Peter Kasting wrote:
> You don't directly subclass DialogDelegate; you get this function through
> DialogDelegateView, and that's the only override section you should have. 
(The
> existing separate section for View shouldn't exist either.)
> 
> Or to put it differently, this class doesn't care about how DialogDelegateView
> is itself implemented, and if we were to change that someday to move this
method
> from one base class to another, we shouldn't need to check all subclasses (of
> any depth) of all these classes to update the comments and organization of
their
> overrides.

Done.

https://codereview.chromium.org/1313843003/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views.cc (right):

https://codereview.chromium.org/1313843003/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views.cc:448:
GetInitiallyFocusedView()->RequestFocus();
On 2015/09/24 20:46:56, Peter Kasting wrote:
> I've asked several times why this code is still here and you repeatedly
haven't
> answered and haven't removed it.  I don't understand why this is happening.
> 
> To reiterate what has already been explained: the whole point of using
> GetInitiallyFocusedView() is to let the widget machinery focus the view for
you.
>  You should not be doing this yourself.  If you need to, something else is
> broken elsewhere and this still isn't the right fix.  If you still don't
> understand something, ask more questions about it, but don't let my feedback
> simply go ignored.

Done.

https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:188: // Verifies
that the MediaListView get the initial focus
On 2015/10/01 18:23:11, Sergey Ulanov wrote:
> nit: add. at the end

Done.

https://codereview.chromium.org/1313843003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:191:
GetMediaListViewForTesting()->HasFocus());
On 2015/10/01 18:23:11, Sergey Ulanov wrote:
> nit: don't need to wrap this line - it can all fit in one line.

Done.

[Peter Kasting, 2015-10-02 18:52:55]

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

[Peter Kasting, 2015-10-02 18:52:56]

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
I was thinking more along the lines of checking that arrows or whatever other
keys this responds to would actually change the selected item correctly.  In
other words, basically verifying all the ways to affect this from the keyboard.

[chromium-reviews, 2015-10-02 19:48:30]

Peter,

We verified that bool DesktopMediaListView::OnKeyPressed() will not be
executed when arrow keys are pressed. It seems that arrow keys navigation
control is done by view base class in this case. This is a issue but out of
the scope of this CL.

Should we ignore unit tests for arrow keys?

George

On Fri, Oct 2, 2015 at 11:52 AM, <pkasting@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
> File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc
> (right):
>
>
>
https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
>
> EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
> I was thinking more along the lines of checking that arrows or whatever
> other keys this responds to would actually change the selected item
> correctly.  In other words, basically verifying all the ways to affect
> this from the keyboard.
>
> https://codereview.chromium.org/1313843003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-10-02 19:49:55]

On 2015/10/02 19:48:30, chromium-reviews wrote:
> Peter,
> 
> We verified that bool DesktopMediaListView::OnKeyPressed() will not be
> executed when arrow keys are pressed. It seems that arrow keys navigation
> control is done by view base class in this case. This is a issue but out of
> the scope of this CL.
> 
> Should we ignore unit tests for arrow keys?

You will likely need to test this as a non-unit test (e.g. an interactive UI
test).

[gyzhou, 2015-10-05 20:50:16]

gyzhou@google.com changed reviewers:
+ sergeyu@chromium.org

[gyzhou, 2015-10-05 20:50:17]

Sergey,

Based on peter's comments, I added two more unit tests for better unit test
coverage. 

Please have a review for me espically for
TEST_F(DesktopMediaPickerViewsTest, DoneCallbackCalledOnCancelButtonPressed)

Thanks,

George

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
On 2015/10/02 18:52:56, Peter Kasting wrote:
> I was thinking more along the lines of checking that arrows or whatever other
> keys this responds to would actually change the selected item correctly.  In
> other words, basically verifying all the ways to affect this from the
keyboard.

We were verified that bool DesktopMediaListView::OnKeyPressed() will not be
executed when arrow keys are pressed. It seems that Keybaord navigation control
is done by view class in this case. This is a issue but out of the scope of this
CL. Therefore, unit test will not cover arrow keys.

For button OK/share, unit test need cover 1, button disable when there is no
selection. 2, button enable when there is a selection. 3, button clicked.

For button cancel, unit test need cover 1, button always enable. 2 button
clicked.

For mouse, unit test need cover 1, mouse left button single clicked, 2 mouse
left button doubly clicked.

Most test cases were already covered by existing unit tests. I added unit tests
for cancel button clicked and mouse left button single clicked which were
missed.

[Peter Kasting, 2015-10-05 20:59:46]

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
On 2015/10/05 20:50:16, gyzhou wrote:
> On 2015/10/02 18:52:56, Peter Kasting wrote:
> > I was thinking more along the lines of checking that arrows or whatever
other
> > keys this responds to would actually change the selected item correctly.  In
> > other words, basically verifying all the ways to affect this from the
> keyboard.
> 
> We were verified that bool DesktopMediaListView::OnKeyPressed() will not be
> executed when arrow keys are pressed. It seems that Keybaord navigation
control
> is done by view class in this case. This is a issue but out of the scope of
this
> CL. Therefore, unit test will not cover arrow keys.
> 
> For button OK/share, unit test need cover 1, button disable when there is no
> selection. 2, button enable when there is a selection. 3, button clicked.
> 
> For button cancel, unit test need cover 1, button always enable. 2 button
> clicked.
> 
> For mouse, unit test need cover 1, mouse left button single clicked, 2 mouse
> left button doubly clicked.
> 
> Most test cases were already covered by existing unit tests. I added unit
tests
> for cancel button clicked and mouse left button single clicked which were
> missed. 
> 

Yes, I know this.  That's why I asked for an interactive test to verify this. 
You seem to have ignored my response to your last message, and instead just
reiterated what you already told me.

I'm not happy with the way the communication on this code review has gone.  I
shouldn't have to ask for something multiple times, but that's seemed to happen
with practically everything I've requested here.  If you don't understand what
I'm asking, please work with someone else who can help ensure you understand and
implement every request made.

https://codereview.chromium.org/1313843003/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:91: // selection
of MediaSourceView.
The comments in and around these new tests have grammar problems.  Please work
with someone to help get them fixed.

It's not clear to me that this test actually verifies that no selection
notification occurs.  Where does that happen?

[gyzhou, 2015-10-05 21:12:27]

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
On 2015/10/05 20:59:45, Peter Kasting wrote:
> On 2015/10/05 20:50:16, gyzhou wrote:
> > On 2015/10/02 18:52:56, Peter Kasting wrote:
> > > I was thinking more along the lines of checking that arrows or whatever
> other
> > > keys this responds to would actually change the selected item correctly. 
In
> > > other words, basically verifying all the ways to affect this from the
> > keyboard.
> > 
> > We were verified that bool DesktopMediaListView::OnKeyPressed() will not be
> > executed when arrow keys are pressed. It seems that Keybaord navigation
> control
> > is done by view class in this case. This is a issue but out of the scope of
> this
> > CL. Therefore, unit test will not cover arrow keys.
> > 
> > For button OK/share, unit test need cover 1, button disable when there is no
> > selection. 2, button enable when there is a selection. 3, button clicked.
> > 
> > For button cancel, unit test need cover 1, button always enable. 2 button
> > clicked.
> > 
> > For mouse, unit test need cover 1, mouse left button single clicked, 2 mouse
> > left button doubly clicked.
> > 
> > Most test cases were already covered by existing unit tests. I added unit
> tests
> > for cancel button clicked and mouse left button single clicked which were
> > missed. 
> > 
> 
> Yes, I know this.  That's why I asked for an interactive test to verify this. 
> You seem to have ignored my response to your last message, and instead just
> reiterated what you already told me.
> 
> I'm not happy with the way the communication on this code review has gone.  I
> shouldn't have to ask for something multiple times, but that's seemed to
happen
> with practically everything I've requested here.  If you don't understand what
> I'm asking, please work with someone else who can help ensure you understand
and
> implement every request made.

We knew that the arrow key wasn't controlled by listview because we did test.
Otherwise, how we can know this.

https://codereview.chromium.org/1313843003/diff/220001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:91: // selection
of MediaSourceView.
On 2015/10/05 20:59:46, Peter Kasting wrote:
> The comments in and around these new tests have grammar problems.  Please work
> with someone to help get them fixed.
> 
> It's not clear to me that this test actually verifies that no selection
> notification occurs.  Where does that happen?

This it the reason I asked Sergey to specially pay attention to this because I
am not sure.

[Peter Kasting, 2015-10-05 21:59:26]

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):

https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
On 2015/10/05 21:12:27, gyzhou wrote:
> On 2015/10/05 20:59:45, Peter Kasting wrote:
> > On 2015/10/05 20:50:16, gyzhou wrote:
> > > On 2015/10/02 18:52:56, Peter Kasting wrote:
> > > > I was thinking more along the lines of checking that arrows or whatever
> > other
> > > > keys this responds to would actually change the selected item correctly.

> In
> > > > other words, basically verifying all the ways to affect this from the
> > > keyboard.
> > > 
> > > We were verified that bool DesktopMediaListView::OnKeyPressed() will not
be
> > > executed when arrow keys are pressed. It seems that Keybaord navigation
> > control
> > > is done by view class in this case. This is a issue but out of the scope
of
> > this
> > > CL. Therefore, unit test will not cover arrow keys.
> > > 
> > > For button OK/share, unit test need cover 1, button disable when there is
no
> > > selection. 2, button enable when there is a selection. 3, button clicked.
> > > 
> > > For button cancel, unit test need cover 1, button always enable. 2 button
> > > clicked.
> > > 
> > > For mouse, unit test need cover 1, mouse left button single clicked, 2
mouse
> > > left button doubly clicked.
> > > 
> > > Most test cases were already covered by existing unit tests. I added unit
> > tests
> > > for cancel button clicked and mouse left button single clicked which were
> > > missed. 
> > > 
> > 
> > Yes, I know this.  That's why I asked for an interactive test to verify
this. 
> > You seem to have ignored my response to your last message, and instead just
> > reiterated what you already told me.
> > 
> > I'm not happy with the way the communication on this code review has gone. 
I
> > shouldn't have to ask for something multiple times, but that's seemed to
> happen
> > with practically everything I've requested here.  If you don't understand
what
> > I'm asking, please work with someone else who can help ensure you understand
> and
> > implement every request made.
> 
> We knew that the arrow key wasn't controlled by listview because we did test.
> Otherwise, how we can know this.

I don't even understand how this sentence has any relevance to what I just said.

To reiterate: you likely can't test keyboard interactions with a unit test, you
probably need to use an interactive UI test to do more of a "full stack" test of
the interaction between inputs and the complete controllable object here.

[Niklas Enbom, 2015-10-05 22:10:23]

There's no UI test for the window picker, and it makes sense to have one.
However this would expand the scope of this CL a lot, which is supposed to fix
and a11y problem. We looking at making larger changes to the picker to enable
tab sharing as well. Peter are you fine with creating a separate bug for the UI
test (which then would block the larger changes)?

On 2015/10/05 21:59:26, Peter Kasting wrote:
>
https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
> File chrome/browser/ui/views/desktop_media_picker_views_unittest.cc (right):
> 
>
https://codereview.chromium.org/1313843003/diff/200001/chrome/browser/ui/view...
> chrome/browser/ui/views/desktop_media_picker_views_unittest.cc:190:
> EXPECT_TRUE(GetPickerDialogView()->GetMediaListViewForTesting()->HasFocus());
> On 2015/10/05 21:12:27, gyzhou wrote:
> > On 2015/10/05 20:59:45, Peter Kasting wrote:
> > > On 2015/10/05 20:50:16, gyzhou wrote:
> > > > On 2015/10/02 18:52:56, Peter Kasting wrote:
> > > > > I was thinking more along the lines of checking that arrows or
whatever
> > > other
> > > > > keys this responds to would actually change the selected item
correctly.
> 
> > In
> > > > > other words, basically verifying all the ways to affect this from the
> > > > keyboard.
> > > > 
> > > > We were verified that bool DesktopMediaListView::OnKeyPressed() will not
> be
> > > > executed when arrow keys are pressed. It seems that Keybaord navigation
> > > control
> > > > is done by view class in this case. This is a issue but out of the scope
> of
> > > this
> > > > CL. Therefore, unit test will not cover arrow keys.
> > > > 
> > > > For button OK/share, unit test need cover 1, button disable when there
is
> no
> > > > selection. 2, button enable when there is a selection. 3, button
clicked.
> > > > 
> > > > For button cancel, unit test need cover 1, button always enable. 2
button
> > > > clicked.
> > > > 
> > > > For mouse, unit test need cover 1, mouse left button single clicked, 2
> mouse
> > > > left button doubly clicked.
> > > > 
> > > > Most test cases were already covered by existing unit tests. I added
unit
> > > tests
> > > > for cancel button clicked and mouse left button single clicked which
were
> > > > missed. 
> > > > 
> > > 
> > > Yes, I know this.  That's why I asked for an interactive test to verify
> this. 
> > > You seem to have ignored my response to your last message, and instead
just
> > > reiterated what you already told me.
> > > 
> > > I'm not happy with the way the communication on this code review has gone.

> I
> > > shouldn't have to ask for something multiple times, but that's seemed to
> > happen
> > > with practically everything I've requested here.  If you don't understand
> what
> > > I'm asking, please work with someone else who can help ensure you
understand
> > and
> > > implement every request made.
> > 
> > We knew that the arrow key wasn't controlled by listview because we did
test.
> > Otherwise, how we can know this.
> 
> I don't even understand how this sentence has any relevance to what I just
said.
> 
> To reiterate: you likely can't test keyboard interactions with a unit test,
you
> probably need to use an interactive UI test to do more of a "full stack" test
of
> the interaction between inputs and the complete controllable object here.

[Peter Kasting, 2015-10-05 22:36:08]

On 2015/10/05 22:10:23, Niklas Enbom wrote:
> There's no UI test for the window picker, and it makes sense to have one.
> However this would expand the scope of this CL a lot, which is supposed to fix
> and a11y problem. We looking at making larger changes to the picker to enable
> tab sharing as well. Peter are you fine with creating a separate bug for the
UI
> test (which then would block the larger changes)?

Yes, if you have a definite plan for who/how to do this, I'm OK with not doing
it in this CL.

LGTM

[gyzhou, 2015-10-06 22:09:09]

The CQ bit was checked by gyzhou@google.com

[gyzhou, 2015-10-06 22:09:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1313843003/#ps240001
(title: "remove a unit test")

[commit-bot: I haz the power, 2015-10-06 22:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1313843003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1313843003/240001

[commit-bot: I haz the power, 2015-10-06 22:18:18]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-10-06 22:19:15]

Patchset 13 (id:??) landed as
https://crrev.com/1998bee9f96031dec220b9b7b9e50c68de5d995d
Cr-Commit-Position: refs/heads/master@{#352701}