Add chrome side webusb permission UI code

Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device chooser permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204
Committed: https://crrev.com/a7dd94484b64397611deaf2177f28dcb83a7903b
Cr-Commit-Position: refs/heads/master@{#364593}

[juncai, 2015-10-20 17:00:11]

Description was changed from

==========
Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204
==========

to

==========
Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device chooser permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204
==========

[juncai, 2015-10-20 17:25:32]

juncai@chromium.org changed reviewers:
+ felt@chromium.org, reillyg@chromium.org, thestig@chromium.org

[juncai, 2015-10-20 17:25:32]

reillyg@chromium.org: Please review changes in 
//chrome/browser/usb/

felt@chromium.org: Please review changes in 
//chrome/browser/ui/views/website_settings/

thestig@chromium.org: Please review changes in 
//chrome/app/generated_resources.grd
//chrome/browser/chrome_content_browser_client.cc
//chrome/chrome_browser.gypi
//chrome/chrome_browser_ui.gypi

[Reilly Grant (use Gerrit), 2015-10-20 22:17:07]

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_delegate.cc
(right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_delegate.cc:25: return
"ExtensionInstalled";
What?

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_delegate.h:26:
scoped_ptr<WebUsbPermissionBubbleRequest> request_;
Instead of holding the request the delegate should be constructed over the
parameters necessary to build the UI or it may be best to extend
ChooserBubbleDelegate in the USB-specific code.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui.h (right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui.h:40:
WebUsbPermissionBubbleRequest* request_;
Instead of holding the request here bubble_delegate_ should probably receive the
index of the selected option.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:11: #if defined(OS_LINUX) ||
defined(OS_WIN) || defined(OS_CHROMEOS)
or... !defined(OS_MACOSX)

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:38: return true;
Replace with if (ContainsValue(config.origins, origin)).

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:44: return true;
Replace with if (ContainsValue(function.origins, origin)).

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:74:
ProfileManager::GetActiveUserProfile(), chrome::HOST_DESKTOP_TYPE_NATIVE);
This is wrong and fragile. Use FindBrowserFromWebContents and make this class a
WebContentsUserData so that its lifetime is scoped to the WebContents of the
RenderFrameHost that it was created from (so you don't get in trouble if the
WebContents is destroyed while waiting for a list of devices. Note that |origin|
should still be the LastCommittedURL() of the render frame, not the web
contents.

[raymes, 2015-10-20 22:17:59]

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

[raymes, 2015-10-20 22:18:00]

groby seemed to know something about the permission bubble stuff, so CC'ing her
:)

[raymes, 2015-10-20 22:18:34]

raymes@chromium.org changed reviewers:
- raymes@chromium.org

[groby-ooo-7-16, 2015-10-21 00:52:20]

On 2015/10/20 22:18:00, raymes wrote:
> groby seemed to know something about the permission bubble stuff, so CC'ing
her
> :)

Is this still a relevant CL with the move to the bubble manager?

[Reilly Grant (use Gerrit), 2015-10-21 00:55:16]

On 2015/10/21 00:52:20, groby wrote:
> On 2015/10/20 22:18:00, raymes wrote:
> > groby seemed to know something about the permission bubble stuff, so CC'ing
> her
> > :)
> 
> Is this still a relevant CL with the move to the bubble manager?

Yes, this is the patch that uses BubbleManager.

[juncai, 2015-10-23 03:36:43]

reillyg@, please review the change.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_delegate.cc
(right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_delegate.cc:25: return
"ExtensionInstalled";
On 2015/10/20 22:17:07, Reilly Grant wrote:
> What?

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
In order to not get DCHECK error, the bubble's GetName() has to be either:
"MockBubble" or "ExtensionInstalled".

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_delegate.h:26:
scoped_ptr<WebUsbPermissionBubbleRequest> request_;
On 2015/10/20 22:17:07, Reilly Grant wrote:
> Instead of holding the request the delegate should be constructed over the
> parameters necessary to build the UI or it may be best to extend
> ChooserBubbleDelegate in the USB-specific code.

Done.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui.h (right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui.h:40:
WebUsbPermissionBubbleRequest* request_;
On 2015/10/20 22:17:07, Reilly Grant wrote:
> Instead of holding the request here bubble_delegate_ should probably receive
the
> index of the selected option.

Done.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:11: #if defined(OS_LINUX) ||
defined(OS_WIN) || defined(OS_CHROMEOS)
On 2015/10/20 22:17:07, Reilly Grant wrote:
> or... !defined(OS_MACOSX)

Done.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:38: return true;
On 2015/10/20 22:17:07, Reilly Grant wrote:
> Replace with if (ContainsValue(config.origins, origin)).

Done.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:44: return true;
On 2015/10/20 22:17:07, Reilly Grant wrote:
> Replace with if (ContainsValue(function.origins, origin)).

Done.

https://codereview.chromium.org/1408193003/diff/100001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:74:
ProfileManager::GetActiveUserProfile(), chrome::HOST_DESKTOP_TYPE_NATIVE);
On 2015/10/20 22:17:07, Reilly Grant wrote:
> This is wrong and fragile. Use FindBrowserFromWebContents and make this class
a
> WebContentsUserData so that its lifetime is scoped to the WebContents of the
> RenderFrameHost that it was created from (so you don't get in trouble if the
> WebContents is destroyed while waiting for a list of devices. Note that
|origin|
> should still be the LastCommittedURL() of the render frame, not the web
> contents.

Done.

[felt, 2015-10-25 03:12:12]

On 2015/10/21 00:55:16, Reilly Grant wrote:
> On 2015/10/21 00:52:20, groby wrote:
> > On 2015/10/20 22:18:00, raymes wrote:
> > > groby seemed to know something about the permission bubble stuff, so
CC'ing
> > her
> > > :)
> > 
> > Is this still a relevant CL with the move to the bubble manager?
> 
> Yes, this is the patch that uses BubbleManager.

I must have missed this, but what happens if both a "regular" permission bubble
and a USB chooser are opened at the same time? Is there logic to choose which
one ought to be displayed, or to queue them so that one appears when the next
one closes?

[juncai, 2015-10-26 17:46:38]

On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> On 2015/10/21 00:55:16, Reilly Grant wrote:
> > On 2015/10/21 00:52:20, groby wrote:
> > > On 2015/10/20 22:18:00, raymes wrote:
> > > > groby seemed to know something about the permission bubble stuff, so
> CC'ing
> > > her
> > > > :)
> > > 
> > > Is this still a relevant CL with the move to the bubble manager?
> > 
> > Yes, this is the patch that uses BubbleManager.
> 
> I must have missed this, but what happens if both a "regular" permission
bubble
> and a USB chooser are opened at the same time? Is there logic to choose which
> one ought to be displayed, or to queue them so that one appears when the next
> one closes?

Right now, both bubbles will be shown. Adding hcarmona@ for this.

[Reilly Grant (use Gerrit), 2015-10-26 18:03:39]

On 2015/10/26 17:46:38, juncai wrote:
> On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> > On 2015/10/21 00:55:16, Reilly Grant wrote:
> > > On 2015/10/21 00:52:20, groby wrote:
> > > > On 2015/10/20 22:18:00, raymes wrote:
> > > > > groby seemed to know something about the permission bubble stuff, so
> > CC'ing
> > > > her
> > > > > :)
> > > > 
> > > > Is this still a relevant CL with the move to the bubble manager?
> > > 
> > > Yes, this is the patch that uses BubbleManager.
> > 
> > I must have missed this, but what happens if both a "regular" permission
> bubble
> > and a USB chooser are opened at the same time? Is there logic to choose
which
> > one ought to be displayed, or to queue them so that one appears when the
next
> > one closes?
> 
> Right now, both bubbles will be shown. Adding hcarmona@ for this.

Since opening a USB device chooser will require a user gesture I think it should
replace an existing permission bubble as clicking on a button within the site
seems like an indication that the user is ignoring the original bubble.
Deferring to hcarmona@ for how this should be coordinated through the
BubbleManager API.

[hcarmona, 2015-10-26 21:45:01]

On 2015/10/26 18:03:39, Reilly Grant wrote:
> On 2015/10/26 17:46:38, juncai wrote:
> > On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> > > On 2015/10/21 00:55:16, Reilly Grant wrote:
> > > > On 2015/10/21 00:52:20, groby wrote:
> > > > > On 2015/10/20 22:18:00, raymes wrote:
> > > > > > groby seemed to know something about the permission bubble stuff, so
> > > CC'ing
> > > > > her
> > > > > > :)
> > > > > 
> > > > > Is this still a relevant CL with the move to the bubble manager?
> > > > 
> > > > Yes, this is the patch that uses BubbleManager.
> > > 
> > > I must have missed this, but what happens if both a "regular" permission
> > bubble
> > > and a USB chooser are opened at the same time? Is there logic to choose
> which
> > > one ought to be displayed, or to queue them so that one appears when the
> next
> > > one closes?
> > 
> > Right now, both bubbles will be shown. Adding hcarmona@ for this.
> 
> Since opening a USB device chooser will require a user gesture I think it
should
> replace an existing permission bubble as clicking on a button within the site
> seems like an indication that the user is ignoring the original bubble.
> Deferring to hcarmona@ for how this should be coordinated through the
> BubbleManager API.

Current behavior of all bubbles is that they do not know about each
other. While we have plans to improve the bubble manager to better
handle overlapping bubbles, we don't have anything in place at the
moment.

The other permissions bubbles are unique in that they know about each
other and delay showing if one is visible. They will also combine if
multiple permissions are requested.

[felt, 2015-10-26 21:50:21]

On 2015/10/26 17:46:38, juncai wrote:
> On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> > On 2015/10/21 00:55:16, Reilly Grant wrote:
> > > On 2015/10/21 00:52:20, groby wrote:
> > > > On 2015/10/20 22:18:00, raymes wrote:
> > > > > groby seemed to know something about the permission bubble stuff, so
> > CC'ing
> > > > her
> > > > > :)
> > > > 
> > > > Is this still a relevant CL with the move to the bubble manager?
> > > 
> > > Yes, this is the patch that uses BubbleManager.
> > 
> > I must have missed this, but what happens if both a "regular" permission
> bubble
> > and a USB chooser are opened at the same time? Is there logic to choose
which
> > one ought to be displayed, or to queue them so that one appears when the
next
> > one closes?
> 
> Right now, both bubbles will be shown. Adding hcarmona@ for this.

Does this actually work without crashing or looking super weird? It seems like
they would both try to hang off the same parent view.

[felt, 2015-10-26 21:51:29]

On 2015/10/26 18:03:39, Reilly Grant wrote:
> On 2015/10/26 17:46:38, juncai wrote:
> > On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> > > On 2015/10/21 00:55:16, Reilly Grant wrote:
> > > > On 2015/10/21 00:52:20, groby wrote:
> > > > > On 2015/10/20 22:18:00, raymes wrote:
> > > > > > groby seemed to know something about the permission bubble stuff, so
> > > CC'ing
> > > > > her
> > > > > > :)
> > > > > 
> > > > > Is this still a relevant CL with the move to the bubble manager?
> > > > 
> > > > Yes, this is the patch that uses BubbleManager.
> > > 
> > > I must have missed this, but what happens if both a "regular" permission
> > bubble
> > > and a USB chooser are opened at the same time? Is there logic to choose
> which
> > > one ought to be displayed, or to queue them so that one appears when the
> next
> > > one closes?
> > 
> > Right now, both bubbles will be shown. Adding hcarmona@ for this.
> 
> Since opening a USB device chooser will require a user gesture I think it
should
> replace an existing permission bubble as clicking on a button within the site
> seems like an indication that the user is ignoring the original bubble.
> Deferring to hcarmona@ for how this should be coordinated through the
> BubbleManager API.

But the permission bubble might also have been triggered by a user gesture,
right? It wouldn't make sense to swap it, to me. I think they should be queued
serially -- don't show the usb chooser until after the existing permission
bubble(s) is/are dismissed.

[Reilly Grant (use Gerrit), 2015-10-27 18:26:07]

On 2015/10/26 21:51:29, felt (slow through Nov 3) wrote:
> On 2015/10/26 18:03:39, Reilly Grant wrote:
> > On 2015/10/26 17:46:38, juncai wrote:
> > > On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> > > > On 2015/10/21 00:55:16, Reilly Grant wrote:
> > > > > On 2015/10/21 00:52:20, groby wrote:
> > > > > > On 2015/10/20 22:18:00, raymes wrote:
> > > > > > > groby seemed to know something about the permission bubble stuff,
so
> > > > CC'ing
> > > > > > her
> > > > > > > :)
> > > > > > 
> > > > > > Is this still a relevant CL with the move to the bubble manager?
> > > > > 
> > > > > Yes, this is the patch that uses BubbleManager.
> > > > 
> > > > I must have missed this, but what happens if both a "regular" permission
> > > bubble
> > > > and a USB chooser are opened at the same time? Is there logic to choose
> > which
> > > > one ought to be displayed, or to queue them so that one appears when the
> > next
> > > > one closes?
> > > 
> > > Right now, both bubbles will be shown. Adding hcarmona@ for this.
> > 
> > Since opening a USB device chooser will require a user gesture I think it
> should
> > replace an existing permission bubble as clicking on a button within the
site
> > seems like an indication that the user is ignoring the original bubble.
> > Deferring to hcarmona@ for how this should be coordinated through the
> > BubbleManager API.
> 
> But the permission bubble might also have been triggered by a user gesture,
> right? It wouldn't make sense to swap it, to me. I think they should be queued
> serially -- don't show the usb chooser until after the existing permission
> bubble(s) is/are dismissed.

I'm not sure I agree. If the user is ignoring an unrelated permission prompt but
clicks a "Connect" button then they will not expect to have to close or deny the
permission prompt before they are shown the UI that button is supposed to
trigger.

[felt, 2015-10-28 04:56:55]

On 2015/10/27 18:26:07, Reilly Grant wrote:
> On 2015/10/26 21:51:29, felt (slow through Nov 3) wrote:
> > On 2015/10/26 18:03:39, Reilly Grant wrote:
> > > On 2015/10/26 17:46:38, juncai wrote:
> > > > On 2015/10/25 03:12:12, felt (slow through Nov 3) wrote:
> > > > > On 2015/10/21 00:55:16, Reilly Grant wrote:
> > > > > > On 2015/10/21 00:52:20, groby wrote:
> > > > > > > On 2015/10/20 22:18:00, raymes wrote:
> > > > > > > > groby seemed to know something about the permission bubble
stuff,
> so
> > > > > CC'ing
> > > > > > > her
> > > > > > > > :)
> > > > > > > 
> > > > > > > Is this still a relevant CL with the move to the bubble manager?
> > > > > > 
> > > > > > Yes, this is the patch that uses BubbleManager.
> > > > > 
> > > > > I must have missed this, but what happens if both a "regular"
permission
> > > > bubble
> > > > > and a USB chooser are opened at the same time? Is there logic to
choose
> > > which
> > > > > one ought to be displayed, or to queue them so that one appears when
the
> > > next
> > > > > one closes?
> > > > 
> > > > Right now, both bubbles will be shown. Adding hcarmona@ for this.
> > > 
> > > Since opening a USB device chooser will require a user gesture I think it
> > should
> > > replace an existing permission bubble as clicking on a button within the
> site
> > > seems like an indication that the user is ignoring the original bubble.
> > > Deferring to hcarmona@ for how this should be coordinated through the
> > > BubbleManager API.
> > 
> > But the permission bubble might also have been triggered by a user gesture,
> > right? It wouldn't make sense to swap it, to me. I think they should be
queued
> > serially -- don't show the usb chooser until after the existing permission
> > bubble(s) is/are dismissed.
> 
> I'm not sure I agree. If the user is ignoring an unrelated permission prompt
but
> clicks a "Connect" button then they will not expect to have to close or deny
the
> permission prompt before they are shown the UI that button is supposed to
> trigger.

If we change this behavior for web usb, we should change it for all permission
requests.

Maybe some timeout could be used to define "ignore" -- for example if one comes
right after the other, it seems bad form to show one bubble and then swap it
30sec later.

[juncai, 2015-10-29 00:32:10]

juncai@chromium.org changed reviewers:
+ palmer@chromium.org

[juncai, 2015-10-29 00:34:04]

I refactored the code, and also add code to support device added/removed and
update the the bubble table view when the bubble was already shown. 

reillyg@chromium.org: Please review changes in 
//chrome/browser/usb/

felt@chromium.org: Please review changes in 
//chrome/browser/ui/views/website_settings/
//chrome/browser/ui/website_settings/

palmer@chromium.org: Please review changes in
//chrome/browser/ui/cocoa/website_settings/

thestig@chromium.org: Please review changes in 
//chrome/app/generated_resources.grd
//chrome/browser/chrome_content_browser_client.cc
//chrome/chrome_browser.gypi
//chrome/chrome_browser_ui.gypi

[Reilly Grant (use Gerrit), 2015-10-29 01:11:31]

reillyg@chromium.org changed reviewers:
- felt@chromium.org, palmer@chromium.org, thestig@chromium.org

[Reilly Grant (use Gerrit), 2015-10-29 01:11:33]

I'm removing other reviewers while we get this code cleaned up.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2610: #if !defined(OS_ANDROID)
&& !defined(OS_IOS)
It is possible, like for DeviceManager, to avoid these #ifdefs except in the
Create() function.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:7:
ChooserBubbleUiCocoa::ChooserBubbleUiCocoa() {}
This is not ready for review. If you want to wait on the Cocoa implementation
delete these files and just fail the call to navigator.usb.requestDevice on OS X
in ChromeWebUsbPermissionBubble.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:271:
connect_button_->SetEnabled(true);
This will be called both when an item is selected and when an item is
de-selected so you should be able to check the selection model to enable/disable
the button and won't need the code below in Update that tracks the most recently
updated row.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:297:
table_view_->OnModelChanged();
This should be called by ChooserTableModel on its Observer, not here. The
ChooserTableModel should be a ChooserChoices::Observer which should simplify
some of the plumbing.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:42: #if defined(OS_MACOSX)
I don't see the purpose of having two different fields and ifdefs here.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_choices.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:70: if (!callback_.is_null()) {
nit: No braces for single line ifs.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:88: callback_.reset();  // reset
callback so that it is only called once.
nit: "Reset |callback| so that it is only run once."

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:93: if (itor == devices_.end() &&
There will never be an OnDeviceAdded call for a device that has already been
added. If you wish you can DCHECK(!ContainsValue(devices_, device));

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:84: if (it == frame_usb_services_.end()) {
Create a helper function that prevents duplication of this code.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:31:
browser->GetBubbleManager()->ShowBubble(bubble_delegate.Pass());
I think we need to keep track of open bubbles in case render_frame_host_ is
destroyed, which is why this class is owned by the UsbTabHelper.

[juncai, 2015-10-31 04:15:36]

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2610: #if !defined(OS_ANDROID)
&& !defined(OS_IOS)
On 2015/10/29 01:11:32, Reilly Grant wrote:
> It is possible, like for DeviceManager, to avoid these #ifdefs except in the
> Create() function.

Tried moving webusb.gypi to be included in all platforms in components.gyp, then
ios trybot got "gyp: Cycles in .gyp file dependency graph detected" error.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:7:
ChooserBubbleUiCocoa::ChooserBubbleUiCocoa() {}
On 2015/10/29 01:11:32, Reilly Grant wrote:
> This is not ready for review. If you want to wait on the Cocoa implementation
> delete these files and just fail the call to navigator.usb.requestDevice on OS
X
> in ChromeWebUsbPermissionBubble.

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:271:
connect_button_->SetEnabled(true);
On 2015/10/29 01:11:32, Reilly Grant wrote:
> This will be called both when an item is selected and when an item is
> de-selected so you should be able to check the selection model to
enable/disable
> the button and won't need the code below in Update that tracks the most
recently
> updated row.

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:297:
table_view_->OnModelChanged();
On 2015/10/29 01:11:32, Reilly Grant wrote:
> This should be called by ChooserTableModel on its Observer, not here. The
> ChooserTableModel should be a ChooserChoices::Observer which should simplify
> some of the plumbing.

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:42: #if defined(OS_MACOSX)
On 2015/10/29 01:11:32, Reilly Grant wrote:
> I don't see the purpose of having two different fields and ifdefs here.

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_choices.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:70: if (!callback_.is_null()) {
On 2015/10/29 01:11:33, Reilly Grant wrote:
> nit: No braces for single line ifs.

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:88: callback_.reset();  // reset
callback so that it is only called once.
On 2015/10/29 01:11:32, Reilly Grant wrote:
> nit: "Reset |callback| so that it is only run once."

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:93: if (itor == devices_.end() &&
On 2015/10/29 01:11:32, Reilly Grant wrote:
> There will never be an OnDeviceAdded call for a device that has already been
> added. If you wish you can DCHECK(!ContainsValue(devices_, device));

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:84: if (it == frame_usb_services_.end()) {
On 2015/10/29 01:11:33, Reilly Grant wrote:
> Create a helper function that prevents duplication of this code.

Done.

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/260001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:31:
browser->GetBubbleManager()->ShowBubble(bubble_delegate.Pass());
On 2015/10/29 01:11:33, Reilly Grant wrote:
> I think we need to keep track of open bubbles in case render_frame_host_ is
> destroyed, which is why this class is owned by the UsbTabHelper.

Done.

[juncai, 2015-11-12 03:13:52]

Please review the change.

[Reilly Grant (use Gerrit), 2015-11-12 21:44:37]

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:215:
cancel_button_(nullptr) {
No need to initialize these two fields since they are always initialized in the
constructor body.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:263:
base::string16 connect_text = base::ASCIIToUTF16("Connect");
Add localizable strings for these button labels to
chrome/app/generated_resources.grd.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:321:
connect_button_->SetEnabled(true);
Didn't we determine that OnSelectionChanged not being called when the table
model was updated was a bug we could fix?

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:12: return
"ExtensionInstalled";
"ChooserBubble".

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_choices.h (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_choices.h:14: class ChooserChoices {
s/Choice/Option/g and I think these class and method names will read better.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:33: chooser_bubble_ui_view_ =
bubble_ui.get();
Since ChooserBubbleUiView inherits from BubbleUi and BubbleUi has a Close()
method do we need a chooser_bubble_ui_view_ member here?

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_choices.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:111:
devices_names_.push_back(base::UTF16ToASCII(device->product_string()));
Keep the UTF16 string all the way through, definitely don't convert it to ASCII.
UI code generally works with UTF16 strings as base::string16.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:119: auto itor =
std::find(devices_.begin(), devices_.end(), device);
s/itor/iter/g

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:87: if (!it->second->permission_bubble) {
This is just going to drop the interface request on the floor if the page calls
requestDevice() again. ChromeWebUsbPermissionBubble (if it supports handling
more than one bubble) should have a Bind() method like WebUSBPermissionProvider.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:30:
bubble->CloseBubble(BUBBLE_CLOSE_FORCED);
Shouldn't this be in the destructor?

[juncai, 2015-11-14 01:54:36]

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:215:
cancel_button_(nullptr) {
On 2015/11/12 21:44:36, Reilly Grant wrote:
> No need to initialize these two fields since they are always initialized in
the
> constructor body.

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:263:
base::string16 connect_text = base::ASCIIToUTF16("Connect");
On 2015/11/12 21:44:36, Reilly Grant wrote:
> Add localizable strings for these button labels to
> chrome/app/generated_resources.grd.

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:321:
connect_button_->SetEnabled(true);
On 2015/11/12 21:44:36, Reilly Grant wrote:
> Didn't we determine that OnSelectionChanged not being called when the table
> model was updated was a bug we could fix?

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.cc:12: return
"ExtensionInstalled";
On 2015/11/12 21:44:36, Reilly Grant wrote:
> "ChooserBubble".

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_choices.h (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_choices.h:14: class ChooserChoices {
On 2015/11/12 21:44:36, Reilly Grant wrote:
> s/Choice/Option/g and I think these class and method names will read better.

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:33: chooser_bubble_ui_view_ =
bubble_ui.get();
On 2015/11/12 21:44:36, Reilly Grant wrote:
> Since ChooserBubbleUiView inherits from BubbleUi and BubbleUi has a Close()
> method do we need a chooser_bubble_ui_view_ member here?

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_choices.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:111:
devices_names_.push_back(base::UTF16ToASCII(device->product_string()));
On 2015/11/12 21:44:37, Reilly Grant wrote:
> Keep the UTF16 string all the way through, definitely don't convert it to
ASCII.
> UI code generally works with UTF16 strings as base::string16.

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_choices.cc:119: auto itor =
std::find(devices_.begin(), devices_.end(), device);
On 2015/11/12 21:44:37, Reilly Grant wrote:
> s/itor/iter/g

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:87: if (!it->second->permission_bubble) {
On 2015/11/12 21:44:37, Reilly Grant wrote:
> This is just going to drop the interface request on the floor if the page
calls
> requestDevice() again. ChromeWebUsbPermissionBubble (if it supports handling
> more than one bubble) should have a Bind() method like
WebUSBPermissionProvider.

Done.

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/340001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:30:
bubble->CloseBubble(BUBBLE_CLOSE_FORCED);
On 2015/11/12 21:44:37, Reilly Grant wrote:
> Shouldn't this be in the destructor?

Done.

[Reilly Grant (use Gerrit), 2015-11-16 22:45:44]

reillyg@chromium.org changed reviewers:
+ felt@chromium.org

[Reilly Grant (use Gerrit), 2015-11-16 22:45:46]

Adding back felt@ to review the UI side of this.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:316:
chooser_table_model_->Update();
Just move line 188 here.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:28: if (ContainsValue(set->origins,
origin)) {
nit: no braces

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:33: if (ContainsValue(config.origins,
origin)) {
nit: no braces

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:38: if
(ContainsValue(function.origins, origin)) {
nit: no braces

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:67: origin_ =
render_frame_host_->GetLastCommittedURL().GetOrigin();
Since this object has a reference to the RenderFrameHost anyways there's no need
to keep this in a member variable.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:70: base::Unretained(this)));
Use a WeakPtr here in case the bubble is closed before the device list is
returned.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.h:43: void GetUsbDevicesList(
s/GetUsbDevicesList/GotUsbDeviceList/

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:59: FrameUsbServicesMap::const_iterator
UsbTabHelper::GetFrameUsbService(
Return a FrameUsbServices*.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.h (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.h:27: typedef
base::ScopedPtrMap<content::RenderFrameHost*,
You can now use std::map instead of base::ScopedPtrMap.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:31: if
(render_frame_host_->IsRenderFrameLive()) {
This check is now unnecessary.

https://codereview.chromium.org/1408193003/diff/400001/components/webusb/publ...
File components/webusb/public/interfaces/webusb_permission_bubble.mojom (right):

https://codereview.chromium.org/1408193003/diff/400001/components/webusb/publ...
components/webusb/public/interfaces/webusb_permission_bubble.mojom:21:
Bind(WebUsbPermissionBubble& request);
This method doesn't have to be added to the mojo interface.

[juncai, 2015-11-17 03:42:15]

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:316:
chooser_table_model_->Update();
On 2015/11/16 22:45:45, Reilly Grant wrote:
> Just move line 188 here.

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:28: if (ContainsValue(set->origins,
origin)) {
On 2015/11/16 22:45:45, Reilly Grant wrote:
> nit: no braces

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:33: if (ContainsValue(config.origins,
origin)) {
On 2015/11/16 22:45:45, Reilly Grant wrote:
> nit: no braces

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:38: if
(ContainsValue(function.origins, origin)) {
On 2015/11/16 22:45:46, Reilly Grant wrote:
> nit: no braces

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:67: origin_ =
render_frame_host_->GetLastCommittedURL().GetOrigin();
On 2015/11/16 22:45:45, Reilly Grant wrote:
> Since this object has a reference to the RenderFrameHost anyways there's no
need
> to keep this in a member variable.

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:70: base::Unretained(this)));
On 2015/11/16 22:45:45, Reilly Grant wrote:
> Use a WeakPtr here in case the bubble is closed before the device list is
> returned.

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.h:43: void GetUsbDevicesList(
On 2015/11/16 22:45:46, Reilly Grant wrote:
> s/GetUsbDevicesList/GotUsbDeviceList/

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.cc:59: FrameUsbServicesMap::const_iterator
UsbTabHelper::GetFrameUsbService(
On 2015/11/16 22:45:46, Reilly Grant wrote:
> Return a FrameUsbServices*.

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_tab_helper.h (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/usb...
chrome/browser/usb/usb_tab_helper.h:27: typedef
base::ScopedPtrMap<content::RenderFrameHost*,
On 2015/11/16 22:45:46, Reilly Grant wrote:
> You can now use std::map instead of base::ScopedPtrMap.

Done.

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/web...
File chrome/browser/usb/web_usb_permission_bubble.cc (right):

https://codereview.chromium.org/1408193003/diff/400001/chrome/browser/usb/web...
chrome/browser/usb/web_usb_permission_bubble.cc:31: if
(render_frame_host_->IsRenderFrameLive()) {
On 2015/11/16 22:45:46, Reilly Grant wrote:
> This check is now unnecessary.

Done.

https://codereview.chromium.org/1408193003/diff/400001/components/webusb/publ...
File components/webusb/public/interfaces/webusb_permission_bubble.mojom (right):

https://codereview.chromium.org/1408193003/diff/400001/components/webusb/publ...
components/webusb/public/interfaces/webusb_permission_bubble.mojom:21:
Bind(WebUsbPermissionBubble& request);
On 2015/11/16 22:45:46, Reilly Grant wrote:
> This method doesn't have to be added to the mojo interface.

Done.

[Reilly Grant (use Gerrit), 2015-11-18 00:17:38]

chrome/browser/usb lgtm with nits

https://codereview.chromium.org/1408193003/diff/420001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.cc (right):

https://codereview.chromium.org/1408193003/diff/420001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:50: :
device_filters_(device_filters.Pass()),
This doesn't need to be a member variable if it's only used to populate filters_
below.

https://codereview.chromium.org/1408193003/diff/420001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:100: callback_.reset();  // Reset
|callback| so that it is only run once.
|callback_|

[juncai, 2015-11-19 05:42:00]

https://codereview.chromium.org/1408193003/diff/420001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.cc (right):

https://codereview.chromium.org/1408193003/diff/420001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:50: :
device_filters_(device_filters.Pass()),
On 2015/11/18 00:17:38, Reilly Grant wrote:
> This doesn't need to be a member variable if it's only used to populate
filters_
> below.

Done.

https://codereview.chromium.org/1408193003/diff/420001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:100: callback_.reset();  // Reset
|callback| so that it is only run once.
On 2015/11/18 00:17:38, Reilly Grant wrote:
> |callback_|

Done.

[juncai, 2015-11-20 18:58:14]

ping felt@.

[juncai, 2015-11-24 17:45:16]

On 2015/11/20 18:58:14, juncai wrote:
ping felt@ again.

[felt, 2015-12-02 17:56:40]

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:39: //
chooser permission bubble width
nit: "Chooser", not "chooser"

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:42: //
chooser permission bubble height
not: "Chooser", not "chooser"

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:126: return
1;
Why is this returning 1 instead of 0? (Is it because you want to be able to
display a string saying none found, below?)

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:128: return
static_cast<int>(chooser_options_->GetOptions().size());
device_names.size()

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:140: } else {
simplify: else if here, then else below

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:144: return
base::string16();
Should this be a NOTREACHED()? Under what circumstances could we end up with a
blank string in a row?

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:194:
ui::TableModelObserver* observer_ = nullptr;
initialize in the constructor

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:218: //
create a table view
nit: comments should always be sentence-case ("Create a table view"), here and
elsewhere

[juncai, 2015-12-03 00:17:35]

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:39: //
chooser permission bubble width
On 2015/12/02 17:56:39, felt wrote:
> nit: "Chooser", not "chooser"

Done.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:42: //
chooser permission bubble height
On 2015/12/02 17:56:40, felt wrote:
> not: "Chooser", not "chooser"

Done.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:126: return
1;
On 2015/12/02 17:56:39, felt wrote:
> Why is this returning 1 instead of 0? (Is it because you want to be able to
> display a string saying none found, below?)

Yes, it needs to display the string: IDS_CHOOSER_BUBBLE_NO_DEVICES_FOUND_PROMPT.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:128: return
static_cast<int>(chooser_options_->GetOptions().size());
On 2015/12/02 17:56:39, felt wrote:
> device_names.size()

Done.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:140: } else {
On 2015/12/02 17:56:40, felt wrote:
> simplify: else if here, then else below

Done.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:144: return
base::string16();
On 2015/12/02 17:56:40, felt wrote:
> Should this be a NOTREACHED()? Under what circumstances could we end up with a
> blank string in a row?

Done.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:194:
ui::TableModelObserver* observer_ = nullptr;
On 2015/12/02 17:56:39, felt wrote:
> initialize in the constructor

Done.

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:218: //
create a table view
On 2015/12/02 17:56:39, felt wrote:
> nit: comments should always be sentence-case ("Create a table view"), here and
> elsewhere

Done.

[felt, 2015-12-03 01:19:19]

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:126: return
1;
On 2015/12/03 00:17:34, juncai wrote:
> On 2015/12/02 17:56:39, felt wrote:
> > Why is this returning 1 instead of 0? (Is it because you want to be able to
> > display a string saying none found, below?)
> 
> Yes, it needs to display the string:
IDS_CHOOSER_BUBBLE_NO_DEVICES_FOUND_PROMPT.

Could you add a comment to that effect?

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:399: return
views::BubbleBorder::TOP_LEFT;
What happens if the browser is in RTL? Does the bubble stay on the left or does
it get moved to the right?

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:28: scoped_ptr<BubbleUi>
bubble_ui;
this seems like an odd pattern. why is this being initialized for non-mac here,
but not for mac? (separate CL?)

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:80: if (index >= 0 && index <
static_cast<int>(devices_.size())) {
should the inverse of this condition (index >= # of devices) be DCHECKed?

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:124: }
why doesn't anything happen if it's the last device? shouldn't the window close
or something?

[felt, 2015-12-03 01:21:17]

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:28: scoped_ptr<BubbleUi>
bubble_ui;
On 2015/12/03 01:19:19, felt wrote:
> this seems like an odd pattern. why is this being initialized for non-mac
here,
> but not for mac? (separate CL?)

i now realize the answer is yes, a separate CL. can you please add a TODO with
the crbug here?

[felt, 2015-12-03 01:21:52]

On 2015/10/29 00:34:04, juncai wrote:
> I refactored the code, and also add code to support device added/removed and
> update the the bubble table view when the bubble was already shown. 

^ hey i think i missed where this was done, which class is it in?

> 
> mailto:reillyg@chromium.org: Please review changes in 
> //chrome/browser/usb/
> 
> mailto:felt@chromium.org: Please review changes in 
> //chrome/browser/ui/views/website_settings/
> //chrome/browser/ui/website_settings/
> 
> mailto:palmer@chromium.org: Please review changes in
> //chrome/browser/ui/cocoa/website_settings/
> 
> mailto:thestig@chromium.org: Please review changes in 
> //chrome/app/generated_resources.grd
> //chrome/browser/chrome_content_browser_client.cc
> //chrome/chrome_browser.gypi
> //chrome/chrome_browser_ui.gypi

[juncai, 2015-12-03 17:11:28]

On 2015/12/03 01:21:52, felt wrote:
> On 2015/10/29 00:34:04, juncai wrote:
> > I refactored the code, and also add code to support device added/removed and
> > update the the bubble table view when the bubble was already shown. 
> 
> ^ hey i think i missed where this was done, which class is it in?
> 
> > 
> > mailto:reillyg@chromium.org: Please review changes in 
> > //chrome/browser/usb/
> > 
> > mailto:felt@chromium.org: Please review changes in 
> > //chrome/browser/ui/views/website_settings/
> > //chrome/browser/ui/website_settings/
> > 
> > mailto:palmer@chromium.org: Please review changes in
> > //chrome/browser/ui/cocoa/website_settings/
> > 
> > mailto:thestig@chromium.org: Please review changes in 
> > //chrome/app/generated_resources.grd
> > //chrome/browser/chrome_content_browser_client.cc
> > //chrome/chrome_browser.gypi
> > //chrome/chrome_browser_ui.gypi

This was done at:
usb_chooser_options.cc/h by making class UsbChooserOptions a subclass of
device::UsbService::Observer and override:
// device::UsbService::Observer:
void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override;
void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;

Then in the above two functions, call:
observer()->OnOptionAdded
observer()->OnOptionRemoved

And at chooser_bubble_ui_view.cc, make ChooserTableModel a subclass of
ChooserOptions::Observer, and set itself as UsbChooserOptions's observer, and
override:
void OnOptionAdded(int index) override
void OnOptionRemoved(int index) override
which update the chooser table view.

[juncai, 2015-12-03 18:33:59]

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/490001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:126: return
1;
On 2015/12/03 01:19:18, felt wrote:
> On 2015/12/03 00:17:34, juncai wrote:
> > On 2015/12/02 17:56:39, felt wrote:
> > > Why is this returning 1 instead of 0? (Is it because you want to be able
to
> > > display a string saying none found, below?)
> > 
> > Yes, it needs to display the string:
> IDS_CHOOSER_BUBBLE_NO_DEVICES_FOUND_PROMPT.
> 
> Could you add a comment to that effect?

Done.

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:399: return
views::BubbleBorder::TOP_LEFT;
On 2015/12/03 01:19:19, felt wrote:
> What happens if the browser is in RTL? Does the bubble stay on the left or
does
> it get moved to the right?

For this, I refer to the implementation of:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
Since it is also a permission bubble.

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:28: scoped_ptr<BubbleUi>
bubble_ui;
On 2015/12/03 01:19:19, felt wrote:
> this seems like an odd pattern. why is this being initialized for non-mac
here,
> but not for mac? (separate CL?)

Yes, it is a separate CL which depends on this CL.
https://codereview.chromium.org/1473393003/

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:28: scoped_ptr<BubbleUi>
bubble_ui;
On 2015/12/03 01:21:17, felt wrote:
> On 2015/12/03 01:19:19, felt wrote:
> > this seems like an odd pattern. why is this being initialized for non-mac
> here,
> > but not for mac? (separate CL?)
> 
> i now realize the answer is yes, a separate CL. can you please add a TODO with
> the crbug here?

Done.

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
File chrome/browser/usb/usb_chooser_options.cc (right):

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:80: if (index >= 0 && index <
static_cast<int>(devices_.size())) {
On 2015/12/03 01:19:19, felt wrote:
> should the inverse of this condition (index >= # of devices) be DCHECKed?

Done.

https://codereview.chromium.org/1408193003/diff/510001/chrome/browser/usb/usb...
chrome/browser/usb/usb_chooser_options.cc:124: }
On 2015/12/03 01:19:19, felt wrote:
> why doesn't anything happen if it's the last device? shouldn't the window
close
> or something?

User may accidentally unplug the listed device, and realize it, and plug back
the same device again. In this case, the bubble window can update the device
list and let the user be able to grant permission.

[Jeffrey Yasskin, 2015-12-05 01:13:51]

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

[Jeffrey Yasskin, 2015-12-05 01:13:53]

A couple comments from reviewing the Bluetooth use of this change.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:9: #include
"chrome/browser/ui/website_settings/chooser_options.h"
You can forward declare this type.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:12: class
ChooserBubbleUiView;
Don't forward-declare classes that aren't used in this header.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:14: class
ChooserBubbleDelegate : public BubbleDelegate {
What does this class do? What's its lifetime?

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:22: virtual void
Select(int index) = 0;
When are these functions called? How do subclasses need to override them?

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:14: // Stores all the
available chooser options, handle options initialization,
What is a "chooser option"?

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:21: class Observer {
How are subclasses supposed to use the Observer? BluetoothChooserOptions doesn't
appear to touch it, which surprises me.

[juncai, 2015-12-07 18:28:47]

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:9: #include
"chrome/browser/ui/website_settings/chooser_options.h"
On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> You can forward declare this type.

Done.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:12: class
ChooserBubbleUiView;
On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> Don't forward-declare classes that aren't used in this header.

Done.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:14: class
ChooserBubbleDelegate : public BubbleDelegate {
On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> What does this class do? What's its lifetime?

This class implements class BubbleDelegate:
https://code.google.com/p/chromium/codesearch#chromium/src/components/bubble/...
which defines a bubble. And it is owned by BubbleControler by passing it as a
scoped_ptr to its constructor:
https://code.google.com/p/chromium/codesearch#chromium/src/components/bubble/...

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:22: virtual void
Select(int index) = 0;
On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> When are these functions called? How do subclasses need to override them?

The ChooserBubbleUiView/ChooserBubbleUiCocoa class holds a pointer of
ChooserBubbleDelegate, and it is called when user clicks "Connect" button which
grants permission.
https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/view...
https://codereview.chromium.org/1473393003/diff/60001/chrome/browser/ui/cocoa...
Usually subclass such as UsbChooserBubbleDelegate/BluetoothChooserBubbleDelegate
can override this function to grant permission to user's choice. In the
UsbChooserBubbleDelegate/BluetoothChooserBubbleDelegate implementation, it calls
ChooserOptions::Select.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:14: // Stores all the
available chooser options, handle options initialization,
On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> What is a "chooser option"?

chooser option is the item in the list that the chooser bubble shows for user to
select. For example, it can be usb/bluetooth device names which are listed on
the chooser bubble so that user can grant permission.

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:21: class Observer {
On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> How are subclasses supposed to use the Observer? BluetoothChooserOptions
doesn't
> appear to touch it, which surprises me.

It is the base class of ChooserTableModel:
https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/view...
and base class of ChooserBubbleUiCocoa:
https://codereview.chromium.org/1473393003/diff/60001/chrome/browser/ui/cocoa...
And ChooserTableModel/ChooserBubbleUiCocoa overrides
OnOptionsInitialized/OnOptionsAdded/OnOptionsRemoved to update the UI.

ChooserTableModel and ChooserBubbleUiCocoa are set as the observer of
ChooserOptions, when device added/removed, ChooserOptions calls observer's
OnOptionsInitialized/OnOptionsAdded/OnOptionsRemoved to notify it to update its
UI.

[Jeffrey Yasskin, 2015-12-07 23:57:55]

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:14: class
ChooserBubbleDelegate : public BubbleDelegate {
On 2015/12/07 18:28:47, juncai wrote:
> On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> > What does this class do? What's its lifetime?
> 
> This class implements class BubbleDelegate:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/bubble/...
> which defines a bubble.

Write down what kind of bubble this class defines. We can see from the code that
it implements BubbleDelegate, but we can't see why or how folks are supposed to
use it. You need to comment that. e.g. "Subclass ChooserBubbleDelegate to
implement a chooser bubble, which has some introductory text and a list of
options that users can pick one of. Create an instance of your subclass and pass
it to BubbleManager::ShowBubble() to show the bubble. Your subclass must fill in
chooser_options_ to define the set of options users can pick from. You can also
override GetName() to xyz." (except fix anything I've gotten wrong)

> And it is owned by BubbleControler by passing it as a
> scoped_ptr to its constructor:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/bubble/...

You need to say how users of the class have to use it, in order to avoid using
it after it's been destroyed. For example, are users allowed to call back into
this object after Select(), Close(), or Cancel() is called?

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:22: virtual void
Select(int index) = 0;
On 2015/12/07 18:28:47, juncai wrote:
> On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> > When are these functions called? How do subclasses need to override them?
> 
> The ChooserBubbleUiView/ChooserBubbleUiCocoa class holds a pointer of
> ChooserBubbleDelegate, and it is called when user clicks "Connect" button
which
> grants permission.
>
https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/view...
>
https://codereview.chromium.org/1473393003/diff/60001/chrome/browser/ui/cocoa...
> Usually subclass such as
UsbChooserBubbleDelegate/BluetoothChooserBubbleDelegate
> can override this function to grant permission to user's choice. In the
> UsbChooserBubbleDelegate/BluetoothChooserBubbleDelegate implementation, it
calls
> ChooserOptions::Select.

These functions need comments saying that, so future maintainers can read the
code to understand them.

I'd write something like:

  // These three functions are called just before this object is destroyed:
  // Called when the user selects the |index|th element of |chooser_options_|
from the dialog.
  virtual void Select(int index) = 0;
  // Called when the user presses the 'Cancel' button in the dialog.
  virtual void Cancel() = 0;
  // Called when the user clicks outside the dialog or the dialog otherwise
closes without the user taking an explicit action.
  virtual void Close() = 0;

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:14: // Stores all the
available chooser options, handle options initialization,
On 2015/12/07 18:28:47, juncai wrote:
> On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> > What is a "chooser option"?
> 
> chooser option is the item in the list that the chooser bubble shows for user
to
> select. For example, it can be usb/bluetooth device names which are listed on
> the chooser bubble so that user can grant permission.

Include that in the comment, so folks can read the code to understand it,
instead of having to find this code review comment.

The class comment here should say how a subclass of ChooserOptions needs to
relate to a subclass of ChooserBubbleDelegate. For that matter, do you even need
two parallel class hierarchies? Could the ChooserBubbleDelegate have methods to
retrieve the options' display strings and a way to notify the ChooserTableModel?

https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:21: class Observer {
On 2015/12/07 18:28:47, juncai wrote:
> On 2015/12/05 01:13:53, Jeffrey Yasskin wrote:
> > How are subclasses supposed to use the Observer? BluetoothChooserOptions
> doesn't
> > appear to touch it, which surprises me.
> 
> It is the base class of ChooserTableModel:
>
https://codereview.chromium.org/1408193003/diff/530001/chrome/browser/ui/view...
> and base class of ChooserBubbleUiCocoa:
>
https://codereview.chromium.org/1473393003/diff/60001/chrome/browser/ui/cocoa...
> And ChooserTableModel/ChooserBubbleUiCocoa overrides
> OnOptionsInitialized/OnOptionsAdded/OnOptionsRemoved to update the UI.
> 
> ChooserTableModel and ChooserBubbleUiCocoa are set as the observer of
> ChooserOptions, when device added/removed, ChooserOptions calls observer's
> OnOptionsInitialized/OnOptionsAdded/OnOptionsRemoved to notify it to update
its
> UI.

Again, write this sort of thing down in the class comments. Omit the specific
classes that implement this interface, since that'll change, but say what it
means to receive a call, and say when a ChooserOptions subclass needs to call
these functions. e.g. OnOptionRemoved might say "Called when GetOptions()[index]
is no longer present, and all later options have been moved earlier by 1 slot."
You should be clear about whether calling GetOptions()[index] from inside a call
to OnOption{Added,Removed} will see the added/removed string.

https://codereview.chromium.org/1408193003/diff/550001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/550001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:23: virtual void
OnOptionsInitialized() = 0;
Is there a guarantee that OnOptionsInitialized() is only called once?

https://codereview.chromium.org/1408193003/diff/550001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:32: virtual void Select(int
index) = 0;
Does a call to Select() indicate that the ChooserOptions is about to be
destroyed?

[juncai, 2015-12-08 20:16:29]

Added comments, merged ChooserOptions to ChooserBubbleDelegate.

https://codereview.chromium.org/1408193003/diff/550001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_options.h (right):

https://codereview.chromium.org/1408193003/diff/550001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:23: virtual void
OnOptionsInitialized() = 0;
On 2015/12/07 23:57:55, Jeffrey Yasskin wrote:
> Is there a guarantee that OnOptionsInitialized() is only called once?

Yes, it is called only once at the ChooserBubbleDelegate subclass's constructor.

https://codereview.chromium.org/1408193003/diff/550001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_options.h:32: virtual void Select(int
index) = 0;
On 2015/12/07 23:57:54, Jeffrey Yasskin wrote:
> Does a call to Select() indicate that the ChooserOptions is about to be
> destroyed?

Yes.

[Jeffrey Yasskin, 2015-12-08 20:19:09]

https://codereview.chromium.org/1408193003/diff/570001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/570001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:20: // You can also
override GetName() to xyz.
You have to read the comments I suggest and make sure they're correct. Here,
'xyz' was a placeholder for your own explanation.

[juncai, 2015-12-08 20:59:02]

https://codereview.chromium.org/1408193003/diff/570001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1408193003/diff/570001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:20: // You can also
override GetName() to xyz.
On 2015/12/08 20:19:09, Jeffrey Yasskin wrote:
> You have to read the comments I suggest and make sure they're correct. Here,
> 'xyz' was a placeholder for your own explanation.

Done.

[Jeffrey Yasskin, 2015-12-08 23:48:04]

Thanks! LGTM (although I haven't checked the Bluetooth side yet.)

[juncai, 2015-12-09 20:58:43]

ping felt@, :).

[juncai, 2015-12-09 22:41:44]

juncai@chromium.org changed reviewers:
+ mpearson@chromium.org, sky@chromium.org

[juncai, 2015-12-09 22:41:46]

sky@chromium.org: Please review changes in 
chrome/browser/ui/chrome_bubble_manager.cc

mpearson@chromium.org: Please review changes in 
tools/metrics/histograms/histograms.xml

[Mark P, 2015-12-09 22:44:33]

histograms.xml lgtm

[sky, 2015-12-10 00:32:47]

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.cc:54: else if
(bubble->GetName().compare("ChooserBubble") == 0)
I'm not familiar with this code. Where are the raw strings coming from?

[juncai, 2015-12-10 00:38:39]

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.cc:54: else if
(bubble->GetName().compare("ChooserBubble") == 0)
On 2015/12/10 00:32:47, sky wrote:
> I'm not familiar with this code. Where are the raw strings coming from?

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/webs...
So for the chooser permission bubble, it overrides BubbleDelegate's
std::string GetName(), it can help collecting metrics.

[sky, 2015-12-10 00:59:15]

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.cc:54: else if
(bubble->GetName().compare("ChooserBubble") == 0)
On 2015/12/10 00:38:38, juncai wrote:
> On 2015/12/10 00:32:47, sky wrote:
> > I'm not familiar with this code. Where are the raw strings coming from?
> 
>
https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/webs...
> So for the chooser permission bubble, it overrides BubbleDelegate's
> std::string GetName(), it can help collecting metrics.

Problem with strings is it's easy to change the string and not realize you have
a dependency like this. Can't the bubble return the type?

[juncai, 2015-12-10 01:06:54]

Adding hcarmona@ to cc list regarding sky@'s comments.

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_bubble_manager.cc (right):

https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_bubble_manager.cc:54: else if
(bubble->GetName().compare("ChooserBubble") == 0)
On 2015/12/10 00:59:15, sky wrote:
> On 2015/12/10 00:38:38, juncai wrote:
> > On 2015/12/10 00:32:47, sky wrote:
> > > I'm not familiar with this code. Where are the raw strings coming from?
> > 
> >
>
https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/webs...
> > So for the chooser permission bubble, it overrides BubbleDelegate's
> > std::string GetName(), it can help collecting metrics.
> 
> Problem with strings is it's easy to change the string and not realize you
have
> a dependency like this. Can't the bubble return the type?

Quoting from email from hcarmona@:
"The reason we use a string to identify the bubble, then convert to an enum is
because the enums are specific to the desktop version of chrome, but we want to
keep the bubble manager independent enough to also work on android and iOS in
the future."

[felt, 2015-12-10 02:05:36]

On 2015/12/03 17:11:28, juncai wrote:
> On 2015/12/03 01:21:52, felt wrote:
> > On 2015/10/29 00:34:04, juncai wrote:
> > > I refactored the code, and also add code to support device added/removed
and
> > > update the the bubble table view when the bubble was already shown. 
> > 
> > ^ hey i think i missed where this was done, which class is it in?
> > 
> > > 
> > > mailto:reillyg@chromium.org: Please review changes in 
> > > //chrome/browser/usb/
> > > 
> > > mailto:felt@chromium.org: Please review changes in 
> > > //chrome/browser/ui/views/website_settings/
> > > //chrome/browser/ui/website_settings/
> > > 
> > > mailto:palmer@chromium.org: Please review changes in
> > > //chrome/browser/ui/cocoa/website_settings/
> > > 
> > > mailto:thestig@chromium.org: Please review changes in 
> > > //chrome/app/generated_resources.grd
> > > //chrome/browser/chrome_content_browser_client.cc
> > > //chrome/chrome_browser.gypi
> > > //chrome/chrome_browser_ui.gypi
> 
> This was done at:
> usb_chooser_options.cc/h by making class UsbChooserOptions a subclass of
> device::UsbService::Observer and override:
> // device::UsbService::Observer:
> void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override;
> void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;
> 
> Then in the above two functions, call:
> observer()->OnOptionAdded
> observer()->OnOptionRemoved
> 
> And at chooser_bubble_ui_view.cc, make ChooserTableModel a subclass of
> ChooserOptions::Observer, and set itself as UsbChooserOptions's observer, and
> override:
> void OnOptionAdded(int index) override
> void OnOptionRemoved(int index) override
> which update the chooser table view.

Ah, this is something different than I was trying to ask about. This code
updates the device list if the bubble is already displayed, but that's a
different question. What I'm trying to ask is, what if there is already a
permission bubble (for a non-USB permission) hanging off of the omnibox? How do
you handle that situation?

[sky, 2015-12-10 04:28:06]

I'm still not following. Why can't you use enums as the way to
identify the bubbles, and if you need a text description add something
that takes an the enum and converts to a string.

  -Scott

On Wed, Dec 9, 2015 at 5:06 PM,  <juncai@chromium.org> wrote:
> Adding hcarmona@ to cc list regarding sky@'s comments.
>
>
>
https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
> File chrome/browser/ui/chrome_bubble_manager.cc (right):
>
>
https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/chro...
> chrome/browser/ui/chrome_bubble_manager.cc:54: else if
> (bubble->GetName().compare("ChooserBubble") == 0)
> On 2015/12/10 00:59:15, sky wrote:
>>
>> On 2015/12/10 00:38:38, juncai wrote:
>> > On 2015/12/10 00:32:47, sky wrote:
>> > > I'm not familiar with this code. Where are the raw strings coming
>
> from?
>>
>> >
>> >
>
>
>
https://codereview.chromium.org/1408193003/diff/590001/chrome/browser/ui/webs...
>>
>> > So for the chooser permission bubble, it overrides BubbleDelegate's
>> > std::string GetName(), it can help collecting metrics.
>
>
>> Problem with strings is it's easy to change the string and not realize
>
> you have
>>
>> a dependency like this. Can't the bubble return the type?
>
>
> Quoting from email from hcarmona@:
> "The reason we use a string to identify the bubble, then convert to an
> enum is because the enums are specific to the desktop version of chrome,
> but we want to keep the bubble manager independent enough to also work
> on android and iOS in the future."
>
> https://codereview.chromium.org/1408193003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[juncai, 2015-12-10 18:18:34]

juncai@chromium.org changed reviewers:
+ hcarmona@chromium.org, msw@chromium.org

[juncai, 2015-12-10 18:18:35]

hcarmona@chromium.org
msw@chromium.org

Can you take a look at sky@'s comments? Thanks a lot!

[hcarmona, 2015-12-10 19:17:16]

On 2015/12/10 18:18:35, juncai wrote:
> mailto:hcarmona@chromium.org
> mailto:msw@chromium.org
> 
> Can you take a look at sky@'s comments? Thanks a lot!

Advantages to using a string instead of an enum:
* New bubbles don't have to modify components/bubble
* BubbleManager doesn't have to know about every bubble. This is left to
    the ChromeBubbleManager.

Disadvantages:
* String to Enum conversion logic
* Possibility of the name/enum pair not being updated

We had plans to add metrics for each bubble when we made the decision
to use a string instead of an enum. Now that we add metrics for each
close type, it may not be necessary to keep the name and we can use only
an enum.

[sky, 2015-12-10 20:21:38]

I like enum only!

On Thu, Dec 10, 2015 at 11:17 AM,  <hcarmona@chromium.org> wrote:
> On 2015/12/10 18:18:35, juncai wrote:
>>
>> mailto:hcarmona@chromium.org
>> mailto:msw@chromium.org
>
>
>> Can you take a look at sky@'s comments? Thanks a lot!
>
>
> Advantages to using a string instead of an enum:
> * New bubbles don't have to modify components/bubble
> * BubbleManager doesn't have to know about every bubble. This is left to
>     the ChromeBubbleManager.
>
> Disadvantages:
> * String to Enum conversion logic
> * Possibility of the name/enum pair not being updated
>
> We had plans to add metrics for each bubble when we made the decision
> to use a string instead of an enum. Now that we add metrics for each
> close type, it may not be necessary to keep the name and we can use only
> an enum.
>
> https://codereview.chromium.org/1408193003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[juncai, 2015-12-10 20:49:34]

On 2015/12/10 20:21:38, sky wrote:
> I like enum only!

Yes, I agree that using enum will make the code more clean and easier to
maintain, although it is out of scope for this patch to make those changes. I
will handle it in a follow-up patch.

[sky, 2015-12-10 20:52:16]

Agreed. Please add a TODO and LGTM

[felt, 2015-12-10 21:51:53]

On 2015/12/10 02:05:36, felt wrote:
> On 2015/12/03 17:11:28, juncai wrote:
> > On 2015/12/03 01:21:52, felt wrote:
> > > On 2015/10/29 00:34:04, juncai wrote:
> > > > I refactored the code, and also add code to support device added/removed
> and
> > > > update the the bubble table view when the bubble was already shown. 
> > > 
> > > ^ hey i think i missed where this was done, which class is it in?
> > > 
> > > > 
> > > > mailto:reillyg@chromium.org: Please review changes in 
> > > > //chrome/browser/usb/
> > > > 
> > > > mailto:felt@chromium.org: Please review changes in 
> > > > //chrome/browser/ui/views/website_settings/
> > > > //chrome/browser/ui/website_settings/
> > > > 
> > > > mailto:palmer@chromium.org: Please review changes in
> > > > //chrome/browser/ui/cocoa/website_settings/
> > > > 
> > > > mailto:thestig@chromium.org: Please review changes in 
> > > > //chrome/app/generated_resources.grd
> > > > //chrome/browser/chrome_content_browser_client.cc
> > > > //chrome/chrome_browser.gypi
> > > > //chrome/chrome_browser_ui.gypi
> > 
> > This was done at:
> > usb_chooser_options.cc/h by making class UsbChooserOptions a subclass of
> > device::UsbService::Observer and override:
> > // device::UsbService::Observer:
> > void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override;
> > void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;
> > 
> > Then in the above two functions, call:
> > observer()->OnOptionAdded
> > observer()->OnOptionRemoved
> > 
> > And at chooser_bubble_ui_view.cc, make ChooserTableModel a subclass of
> > ChooserOptions::Observer, and set itself as UsbChooserOptions's observer,
and
> > override:
> > void OnOptionAdded(int index) override
> > void OnOptionRemoved(int index) override
> > which update the chooser table view.
> 
> Ah, this is something different than I was trying to ask about. This code
> updates the device list if the bubble is already displayed, but that's a
> different question. What I'm trying to ask is, what if there is already a
> permission bubble (for a non-USB permission) hanging off of the omnibox? How
do
> you handle that situation?

ping on this question, it got buried a little

[juncai, 2015-12-10 22:20:33]

> > Ah, this is something different than I was trying to ask about. This code
> > updates the device list if the bubble is already displayed, but that's a
> > different question. What I'm trying to ask is, what if there is already a
> > permission bubble (for a non-USB permission) hanging off of the omnibox? How
> do
> > you handle that situation?
> 
> ping on this question, it got buried a little

If there is already a permission bubble (for example, a geolocation permission
bubble), the new bubble will overlap the previous bubble, the previous bubble
still exists.

[juncai, 2015-12-10 22:49:06]

On 2015/12/10 20:52:16, sky wrote:
> Agreed. Please add a TODO and LGTM

Done.

[felt, 2015-12-10 22:53:27]

On 2015/12/10 22:20:33, juncai wrote:
> > > Ah, this is something different than I was trying to ask about. This code
> > > updates the device list if the bubble is already displayed, but that's a
> > > different question. What I'm trying to ask is, what if there is already a
> > > permission bubble (for a non-USB permission) hanging off of the omnibox?
How
> > do
> > > you handle that situation?
> > 
> > ping on this question, it got buried a little
> 
> If there is already a permission bubble (for example, a geolocation permission
> bubble), the new bubble will overlap the previous bubble, the previous bubble
> still exists.

lgtm (for posterity: we're continuing this discussion on an email thread,
overlapping is not intended to be the final state)

[juncai, 2015-12-11 01:37:12]

The CQ bit was checked by juncai@chromium.org

[juncai, 2015-12-11 01:37:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, mpearson@chromium.org, jyasskin@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1408193003/#ps630001
(title: "address sky@'s comments, added TODO to chrome_bubble_manager.cc")

[commit-bot: I haz the power, 2015-12-11 01:38:59]

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

[commit-bot: I haz the power, 2015-12-11 01:49:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 01:49:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[juncai, 2015-12-11 01:53:54]

hcarmona@, please review:
'+components/bubble' dependency that is added to:
//chrome/browser/usb/DEPS

since:
** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+components/bubble',

[hcarmona, 2015-12-11 02:08:18]

On 2015/12/11 01:53:54, juncai wrote:
> hcarmona@, please review:
> '+components/bubble' dependency that is added to:
> //chrome/browser/usb/DEPS
> 
> since:
> ** Presubmit ERRORS **
> Missing LGTM from OWNERS of dependencies added to DEPS:
>     '+components/bubble',

LGTM for adding dep to components/bubble

[juncai, 2015-12-11 02:09:46]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2015-12-11 02:10:20]

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

[commit-bot: I haz the power, 2015-12-11 02:37:48]

Description was changed from

==========
Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device chooser permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204
==========

to

==========
Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device chooser permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204
==========

[commit-bot: I haz the power, 2015-12-11 02:37:49]

Committed patchset #33 (id:630001)

[commit-bot: I haz the power, 2015-12-11 02:38:36]

Description was changed from

==========
Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device chooser permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204
==========

to

==========
Add chrome side webusb permission UI code

This patch added chrome side webusb permission UI code.
It displays a device chooser permission dialog box asking
permission from user so that website can access
the device.

This patch is for Linux, Windows, and ChromeOS. There will
be another patch for Mac.

BUG=492204

Committed: https://crrev.com/a7dd94484b64397611deaf2177f28dcb83a7903b
Cr-Commit-Position: refs/heads/master@{#364593}
==========

[commit-bot: I haz the power, 2015-12-11 02:38:37]

Patchset 33 (id:??) landed as
https://crrev.com/a7dd94484b64397611deaf2177f28dcb83a7903b
Cr-Commit-Position: refs/heads/master@{#364593}