Bootstrapping device lister plus tests

Bootstrapping device lister plus tests

This class lists bootstrapping WiFi devices and provides the necessary information to display/connect to them.

Depends on:
https://codereview.chromium.org/294173002/ [build variable for WiFi bootstrapping]
https://codereview.chromium.org/226883002/ [WiFi library for cloud devices]

BUG=370071
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274896

[Noam Samuel, 2014-05-16 21:14:43]

On 2014/05/15 19:23:14, Noam Samuel wrote:

Looks like gene is changing the SSID format. Please wait to review until I've
updated to the new SSID format (I'll send another email).

[Noam Samuel, 2014-05-21 00:02:50]

OK, updated to new SSID format.

[Vitaly Buka (NO REVIEWS), 2014-05-22 05:51:36]

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc (right):

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:22: const
char* short_name;
const char* const

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:25: {"cam",
"camera"},
I thing it should be 2 spaces

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:30: const
char* kPrivetDeviceLongName = "device";
const char kPrivetDeviceLongName[]

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:35: while
(kPrivetShortNames[i].short_name) {
for loop is better

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:64: bool
ends_with_privet =
EndsWith()

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:73: if
(at_pos == std::string::npos)
if (ssid[ssid.length() - kPrivetCharactersAfterSeparator - 1] != '@' &&
    /*maybe*/ count of '@' != 1)
return false

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:77: return
false;
or maybe just use base::SplitString

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:83: const
std::string internal_id,
string&

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:89: size_t
at_pos = ssid.rfind('@');
base::SplitString ?

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:99: }
: connection_status(SOMETHING)

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:118:
network_watcher_ = wifi_manager_->CreateNetworkListWatcher(base::Bind(
break before base::Bind

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:132: i !=
ssids.end();
usually size_t i = 0 ... for are shorter

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:144:
weak_this && i != active_devices.end();
same

https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:3479:
'browser/local_discovery/wifi/bootstrapping_device_lister.h',
order

[Noam Samuel, 2014-05-22 18:43:58]

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc (right):

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:22: const
char* short_name;
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> const char* const

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:25: {"cam",
"camera"},
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> I thing it should be 2 spaces

I'd be reluctant to make any changes clang-format will just reverse again.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:30: const
char* kPrivetDeviceLongName = "device";
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> const char kPrivetDeviceLongName[]

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:35: while
(kPrivetShortNames[i].short_name) {
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> for loop is better

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:64: bool
ends_with_privet =
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> EndsWith()

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:73: if
(at_pos == std::string::npos)
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> if (ssid[ssid.length() - kPrivetCharactersAfterSeparator - 1] != '@' &&
>     /*maybe*/ count of '@' != 1)
> return false

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:77: return
false;
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> or maybe just use base::SplitString

Did the first one.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:83: const
std::string internal_id,
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> string&

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:89: size_t
at_pos = ssid.rfind('@');
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> base::SplitString ?

We don't really use the split strings directly, so I'm not sure it entirely
makes sense here, since we'd have to get substrs anyways.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:99: }
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> : connection_status(SOMETHING)

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:118:
network_watcher_ = wifi_manager_->CreateNetworkListWatcher(base::Bind(
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> break before base::Bind

Again, clang-format reverts this change when I run it. Unless this is really
bothersome, I'd prefer to keep the formatting clang-format uses.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:132: i !=
ssids.end();
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> usually size_t i = 0 ... for are shorter

Done.

https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:144:
weak_this && i != active_devices.end();
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> same

This is iteration over a map.

https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:3479:
'browser/local_discovery/wifi/bootstrapping_device_lister.h',
On 2014/05/22 05:51:36, Vitaly Buka wrote:
> order

Done.

[Vitaly Buka (NO REVIEWS), 2014-05-22 18:50:11]

On 2014/05/22 18:43:58, Noam Samuel wrote:
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc
(right):
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:22: const
> char* short_name;
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > const char* const
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:25: {"cam",
> "camera"},
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > I thing it should be 2 spaces
> 
> I'd be reluctant to make any changes clang-format will just reverse again.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:30: const
> char* kPrivetDeviceLongName = "device";
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > const char kPrivetDeviceLongName[]
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:35: while
> (kPrivetShortNames[i].short_name) {
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > for loop is better
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:64: bool
> ends_with_privet =
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > EndsWith()
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:73: if
> (at_pos == std::string::npos)
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > if (ssid[ssid.length() - kPrivetCharactersAfterSeparator - 1] != '@' &&
> >     /*maybe*/ count of '@' != 1)
> > return false
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:77: return
> false;
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > or maybe just use base::SplitString
> 
> Did the first one.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:83: const
> std::string internal_id,
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > string&
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:89: size_t
> at_pos = ssid.rfind('@');
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > base::SplitString ?
> 
> We don't really use the split strings directly, so I'm not sure it entirely
> makes sense here, since we'd have to get substrs anyways.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:99: }
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > : connection_status(SOMETHING)
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:118:
> network_watcher_ = wifi_manager_->CreateNetworkListWatcher(base::Bind(
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > break before base::Bind
> 
> Again, clang-format reverts this change when I run it. Unless this is really
> bothersome, I'd prefer to keep the formatting clang-format uses.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:132: i !=
> ssids.end();
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > usually size_t i = 0 ... for are shorter
> 
> Done.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:144:
> weak_this && i != active_devices.end();
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > same
> 
> This is iteration over a map.
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gy...
> chrome/chrome_browser.gypi:3479:
> 'browser/local_discovery/wifi/bootstrapping_device_lister.h',
> On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > order
> 
> Done.

I see no new patch.

[Noam Samuel, 2014-05-22 18:51:51]

On 2014/05/22 18:50:11, Vitaly Buka wrote:
> On 2014/05/22 18:43:58, Noam Samuel wrote:
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:22: const
> > char* short_name;
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > const char* const
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:25:
{"cam",
> > "camera"},
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > I thing it should be 2 spaces
> > 
> > I'd be reluctant to make any changes clang-format will just reverse again.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:30: const
> > char* kPrivetDeviceLongName = "device";
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > const char kPrivetDeviceLongName[]
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:35: while
> > (kPrivetShortNames[i].short_name) {
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > for loop is better
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:64: bool
> > ends_with_privet =
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > EndsWith()
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:73: if
> > (at_pos == std::string::npos)
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > if (ssid[ssid.length() - kPrivetCharactersAfterSeparator - 1] != '@' &&
> > >     /*maybe*/ count of '@' != 1)
> > > return false
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:77:
return
> > false;
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > or maybe just use base::SplitString
> > 
> > Did the first one.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:83: const
> > std::string internal_id,
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > string&
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:89:
size_t
> > at_pos = ssid.rfind('@');
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > base::SplitString ?
> > 
> > We don't really use the split strings directly, so I'm not sure it entirely
> > makes sense here, since we'd have to get substrs anyways.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:99: }
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > : connection_status(SOMETHING)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:118:
> > network_watcher_ = wifi_manager_->CreateNetworkListWatcher(base::Bind(
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > break before base::Bind
> > 
> > Again, clang-format reverts this change when I run it. Unless this is really
> > bothersome, I'd prefer to keep the formatting clang-format uses.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:132: i !=
> > ssids.end();
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > usually size_t i = 0 ... for are shorter
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/browser/local_dis...
> > chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:144:
> > weak_this && i != active_devices.end();
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > same
> > 
> > This is iteration over a map.
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gypi
> > File chrome/chrome_browser.gypi (right):
> > 
> >
>
https://codereview.chromium.org/288043004/diff/40001/chrome/chrome_browser.gy...
> > chrome/chrome_browser.gypi:3479:
> > 'browser/local_discovery/wifi/bootstrapping_device_lister.h',
> > On 2014/05/22 05:51:36, Vitaly Buka wrote:
> > > order
> > 
> > Done.
> 
> I see no new patch.

Weird. I thought I uploaded it. Now it's there.

[Vitaly Buka (NO REVIEWS), 2014-05-22 20:38:13]

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc (right):

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:28: {NULL,
NULL}  // Sentinel value
you don't need Sentinel value, just use arraysize()

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:47: return
BootstrappingDeviceDescription::OFFLINE;
maybe some array with mapping is better here?

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:65:
ssid.find('@', ssid.length() - kPrivetCharactersAfterSeparator) !=
so you check right side of @?

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:77:
description->device_ssid = ssid;
can you put all logic that deppends on ssid into one function
Kind of combine this one and isPrivet...

bool ParsePrivetSsid(const string& ssid, string* name, string* kind,
ConnectionStatus* status) {

}

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:120:
ActiveDeviceMap active_devices;
active_devices -> new_devices
hard to read with two similar names active_devices and active_devices_

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:138: if
(found == active_devices_.end() || found->second != i->second) {
I guess std::set_difference() could be used here

you don't really use ActiveDeviceMap::first as a key for map, you use both, so 
set<pair<string, string>> is more reasonable here

because you replace it every time, sorted vector<pair<string, string>> even
better

[Noam Samuel, 2014-05-29 01:03:31]

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc (right):

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:28: {NULL,
NULL}  // Sentinel value
On 2014/05/22 20:38:13, Vitaly Buka wrote:
> you don't need Sentinel value, just use arraysize()

Done.

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:47: return
BootstrappingDeviceDescription::OFFLINE;
On 2014/05/22 20:38:13, Vitaly Buka wrote:
> maybe some array with mapping is better here?

Done.

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:65:
ssid.find('@', ssid.length() - kPrivetCharactersAfterSeparator) !=
On 2014/05/22 20:38:13, Vitaly Buka wrote:
> so you check right side of @?

Yeah, the point is, the right side should not contain an '@'

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:77:
description->device_ssid = ssid;
On 2014/05/22 20:38:13, Vitaly Buka wrote:
> can you put all logic that deppends on ssid into one function
> Kind of combine this one and isPrivet...
> 
> bool ParsePrivetSsid(const string& ssid, string* name, string* kind,
> ConnectionStatus* status) {
> 
> }

Done.

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:120:
ActiveDeviceMap active_devices;
On 2014/05/22 20:38:13, Vitaly Buka wrote:
> active_devices -> new_devices
> hard to read with two similar names active_devices and active_devices_

Done.

https://codereview.chromium.org/288043004/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:138: if
(found == active_devices_.end() || found->second != i->second) {
On 2014/05/22 20:38:13, Vitaly Buka wrote:
> I guess std::set_difference() could be used here
> 
> you don't really use ActiveDeviceMap::first as a key for map, you use both, so

> set<pair<string, string>> is more reasonable here
> 
> because you replace it every time, sorted vector<pair<string, string>> even
> better

Done.

[Vitaly Buka (NO REVIEWS), 2014-05-29 01:20:06]

lgtm

https://codereview.chromium.org/288043004/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc (right):

https://codereview.chromium.org/288043004/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:141:
added_devices.begin());
current code is OK, but if you like you can use
std::back_inserter(added_devices) and avoid post resize.

https://codereview.chromium.org/288043004/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:153: // Find
removed SSIDs
Probably 135-150 and 155-170 could be extracted into single function called with
different args

[Vitaly Buka (NO REVIEWS), 2014-05-29 01:25:09]

The CQ bit was checked by vitalybuka@chromium.org

[commit-bot: I haz the power, 2014-05-29 01:25:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noamsml@chromium.org/288043004/80001

[commit-bot: I haz the power, 2014-05-29 03:58:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10356)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13434)

[commit-bot: I haz the power, 2014-05-29 04:03:26]

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

[commit-bot: I haz the power, 2014-05-29 04:03:26]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10364)

[Noam Samuel, 2014-05-30 18:58:37]

https://codereview.chromium.org/288043004/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc (right):

https://codereview.chromium.org/288043004/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:141:
added_devices.begin());
On 2014/05/29 01:20:07, Vitaly Buka wrote:
> current code is OK, but if you like you can use
> std::back_inserter(added_devices) and avoid post resize.
> 

Done.

https://codereview.chromium.org/288043004/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/bootstrapping_device_lister.cc:153: // Find
removed SSIDs
On 2014/05/29 01:20:07, Vitaly Buka wrote:
> Probably 135-150 and 155-170 could be extracted into single function called
with
> different args

Done.

[Noam Samuel, 2014-06-04 17:27:42]

The CQ bit was checked by noamsml@chromium.org

[commit-bot: I haz the power, 2014-06-04 17:28:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noamsml@chromium.org/288043004/100001

[commit-bot: I haz the power, 2014-06-04 20:23:39]

Change committed as 274896