Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

I uploaded a screenshot on the issue page.

BUG=629689
Committed: https://crrev.com/3ecef4e6dc7a93987930c1a5a1ad146c3bba58e8
Cr-Commit-Position: refs/heads/master@{#413592}

[juncai, 2016-08-12 19:24:06]

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

[commit-bot: I haz the power, 2016-08-12 19:24:35]

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

[juncai, 2016-08-12 19:27:05]

Description was changed from

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

This CL modified code to add signal strength indicator icon to WebBluetooth
chooser. The icons are originally used by ChromeOS only.

BUG=629689
==========

to

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

This CL modified code to add signal strength indicator icon to WebBluetooth
chooser.

BUG=629689
==========

[juncai, 2016-08-12 20:14:23]

Description was changed from

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

This CL modified code to add signal strength indicator icon to WebBluetooth
chooser.

BUG=629689
==========

to

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

This CL modified code to add signal strength indicator icon to WebBluetooth
chooser.

I uploaded a screenshot on the issue page.

BUG=629689
==========

[juncai, 2016-08-12 20:25:32]

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

[juncai, 2016-08-12 20:25:32]

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

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

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

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

[Reilly Grant (use Gerrit), 2016-08-12 20:28:16]

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.h:47: int
GetSignalStrengthLevel(size_t index) const override;
ChooserController could be given a default implementation of these methods to
avoid code churn for chooser types that don't need them.

[commit-bot: I haz the power, 2016-08-12 21:29:38]

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

[commit-bot: I haz the power, 2016-08-12 21:29:39]

Dry run: This issue passed the CQ dry run.

[msw, 2016-08-12 23:08:49]

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:63: virtual bool
HasIconBeforeText() const = 0;
optional nit: ShouldShowIconBeforeText

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:76: // The |index|th
device signal strength level (non-negative), which is used to
nit: "The signal strength level (non-negative) of the device at |index|,
which"...

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:77: // retrieve
corresponding icon to be displayed before the text. Return -1 for
nit: "retrieve the corresponding", "Returns -1 if no icon should be shown."

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:36: int
CalculateSignalLevel(int rssi) {
nit: add a comment, explain the rssi acronym (I had to look it up)

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:39: } else if (rssi
>= kMaxRSSI) {
nit: no else after return here and below.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:45:
(static_cast<double>(rssi - kMinRSSI) * output_range / input_range));
nit: you shouldn't need this static cast, right?

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:206: //
http://crbug.com/629689 Update RSSI.
perhaps consider landing your update CL first, then including the rssi update in
this cl?

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:210: int level =
rssi ? CalculateSignalLevel(static_cast<int>(*rssi)) : -1;
nit: this static cast shouldn't be necessary, right? Maybe CalculateSignalLevel
should take a int8_t?

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:72: int signal_level;
nit: |signal_strength_level|

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:79:
std::vector<BluetoothDeviceInfo> devices_info_;
optional nit: |devices_|?

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:94: if
(!chooser_controller_->HasIconBeforeText())
nit: can we DCHECK this instead? TableView probably shouldn't ask.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:97: int num_options =
static_cast<int>(chooser_controller_->NumOptions());
nit: avoid this static cast.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:117: return
gfx::ImageSkiaOperations::ExtractSubset(
The icons look a bit squished in the screenshot, can you make out the individual
bars locally? Perhaps something is scaling these images incorrectly, or just
making them so small that it looks bad. I see that the icons will be updated
later according the bug, but I wonder if we could get vector icons, use some
other existing iconography for now, or just make sure the images aren't scaled
weird for now.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:118: *images, gfx::Rect(0, level
* height, width, height));
interesting! perhaps add a comment?

https://codereview.chromium.org/2245603003/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:349: rssi ?
&(rssi.value()) : nullptr);
Just pass a plain int8_t, instead of a const pointer? Nevermind if there's no
reasonable value that could mean "signal strength not available".

https://codereview.chromium.org/2245603003/diff/1/ui/resources/ui_resources.grd
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/2245603003/diff/1/ui/resources/ui_resources.g...
ui/resources/ui_resources.grd:135: <structure type="chrome_scaled_image"
name="IDR_NETWORK_BARS_DARK" file="common/statusbar_network_bars_dark.png" />
What does the "statusbar" prefix mean, consider removing it?

[juncai, 2016-08-15 21:43:08]

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

[commit-bot: I haz the power, 2016-08-15 21:43:38]

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

[juncai, 2016-08-15 21:47:24]

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

[commit-bot: I haz the power, 2016-08-15 21:47:35]

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

[juncai, 2016-08-15 21:52:03]

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

[commit-bot: I haz the power, 2016-08-15 21:52: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-08-15 21:53:20]

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:63: virtual bool
HasIconBeforeText() const = 0;
On 2016/08/12 23:08:48, msw wrote:
> optional nit: ShouldShowIconBeforeText

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:76: // The |index|th
device signal strength level (non-negative), which is used to
On 2016/08/12 23:08:48, msw wrote:
> nit: "The signal strength level (non-negative) of the device at |index|,
> which"...

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/chooser_cont...
chrome/browser/chooser_controller/chooser_controller.h:77: // retrieve
corresponding icon to be displayed before the text. Return -1 for
On 2016/08/12 23:08:48, msw wrote:
> nit: "retrieve the corresponding", "Returns -1 if no icon should be shown."

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:36: int
CalculateSignalLevel(int rssi) {
On 2016/08/12 23:08:48, msw wrote:
> nit: add a comment, explain the rssi acronym (I had to look it up)

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:39: } else if (rssi
>= kMaxRSSI) {
On 2016/08/12 23:08:48, msw wrote:
> nit: no else after return here and below.

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:45:
(static_cast<double>(rssi - kMinRSSI) * output_range / input_range));
On 2016/08/12 23:08:48, msw wrote:
> nit: you shouldn't need this static cast, right?

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:206: //
http://crbug.com/629689 Update RSSI.
On 2016/08/12 23:08:48, msw wrote:
> perhaps consider landing your update CL first, then including the rssi update
in
> this cl?

Yes, I'll land the update device name CL first:
https://codereview.chromium.org/2242943002/
Then modify this CL to include the RSSI update.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:210: int level =
rssi ? CalculateSignalLevel(static_cast<int>(*rssi)) : -1;
On 2016/08/12 23:08:48, msw wrote:
> nit: this static cast shouldn't be necessary, right? Maybe
CalculateSignalLevel
> should take a int8_t?

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:72: int signal_level;
On 2016/08/12 23:08:48, msw wrote:
> nit: |signal_strength_level|

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:79:
std::vector<BluetoothDeviceInfo> devices_info_;
On 2016/08/12 23:08:48, msw wrote:
> optional nit: |devices_|?

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:94: if
(!chooser_controller_->HasIconBeforeText())
On 2016/08/12 23:08:49, msw wrote:
> nit: can we DCHECK this instead? TableView probably shouldn't ask.

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:97: int num_options =
static_cast<int>(chooser_controller_->NumOptions());
On 2016/08/12 23:08:49, msw wrote:
> nit: avoid this static cast.

Use base::checked_cast instead of static_cast.

Done.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:117: return
gfx::ImageSkiaOperations::ExtractSubset(
On 2016/08/12 23:08:49, msw wrote:
> The icons look a bit squished in the screenshot, can you make out the
individual
> bars locally? Perhaps something is scaling these images incorrectly, or just
> making them so small that it looks bad. I see that the icons will be updated
> later according the bug, but I wonder if we could get vector icons, use some
> other existing iconography for now, or just make sure the images aren't scaled
> weird for now.

I update and use icons from:
https://icons.googleplex.com/#signal
And I uploaded a screenshot to the issue page.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/views/cho...
chrome/browser/ui/views/chooser_content_view.cc:118: *images, gfx::Rect(0, level
* height, width, height));
On 2016/08/12 23:08:49, msw wrote:
> interesting! perhaps add a comment?

I borrowed the idea from:
https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?cl=G...
But now since each strength level has its own icon, this is not used any more.

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/usb/usb_choo...
File chrome/browser/usb/usb_chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/1/chrome/browser/usb/usb_choo...
chrome/browser/usb/usb_chooser_controller.h:47: int
GetSignalStrengthLevel(size_t index) const override;
On 2016/08/12 20:28:16, Reilly Grant wrote:
> ChooserController could be given a default implementation of these methods to
> avoid code churn for chooser types that don't need them.

Done.

https://codereview.chromium.org/2245603003/diff/1/content/browser/bluetooth/b...
File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:349: rssi ?
&(rssi.value()) : nullptr);
On 2016/08/12 23:08:49, msw wrote:
> Just pass a plain int8_t, instead of a const pointer? Nevermind if there's no
> reasonable value that could mean "signal strength not available".

Some context:
We discussed this at:
https://codereview.chromium.org/2210873003/
And it seems that the range of int8_t are all possible values for RSSI. So we
chose using a pointer here.

https://codereview.chromium.org/2245603003/diff/1/ui/resources/ui_resources.grd
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/2245603003/diff/1/ui/resources/ui_resources.g...
ui/resources/ui_resources.grd:135: <structure type="chrome_scaled_image"
name="IDR_NETWORK_BARS_DARK" file="common/statusbar_network_bars_dark.png" />
On 2016/08/12 23:08:49, msw wrote:
> What does the "statusbar" prefix mean, consider removing it?

Done.

[commit-bot: I haz the power, 2016-08-15 23:13:36]

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

[commit-bot: I haz the power, 2016-08-15 23:13:36]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-16 15:39:17]

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

[ortuno, 2016-08-16 15:39:19]

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

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:211: int level =
rssi ? CalculateSignalLevel(*rssi) : -1;
Is it possible to update the icon?

[msw, 2016-08-16 16:51:19]

lgtm with nits.

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:99: int num_options =
base::checked_cast<int>(chooser_controller_->NumOptions());
nit: Avoid casting to compare with 0. Instead, you could inline |if
(chooser_controller_->NumOptions() == 0) {| below or make |num_options| a
size_t, then cast (if needed) in the DCHECK, which will be debug-only.

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:115: strength_bar_ids[level]);
nit: CHECK that the level is in the range of the array.

[juncai, 2016-08-16 17:47:22]

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

[commit-bot: I haz the power, 2016-08-16 17:47:48]

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

[juncai, 2016-08-16 17:49:52]

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

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:211: int level =
rssi ? CalculateSignalLevel(*rssi) : -1;
On 2016/08/16 15:39:18, ortuno wrote:
> Is it possible to update the icon?

Yes, I commented at:
https://codereview.chromium.org/2245603003/diff/1/chrome/browser/ui/bluetooth...

So I will land the following CL first:
https://codereview.chromium.org/2242943002/
Then modify this CL to update the RSSI icon.

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:99: int num_options =
base::checked_cast<int>(chooser_controller_->NumOptions());
On 2016/08/16 16:51:18, msw wrote:
> nit: Avoid casting to compare with 0. Instead, you could inline |if
> (chooser_controller_->NumOptions() == 0) {| below or make |num_options| a
> size_t, then cast (if needed) in the DCHECK, which will be debug-only.

Done.

https://codereview.chromium.org/2245603003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:115: strength_bar_ids[level]);
On 2016/08/16 16:51:18, msw wrote:
> nit: CHECK that the level is in the range of the array.

Done.

[commit-bot: I haz the power, 2016-08-16 18:09:09]

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

[commit-bot: I haz the power, 2016-08-16 18:09:11]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[oshima, 2016-08-16 18:35:49]

Do these icon have to be png? Looks like it's simple enough to just draw it in
CanvasImageSource.

ex:
https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=...

[juncai, 2016-08-16 18:47:20]

On 2016/08/16 18:35:49, oshima wrote:
> Do these icon have to be png? Looks like it's simple enough to just draw it in
> CanvasImageSource.
> 
> ex:
>
https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=...

I guess drawing these icons in CanvasImageSource only works on non-Mac desktops?
There will be a similar CL that adds signal strength indicator to chooser on
Mac. So in order to reuse those icons on Mac, using png seems to be a better
option?

[msw, 2016-08-16 18:55:13]

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

https://codereview.chromium.org/2245603003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:116:
base::checked_cast<int>(sizeof(strength_bar_ids) / sizeof(int)));
nit: use base::checked_cast<int>(arraysize(strength_bar_ids)) see base/macros.h

[juncai, 2016-08-16 19:02:41]

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

[commit-bot: I haz the power, 2016-08-16 19:02:55]

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

[juncai, 2016-08-16 19:06:26]

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

https://codereview.chromium.org/2245603003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/chooser_content_view.cc:116:
base::checked_cast<int>(sizeof(strength_bar_ids) / sizeof(int)));
On 2016/08/16 18:55:13, msw wrote:
> nit: use base::checked_cast<int>(arraysize(strength_bar_ids)) see
base/macros.h

Done.

[oshima, 2016-08-16 21:56:06]

On 2016/08/16 18:47:20, juncai wrote:
> On 2016/08/16 18:35:49, oshima wrote:
> > Do these icon have to be png? Looks like it's simple enough to just draw it
in
> > CanvasImageSource.
> > 
> > ex:
> >
>
https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=...
> 
> I guess drawing these icons in CanvasImageSource only works on non-Mac
desktops?
> There will be a similar CL that adds signal strength indicator to chooser on
> Mac. So in order to reuse those icons on Mac, using png seems to be a better
> option?

It depends on the code. Do you need to use NSImage? If yes, then it should be
png.

[juncai, 2016-08-16 22:04:10]

On 2016/08/16 21:56:06, oshima wrote:
> On 2016/08/16 18:47:20, juncai wrote:
> > On 2016/08/16 18:35:49, oshima wrote:
> > > Do these icon have to be png? Looks like it's simple enough to just draw
it
> in
> > > CanvasImageSource.
> > > 
> > > ex:
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/chromeos/network/network_icon.cc?rcl=...
> > 
> > I guess drawing these icons in CanvasImageSource only works on non-Mac
> desktops?
> > There will be a similar CL that adds signal strength indicator to chooser on
> > Mac. So in order to reuse those icons on Mac, using png seems to be a better
> > option?
> 
> It depends on the code. Do you need to use NSImage? If yes, then it should be
> png.

Yes, I need to use NSImage.

[commit-bot: I haz the power, 2016-08-16 22:28:58]

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

[commit-bot: I haz the power, 2016-08-16 22:28:58]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-08-16 23:07:03]

lgtm

[Jeffrey Yasskin, 2016-08-19 17:46:55]

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/chooser...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/chooser...
chrome/browser/chooser_controller/chooser_controller.h:66: virtual bool
ShouldShowIconBeforeText() const;
This should say that it's a signal strength icon. At least in the comment, and
probably in the function name.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/chooser...
chrome/browser/chooser_controller/chooser_controller.h:79: // The signal
strength level (non-negative) of the device at |index|, which
Can you say what the valid levels are? This is 0-4 inclusive, right?

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:40: int
CalculateSignalLevel(int8_t rssi) {
Write a test that this returns the right levels.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:204: int level =
rssi ? CalculateSignalLevel(*rssi) : -1;
Name this either signal_level or signal_strength_level.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:222: for (index = 0;
index < devices_.size(); ++index) {
You could use
auto device_it = std::find_if(devices_.begin(), devices_.end(),
[&device_id](const BluetoothDeviceInfo& device) {
    return device.id == device_id;
  });

DCHECK(device_it != devices_.end());
device_it->signal_strength_level = signal_strength_level;
view()->OnOptionUpdated(device_id - devices_.begin());

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:28: const int
strength_bar_ids[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR,
Name this kStrengthBarIds:
https://google.github.io/styleguide/cppguide.html#Constant_Names

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:45: true /* single_selection
*/);
Thanks for the comment about what the boolean means.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:109:
chooser_controller_->GetSignalStrengthLevel(static_cast<size_t>(row));
You can let this be an implicit cast. The DCHECK_GE(row, 0) makes sure it's in
bounds for size_t.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:115: DCHECK_LT(level,
base::checked_cast<int>(arraysize(strength_bar_ids)));
You can omit the checked_cast here: you know this arraysize fits into an int.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:117: return
*ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
Can GetImageSkiaNamed ever return null?

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

https://codereview.chromium.org/2245603003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:349: rssi ?
&(rssi.value()) : nullptr);
You don't need the parentheses around (rssi.value()), although I don't hate them
either.

[ortuno, 2016-08-19 17:58:36]

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:40: int
CalculateSignalLevel(int8_t rssi) {
On 2016/08/19 at 17:46:54, Jeffrey Yasskin wrote:
> Write a test that this returns the right levels.

Could you actually move this to BluetoothDeviceChooserController in this patch
and have AddOrUpdateDevice take the level directly? Otherwise we would have to
repeat the same algorithm in the android implementation.

[Jeffrey Yasskin, 2016-08-19 18:34:44]

Also, in the change description, you have "Add signal strength indicator icon to
WebBluetooth chooser" twice. Try to only say things once. :)

[juncai, 2016-08-19 21:52:48]

Description was changed from

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

This CL modified code to add signal strength indicator icon to WebBluetooth
chooser.

I uploaded a screenshot on the issue page.

BUG=629689
==========

to

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

I uploaded a screenshot on the issue page.

BUG=629689
==========

[juncai, 2016-08-19 22:56:30]

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

[commit-bot: I haz the power, 2016-08-19 22:57:10]

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

[juncai, 2016-08-19 22:59:02]

On 2016/08/19 18:34:44, Jeffrey Yasskin wrote:
> Also, in the change description, you have "Add signal strength indicator icon
to
> WebBluetooth chooser" twice. Try to only say things once. :)

Done.

[juncai, 2016-08-19 22:59:17]

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/chooser...
File chrome/browser/chooser_controller/chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/chooser...
chrome/browser/chooser_controller/chooser_controller.h:66: virtual bool
ShouldShowIconBeforeText() const;
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> This should say that it's a signal strength icon. At least in the comment, and
> probably in the function name.

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/chooser...
chrome/browser/chooser_controller/chooser_controller.h:79: // The signal
strength level (non-negative) of the device at |index|, which
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> Can you say what the valid levels are? This is 0-4 inclusive, right?

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:40: int
CalculateSignalLevel(int8_t rssi) {
On 2016/08/19 17:58:36, ortuno wrote:
> On 2016/08/19 at 17:46:54, Jeffrey Yasskin wrote:
> > Write a test that this returns the right levels.
> 
> Could you actually move this to BluetoothDeviceChooserController in this patch
> and have AddOrUpdateDevice take the level directly? Otherwise we would have to
> repeat the same algorithm in the android implementation.

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:40: int
CalculateSignalLevel(int8_t rssi) {
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> Write a test that this returns the right levels.

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:204: int level =
rssi ? CalculateSignalLevel(*rssi) : -1;
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> Name this either signal_level or signal_strength_level.

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:222: for (index = 0;
index < devices_.size(); ++index) {
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> You could use
> auto device_it = std::find_if(devices_.begin(), devices_.end(),
> [&device_id](const BluetoothDeviceInfo& device) {
>     return device.id == device_id;
>   });
> 
> DCHECK(device_it != devices_.end());
> device_it->signal_strength_level = signal_strength_level;
> view()->OnOptionUpdated(device_id - devices_.begin());

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/chooser_content_view.cc (right):

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:28: const int
strength_bar_ids[5] = {IDR_SIGNAL_0_BAR, IDR_SIGNAL_1_BAR,
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> Name this kStrengthBarIds:
> https://google.github.io/styleguide/cppguide.html#Constant_Names

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:45: true /* single_selection
*/);
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> Thanks for the comment about what the boolean means.

Acknowledged.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:109:
chooser_controller_->GetSignalStrengthLevel(static_cast<size_t>(row));
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> You can let this be an implicit cast. The DCHECK_GE(row, 0) makes sure it's in
> bounds for size_t.

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:115: DCHECK_LT(level,
base::checked_cast<int>(arraysize(strength_bar_ids)));
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> You can omit the checked_cast here: you know this arraysize fits into an int.

Changed to static_cast<int> here, since if not using it, got compile error:
error: comparison of integers of different signs: 'const int' and 'const
unsigned long'

Done.

https://codereview.chromium.org/2245603003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:117: return
*ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> Can GetImageSkiaNamed ever return null?

From:
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?type...
https://cs.chromium.org/chromium/src/ui/gfx/image/image.cc?type=cs&sq=package...
It seems it doesn't return nullptr.

But added a DCHECK here to make sure.

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

https://codereview.chromium.org/2245603003/diff/100001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:349: rssi ?
&(rssi.value()) : nullptr);
On 2016/08/19 17:46:54, Jeffrey Yasskin wrote:
> You don't need the parentheses around (rssi.value()), although I don't hate
them
> either.

Done.

[ortuno, 2016-08-19 23:07:24]

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

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:360: rssi ?
&rssi.value() : nullptr);
Would it make sense to pass the level here directly? You could pass -1 for no
rssi:

rssi ? CalculateSignalStrengthLevel(rssi.value()) : -1);

[juncai, 2016-08-19 23:16:23]

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

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:360: rssi ?
&rssi.value() : nullptr);
On 2016/08/19 23:07:24, ortuno wrote:
> Would it make sense to pass the level here directly? You could pass -1 for no
> rssi:
> 
> rssi ? CalculateSignalStrengthLevel(rssi.value()) : -1);

That needs to change the BluetoothChooser's interface. Probably we can do it in
a separate CL?

[juncai, 2016-08-19 23:58:43]

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

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:360: rssi ?
&rssi.value() : nullptr);
On 2016/08/19 23:16:23, juncai wrote:
> On 2016/08/19 23:07:24, ortuno wrote:
> > Would it make sense to pass the level here directly? You could pass -1 for
no
> > rssi:
> > 
> > rssi ? CalculateSignalStrengthLevel(rssi.value()) : -1);
> 
> That needs to change the BluetoothChooser's interface. Probably we can do it
in
> a separate CL?

Actually now I think that doing it in this patch seems more convenient, :). I'll
do it in the next patch.

[Jeffrey Yasskin, 2016-08-20 00:03:29]

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:237: size_t index =
device_it - devices_.begin();
I'd just inline this in the OnOptionRemoved call.

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:58: int
signal_strength_level);
Say what the bounds on the signal strength are (-1 to 4 inclusive, right?).

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

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:119: DCHECK(image);
The dereference into a return by value is an implicit CHECK(), so you don't need
the DCHECK. I just wanted to make sure we didn't need to handle null by doing
something other than crashing.

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

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:360: rssi ?
&rssi.value() : nullptr);
On 2016/08/19 23:58:42, juncai wrote:
> On 2016/08/19 23:16:23, juncai wrote:
> > On 2016/08/19 23:07:24, ortuno wrote:
> > > Would it make sense to pass the level here directly? You could pass -1 for
> no
> > > rssi:
> > > 
> > > rssi ? CalculateSignalStrengthLevel(rssi.value()) : -1);
> > 
> > That needs to change the BluetoothChooser's interface. Probably we can do it
> in
> > a separate CL?
> 
> Actually now I think that doing it in this patch seems more convenient, :).
I'll
> do it in the next patch.

Thanks. That should remove some awkward dependencies.

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc
(right):

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc:12:
public:
You should omit all of the contents of this class.

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc:23:
for (int rssi = low; rssi <= high; ++rssi) {
Instead of the exhaustive test, check some concrete values, like:

EXPECT_EQ(0,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-128));
EXPECT_EQ(0,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-100));
EXPECT_EQ(0,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-89));
EXPECT_EQ(1,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-88));
EXPECT_EQ(3,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-56));
EXPECT_EQ(4,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-55));
EXPECT_EQ(4,
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(127));

[commit-bot: I haz the power, 2016-08-20 00:35:37]

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

[commit-bot: I haz the power, 2016-08-20 00:35:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[juncai, 2016-08-20 01:13:15]

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

[commit-bot: I haz the power, 2016-08-20 01:14:05]

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

[juncai, 2016-08-20 01:17:01]

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

[juncai, 2016-08-20 01:17:02]

avi@chromium.org: Please review changes in 
//content/public/browser/bluetooth_chooser.h

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc (right):

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:237: size_t index =
device_it - devices_.begin();
On 2016/08/20 00:03:29, Jeffrey Yasskin wrote:
> I'd just inline this in the OnOptionRemoved call.

Done.

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h (right):

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/blue...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:58: int
signal_strength_level);
On 2016/08/20 00:03:29, Jeffrey Yasskin wrote:
> Say what the bounds on the signal strength are (-1 to 4 inclusive, right?).

Done.

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

https://codereview.chromium.org/2245603003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/chooser_content_view.cc:119: DCHECK(image);
On 2016/08/20 00:03:29, Jeffrey Yasskin wrote:
> The dereference into a return by value is an implicit CHECK(), so you don't
need
> the DCHECK. I just wanted to make sure we didn't need to handle null by doing
> something other than crashing.

Done.

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

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller.cc:360: rssi ?
&rssi.value() : nullptr);
On 2016/08/20 00:03:29, Jeffrey Yasskin wrote:
> On 2016/08/19 23:58:42, juncai wrote:
> > On 2016/08/19 23:16:23, juncai wrote:
> > > On 2016/08/19 23:07:24, ortuno wrote:
> > > > Would it make sense to pass the level here directly? You could pass -1
for
> > no
> > > > rssi:
> > > > 
> > > > rssi ? CalculateSignalStrengthLevel(rssi.value()) : -1);
> > > 
> > > That needs to change the BluetoothChooser's interface. Probably we can do
it
> > in
> > > a separate CL?
> > 
> > Actually now I think that doing it in this patch seems more convenient, :).
> I'll
> > do it in the next patch.
> 
> Thanks. That should remove some awkward dependencies.

Done.

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
File content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc
(right):

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc:12:
public:
On 2016/08/20 00:03:29, Jeffrey Yasskin wrote:
> You should omit all of the contents of this class.

Done.

https://codereview.chromium.org/2245603003/diff/120001/content/browser/blueto...
content/browser/bluetooth/bluetooth_device_chooser_controller_unittest.cc:23:
for (int rssi = low; rssi <= high; ++rssi) {
On 2016/08/20 00:03:29, Jeffrey Yasskin wrote:
> Instead of the exhaustive test, check some concrete values, like:
> 
> EXPECT_EQ(0,
>
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-128));
> EXPECT_EQ(0,
>
content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-100));
> EXPECT_EQ(0,
> content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-89));
> EXPECT_EQ(1,
> content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-88));
> EXPECT_EQ(3,
> content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-56));
> EXPECT_EQ(4,
> content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(-55));
> EXPECT_EQ(4,
> content::BluetoothDeviceChooserController::CalculateSignalStrengthLevel(127));

Done.

[commit-bot: I haz the power, 2016-08-20 04:19:56]

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

[commit-bot: I haz the power, 2016-08-20 04:19:58]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2016-08-21 04:34:19]

content/public/browser/bluetooth_chooser.h lgtm

[Jeffrey Yasskin, 2016-08-22 17:17:52]

lgtm

[juncai, 2016-08-22 17:45:56]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-08-22 17:45:56]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2245603003/#ps140001
(title: "address more comments")

[commit-bot: I haz the power, 2016-08-22 17:46:46]

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-08-22 22:09:32]

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

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

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_...)

[juncai, 2016-08-22 22:12:24]

The CQ bit was checked by juncai@chromium.org

[commit-bot: I haz the power, 2016-08-22 22:12:56]

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-08-23 00:11:11]

Description was changed from

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

I uploaded a screenshot on the issue page.

BUG=629689
==========

to

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

I uploaded a screenshot on the issue page.

BUG=629689
==========

[commit-bot: I haz the power, 2016-08-23 00:11:13]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-23 00:13:58]

Description was changed from

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

I uploaded a screenshot on the issue page.

BUG=629689
==========

to

==========
Add signal strength indicator icon to WebBluetooth chooser on non-Mac desktops

I uploaded a screenshot on the issue page.

BUG=629689

Committed: https://crrev.com/3ecef4e6dc7a93987930c1a5a1ad146c3bba58e8
Cr-Commit-Position: refs/heads/master@{#413592}
==========

[commit-bot: I haz the power, 2016-08-23 00:14:00]

Patchset 8 (id:??) landed as
https://crrev.com/3ecef4e6dc7a93987930c1a5a1ad146c3bba58e8
Cr-Commit-Position: refs/heads/master@{#413592}