Show device connection and paired status in chooser on Mac

Show device connection and paired status in chooser on Mac

This CL added code to show an icon for connected device in the
chooser, and show "Paired" text below the device name if the
device is paired.

I uploaded some screenshots in the issue page.

BUG=543466
Committed: https://crrev.com/87d092908d6e01e183bb02721ad7add4e4babeb4
Cr-Commit-Position: refs/heads/master@{#418774}

[juncai, 2016-09-02 17:46:42]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 17:47:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 17:49:41]

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

[commit-bot: I haz the power, 2016-09-02 17:49:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[juncai, 2016-09-02 21:09:23]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 21:10:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-02 21:20:22]

juncai@chromium.org changed reviewers:
+ jyasskin@chromium.org, msw@chromium.org, oshima@chromium.org,
rsesek@chromium.org

[juncai, 2016-09-02 21:20:23]

jyasskin@chromium.org: Please review changes in 
//chrome/browser/chooser_controller/
//chrome/browser/ui/bluetooth/
//content/browser/bluetooth/

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

oshima@chromium.org: Please review changes in 
//ui/resources/

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

[commit-bot: I haz the power, 2016-09-02 22:37:38]

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

[commit-bot: I haz the power, 2016-09-02 22:37:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msw, 2016-09-06 19:25:56]

c/b/ui/views lgtm

[oshima, 2016-09-06 19:50:37]

oshima@chromium.org changed reviewers:
+ estade@chromium.org, tdanderson@chromium.org

[oshima, 2016-09-06 19:50:38]

+estade@, tdanderson@

so should we use vector graphics for this? I'm trying to understand if I should
ask for vector graphics instead of png in general.

[Robert Sesek, 2016-09-06 21:32:52]

lgtm

[Evan Stade, 2016-09-06 21:51:17]

yes, this should use vector icons. We should only use pngs for complicated
assets that don't lend themselves to easy vectorization.

[juncai, 2016-09-07 00:39:52]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 00:40:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-07 01:03:52]

On 2016/09/06 21:51:17, Evan Stade wrote:
> yes, this should use vector icons. We should only use pngs for complicated
> assets that don't lend themselves to easy vectorization.

Done.

[Jeffrey Yasskin, 2016-09-07 02:01:48]

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
chrome/browser/chooser_controller/chooser_controller.h:88: // Returns if the
|index|th option is connected.
For virtual functions with a default, mention the default in the .h.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
File chrome/browser/chooser_controller/mock_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
chrome/browser/chooser_controller/mock_chooser_controller.h:42: bool
is_connected,
I'm unhappy with adjacent boolean arguments. Would it make sense to use an enum
with PAIRED and CONNECTED values, and pass "PAIRED | CONNECTED" to represent
being both?

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:74: -
(base::scoped_nsobject<NSView>)createTableRowView:(NSInteger)row;
Is 'row' no longer an index?

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:68: // Creates a text
field with |text|.
I think this is a label, rather than a general text field, since it's not
editable or selectable.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:72: [text_field
setDrawsBackground:NO];
I hate that we need this long list of setters in order to create a label, but I
don't see any better way to do it in Chrome.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:575: if (numOptions == 0)
{
There are enough special cases for numOptions==0 here that I suspect it'd be
simpler to have a different kind of view for the no-options text, instead of
multiplexing both cases into a single kind of view.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:613: if
(chooserController_->IsPaired(rowIndex))
This part of the calculation should be done inside the
ChooserContentTableRowView:initWithText.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:87:
bool expected_is_connected) {
Use a separate function to assert that the row is showing the connected image.
Saving the tableRowViewImage: call in a test isn't worth having a boolean
parameter.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:115:
void ExpectRowPairedStatusIs(int row, bool expected_is_paired) {
Here you can just return whether the row is paired, and put the EXPECT_TRUE
outside the function.

Do EXPECT inside this function that the paired text is either
IDS_DEVICE_CHOOSER_PAIRED_STATUS_TEXT or nothing, but then returning true or
false will be clearer to read than passing them in.

[ortuno, 2016-09-07 02:47:01]

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

[ortuno, 2016-09-07 02:47:02]

https://codereview.chromium.org/2304213002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:357:
device.IsPaired() /* is_paired */,
BluetoothDevice::IsPaired() returns whether or not the machine is physically
paired with the bt device. But we want to show if the website is paired with the
device. A website is paired with a device if it's in WebBluetoothServiceImpl's
allowed_devices_map_.

You could pass in a reference to the map of allowed devices and query it here.
You will probably have to pass in the origin as well.

https://codereview.chromium.org/2304213002/diff/60001/ui/gfx/vector_icons/BUI...
File ui/gfx/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2304213002/diff/60001/ui/gfx/vector_icons/BUI...
ui/gfx/vector_icons/BUILD.gn:20: "bluetooth_connected.icon",
Why can't we use vector icons for the signal strength icons?

[commit-bot: I haz the power, 2016-09-07 04:18:30]

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

[commit-bot: I haz the power, 2016-09-07 04:18:30]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-09-07 16:41:42]

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:129:
gfx::VectorIconId::BLUETOOTH_CONNECTED, SK_ColorBLACK));
are you sure you want black and not kChromeIconGrey? As the name suggests, it's
the color used for more or less all icons in Chrome.

https://codereview.chromium.org/2304213002/diff/60001/ui/gfx/vector_icons/BUI...
File ui/gfx/vector_icons/BUILD.gn (right):

https://codereview.chromium.org/2304213002/diff/60001/ui/gfx/vector_icons/BUI...
ui/gfx/vector_icons/BUILD.gn:20: "bluetooth_connected.icon",
On 2016/09/07 02:47:02, ortuno wrote:
> Why can't we use vector icons for the signal strength icons?

We could, but imo that's not necessarily blocking for this change since it's
pre-existing. Note that for signal strength (both wifi and mobile) we actually
have code to draw directly instead of using .icon files, and we should expose
this publicly for use in more places.

[juncai, 2016-09-09 19:50:38]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 19:51:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-09 20:01:28]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 20:02:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-09 20:06:15]

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
chrome/browser/chooser_controller/chooser_controller.h:88: // Returns if the
|index|th option is connected.
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> For virtual functions with a default, mention the default in the .h.

Done.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
File chrome/browser/chooser_controller/mock_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/chooser_...
chrome/browser/chooser_controller/mock_chooser_controller.h:42: bool
is_connected,
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> I'm unhappy with adjacent boolean arguments. Would it make sense to use an
enum
> with PAIRED and CONNECTED values, and pass "PAIRED | CONNECTED" to represent
> being both?

Done.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:74: -
(base::scoped_nsobject<NSView>)createTableRowView:(NSInteger)row;
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> Is 'row' no longer an index?

It is still an index. Just want to make the variable name consistent with
functions like:
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...:
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...:
which uses |row| instead of |rowIndex|.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:68: // Creates a text
field with |text|.
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> I think this is a label, rather than a general text field, since it's not
> editable or selectable.

Done.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:72: [text_field
setDrawsBackground:NO];
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> I hate that we need this long list of setters in order to create a label, but
I
> don't see any better way to do it in Chrome.

Acknowledged.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:129:
gfx::VectorIconId::BLUETOOTH_CONNECTED, SK_ColorBLACK));
On 2016/09/07 16:41:42, Evan Stade wrote:
> are you sure you want black and not kChromeIconGrey? As the name suggests,
it's
> the color used for more or less all icons in Chrome.

Done.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:575: if (numOptions == 0)
{
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> There are enough special cases for numOptions==0 here that I suspect it'd be
> simpler to have a different kind of view for the no-options text, instead of
> multiplexing both cases into a single kind of view.

If using a different view, it will make the test code more complicated. So here
I added a constructor for class ChooserContentTableRowView that just initialize
its |text_|.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:613: if
(chooserController_->IsPaired(rowIndex))
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> This part of the calculation should be done inside the
> ChooserContentTableRowView:initWithText. 
Since
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/website_settings...
needs to implement the heightOfRow function:
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...:
to support varying row heights, this function needs to return the appropriate
row height in this function.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm
(right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:87:
bool expected_is_connected) {
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> Use a separate function to assert that the row is showing the connected image.
> Saving the tableRowViewImage: call in a test isn't worth having a boolean
> parameter.

Done.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/chooser_dialog_cocoa_controller_unittest.mm:115:
void ExpectRowPairedStatusIs(int row, bool expected_is_paired) {
On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> Here you can just return whether the row is paired, and put the EXPECT_TRUE
> outside the function.
> 
> Do EXPECT inside this function that the paired text is either
> IDS_DEVICE_CHOOSER_PAIRED_STATUS_TEXT or nothing, but then returning true or
> false will be clearer to read than passing them in.

Done.

https://codereview.chromium.org/2304213002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:357:
device.IsPaired() /* is_paired */,
On 2016/09/07 02:47:02, ortuno wrote:
> BluetoothDevice::IsPaired() returns whether or not the machine is physically
> paired with the bt device. But we want to show if the website is paired with
the
> device. A website is paired with a device if it's in WebBluetoothServiceImpl's
> allowed_devices_map_.
> 
> You could pass in a reference to the map of allowed devices and query it here.
> You will probably have to pass in the origin as well.

Done.

[Evan Stade, 2016-09-09 20:44:48]

lgtm

https://codereview.chromium.org/2304213002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:624: return
kTableRowViewOneLineHeight;
nit: no else after return (I'd replace this with a ternary)

[commit-bot: I haz the power, 2016-09-09 22:07:09]

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

[commit-bot: I haz the power, 2016-09-09 22:07:11]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-09-12 03:45:22]

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:361: if
(device_id_ptr) {
As long as the user has paired with the device in the past we want to show the
device as paired. Besides that I don't think we can use GetUUIDs to decide if a
device contains a set of UUIDs or not, for example a device that is constantly
alternating between two different UUIDs will keep changing between paired and
not paired and that will probably confuse users. I think the following is
enough:

bool is_paired = bluetooth_allowed_devices_map->GetDeviceId(origin_,
device.GetAddress()) != nullptr;

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:375:
device.IsGattConnected() /* is_gatt_connected */,
I don't think you need the comments anymore. It's clear what the parameters are.

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:54:
BluetoothAllowedDevicesMap* bluetooth_allowed_devices_map,
Would it be too much work to pass in a const& here?

[juncai, 2016-09-12 20:35:58]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 20:36:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-12 20:37:12]

https://codereview.chromium.org/2304213002/diff/100001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:624: return
kTableRowViewOneLineHeight;
On 2016/09/09 20:44:47, Evan Stade wrote:
> nit: no else after return (I'd replace this with a ternary)

Done.

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:361: if
(device_id_ptr) {
On 2016/09/12 03:45:22, ortuno wrote:
> As long as the user has paired with the device in the past we want to show the
> device as paired. Besides that I don't think we can use GetUUIDs to decide if
a
> device contains a set of UUIDs or not, for example a device that is constantly
> alternating between two different UUIDs will keep changing between paired and
> not paired and that will probably confuse users. I think the following is
> enough:
> 
> bool is_paired = bluetooth_allowed_devices_map->GetDeviceId(origin_,
> device.GetAddress()) != nullptr;

Done.

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:375:
device.IsGattConnected() /* is_gatt_connected */,
On 2016/09/12 03:45:22, ortuno wrote:
> I don't think you need the comments anymore. It's clear what the parameters
are.

Done.

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:54:
BluetoothAllowedDevicesMap* bluetooth_allowed_devices_map,
On 2016/09/12 03:45:22, ortuno wrote:
> Would it be too much work to pass in a const& here?

Done.

[commit-bot: I haz the power, 2016-09-12 22:49:43]

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

[commit-bot: I haz the power, 2016-09-12 22:49:44]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-09-13 01:18:18]

thanks! Web Bluetooth lgtm

[juncai, 2016-09-14 00:02:36]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 00:03:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-14 00:52:58]

ping jyasskin@, :).

[commit-bot: I haz the power, 2016-09-14 04:27:51]

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

[commit-bot: I haz the power, 2016-09-14 04:27:52]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-09-14 20:08:37]

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.h (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.h:74: -
(base::scoped_nsobject<NSView>)createTableRowView:(NSInteger)row;
On 2016/09/09 20:06:15, juncai wrote:
> On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> > Is 'row' no longer an index?
> 
> It is still an index. Just want to make the variable name consistent with
> functions like:
>
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...:
>
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...:
> which uses |row| instead of |rowIndex|.

Ok.

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:613: if
(chooserController_->IsPaired(rowIndex))
On 2016/09/09 20:06:15, juncai wrote:
> On 2016/09/07 02:01:47, Jeffrey Yasskin wrote:
> > This part of the calculation should be done inside the
> > ChooserContentTableRowView:initWithText. 
> Since
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/website_settings...
> needs to implement the heightOfRow function:
>
https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSTable...:
> to support varying row heights, this function needs to return the appropriate
> row height in this function.

Yep, oh well.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
File chrome/browser/chooser_controller/mock_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
chrome/browser/chooser_controller/mock_chooser_controller.cc:48: return
!!(options_[index].connected_paired_status &
Use "!= 0" instead of !! to explicitly convert integers to bool, but in this
case I think you can use the implicit conversion.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
File chrome/browser/chooser_controller/mock_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
chrome/browser/chooser_controller/mock_chooser_controller.h:68: int
connected_paired_status;
Comment that the value is the '|' of ConnectedPairedStatus values.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:93: // This initializer is
used when the chooser needs to show no devices found
"show the no-devices-found message"

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:95: -
(instancetype)initWithText:(NSString*)text;
https://google.github.io/styleguide/objcguide.xml?showone=Identify_Designated...
says to always have a designated initializer, but here we have two separate
ones. I don't know ObjC well enough to say if that's a problem. Maybe comment
both as designated initializers?

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:605: if
(chooserController_->GetSignalStrengthLevel(i) != -1 ||
I think we want to indent rows that have neither a signal strength nor a
connected icon. Your ChromeOS screenshot
(https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c62) shows this,
but your Mac screenshot
(https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c61) only shows
rows with icons, so the UI folks didn't get a chance to comment on the varying
indentation that I think you've implemented here.

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:264: const
url::Origin requesting_origin =
I think we're assuming that this origin matches the one passed into the
constructor. Either DCHECK that they're the same, or just use this one.

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:54: const
BluetoothAllowedDevicesMap& bluetooth_allowed_devices_map,
Taking this by reference requires that the ChooserController be destroyed before
the BluetoothAllowedDevicesMap. That's guaranteed if web_bluetooth_service_ owns
the bluetooth_allowed_devices_map, and you should comment that requirement in
this function's comment block.

I'd be a little more comfortable if you added a method to
WebBluetoothServiceImpl that allowed the controller to call through there to
check the contents of the AllowedDevicesMap, instead of adding this extra
constraint on lifetimes.

[juncai, 2016-09-14 23:10:37]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 23:11:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-14 23:13:07]

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
File chrome/browser/chooser_controller/mock_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
chrome/browser/chooser_controller/mock_chooser_controller.cc:48: return
!!(options_[index].connected_paired_status &
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> Use "!= 0" instead of !! to explicitly convert integers to bool, but in this
> case I think you can use the implicit conversion.

Done.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
File chrome/browser/chooser_controller/mock_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/chooser...
chrome/browser/chooser_controller/mock_chooser_controller.h:68: int
connected_paired_status;
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> Comment that the value is the '|' of ConnectedPairedStatus values.

Done.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:93: // This initializer is
used when the chooser needs to show no devices found
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> "show the no-devices-found message"

Done.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:95: -
(instancetype)initWithText:(NSString*)text;
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
>
https://google.github.io/styleguide/objcguide.xml?showone=Identify_Designated...
> says to always have a designated initializer, but here we have two separate
> ones. I don't know ObjC well enough to say if that's a problem. Maybe comment
> both as designated initializers?

Done.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:605: if
(chooserController_->GetSignalStrengthLevel(i) != -1 ||
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> I think we want to indent rows that have neither a signal strength nor a
> connected icon. Your ChromeOS screenshot
> (https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c62) shows this,
> but your Mac screenshot
> (https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c61) only shows
> rows with icons, so the UI folks didn't get a chance to comment on the varying
> indentation that I think you've implemented here.

I uploaded a screenshot from Mac showing the text is indented in a row which has
no image while other row has image.

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:264: const
url::Origin requesting_origin =
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> I think we're assuming that this origin matches the one passed into the
> constructor. Either DCHECK that they're the same, or just use this one.

Added WebBluetoothServiceImpl::IsDevicePaired(), so not using the origin
directly at BluetoothDeviceChooserController::AddFilteredDevice().

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.h (right):

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.h:54: const
BluetoothAllowedDevicesMap& bluetooth_allowed_devices_map,
On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> Taking this by reference requires that the ChooserController be destroyed
before
> the BluetoothAllowedDevicesMap. That's guaranteed if web_bluetooth_service_
owns
> the bluetooth_allowed_devices_map, and you should comment that requirement in
> this function's comment block.
> 
> I'd be a little more comfortable if you added a method to
> WebBluetoothServiceImpl that allowed the controller to call through there to
> check the contents of the AllowedDevicesMap, instead of adding this extra
> constraint on lifetimes.

Done.

[Jeffrey Yasskin, 2016-09-14 23:52:04]

LGTM; we can figure out the detailed appearance later if it needs to change.

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:605: if
(chooserController_->GetSignalStrengthLevel(i) != -1 ||
On 2016/09/14 23:13:07, juncai wrote:
> On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> > I think we want to indent rows that have neither a signal strength nor a
> > connected icon. Your ChromeOS screenshot
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c62) shows
this,
> > but your Mac screenshot
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c61) only
shows
> > rows with icons, so the UI folks didn't get a chance to comment on the
varying
> > indentation that I think you've implemented here.
> 
> I uploaded a screenshot from Mac showing the text is indented in a row which
has
> no image while other row has image.

Thanks, I missed the loop somehow. :-/ If the first couple devices don't have
signal strengths, is the text going to jump horizontally when one gets
connected?

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2304213002/diff/140001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:264: const
url::Origin requesting_origin =
On 2016/09/14 23:13:07, juncai wrote:
> On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> > I think we're assuming that this origin matches the one passed into the
> > constructor. Either DCHECK that they're the same, or just use this one.
> 
> Added WebBluetoothServiceImpl::IsDevicePaired(), so not using the origin
> directly at BluetoothDeviceChooserController::AddFilteredDevice().

Great.

https://codereview.chromium.org/2304213002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2304213002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:64: bool
IsDevicePaired(const std::string& device_address);
Comment that this returns whether the device is paired with the
RenderFrameHost's LastCommittedOrigin().

[juncai, 2016-09-15 00:56:07]

The CQ bit was checked by juncai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 00:56:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2016-09-15 01:00:03]

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm (right):

https://codereview.chromium.org/2304213002/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/chooser_content_view_cocoa.mm:605: if
(chooserController_->GetSignalStrengthLevel(i) != -1 ||
On 2016/09/14 23:52:03, Jeffrey Yasskin wrote:
> On 2016/09/14 23:13:07, juncai wrote:
> > On 2016/09/14 20:08:36, Jeffrey Yasskin wrote:
> > > I think we want to indent rows that have neither a signal strength nor a
> > > connected icon. Your ChromeOS screenshot
> > > (https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c62) shows
> this,
> > > but your Mac screenshot
> > > (https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c61) only
> shows
> > > rows with icons, so the UI folks didn't get a chance to comment on the
> varying
> > > indentation that I think you've implemented here.
> > 
> > I uploaded a screenshot from Mac showing the text is indented in a row which
> has
> > no image while other row has image.
> 
> Thanks, I missed the loop somehow. :-/ If the first couple devices don't have
> signal strengths, is the text going to jump horizontally when one gets
> connected?

Yes, it will keep all the text aligned. It was suggested by comment at:
https://bugs.chromium.org/p/chromium/issues/detail?id=543466#c41

https://codereview.chromium.org/2304213002/diff/160001/content/browser/blueto...
File content/browser/bluetooth/web_bluetooth_service_impl.h (right):

https://codereview.chromium.org/2304213002/diff/160001/content/browser/blueto...
content/browser/bluetooth/web_bluetooth_service_impl.h:64: bool
IsDevicePaired(const std::string& device_address);
On 2016/09/14 23:52:03, Jeffrey Yasskin wrote:
> Comment that this returns whether the device is paired with the
> RenderFrameHost's LastCommittedOrigin().

Done.

[commit-bot: I haz the power, 2016-09-15 02:38:41]

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

[commit-bot: I haz the power, 2016-09-15 02:38:43]

Dry run: This issue passed the CQ dry run.

[juncai, 2016-09-15 03:57:54]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-09-15 03:57:55]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
rsesek@chromium.org, estade@chromium.org, ortuno@chromium.org,
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/2304213002/#ps180001
(title: "added comments")

[commit-bot: I haz the power, 2016-09-15 03:58:20]

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

[commit-bot: I haz the power, 2016-09-15 04:03:14]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-15 04:06:48]

Description was changed from

==========
Show device connection and paired status in chooser on Mac

This CL added code to show an icon for connected device in the
chooser, and show "Paired" text below the device name if the
device is paired.

I uploaded some screenshots in the issue page.

BUG=543466
==========

to

==========
Show device connection and paired status in chooser on Mac

This CL added code to show an icon for connected device in the
chooser, and show "Paired" text below the device name if the
device is paired.

I uploaded some screenshots in the issue page.

BUG=543466

Committed: https://crrev.com/87d092908d6e01e183bb02721ad7add4e4babeb4
Cr-Commit-Position: refs/heads/master@{#418774}
==========

[commit-bot: I haz the power, 2016-09-15 04:06:50]

Patchset 10 (id:??) landed as
https://crrev.com/87d092908d6e01e183bb02721ad7add4e4babeb4
Cr-Commit-Position: refs/heads/master@{#418774}