Address some TODOs for ChooserBubbleDelegate class

Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector
containing a pair of device information instead of two
vectors.

5. Changed code that uses the above functions.

BUG=492204, 535633, 535635
Committed: https://crrev.com/7d445832300217e3951ce0dd6f9f569ab4912351
Cr-Commit-Position: refs/heads/master@{#367476}

[juncai, 2015-12-22 21:34:15]

Description was changed from

==========
Use size_t instead of int for
ChooserBubbleDelegate::Observer::OnOptionAdded etc.

This patch use size_t instead of int for
ChooserBubbleDelegate::Observer::OnOptionAdded etc.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:
1. Changed int to size_t for
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector containing a pair of
device information instead of two vectors.

BUG=492204, 535633, 535635
==========

[juncai, 2015-12-22 21:35:46]

Description was changed from

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:
1. Changed int to size_t for
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector containing a pair of
device information instead of two vectors.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:
1. Changed int to size_t for
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

BUG=492204, 535633, 535635
==========

[juncai, 2015-12-22 21:36:10]

Description was changed from

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:
1. Changed int to size_t for
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:
1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

BUG=492204, 535633, 535635
==========

[juncai, 2015-12-23 00:45:24]

Description was changed from

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:
1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

BUG=492204, 535633, 535635
==========

[juncai, 2015-12-23 00:46:29]

Description was changed from

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

5. Changed code that uses the above functions.

BUG=492204, 535633, 535635
==========

[juncai, 2016-01-04 17:54:19]

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

[juncai, 2016-01-04 17:54:20]

Please review this patch.

[Reilly Grant (use Gerrit), 2016-01-04 19:00:26]

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:71:
device_names_and_ids_.erase(device_names_and_ids_.begin() + index);
Change this loop to use an iterator and call device_names_and_ids_.erase(it)
instead of constructing the iterator here. You can then use std::distance()
below to compute the index to pass to OnOptionRemoved().

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:344:
[tableView_ setEnabled:chooserBubbleDelegate_->NumOptions()];
Please explicitly compare this value to 0.

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:136:
devices_.erase(devices_.begin() + index);
Same comment here about just using an iterator for the loop.

[juncai, 2016-01-04 19:35:55]

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:71:
device_names_and_ids_.erase(device_names_and_ids_.begin() + index);
On 2016/01/04 19:00:25, Reilly Grant wrote:
> Change this loop to use an iterator and call device_names_and_ids_.erase(it)
> instead of constructing the iterator here. You can then use std::distance()
> below to compute the index to pass to OnOptionRemoved().

Done.

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:344:
[tableView_ setEnabled:chooserBubbleDelegate_->NumOptions()];
On 2016/01/04 19:00:25, Reilly Grant wrote:
> Please explicitly compare this value to 0.

Done.

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/60001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:136:
devices_.erase(devices_.begin() + index);
On 2016/01/04 19:00:25, Reilly Grant wrote:
> Same comment here about just using an iterator for the loop.

Done.

[Reilly Grant (use Gerrit), 2016-01-04 20:34:11]

lgtm

[juncai, 2016-01-04 20:38:51]

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

[juncai, 2016-01-04 20:38:51]

pkasting@chromium.org: Please review changes in 
//chrome/browser/ui

[Peter Kasting, 2016-01-04 21:02:52]

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:71: end =
device_names_and_ids_.end();
Don't declare a temp for |end|.  Compare directly to device_names_and_ids_.end()
instead.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:74: size_t
index = std::distance(device_names_and_ids_.begin(), it);
You don't need to use std::distance() here since these are random-access
iterators.  Just use "it - device_names_and_ids_.begin()".

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:78: break;
Prefer "return" to "break" here.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:298: return
num_options == 0 ? 1 : static_cast<NSInteger>(num_options);
Nit: Shorter:

  return static_cast<NSInteger>(std::min(chooserBubbleDelegate_->NumOptions(),
1));

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:307:
DCHECK(rowIndex == 0);
Nit: While here: DCHECK_EQ(0, rowIndex);

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:309: } else
{
Nit: While here: No "else" after return (multiple places)

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:310: if
(rowIndex >= 0 && rowIndex < num_options) {
Should this be DCHECKs instead of conditionals?  How can we be called with an
out-of-bounds value?

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:119: :
observer_(nullptr), chooser_bubble_delegate_(chooser_bubble_delegate) {
While here: Can you de-inline the methods of this class?  I think it would make
things more readable to do so, and there doesn't seem to be any clear reason to
inline them.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:128: //
devices found, so the number of rows is 1.
This comment is confusing.  How about:

When there are no devices, the table contains a message saying there are no
devices, so the number of rows is always at least 1.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:129: return
num_options == 0 ? 1 : static_cast<int>(num_options);
Same suggestion re: std::min().

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:133:
base::string16 GetText(int row, int column_id) override {
Same comments in this function as in the .mm file

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:152: //
ChooserOptions::Observer:
Isn't this really "ChooserBubbleDelegate::Observer"?

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:38: // Called after
GetOption(index) has been added to the options and the
Nit: Everywhere in this file: Please put a blank line above a function comment
like this, so the comment + function are visually distinct from the previous
function.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:133:
scoped_refptr<device::UsbDevice> device) {
Same comments in this function as in the other file

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.h:65: // same device.
Nit: How about:

Each pair is a (device, device name).

[juncai, 2016-01-04 22:44:47]

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:71: end =
device_names_and_ids_.end();
On 2016/01/04 21:02:51, Peter Kasting wrote:
> Don't declare a temp for |end|.  Compare directly to
device_names_and_ids_.end()
> instead.

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:74: size_t
index = std::distance(device_names_and_ids_.begin(), it);
On 2016/01/04 21:02:51, Peter Kasting wrote:
> You don't need to use std::distance() here since these are random-access
> iterators.  Just use "it - device_names_and_ids_.begin()".

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.cc:78: break;
On 2016/01/04 21:02:51, Peter Kasting wrote:
> Prefer "return" to "break" here.

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:298: return
num_options == 0 ? 1 : static_cast<NSInteger>(num_options);
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return static_cast<NSInteger>(std::min(chooserBubbleDelegate_->NumOptions(),
> 1));

I think here it needs to use std::max instead of std::min.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:307:
DCHECK(rowIndex == 0);
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Nit: While here: DCHECK_EQ(0, rowIndex);

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:309: } else
{
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Nit: While here: No "else" after return (multiple places)

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:310: if
(rowIndex >= 0 && rowIndex < num_options) {
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Should this be DCHECKs instead of conditionals?  How can we be called with an
> out-of-bounds value?

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:119: :
observer_(nullptr), chooser_bubble_delegate_(chooser_bubble_delegate) {
On 2016/01/04 21:02:52, Peter Kasting wrote:
> While here: Can you de-inline the methods of this class?  I think it would
make
> things more readable to do so, and there doesn't seem to be any clear reason
to
> inline them.

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:128: //
devices found, so the number of rows is 1.
On 2016/01/04 21:02:52, Peter Kasting wrote:
> This comment is confusing.  How about:
> 
> When there are no devices, the table contains a message saying there are no
> devices, so the number of rows is always at least 1.

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:129: return
num_options == 0 ? 1 : static_cast<int>(num_options);
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Same suggestion re: std::min().

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:133:
base::string16 GetText(int row, int column_id) override {
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Same comments in this function as in the .mm file

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/website_settings/chooser_bubble_ui_view.cc:152: //
ChooserOptions::Observer:
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Isn't this really "ChooserBubbleDelegate::Observer"?

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/chooser_bubble_delegate.h:38: // Called after
GetOption(index) has been added to the options and the
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Nit: Everywhere in this file: Please put a blank line above a function comment
> like this, so the comment + function are visually distinct from the previous
> function.

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.cc (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.cc:133:
scoped_refptr<device::UsbDevice> device) {
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Same comments in this function as in the other file

Done.

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
File chrome/browser/usb/usb_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/usb/usb_...
chrome/browser/usb/usb_chooser_bubble_delegate.h:65: // same device.
On 2016/01/04 21:02:52, Peter Kasting wrote:
> Nit: How about:
> 
> Each pair is a (device, device name).

Done.

[Peter Kasting, 2016-01-04 23:54:00]

LGTM

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm
(right):

https://codereview.chromium.org/1545773002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:298: return
num_options == 0 ? 1 : static_cast<NSInteger>(num_options);
On 2016/01/04 22:44:46, juncai wrote:
> On 2016/01/04 21:02:52, Peter Kasting wrote:
> > Nit: Shorter:
> > 
> >   return
static_cast<NSInteger>(std::min(chooserBubbleDelegate_->NumOptions(),
> > 1));
> 
> I think here it needs to use std::max instead of std::min.

Oops!  Yes.

https://codereview.chromium.org/1545773002/diff/140001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1545773002/diff/140001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h:49: // The
vector contains device name and device id pair for the same device.
Nit: To follow the other comment, how about:

Each pair is a (device name, device id).

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

https://codereview.chromium.org/1545773002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:297: return
std::max(static_cast<NSInteger>(chooserBubbleDelegate_->NumOptions()),
Nit: Might as well copy the comment from the other file here:

When there are no devices, the table contains a message saying there are no
devices, so the number of rows is always at least 1.

[juncai, 2016-01-05 03:37:27]

https://codereview.chromium.org/1545773002/diff/140001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h (right):

https://codereview.chromium.org/1545773002/diff/140001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_bubble_delegate.h:49: // The
vector contains device name and device id pair for the same device.
On 2016/01/04 23:54:00, Peter Kasting wrote:
> Nit: To follow the other comment, how about:
> 
> Each pair is a (device name, device id).

Done.

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

https://codereview.chromium.org/1545773002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/chooser_bubble_ui_cocoa.mm:297: return
std::max(static_cast<NSInteger>(chooserBubbleDelegate_->NumOptions()),
On 2016/01/04 23:54:00, Peter Kasting wrote:
> Nit: Might as well copy the comment from the other file here:
> 
> When there are no devices, the table contains a message saying there are no
> devices, so the number of rows is always at least 1.

Done.

[juncai, 2016-01-05 03:37:35]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-01-05 03:37:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1545773002/#ps160001
(title: "updated comments")

[commit-bot: I haz the power, 2016-01-05 03:37:55]

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

[commit-bot: I haz the power, 2016-01-05 03:42:46]

Description was changed from

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

5. Changed code that uses the above functions.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

5. Changed code that uses the above functions.

BUG=492204, 535633, 535635
==========

[commit-bot: I haz the power, 2016-01-05 03:42:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-05 03:43:42]

Description was changed from

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector 
containing a pair of device information instead of two 
vectors.

5. Changed code that uses the above functions.

BUG=492204, 535633, 535635
==========

to

==========
Address some TODOs for ChooserBubbleDelegate class.

This patch did the following changes:

1. Changed
ChooserBubbleDelegate::Observer::OnOptionAdded(int index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(int index)
to
ChooserBubbleDelegate::Observer::OnOptionAdded(size_t index)
ChooserBubbleDelegate::Observer::OnOptionRemoved(size_t index)

2. Changed
ChooserBubbleDelegate::GetOptions()
function to two functions:
ChooserBubbleDelegate::NumOptions
ChooserBubbleDelegate::GetOption(size_t index)
to improve class encapsulation.

3. Changed
ChooserBubbleDelegate::Select(int index)
to:
ChooserBubbleDelegate::Select(size_t index)

4. For USB/Bluetooth bubble delegate class, use a vector
containing a pair of device information instead of two
vectors.

5. Changed code that uses the above functions.

BUG=492204, 535633, 535635

Committed: https://crrev.com/7d445832300217e3951ce0dd6f9f569ab4912351
Cr-Commit-Position: refs/heads/master@{#367476}
==========

[commit-bot: I haz the power, 2016-01-05 03:43:43]

Patchset 9 (id:??) landed as
https://crrev.com/7d445832300217e3951ce0dd6f9f569ab4912351
Cr-Commit-Position: refs/heads/master@{#367476}