bluetooth: When adding a device a second time merge the list of services.

bluetooth: When adding a device a second time merge the list of services.

BUG=581849
Committed: https://crrev.com/f12619e0208342d93a40d6010c4c5f3f9454cd92
Cr-Commit-Position: refs/heads/master@{#373453}

[ortuno, 2016-02-03 17:49:30]

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

[ortuno, 2016-02-03 17:49:31]

jyasskin: PTAL

[Jeffrey Yasskin, 2016-02-03 23:18:18]

lgtm

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

https://codereview.chromium.org/1663883003/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:65: auto new_services
= UnionOfServices(filters, optional_services);
We could optimize this by having UnionOfServices append to the existing set from
origin_to_device_id_to_services_map_. I think it might be more readable too.
Since this only happens on user interaction, and then only once, we should care
more about readability than performance.

Something like:

void BluetoothAllowedDevicesMap::AddUnionOfServicesTo(
    const std::vector<BluetoothScanFilter>& filters,
    const std::vector<BluetoothUUID>& optional_services,
    std::set<std::string>* unionOfServices) {
  for (const auto& filter : filters) {
    for (const BluetoothUUID& uuid : filter.services) {
      unionOfServices->insert(uuid.canonical_value());
    }
  }
  for (const BluetoothUUID& uuid : optional_services) {
    unionOfServices->insert(uuid.canonical_value());
  }
}

AddUnionOfServicesTo(filters, optional_services,
&origin_to_device_id_to_services_map_[origin][device_id]);

[ortuno, 2016-02-04 02:21:09]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-02-04 02:21:09]

Thanks!

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

https://codereview.chromium.org/1663883003/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:65: auto new_services
= UnionOfServices(filters, optional_services);
On 2016/02/03 at 23:18:18, Jeffrey Yasskin wrote:
> We could optimize this by having UnionOfServices append to the existing set
from origin_to_device_id_to_services_map_. I think it might be more readable
too. Since this only happens on user interaction, and then only once, we should
care more about readability than performance.
> 
> Something like:
> 
> void BluetoothAllowedDevicesMap::AddUnionOfServicesTo(
>     const std::vector<BluetoothScanFilter>& filters,
>     const std::vector<BluetoothUUID>& optional_services,
>     std::set<std::string>* unionOfServices) {
>   for (const auto& filter : filters) {
>     for (const BluetoothUUID& uuid : filter.services) {
>       unionOfServices->insert(uuid.canonical_value());
>     }
>   }
>   for (const BluetoothUUID& uuid : optional_services) {
>     unionOfServices->insert(uuid.canonical_value());
>   }
> }
> 
> AddUnionOfServicesTo(filters, optional_services,
&origin_to_device_id_to_services_map_[origin][device_id]);

Done. Also replaced the UnionOfServices function since it was too similar.

[ortuno, 2016-02-04 02:21:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1663883003/#ps40001
(title: "Address jyasskin's comments")

[commit-bot: I haz the power, 2016-02-04 02:23:31]

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

[Jeffrey Yasskin, 2016-02-04 04:03:40]

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

https://codereview.chromium.org/1663883003/diff/1/content/browser/bluetooth/b...
content/browser/bluetooth/bluetooth_allowed_devices_map.cc:65: auto new_services
= UnionOfServices(filters, optional_services);
On 2016/02/04 02:21:09, ortuno wrote:
> On 2016/02/03 at 23:18:18, Jeffrey Yasskin wrote:
> > We could optimize this by having UnionOfServices append to the existing set
> from origin_to_device_id_to_services_map_. I think it might be more readable
> too. Since this only happens on user interaction, and then only once, we
should
> care more about readability than performance.
> > 
> > Something like:
> > 
> > void BluetoothAllowedDevicesMap::AddUnionOfServicesTo(
> >     const std::vector<BluetoothScanFilter>& filters,
> >     const std::vector<BluetoothUUID>& optional_services,
> >     std::set<std::string>* unionOfServices) {
> >   for (const auto& filter : filters) {
> >     for (const BluetoothUUID& uuid : filter.services) {
> >       unionOfServices->insert(uuid.canonical_value());
> >     }
> >   }
> >   for (const BluetoothUUID& uuid : optional_services) {
> >     unionOfServices->insert(uuid.canonical_value());
> >   }
> > }
> > 
> > AddUnionOfServicesTo(filters, optional_services,
> &origin_to_device_id_to_services_map_[origin][device_id]);
> 
> Done. Also replaced the UnionOfServices function since it was too similar.

Yep, that's what I meant. Thanks!

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

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-04 04:11:04]

Description was changed from

==========
bluetooth: When adding a device a second time merge the list of services.

BUG=581849
==========

to

==========
bluetooth: When adding a device a second time merge the list of services.

BUG=581849

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

[commit-bot: I haz the power, 2016-02-04 04:11:04]

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