bluetooth: Allow updates on chooser items.

bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

Second of three patches to improve the devices that appear in the chooser:

 [1] Replace old advertised uuids (http://crrev.com/2205693003)
 [2] This patch
 [3] Only add new devices, connected devices and devices that changed
     (http://crrev.com/2217573002)

BUG=508904
Committed: https://crrev.com/4178c185640b37e621703ac415141ac7caae06f4
Cr-Commit-Position: refs/heads/master@{#410954}

[ortuno, 2016-08-04 17:21:23]

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

[commit-bot: I haz the power, 2016-08-04 17:21:45]

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-08-04 17:39:44]

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

[commit-bot: I haz the power, 2016-08-04 17:39:45]

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

[ortuno, 2016-08-04 17:49:57]

Description was changed from

==========
bluetooth: Change AddDevice to AddOrUpdateDevice.

This will allow to post updates when a device changes names or
in the future rssi.

BUG=508904
==========

to

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

BUG=508904
==========

[ortuno, 2016-08-04 17:55:14]

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

[commit-bot: I haz the power, 2016-08-04 17:55:47]

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-08-04 19:47:59]

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

[commit-bot: I haz the power, 2016-08-04 19:48:00]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-04 19:55:50]

Description was changed from

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

BUG=508904
==========

to

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

Second of three patches to improve the devices that appear in the chooser:

 [1] Replace old advertised uuids (http://crrev.com/2210873003)
 [2] This patch
 [3] Only add new devices, connected devices and devices that changed
     (http://crrev.com/2217573002)

BUG=508904
==========

[ortuno, 2016-08-04 19:57:43]

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

[ortuno, 2016-08-04 19:57:44]

juncai: PTAL

[juncai, 2016-08-04 21:52:25]

Instead of having AddOrUpdateDevice(), does it make sense to have a separate
function:
UpdateDevice()?

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

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:58: const auto&
device_name_it = device_id_to_name_map_.find(device_id);
nit: DCHECK(device_name_it != device_id_to_name_map_.end());

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:170: if (name_it ==
device_id_to_name_map_.end()) {
nit: no {} for single line if statement.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:174: int index = 0;
use size_t instead of int.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:177: index++;
nit: ++index;

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:188: if (it->second
== 0) {
nit: use if (--it->second == 0) ?

no {} for single line statement.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h (right):

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:50: // Shows a new
device in the chooser.
nit: update the comments.

https://codereview.chromium.org/2210873003/diff/40001/content/public/browser/...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/40001/content/public/browser/...
content/public/browser/bluetooth_chooser.h:73: virtual void
AddOrUpdateDevice(const std::string& device_id,
Does it make sense to have a separate function:
UpdateDevice()
instead of doing these two things in one function? This probably will also make
testing easier since we can test them separately.

[ortuno, 2016-08-05 02:16:03]

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

[ortuno, 2016-08-05 02:16:09]

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

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:58: const auto&
device_name_it = device_id_to_name_map_.find(device_id);
On 2016/08/04 at 21:52:23, juncai wrote:
> nit: DCHECK(device_name_it != device_id_to_name_map_.end());

Done.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:170: if (name_it ==
device_id_to_name_map_.end()) {
On 2016/08/04 at 21:52:23, juncai wrote:
> nit: no {} for single line if statement.

Done.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:174: int index = 0;
On 2016/08/04 at 21:52:23, juncai wrote:
> use size_t instead of int.

Done.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:177: index++;
On 2016/08/04 at 21:52:23, juncai wrote:
> nit: ++index;

Done.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.cc:188: if (it->second
== 0) {
On 2016/08/04 at 21:52:25, juncai wrote:
> nit: use if (--it->second == 0) ?
> 
> no {} for single line statement.

Done.

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
File chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h (right):

https://codereview.chromium.org/2210873003/diff/40001/chrome/browser/ui/bluet...
chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h:50: // Shows a new
device in the chooser.
On 2016/08/04 at 21:52:25, juncai wrote:
> nit: update the comments.

Done.

https://codereview.chromium.org/2210873003/diff/40001/content/public/browser/...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/40001/content/public/browser/...
content/public/browser/bluetooth_chooser.h:73: virtual void
AddOrUpdateDevice(const std::string& device_id,
On 2016/08/04 at 21:52:25, juncai wrote:
> Does it make sense to have a separate function:
> UpdateDevice()
> instead of doing these two things in one function? This probably will also
make testing easier since we can test them separately.

If we were to separate these two functions, chooser clients would have to keep
track of which devices they have already added which means yet another set of
device_ids. We could expose a HasDevice() function so that clients don't have to
keep track, but that would mean two extra functions HasDevice() and
UpdateDevice() that we have to implement all the way down to java.

[commit-bot: I haz the power, 2016-08-05 02:16:24]

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-08-05 03:20:57]

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

[commit-bot: I haz the power, 2016-08-05 03:20:57]

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-05 20:44:30]

//chrome/browser/ui/bluetooth/ LGTM.

https://codereview.chromium.org/2210873003/diff/40001/content/public/browser/...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/40001/content/public/browser/...
content/public/browser/bluetooth_chooser.h:73: virtual void
AddOrUpdateDevice(const std::string& device_id,
On 2016/08/05 02:16:09, ortuno wrote:
> On 2016/08/04 at 21:52:25, juncai wrote:
> > Does it make sense to have a separate function:
> > UpdateDevice()
> > instead of doing these two things in one function? This probably will also
> make testing easier since we can test them separately.
> 
> If we were to separate these two functions, chooser clients would have to keep
> track of which devices they have already added which means yet another set of
> device_ids. We could expose a HasDevice() function so that clients don't have
to
> keep track, but that would mean two extra functions HasDevice() and
> UpdateDevice() that we have to implement all the way down to java.

Acknowledged.

[ortuno, 2016-08-05 20:49:15]

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

[ortuno, 2016-08-05 20:49:15]

tedchoc: PTAL at chrome/android

[ortuno, 2016-08-08 15:56:20]

ortuno@chromium.org changed reviewers:
+ dfalcantara@chromium.org
- tedchoc@chromium.org

[ortuno, 2016-08-08 15:56:20]

dfalcantara: tedchoc is OOO. PTAL at chrome/android

[gone, 2016-08-08 17:09:42]

chrome/android lgtm

[ortuno, 2016-08-09 14:13:19]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-08-09 14:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, juncai@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2210873003/#ps70001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-09 14:13: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-09 14:20:06]

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

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

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

[ortuno, 2016-08-09 17:58:12]

juncai: PTAL, I changed AddOrUpdate to take the discussed arguments.

[ortuno, 2016-08-09 17:58:33]

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

[commit-bot: I haz the power, 2016-08-09 17:59:08]

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-08-09 18:01:29]

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

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

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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[juncai, 2016-08-09 19:14:45]

https://codereview.chromium.org/2210873003/diff/110001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/110001/content/public/browser...
content/public/browser/bluetooth_chooser.h:78: int8_t* rssi) {}
Some questions:
1. Does AddOrUpdateDevice() need to change rssi so here it uses int8_t pointer?

2.
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p...

it uses int16_t for the rssi, here uses int8_t, any reason?

[ortuno, 2016-08-09 19:23:10]

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

[ortuno, 2016-08-09 19:23:12]

https://codereview.chromium.org/2210873003/diff/110001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/110001/content/public/browser...
content/public/browser/bluetooth_chooser.h:78: int8_t* rssi) {}
On 2016/08/09 at 19:14:44, juncai wrote:
> Some questions:
> 1. Does AddOrUpdateDevice() need to change rssi so here it uses int8_t
pointer?
> 

Added comment to clarify why we pass in a pointer for the rssi. wdyt?

> 2.
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p...
> 
> it uses int16_t for the rssi, here uses int8_t, any reason?

This is because of http://crrev.com/2228953003

[ortuno, 2016-08-09 19:23:17]

https://codereview.chromium.org/2210873003/diff/110001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/110001/content/public/browser...
content/public/browser/bluetooth_chooser.h:78: int8_t* rssi) {}
On 2016/08/09 at 19:14:44, juncai wrote:
> Some questions:
> 1. Does AddOrUpdateDevice() need to change rssi so here it uses int8_t
pointer?
> 

Added comment to clarify why we pass in a pointer for the rssi. wdyt?

> 2.
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?sq=p...
> 
> it uses int16_t for the rssi, here uses int8_t, any reason?

This is because of http://crrev.com/2228953003

[commit-bot: I haz the power, 2016-08-09 19:24:21]

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-09 19:30:26]

LGTM wit a nit.

https://codereview.chromium.org/2210873003/diff/130001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/130001/content/public/browser...
content/public/browser/bluetooth_chooser.h:79: int8_t* rssi) {}
nit: maybe add const?
const int8_t*

[commit-bot: I haz the power, 2016-08-09 19:41:25]

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

[commit-bot: I haz the power, 2016-08-09 19:41:26]

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

[ortuno, 2016-08-09 19:52:48]

Thanks!

https://codereview.chromium.org/2210873003/diff/130001/content/public/browser...
File content/public/browser/bluetooth_chooser.h (right):

https://codereview.chromium.org/2210873003/diff/130001/content/public/browser...
content/public/browser/bluetooth_chooser.h:79: int8_t* rssi) {}
On 2016/08/09 at 19:30:26, juncai wrote:
> nit: maybe add const?
> const int8_t*

Done.

[ortuno, 2016-08-09 19:52:55]

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

[ortuno, 2016-08-09 19:53:30]

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

[ortuno, 2016-08-09 19:53:31]

jam: PTAL at content/public/browser/bluetooth_chooser.h

[commit-bot: I haz the power, 2016-08-09 19:53:47]

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

[scheib, 2016-08-09 20:25:03]

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

[scheib, 2016-08-09 20:25:04]

I think that patch description URLs are incorrect for [1] in this and following
patch.

[ortuno, 2016-08-09 20:33:28]

Description was changed from

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

Second of three patches to improve the devices that appear in the chooser:

 [1] Replace old advertised uuids (http://crrev.com/2210873003)
 [2] This patch
 [3] Only add new devices, connected devices and devices that changed
     (http://crrev.com/2217573002)

BUG=508904
==========

to

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

Second of three patches to improve the devices that appear in the chooser:

 [1] Replace old advertised uuids (http://crrev.com/2205693003)
 [2] This patch
 [3] Only add new devices, connected devices and devices that changed
     (http://crrev.com/2217573002)

BUG=508904
==========

[ortuno, 2016-08-09 20:33:29]

ortuno@chromium.org changed reviewers:
- scheib@chromium.org

[ortuno, 2016-08-09 20:34:03]

On 2016/08/09 at 20:25:04, scheib wrote:
> I think that patch description URLs are incorrect for [1] in this and
following patch.

Fixed.

[jam, 2016-08-09 22:29:26]

On 2016/08/09 19:53:31, ortuno wrote:
> jam: PTAL at content/public/browser/bluetooth_chooser.h

lgtm

[commit-bot: I haz the power, 2016-08-10 00:12:00]

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

[commit-bot: I haz the power, 2016-08-10 00:12:01]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-08-10 04:30:57]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-08-10 04:30:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, juncai@chromium.org
Link to the patchset: https://codereview.chromium.org/2210873003/#ps150001
(title: "Make const")

[commit-bot: I haz the power, 2016-08-10 04:31:40]

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-10 04:36:01]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-08-10 04:37:52]

Description was changed from

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

Second of three patches to improve the devices that appear in the chooser:

 [1] Replace old advertised uuids (http://crrev.com/2205693003)
 [2] This patch
 [3] Only add new devices, connected devices and devices that changed
     (http://crrev.com/2217573002)

BUG=508904
==========

to

==========
bluetooth: Allow updates on chooser items.

Changes AddDevice to AddOrUpdateDevice. This will allow to post updates when a
device changes names or in the future rssi.

Second of three patches to improve the devices that appear in the chooser:

 [1] Replace old advertised uuids (http://crrev.com/2205693003)
 [2] This patch
 [3] Only add new devices, connected devices and devices that changed
     (http://crrev.com/2217573002)

BUG=508904

Committed: https://crrev.com/4178c185640b37e621703ac415141ac7caae06f4
Cr-Commit-Position: refs/heads/master@{#410954}
==========

[commit-bot: I haz the power, 2016-08-10 04:37:54]

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