Issue 226883002: WiFi client for GCD bootstrapping

Noam Samuel

6 years, 7 months ago (2014-05-05 17:57:01 UTC) #1

Noam Samuel

+vitalybuka@ and gene@ for general review +mef@ for adding dependency on components/wifi +stevenjb@ for adding ...

6 years, 7 months ago (2014-05-05 18:01:57 UTC) #3

mef

https://codereview.chromium.org/226883002/diff/40001/chrome/browser/local_discovery/wifi/wifi_service_client.cc File chrome/browser/local_discovery/wifi/wifi_service_client.cc (right): https://codereview.chromium.org/226883002/diff/40001/chrome/browser/local_discovery/wifi/wifi_service_client.cc#newcode198 chrome/browser/local_discovery/wifi/wifi_service_client.cc:198: wifi_service_->GetKeyFromSystem(network_guid, &key, &error_string); On Windows |GetKeyFromSystem| without UAC escalation ...

6 years, 7 months ago (2014-05-05 18:14:39 UTC) #4

stevenjb

I only glanced through this, but I have two high level questions: 1. It's not ...

6 years, 7 months ago (2014-05-05 18:19:49 UTC) #5

Noam Samuel

On 2014/05/05 18:19:49, stevenjb wrote: > I only glanced through this, but I have two ...

6 years, 7 months ago (2014-05-05 21:00:30 UTC) #6

stevenjb

> 2. This is an interface for the component that depends on chrome-side things like ...

6 years, 7 months ago (2014-05-05 21:11:59 UTC) #7

Noam Samuel

On 2014/05/05 21:11:59, stevenjb wrote: > > 2. This is an interface for the component ...

6 years, 7 months ago (2014-05-05 21:42:23 UTC) #8

Noam Samuel

https://codereview.chromium.org/226883002/diff/40001/chrome/browser/local_discovery/wifi/wifi_client.cc File chrome/browser/local_discovery/wifi/wifi_client.cc (right): https://codereview.chromium.org/226883002/diff/40001/chrome/browser/local_discovery/wifi/wifi_client.cc#newcode18 chrome/browser/local_discovery/wifi/wifi_client.cc:18: #endif On 2014/05/05 18:19:49, stevenjb wrote: > This won't ...

6 years, 7 months ago (2014-05-05 21:42:51 UTC) #9

Vitaly Buka (NO REVIEWS)

https://codereview.chromium.org/226883002/diff/60001/chrome/browser/local_discovery/wifi/wifi_service_client.cc File chrome/browser/local_discovery/wifi/wifi_service_client.cc (right): https://codereview.chromium.org/226883002/diff/60001/chrome/browser/local_discovery/wifi/wifi_service_client.cc#newcode60 chrome/browser/local_discovery/wifi/wifi_service_client.cc:60: wifi_service_->GetVisibleNetworks(onc::network_type::kWiFi, it's better to make wifi component to user ...

6 years, 7 months ago (2014-05-05 22:05:57 UTC) #10

Vitaly Buka (NO REVIEWS)

On 2014/05/05 21:00:30, Noam Samuel wrote: > On 2014/05/05 18:19:49, stevenjb wrote: > > I ...

6 years, 7 months ago (2014-05-05 22:06:33 UTC) #11

stevenjb

1) I am still not convinced that this belongs in src/chrome. There shouldn't be any ...

6 years, 7 months ago (2014-05-07 17:31:05 UTC) #12

Noam Samuel

On 2014/05/07 17:31:05, stevenjb wrote: > 1) I am still not convinced that this belongs ...

6 years, 7 months ago (2014-05-07 21:54:11 UTC) #13

On 2014/05/07 17:31:05, stevenjb wrote:
> 1) I am still not convinced that this belongs in src/chrome. There shouldn't
be
> any actual Browser dependencies, and we should be able to avoid dependencies
on
> code in the src/chrome directory. If you have a specific example where you do
> not expect that to be the case, please provide that.
> 
> 2) Much of this code is non-chromeos specific and should be in an
> _nonchromeos.cc file, unless it is part of an entire component that is
> non-chromeos, e.g. components/wifi.
> 
> 3) Who is the consumer of this code going to be, and is there a reason why we
> can't just use the networkingPrivate extensions API?
>
(https://docs.google.com/a/google.com/document/d/1QWIzDvf_-iZJW8CINvhxzIERwwKe...).
> This looks like it could just be a subset of it. 
> 
> +tbarzic@ who has been working with the ChromeCast team to do something
similar
> using networkingPrivate.

1. This is supposed to be a unified interface between ChromeOS and non-ChromeOS.
This means it could not easily be part of a non-ChromeOS component. That being
said, it doesn't look like it needs to be part of chrome/browser/.

2. I'll rename wifi_service_client to wifi_client_nonchromeos

3. The consumer for this is a more general WiFi provisioning process than the
one the networkingPrivate API is designed for. In particular, it is designed for
a variety of non-cast devices on GCD (http://go/gcd_chrome). These devices come
from a variety of manufacturers and we plan to use a protocol not based on
private key verification, but instead on user verification of device
authenticity (http://go/gcd_security). The protocol, for bootstrapping is
described in this doc (pending some updates, but the rough idea is at
http://go/gcd_wifi). 

There are a couple of reasons not to use the networkingPrivate API: The first
and most pertinent is that the credential-getting functions for the
networkingPrivate API are very specific to the Cast protocol. The style of
encryption and verification used wouldn't work for the GCD WiFi protocol. The
second is that we want to be able to develop this as part of a unified API that
can use connecting to a device AP, creating a host AP on your machine (on
Windows and potentially other platforms), and potentially (still investigating
the possibility) sending data over information elements of WiFi frames.

stevenjb

1. All of the Chrome OS specific networking code has been moved to src/chromeos/networking. A ...

6 years, 7 months ago (2014-05-07 23:22:59 UTC) #14

1. All of the Chrome OS specific networking code has been moved to
src/chromeos/networking. A component in src/componets/ can depend on
src/chromeos, we already have a couple of examples. We've been making an
effort to componetize the Chrome codebase to avoid spaghetti code, and this
seems like an excellent candidate.

2. Thanks!

3. Thanks for the summary, that helps. The networkingPrivate API is still
expanding, we wouldn't need to rely solely on existing methods, but if the
methods are significantly different maybe it makes more sense to start with
a clean plate. I will try to read through the document, it might be worth
meeting at some point (with someone from the Shill team to) to discuss how
this will eventually be implemented on Chrome OS so that we can plan ahead.

I'll try to do a code review pass later today or early tomorrow.



On Wed, May 7, 2014 at 2:54 PM, <noamsml@chromium.org> wrote:

> On 2014/05/07 17:31:05, stevenjb wrote:
>
>> 1) I am still not convinced that this belongs in src/chrome. There
>> shouldn't
>>
> be
>
>> any actual Browser dependencies, and we should be able to avoid
>> dependencies
>>
> on
>
>> code in the src/chrome directory. If you have a specific example where
>> you do
>> not expect that to be the case, please provide that.
>>
>
>  2) Much of this code is non-chromeos specific and should be in an
>> _nonchromeos.cc file, unless it is part of an entire component that is
>> non-chromeos, e.g. components/wifi.
>>
>
>  3) Who is the consumer of this code going to be, and is there a reason
>> why we
>> can't just use the networkingPrivate extensions API?
>>
>
> (https://docs.google.com/a/google.com/document/d/1QWIzDvf_-
> iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit).
>
>> This looks like it could just be a subset of it.
>>
>
>  +tbarzic@ who has been working with the ChromeCast team to do something
>>
> similar
>
>> using networkingPrivate.
>>
>
> 1. This is supposed to be a unified interface between ChromeOS and
> non-ChromeOS.
> This means it could not easily be part of a non-ChromeOS component. That
> being
> said, it doesn't look like it needs to be part of chrome/browser/.
>
> 2. I'll rename wifi_service_client to wifi_client_nonchromeos
>
> 3. The consumer for this is a more general WiFi provisioning process than
> the
> one the networkingPrivate API is designed for. In particular, it is
> designed for
> a variety of non-cast devices on GCD (http://go/gcd_chrome). These
> devices come
> from a variety of manufacturers and we plan to use a protocol not based on
> private key verification, but instead on user verification of device
> authenticity (http://go/gcd_security). The protocol, for bootstrapping is
> described in this doc (pending some updates, but the rough idea is at
> http://go/gcd_wifi).
>
> There are a couple of reasons not to use the networkingPrivate API: The
> first
> and most pertinent is that the credential-getting functions for the
> networkingPrivate API are very specific to the Cast protocol. The style of
> encryption and verification used wouldn't work for the GCD WiFi protocol.
> The
> second is that we want to be able to develop this as part of a unified API
> that
> can use connecting to a device AP, creating a host AP on your machine (on
> Windows and potentially other platforms), and potentially (still
> investigating
> the possibility) sending data over information elements of WiFi frames.
>
> https://codereview.chromium.org/226883002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

chromium-reviews

stevenjb and mef: How does putting this code in components/wifi/browser/ sound? I think that's a ...

6 years, 7 months ago (2014-05-07 23:43:53 UTC) #15

stevenjb and mef: How does putting this code in components/wifi/browser/
sound? I think that's a semantically accurate place for it.


On Wed, May 7, 2014 at 4:22 PM, Steven Bennetts <stevenjb@chromium.org>wrote:

> 1. All of the Chrome OS specific networking code has been moved to
> src/chromeos/networking. A component in src/componets/ can depend on
> src/chromeos, we already have a couple of examples. We've been making an
> effort to componetize the Chrome codebase to avoid spaghetti code, and this
> seems like an excellent candidate.
>
> 2. Thanks!
>
> 3. Thanks for the summary, that helps. The networkingPrivate API is still
> expanding, we wouldn't need to rely solely on existing methods, but if the
> methods are significantly different maybe it makes more sense to start with
> a clean plate. I will try to read through the document, it might be worth
> meeting at some point (with someone from the Shill team to) to discuss how
> this will eventually be implemented on Chrome OS so that we can plan ahead.
>
> I'll try to do a code review pass later today or early tomorrow.
>
>
>
> On Wed, May 7, 2014 at 2:54 PM, <noamsml@chromium.org> wrote:
>
>> On 2014/05/07 17:31:05, stevenjb wrote:
>>
>>> 1) I am still not convinced that this belongs in src/chrome. There
>>> shouldn't
>>>
>> be
>>
>>> any actual Browser dependencies, and we should be able to avoid
>>> dependencies
>>>
>> on
>>
>>> code in the src/chrome directory. If you have a specific example where
>>> you do
>>> not expect that to be the case, please provide that.
>>>
>>
>>  2) Much of this code is non-chromeos specific and should be in an
>>> _nonchromeos.cc file, unless it is part of an entire component that is
>>> non-chromeos, e.g. components/wifi.
>>>
>>
>>  3) Who is the consumer of this code going to be, and is there a reason
>>> why we
>>> can't just use the networkingPrivate extensions API?
>>>
>>
>> (https://docs.google.com/a/google.com/document/d/1QWIzDvf_-
>> iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit).
>>
>>> This looks like it could just be a subset of it.
>>>
>>
>>  +tbarzic@ who has been working with the ChromeCast team to do something
>>>
>> similar
>>
>>> using networkingPrivate.
>>>
>>
>> 1. This is supposed to be a unified interface between ChromeOS and
>> non-ChromeOS.
>> This means it could not easily be part of a non-ChromeOS component. That
>> being
>> said, it doesn't look like it needs to be part of chrome/browser/.
>>
>> 2. I'll rename wifi_service_client to wifi_client_nonchromeos
>>
>> 3. The consumer for this is a more general WiFi provisioning process than
>> the
>> one the networkingPrivate API is designed for. In particular, it is
>> designed for
>> a variety of non-cast devices on GCD (http://go/gcd_chrome). These
>> devices come
>> from a variety of manufacturers and we plan to use a protocol not based on
>> private key verification, but instead on user verification of device
>> authenticity (http://go/gcd_security). The protocol, for bootstrapping is
>> described in this doc (pending some updates, but the rough idea is at
>> http://go/gcd_wifi).
>>
>> There are a couple of reasons not to use the networkingPrivate API: The
>> first
>> and most pertinent is that the credential-getting functions for the
>> networkingPrivate API are very specific to the Cast protocol. The style of
>> encryption and verification used wouldn't work for the GCD WiFi protocol.
>> The
>> second is that we want to be able to develop this as part of a unified
>> API that
>> can use connecting to a device AP, creating a host AP on your machine (on
>> Windows and potentially other platforms), and potentially (still
>> investigating
>> the possibility) sending data over information elements of WiFi frames.
>>
>> https://codereview.chromium.org/226883002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

stevenjb

I think that adding it to components/wifi makes sense, it doesn't seem like we need ...

6 years, 7 months ago (2014-05-07 23:50:31 UTC) #16

I think that adding it to components/wifi makes sense, it doesn't seem like
we need an entirely separate component.

Components should generally be agnostic of the concept of a "browser". I
would suggest wifi/service_client/



On Wed, May 7, 2014 at 4:43 PM, Noam Samuel <noamsml@google.com> wrote:

> stevenjb and mef: How does putting this code in components/wifi/browser/
> sound? I think that's a semantically accurate place for it.
>
>
> On Wed, May 7, 2014 at 4:22 PM, Steven Bennetts <stevenjb@chromium.org>wrote:
>
>> 1. All of the Chrome OS specific networking code has been moved to
>> src/chromeos/networking. A component in src/componets/ can depend on
>> src/chromeos, we already have a couple of examples. We've been making an
>> effort to componetize the Chrome codebase to avoid spaghetti code, and this
>> seems like an excellent candidate.
>>
>> 2. Thanks!
>>
>> 3. Thanks for the summary, that helps. The networkingPrivate API is still
>> expanding, we wouldn't need to rely solely on existing methods, but if the
>> methods are significantly different maybe it makes more sense to start with
>> a clean plate. I will try to read through the document, it might be worth
>> meeting at some point (with someone from the Shill team to) to discuss how
>> this will eventually be implemented on Chrome OS so that we can plan ahead.
>>
>> I'll try to do a code review pass later today or early tomorrow.
>>
>>
>>
>> On Wed, May 7, 2014 at 2:54 PM, <noamsml@chromium.org> wrote:
>>
>>> On 2014/05/07 17:31:05, stevenjb wrote:
>>>
>>>> 1) I am still not convinced that this belongs in src/chrome. There
>>>> shouldn't
>>>>
>>> be
>>>
>>>> any actual Browser dependencies, and we should be able to avoid
>>>> dependencies
>>>>
>>> on
>>>
>>>> code in the src/chrome directory. If you have a specific example where
>>>> you do
>>>> not expect that to be the case, please provide that.
>>>>
>>>
>>>  2) Much of this code is non-chromeos specific and should be in an
>>>> _nonchromeos.cc file, unless it is part of an entire component that is
>>>> non-chromeos, e.g. components/wifi.
>>>>
>>>
>>>  3) Who is the consumer of this code going to be, and is there a reason
>>>> why we
>>>> can't just use the networkingPrivate extensions API?
>>>>
>>>
>>> (https://docs.google.com/a/google.com/document/d/1QWIzDvf_-
>>> iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit).
>>>
>>>> This looks like it could just be a subset of it.
>>>>
>>>
>>>  +tbarzic@ who has been working with the ChromeCast team to do something
>>>>
>>> similar
>>>
>>>> using networkingPrivate.
>>>>
>>>
>>> 1. This is supposed to be a unified interface between ChromeOS and
>>> non-ChromeOS.
>>> This means it could not easily be part of a non-ChromeOS component. That
>>> being
>>> said, it doesn't look like it needs to be part of chrome/browser/.
>>>
>>> 2. I'll rename wifi_service_client to wifi_client_nonchromeos
>>>
>>> 3. The consumer for this is a more general WiFi provisioning process
>>> than the
>>> one the networkingPrivate API is designed for. In particular, it is
>>> designed for
>>> a variety of non-cast devices on GCD (http://go/gcd_chrome). These
>>> devices come
>>> from a variety of manufacturers and we plan to use a protocol not based
>>> on
>>> private key verification, but instead on user verification of device
>>> authenticity (http://go/gcd_security). The protocol, for bootstrapping
>>> is
>>> described in this doc (pending some updates, but the rough idea is at
>>> http://go/gcd_wifi).
>>>
>>> There are a couple of reasons not to use the networkingPrivate API: The
>>> first
>>> and most pertinent is that the credential-getting functions for the
>>> networkingPrivate API are very specific to the Cast protocol. The style
>>> of
>>> encryption and verification used wouldn't work for the GCD WiFi
>>> protocol. The
>>> second is that we want to be able to develop this as part of a unified
>>> API that
>>> can use connecting to a device AP, creating a host AP on your machine (on
>>> Windows and potentially other platforms), and potentially (still
>>> investigating
>>> the possibility) sending data over information elements of WiFi frames.
>>>
>>> https://codereview.chromium.org/226883002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Noam Samuel

On 2014/05/07 23:50:31, stevenjb wrote: > I think that adding it to components/wifi makes sense, ...

6 years, 7 months ago (2014-05-08 17:50:27 UTC) #17

On 2014/05/07 23:50:31, stevenjb wrote:
> I think that adding it to components/wifi makes sense, it doesn't seem like
> we need an entirely separate component.
> 
> Components should generally be agnostic of the concept of a "browser". I
> would suggest wifi/service_client/
> 
> 
> 
> On Wed, May 7, 2014 at 4:43 PM, Noam Samuel <mailto:noamsml@google.com> wrote:
> 
> > stevenjb and mef: How does putting this code in components/wifi/browser/
> > sound? I think that's a semantically accurate place for it.
> >
> >
> > On Wed, May 7, 2014 at 4:22 PM, Steven Bennetts
<mailto:stevenjb@chromium.org>wrote:
> >
> >> 1. All of the Chrome OS specific networking code has been moved to
> >> src/chromeos/networking. A component in src/componets/ can depend on
> >> src/chromeos, we already have a couple of examples. We've been making an
> >> effort to componetize the Chrome codebase to avoid spaghetti code, and this
> >> seems like an excellent candidate.
> >>
> >> 2. Thanks!
> >>
> >> 3. Thanks for the summary, that helps. The networkingPrivate API is still
> >> expanding, we wouldn't need to rely solely on existing methods, but if the
> >> methods are significantly different maybe it makes more sense to start with
> >> a clean plate. I will try to read through the document, it might be worth
> >> meeting at some point (with someone from the Shill team to) to discuss how
> >> this will eventually be implemented on Chrome OS so that we can plan ahead.
> >>
> >> I'll try to do a code review pass later today or early tomorrow.
> >>
> >>
> >>
> >> On Wed, May 7, 2014 at 2:54 PM, <mailto:noamsml@chromium.org> wrote:
> >>
> >>> On 2014/05/07 17:31:05, stevenjb wrote:
> >>>
> >>>> 1) I am still not convinced that this belongs in src/chrome. There
> >>>> shouldn't
> >>>>
> >>> be
> >>>
> >>>> any actual Browser dependencies, and we should be able to avoid
> >>>> dependencies
> >>>>
> >>> on
> >>>
> >>>> code in the src/chrome directory. If you have a specific example where
> >>>> you do
> >>>> not expect that to be the case, please provide that.
> >>>>
> >>>
> >>>  2) Much of this code is non-chromeos specific and should be in an
> >>>> _nonchromeos.cc file, unless it is part of an entire component that is
> >>>> non-chromeos, e.g. components/wifi.
> >>>>
> >>>
> >>>  3) Who is the consumer of this code going to be, and is there a reason
> >>>> why we
> >>>> can't just use the networkingPrivate extensions API?
> >>>>
> >>>
> >>> (https://docs.google.com/a/google.com/document/d/1QWIzDvf_-
> >>> iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit).
> >>>
> >>>> This looks like it could just be a subset of it.
> >>>>
> >>>
> >>>  +tbarzic@ who has been working with the ChromeCast team to do something
> >>>>
> >>> similar
> >>>
> >>>> using networkingPrivate.
> >>>>
> >>>
> >>> 1. This is supposed to be a unified interface between ChromeOS and
> >>> non-ChromeOS.
> >>> This means it could not easily be part of a non-ChromeOS component. That
> >>> being
> >>> said, it doesn't look like it needs to be part of chrome/browser/.
> >>>
> >>> 2. I'll rename wifi_service_client to wifi_client_nonchromeos
> >>>
> >>> 3. The consumer for this is a more general WiFi provisioning process
> >>> than the
> >>> one the networkingPrivate API is designed for. In particular, it is
> >>> designed for
> >>> a variety of non-cast devices on GCD (http://go/gcd_chrome). These
> >>> devices come
> >>> from a variety of manufacturers and we plan to use a protocol not based
> >>> on
> >>> private key verification, but instead on user verification of device
> >>> authenticity (http://go/gcd_security). The protocol, for bootstrapping
> >>> is
> >>> described in this doc (pending some updates, but the rough idea is at
> >>> http://go/gcd_wifi).
> >>>
> >>> There are a couple of reasons not to use the networkingPrivate API: The
> >>> first
> >>> and most pertinent is that the credential-getting functions for the
> >>> networkingPrivate API are very specific to the Cast protocol. The style
> >>> of
> >>> encryption and verification used wouldn't work for the GCD WiFi
> >>> protocol. The
> >>> second is that we want to be able to develop this as part of a unified
> >>> API that
> >>> can use connecting to a device AP, creating a host AP on your machine (on
> >>> Windows and potentially other platforms), and potentially (still
> >>> investigating
> >>> the possibility) sending data over information elements of WiFi frames.
> >>>
> >>> https://codereview.chromium.org/226883002/
> >>>
> >>
> >>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

One thing is, it seems like the wifi component is currently mac-and-windows
only. We plan on expanding the WifiClient to have a ChromeOS implementation, so
we would need to have the component exist on ChromeOS as well. This shouldn't be
too hard to do, though. mef@, thoughts?

Noam Samuel

On 2014/05/08 17:50:27, Noam Samuel wrote: > On 2014/05/07 23:50:31, stevenjb wrote: > > I ...

6 years, 7 months ago (2014-05-08 17:52:41 UTC) #18

On 2014/05/08 17:50:27, Noam Samuel wrote:
> On 2014/05/07 23:50:31, stevenjb wrote:
> > I think that adding it to components/wifi makes sense, it doesn't seem like
> > we need an entirely separate component.
> > 
> > Components should generally be agnostic of the concept of a "browser". I
> > would suggest wifi/service_client/
> > 
> > 
> > 
> > On Wed, May 7, 2014 at 4:43 PM, Noam Samuel <mailto:noamsml@google.com>
wrote:
> > 
> > > stevenjb and mef: How does putting this code in components/wifi/browser/
> > > sound? I think that's a semantically accurate place for it.
> > >
> > >
> > > On Wed, May 7, 2014 at 4:22 PM, Steven Bennetts
> <mailto:stevenjb@chromium.org>wrote:
> > >
> > >> 1. All of the Chrome OS specific networking code has been moved to
> > >> src/chromeos/networking. A component in src/componets/ can depend on
> > >> src/chromeos, we already have a couple of examples. We've been making an
> > >> effort to componetize the Chrome codebase to avoid spaghetti code, and
this
> > >> seems like an excellent candidate.
> > >>
> > >> 2. Thanks!
> > >>
> > >> 3. Thanks for the summary, that helps. The networkingPrivate API is still
> > >> expanding, we wouldn't need to rely solely on existing methods, but if
the
> > >> methods are significantly different maybe it makes more sense to start
with
> > >> a clean plate. I will try to read through the document, it might be worth
> > >> meeting at some point (with someone from the Shill team to) to discuss
how
> > >> this will eventually be implemented on Chrome OS so that we can plan
ahead.
> > >>
> > >> I'll try to do a code review pass later today or early tomorrow.
> > >>
> > >>
> > >>
> > >> On Wed, May 7, 2014 at 2:54 PM, <mailto:noamsml@chromium.org> wrote:
> > >>
> > >>> On 2014/05/07 17:31:05, stevenjb wrote:
> > >>>
> > >>>> 1) I am still not convinced that this belongs in src/chrome. There
> > >>>> shouldn't
> > >>>>
> > >>> be
> > >>>
> > >>>> any actual Browser dependencies, and we should be able to avoid
> > >>>> dependencies
> > >>>>
> > >>> on
> > >>>
> > >>>> code in the src/chrome directory. If you have a specific example where
> > >>>> you do
> > >>>> not expect that to be the case, please provide that.
> > >>>>
> > >>>
> > >>>  2) Much of this code is non-chromeos specific and should be in an
> > >>>> _nonchromeos.cc file, unless it is part of an entire component that is
> > >>>> non-chromeos, e.g. components/wifi.
> > >>>>
> > >>>
> > >>>  3) Who is the consumer of this code going to be, and is there a reason
> > >>>> why we
> > >>>> can't just use the networkingPrivate extensions API?
> > >>>>
> > >>>
> > >>> (https://docs.google.com/a/google.com/document/d/1QWIzDvf_-
> > >>> iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit).
> > >>>
> > >>>> This looks like it could just be a subset of it.
> > >>>>
> > >>>
> > >>>  +tbarzic@ who has been working with the ChromeCast team to do something
> > >>>>
> > >>> similar
> > >>>
> > >>>> using networkingPrivate.
> > >>>>
> > >>>
> > >>> 1. This is supposed to be a unified interface between ChromeOS and
> > >>> non-ChromeOS.
> > >>> This means it could not easily be part of a non-ChromeOS component. That
> > >>> being
> > >>> said, it doesn't look like it needs to be part of chrome/browser/.
> > >>>
> > >>> 2. I'll rename wifi_service_client to wifi_client_nonchromeos
> > >>>
> > >>> 3. The consumer for this is a more general WiFi provisioning process
> > >>> than the
> > >>> one the networkingPrivate API is designed for. In particular, it is
> > >>> designed for
> > >>> a variety of non-cast devices on GCD (http://go/gcd_chrome). These
> > >>> devices come
> > >>> from a variety of manufacturers and we plan to use a protocol not based
> > >>> on
> > >>> private key verification, but instead on user verification of device
> > >>> authenticity (http://go/gcd_security). The protocol, for bootstrapping
> > >>> is
> > >>> described in this doc (pending some updates, but the rough idea is at
> > >>> http://go/gcd_wifi).
> > >>>
> > >>> There are a couple of reasons not to use the networkingPrivate API: The
> > >>> first
> > >>> and most pertinent is that the credential-getting functions for the
> > >>> networkingPrivate API are very specific to the Cast protocol. The style
> > >>> of
> > >>> encryption and verification used wouldn't work for the GCD WiFi
> > >>> protocol. The
> > >>> second is that we want to be able to develop this as part of a unified
> > >>> API that
> > >>> can use connecting to a device AP, creating a host AP on your machine
(on
> > >>> Windows and potentially other platforms), and potentially (still
> > >>> investigating
> > >>> the possibility) sending data over information elements of WiFi frames.
> > >>>
> > >>> https://codereview.chromium.org/226883002/
> > >>>
> > >>
> > >>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> One thing is, it seems like the wifi component is currently mac-and-windows
> only. We plan on expanding the WifiClient to have a ChromeOS implementation,
so
> we would need to have the component exist on ChromeOS as well. This shouldn't
be
> too hard to do, though. mef@, thoughts?

(To be clear, this would just mean allowing the component to be compiled only
with the ChromeOS WifiClient implementation and without WifiService)

tbarzic

What do you think about moving only parts that are used to manage wifi_service to ...

6 years, 7 months ago (2014-05-08 18:32:06 UTC) #19

stevenjb

On 2014/05/08 18:32:06, tbarzic wrote: > What do you think about moving only parts that ...

6 years, 7 months ago (2014-05-08 18:54:31 UTC) #20

mef

On 2014/05/08 18:32:06, tbarzic wrote: > What do you think about moving only parts that ...

6 years, 7 months ago (2014-05-08 19:07:10 UTC) #21

mef

On 2014/05/08 17:50:27, Noam Samuel wrote: > On 2014/05/07 23:50:31, stevenjb wrote: > > I ...

6 years, 7 months ago (2014-05-08 19:12:12 UTC) #22

On 2014/05/08 17:50:27, Noam Samuel wrote:
> On 2014/05/07 23:50:31, stevenjb wrote:
> > I think that adding it to components/wifi makes sense, it doesn't seem like
> > we need an entirely separate component.
> > 
> > Components should generally be agnostic of the concept of a "browser". I
> > would suggest wifi/service_client/
> > 
> > 
> > 
> > On Wed, May 7, 2014 at 4:43 PM, Noam Samuel <mailto:noamsml@google.com>
wrote:
> > 
> > > stevenjb and mef: How does putting this code in components/wifi/browser/
> > > sound? I think that's a semantically accurate place for it.
> > >
> > >
> > > On Wed, May 7, 2014 at 4:22 PM, Steven Bennetts
> <mailto:stevenjb@chromium.org>wrote:
> > >
> > >> 1. All of the Chrome OS specific networking code has been moved to
> > >> src/chromeos/networking. A component in src/componets/ can depend on
> > >> src/chromeos, we already have a couple of examples. We've been making an
> > >> effort to componetize the Chrome codebase to avoid spaghetti code, and
this
> > >> seems like an excellent candidate.
> > >>
> > >> 2. Thanks!
> > >>
> > >> 3. Thanks for the summary, that helps. The networkingPrivate API is still
> > >> expanding, we wouldn't need to rely solely on existing methods, but if
the
> > >> methods are significantly different maybe it makes more sense to start
with
> > >> a clean plate. I will try to read through the document, it might be worth
> > >> meeting at some point (with someone from the Shill team to) to discuss
how
> > >> this will eventually be implemented on Chrome OS so that we can plan
ahead.
> > >>
> > >> I'll try to do a code review pass later today or early tomorrow.
> > >>
> > >>
> > >>
> > >> On Wed, May 7, 2014 at 2:54 PM, <mailto:noamsml@chromium.org> wrote:
> > >>
> > >>> On 2014/05/07 17:31:05, stevenjb wrote:
> > >>>
> > >>>> 1) I am still not convinced that this belongs in src/chrome. There
> > >>>> shouldn't
> > >>>>
> > >>> be
> > >>>
> > >>>> any actual Browser dependencies, and we should be able to avoid
> > >>>> dependencies
> > >>>>
> > >>> on
> > >>>
> > >>>> code in the src/chrome directory. If you have a specific example where
> > >>>> you do
> > >>>> not expect that to be the case, please provide that.
> > >>>>
> > >>>
> > >>>  2) Much of this code is non-chromeos specific and should be in an
> > >>>> _nonchromeos.cc file, unless it is part of an entire component that is
> > >>>> non-chromeos, e.g. components/wifi.
> > >>>>
> > >>>
> > >>>  3) Who is the consumer of this code going to be, and is there a reason
> > >>>> why we
> > >>>> can't just use the networkingPrivate extensions API?
> > >>>>
> > >>>
> > >>> (https://docs.google.com/a/google.com/document/d/1QWIzDvf_-
> > >>> iZJW8CINvhxzIERwwKeg72302hNUw0ZrSM/edit).
> > >>>
> > >>>> This looks like it could just be a subset of it.
> > >>>>
> > >>>
> > >>>  +tbarzic@ who has been working with the ChromeCast team to do something
> > >>>>
> > >>> similar
> > >>>
> > >>>> using networkingPrivate.
> > >>>>
> > >>>
> > >>> 1. This is supposed to be a unified interface between ChromeOS and
> > >>> non-ChromeOS.
> > >>> This means it could not easily be part of a non-ChromeOS component. That
> > >>> being
> > >>> said, it doesn't look like it needs to be part of chrome/browser/.
> > >>>
> > >>> 2. I'll rename wifi_service_client to wifi_client_nonchromeos
> > >>>
> > >>> 3. The consumer for this is a more general WiFi provisioning process
> > >>> than the
> > >>> one the networkingPrivate API is designed for. In particular, it is
> > >>> designed for
> > >>> a variety of non-cast devices on GCD (http://go/gcd_chrome). These
> > >>> devices come
> > >>> from a variety of manufacturers and we plan to use a protocol not based
> > >>> on
> > >>> private key verification, but instead on user verification of device
> > >>> authenticity (http://go/gcd_security). The protocol, for bootstrapping
> > >>> is
> > >>> described in this doc (pending some updates, but the rough idea is at
> > >>> http://go/gcd_wifi).
> > >>>
> > >>> There are a couple of reasons not to use the networkingPrivate API: The
> > >>> first
> > >>> and most pertinent is that the credential-getting functions for the
> > >>> networkingPrivate API are very specific to the Cast protocol. The style
> > >>> of
> > >>> encryption and verification used wouldn't work for the GCD WiFi
> > >>> protocol. The
> > >>> second is that we want to be able to develop this as part of a unified
> > >>> API that
> > >>> can use connecting to a device AP, creating a host AP on your machine
(on
> > >>> Windows and potentially other platforms), and potentially (still
> > >>> investigating
> > >>> the possibility) sending data over information elements of WiFi frames.
> > >>>
> > >>> https://codereview.chromium.org/226883002/
> > >>>
> > >>
> > >>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> One thing is, it seems like the wifi component is currently mac-and-windows
> only. We plan on expanding the WifiClient to have a ChromeOS implementation,
so
> we would need to have the component exist on ChromeOS as well. This shouldn't
be
> too hard to do, though. mef@, thoughts?

I actually don't know ChromeOS enough to comment on this. 
WiFiService was created to wrap platform-specific wifi interface for consumption
by Networking Private API.
I think it would be cool if we could implement it on other platforms. 
At the same time I'm not aware of short or long term reasons/benefits to do
that.

tbarzic

few comments https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_service_client.cc File chrome/browser/local_discovery/wifi/wifi_service_client.cc (right): https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_service_client.cc#newcode36 chrome/browser/local_discovery/wifi/wifi_service_client.cc:36: class WifiServiceContainer { the name is somewhat ...

6 years, 7 months ago (2014-05-08 21:54:05 UTC) #23

few comments

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

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:36: class
WifiServiceContainer {
the name is somewhat confusing.
It sounds more like something for stashing multiple wifi_service instances.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:133:
content::BrowserThread::PostTask(content::BrowserThread::UI,
just a warning:
StartConnect succeeding does not necessary mean that the connect request
succeeded; the network may still fail to connect.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:208:
base::Bind(&WifiServiceClient::PostClosure,
I think it would make more sense to do this binding in WifiServiceClient when
GetNetworkCredentails is called (and get rid of dependecy on
wifi_service_client_ from WifiServiceClientContainer)

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:240:
base::Bind(&WifiServiceContainer::Start, wifi_container_->AsWeakPtr()));
AsWeakPtr is no particularly useful here:
AsWeakPtr will return a weak ptr bound to the current thread, which is not the
thread it's used on; so this the weak ptr being set will not guarantee that
wifi_container is actually alive.

(this will probably crash on debug build)

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:251:
scan_callbacks_.push_back(callback);
I wouldn't rely on RequestScan causing OnNetworkListChanged event (the visible
network list may not change for a while) or that the next network list event is
necessary the result of the scan

stevenjb

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_client.h File chrome/browser/local_discovery/wifi/wifi_client.h (right): https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_client.h#newcode24 chrome/browser/local_discovery/wifi/wifi_client.h:24: class WifiClient { On 2014/05/05 22:05:58, Vitaly Buka wrote: ...

6 years, 7 months ago (2014-05-09 18:46:15 UTC) #24

Noam Samuel

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_client.h File chrome/browser/local_discovery/wifi/wifi_client.h (right): https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_client.h#newcode24 chrome/browser/local_discovery/wifi/wifi_client.h:24: class WifiClient { On 2014/05/09 18:46:15, stevenjb wrote: > ...

6 years, 7 months ago (2014-05-12 23:54:12 UTC) #25

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/wifi_client.h (right):

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_client.h:24: class WifiClient {
On 2014/05/09 18:46:15, stevenjb wrote:
> On 2014/05/05 22:05:58, Vitaly Buka wrote:
> > Maybe WifiClient -> WifiManager.
> > "Client" is about just our single implementation of this interface.
> 
> I agree with the naming, and this class should have a comment describing its
> purpose.

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_client.h:43: const std::string&
internal_id,
On 2014/05/09 18:46:15, stevenjb wrote:
> Maybe take 'NetworkProperties' instead of ssid + internal_id if both are
> actually needed?

Removed the need for network_id by adding a search through available networks.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_client.h:44: const std::string&
password,
On 2014/05/09 18:46:15, stevenjb wrote:
> Is this API only ever going to work with PSK networks that only require a
> passphrase, or might we want to support other types of networks, e.g. 802.11x,
> in which case we should probably use a "Credentials" struct here.

Revision 1 of GCD bootstrapping is designed for PSK networks only, since that
covers most consumer use cases. As such, I don't want to spend too much time on
802.11X networks, but it might make sense to change this to a struct that can be
expanded later.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_client.h:48: const SuccessCallback&
callback) = 0;
On 2014/05/09 18:46:15, stevenjb wrote:
> Is this designed for networks that don't require a passphrase, or ones that
are
> already configured? Please provide comments for each of these methods.

This is for networks that are already configured. I'll add a comment.

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

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:36: class
WifiServiceContainer {
On 2014/05/08 21:54:06, tbarzic wrote:
> the name is somewhat confusing.
> It sounds more like something for stashing multiple wifi_service instances.

Renamed WifiServiceWrapper

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:40: :
service_client_(service_client), weak_factory_(this) {}
On 2014/05/09 18:46:15, stevenjb wrote:
> One arg per line

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:51:
base::Bind(&WifiServiceContainer::OnNetworksChangedEvent,
On 2014/05/09 18:46:15, stevenjb wrote:
> OnNetworksChangedEvent is a no-op, so why pass it at all?

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:54:
base::Unretained(this)));
On 2014/05/09 18:46:15, stevenjb wrote:
> Rather than using Unretained (which implies that this might be unsafe), make
> OnNetworkListChangedEvent a local function (since it doesn't actually have any
> class dependencies).

OnNetworkListChangedEvent does have class dependencies (service_client_ in this
version or network_list_update_). I could bind them and make it a local
function, but I think that's somewhat clunky.

Also, since this class manages WifiService, the use of Unretained doesn't imply
lack of safety. I can use a weak pointer, but I think it's unnecessary and
implies more complexity than exists.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:57: void
GetSSIDList(const WifiClient::SSIDListCallback& callback) {
On 2014/05/09 18:46:15, stevenjb wrote:
> For a complex implementation like this it is nicer to declare the class
> separately from the implementation. (For smaller classes inlining the
> implementation is fine).

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:133:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/08 21:54:06, tbarzic wrote:
> just a warning:
> StartConnect succeeding does not necessary mean that the connect request
> succeeded; the network may still fail to connect.

Could you expand on that? Will the network either have connected or failed to
connect, or could the connection state of the network only be apparent later?

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:172:
base::Bind(callback, !error)));
On 2014/05/09 18:46:15, stevenjb wrote:
> It would be nice to share the common code between this and ConnectToNetwork.

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:208:
base::Bind(&WifiServiceClient::PostClosure,
On 2014/05/08 21:54:06, tbarzic wrote:
> I think it would make more sense to do this binding in WifiServiceClient when
> GetNetworkCredentails is called (and get rid of dependecy on
> wifi_service_client_ from WifiServiceClientContainer)

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:236:
wifi_container_.swap(wifi_container);
On 2014/05/05 22:05:58, Vitaly Buka wrote:
> why do you need swap here?
> 
> why not just
> wifi_container_.reset(new container)?

It was in order to have a stateful deleter with a task runner in it. However,
since we're using the file thread, I can change that to use
content::BrowserThread::DeleteOnThread.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:240:
base::Bind(&WifiServiceContainer::Start, wifi_container_->AsWeakPtr()));
On 2014/05/08 21:54:06, tbarzic wrote:
> AsWeakPtr is no particularly useful here:
> AsWeakPtr will return a weak ptr bound to the current thread, which is not the
> thread it's used on; so this the weak ptr being set will not guarantee that
> wifi_container is actually alive.
> 
> (this will probably crash on debug build)

Are you sure about this?
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
doesn't say anything about the thread the WeakPtr is issued on, it says that the
WeakPtr is bound to the thread it's dereferenced on.

I get the weak ptr on UI thread, but dereference and invalidate it only on the
file thread. If the constraints in the comments of weak_ptr are accurate, that
should be a valid way to use weak pointers.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:251:
scan_callbacks_.push_back(callback);
On 2014/05/08 21:54:06, tbarzic wrote:
> I wouldn't rely on RequestScan causing OnNetworkListChanged event (the visible
> network list may not change for a while) or that the next network list event
is
> necessary the result of the scan 

Thanks, changed the model a bit to add observers instead.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:269: callback.Run();
On 2014/05/05 22:05:58, Vitaly Buka wrote:
> Thread checks around would be useful for understanding this code. 

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:269: callback.Run();
On 2014/05/09 18:46:15, stevenjb wrote:
> No need for this to be a member function (unless thread checks against a
member
> of WifiServiceClient are added).

This needs to be a member function because we use the WeakPtr to ensure
callbacks are not called after the WifiServiceClient (now WifiManager) is
deleted.

This is explained in a comment in wifi_manager_nonchromeos.h
(wifi_service_client.h)

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/wifi_service_client.h (right):

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.h:37: };
On 2014/05/09 18:46:15, stevenjb wrote:
> I don't think this class provides much clarity to the code, I think you would
be
> better off explicitly managing WifiServiceContainer. (I think the comments in
> the .cc file already support this, I won't add my own there). In particular,
> using a template for a single-use case just makes the code more difficult to
> read.

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.h:45: 
On 2014/05/09 18:46:15, stevenjb wrote:
> // WifiClient

Done.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.h:48: virtual void
GetSSIDList(const SSIDListCallback& callback) OVERRIDE;
On 2014/05/09 18:46:15, stevenjb wrote:
> WS

What does this mean?

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.h:63: void
OnNetworkListChanged();
On 2014/05/09 18:46:15, stevenjb wrote:
> Comment

Done.

stevenjb

Replying quickly to questions, will try to look at the changes tomorrow (unfortunately combining changes ...

6 years, 7 months ago (2014-05-13 00:18:06 UTC) #26

Replying quickly to questions, will try to look at the changes tomorrow
(unfortunately combining changes with the rename means no relative diffs).

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/wifi_client.h (right):

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_client.h:44: const std::string&
password,
On 2014/05/12 23:54:12, Noam Samuel wrote:
> On 2014/05/09 18:46:15, stevenjb wrote:
> > Is this API only ever going to work with PSK networks that only require a
> > passphrase, or might we want to support other types of networks, e.g.
802.11x,
> > in which case we should probably use a "Credentials" struct here.
> 
> Revision 1 of GCD bootstrapping is designed for PSK networks only, since that
> covers most consumer use cases. As such, I don't want to spend too much time
on
> 802.11X networks, but it might make sense to change this to a struct that can
be
> expanded later.

I mention it because changing the signature of an API later can be kind of a
pain. As long as it is commented reasonably, it can certainly be expanded later
if necessary.

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

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:54:
base::Unretained(this)));
On 2014/05/12 23:54:12, Noam Samuel wrote:
> On 2014/05/09 18:46:15, stevenjb wrote:
> > Rather than using Unretained (which implies that this might be unsafe), make
> > OnNetworkListChangedEvent a local function (since it doesn't actually have
any
> > class dependencies).
> 
> OnNetworkListChangedEvent does have class dependencies (service_client_ in
this
> version or network_list_update_). I could bind them and make it a local
> function, but I think that's somewhat clunky.
> 
> Also, since this class manages WifiService, the use of Unretained doesn't
imply
> lack of safety. I can use a weak pointer, but I think it's unnecessary and
> implies more complexity than exists.

My bad, I didn't read OnNetworkListChangedEvent closely enough. And you're
correct, since wifi_service_ is owned, Unretained is fine.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:133:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/12 23:54:12, Noam Samuel wrote:
> On 2014/05/08 21:54:06, tbarzic wrote:
> > just a warning:
> > StartConnect succeeding does not necessary mean that the connect request
> > succeeded; the network may still fail to connect.
> 
> Could you expand on that? Will the network either have connected or failed to
> connect, or could the connection state of the network only be apparent later? 

I'm less familiar with WifiService, but connecting to a service can take a
while, so StartConnect generally just sends a connection request to the system.
The state at this point may or may not even have changed to "Connecting" yet. An
error would generally only indicate that the service is unknown or unconfigured
so a connect request would be guaranteed to fail.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/wifi_service_client.h (right):

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.h:48: virtual void
GetSSIDList(const SSIDListCallback& callback) OVERRIDE;
On 2014/05/12 23:54:12, Noam Samuel wrote:
> On 2014/05/09 18:46:15, stevenjb wrote:
> > WS
> 
> What does this mean?
White Space (all methods should have a blank line between them).

Noam Samuel

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_client.h File chrome/browser/local_discovery/wifi/wifi_client.h (right): https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_client.h#newcode44 chrome/browser/local_discovery/wifi/wifi_client.h:44: const std::string& password, On 2014/05/13 00:18:06, stevenjb wrote: > ...

6 years, 7 months ago (2014-05-13 23:51:54 UTC) #27

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/wifi_client.h (right):

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_client.h:44: const std::string&
password,
On 2014/05/13 00:18:06, stevenjb wrote:
> On 2014/05/12 23:54:12, Noam Samuel wrote:
> > On 2014/05/09 18:46:15, stevenjb wrote:
> > > Is this API only ever going to work with PSK networks that only require a
> > > passphrase, or might we want to support other types of networks, e.g.
> 802.11x,
> > > in which case we should probably use a "Credentials" struct here.
> > 
> > Revision 1 of GCD bootstrapping is designed for PSK networks only, since
that
> > covers most consumer use cases. As such, I don't want to spend too much time
> on
> > 802.11X networks, but it might make sense to change this to a struct that
can
> be
> > expanded later.
> 
> I mention it because changing the signature of an API later can be kind of a
> pain. As long as it is commented reasonably, it can certainly be expanded
later
> if necessary.
> 

Added struct.

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

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:133:
content::BrowserThread::PostTask(content::BrowserThread::UI,
On 2014/05/13 00:18:06, stevenjb wrote:
> On 2014/05/12 23:54:12, Noam Samuel wrote:
> > On 2014/05/08 21:54:06, tbarzic wrote:
> > > just a warning:
> > > StartConnect succeeding does not necessary mean that the connect request
> > > succeeded; the network may still fail to connect.
> > 
> > Could you expand on that? Will the network either have connected or failed
to
> > connect, or could the connection state of the network only be apparent
later? 
> 
> I'm less familiar with WifiService, but connecting to a service can take a
> while, so StartConnect generally just sends a connection request to the
system.
> The state at this point may or may not even have changed to "Connecting" yet.
An
> error would generally only indicate that the service is unknown or
unconfigured
> so a connect request would be guaranteed to fail.

Refactored the code to be asynchronous and use a timeout , returning the user to
their old network upon failure.

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
File chrome/browser/local_discovery/wifi/wifi_service_client.h (right):

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.h:48: virtual void
GetSSIDList(const SSIDListCallback& callback) OVERRIDE;
On 2014/05/13 00:18:06, stevenjb wrote:
> On 2014/05/12 23:54:12, Noam Samuel wrote:
> > On 2014/05/09 18:46:15, stevenjb wrote:
> > > WS
> > 
> > What does this mean?
> White Space (all methods should have a blank line between them).
> 

Done.

stevenjb

Huge apologies for the delay on this. I got hit by a bunch of things ...

6 years, 7 months ago (2014-05-15 18:16:26 UTC) #28

tbarzic

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_service_client.cc File chrome/browser/local_discovery/wifi/wifi_service_client.cc (right): https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_discovery/wifi/wifi_service_client.cc#newcode240 chrome/browser/local_discovery/wifi/wifi_service_client.cc:240: base::Bind(&WifiServiceContainer::Start, wifi_container_->AsWeakPtr())); On 2014/05/12 23:54:12, Noam Samuel wrote: > ...

6 years, 7 months ago (2014-05-15 20:50:07 UTC) #29

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

https://codereview.chromium.org/226883002/diff/80001/chrome/browser/local_dis...
chrome/browser/local_discovery/wifi/wifi_service_client.cc:240:
base::Bind(&WifiServiceContainer::Start, wifi_container_->AsWeakPtr()));
On 2014/05/12 23:54:12, Noam Samuel wrote:
> On 2014/05/08 21:54:06, tbarzic wrote:
> > AsWeakPtr is no particularly useful here:
> > AsWeakPtr will return a weak ptr bound to the current thread, which is not
the
> > thread it's used on; so this the weak ptr being set will not guarantee that
> > wifi_container is actually alive.
> > 
> > (this will probably crash on debug build)
> 
> Are you sure about this?
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> doesn't say anything about the thread the WeakPtr is issued on, it says that
the
> WeakPtr is bound to the thread it's dereferenced on.
> 
> I get the weak ptr on UI thread, but dereference and invalidate it only on the
> file thread. If the constraints in the comments of weak_ptr are accurate, that
> should be a valid way to use weak pointers.

yeah, looks like I was wrong about what's allowed.
Though, getting weak ptr on UI thread and later using it on FILE thread still
seems fragile, and not really useful in this case. It just makes ownership model
for the wrapper more confusing.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right):

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:173: void
WifiServiceWrapper::ConnectToNetwork(
how is this going to be used as opposed to ConnectToNetworkByID?

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:200:
wifi_service_->SetProperties(
SetProperties for setting passphrase won't work on Windows.
Use CreateNetwork instead (which could probably renamed to CreateConfiguration)
Note that it's ok for this to fail in case the network is already configured.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:353:
wifi_service_->GetKeyFromSystem(network_guid, &key, &error_string);
I'm not sure this will work from browser process.
afaik this requires (utility) process with elevated privilege.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:413:
std::vector<NetworkProperties> ssid_list;
how about:
return !network_guid.empty() && network_guid == GetConnectedGUID();

Noam Samuel

Publishing questions I have first. Will update CL soon. https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_discovery/wifi/wifi_manager.h File chrome/browser/local_discovery/wifi/wifi_manager.h (right): https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_discovery/wifi/wifi_manager.h#newcode80 chrome/browser/local_discovery/wifi/wifi_manager.h:80: ...

6 years, 7 months ago (2014-05-16 17:56:02 UTC) #30

Publishing questions I have first. Will update CL soon.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager.h (right):

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:80: // it changes.
On 2014/05/15 18:16:26, stevenjb wrote:
> It's not clear to me how this is intended to be used, and I don't see it
getting
> called in this CL. I think it might be better to leave this call out of this
CL
> and add it when it is being used so that the reviewers can more easily
> understand the intention.
> 
> Usually I would expect to see an implementer inherit from NetworkListObserver
> and Add/Remove itself from the observed class (WifiManager).

This is an RAII observer tied to a callback. An object of some other class would
call this with a callback, and get back a scoped_ptr it can store. Then, when
the object is destroyed, the callback will automatically stop being called.
Maybe "observer" is the wrong word, does "watcher" make more sense?

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right):

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:353:
wifi_service_->GetKeyFromSystem(network_guid, &key, &error_string);
On 2014/05/15 20:50:07, tbarzic wrote:
> I'm not sure this will work from browser process.
> afaik this requires (utility) process with elevated privilege.

This does work on OSX but not on Windows. There's a larger discussion of what
needs to be done on Windows (in terms of IPC security and reuse), so my instinct
is to put a NOTIMPLEMENTED() for Windows in this CL and write the Windows
implementation in a different CL.

Does this make sense?

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:413:
std::vector<NetworkProperties> ssid_list;
On 2014/05/15 20:50:07, tbarzic wrote:
> how about:
> return !network_guid.empty() && network_guid == GetConnectedGUID();

There's a slight neuance that I should maybe comment on: At least on OSX, at
certain times during the connection phase, both the old and new networks may at
some point seem connected, so writing the code like this ensures that we
correctly detect the network as connected.

stevenjb

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_discovery/wifi/wifi_manager.h File chrome/browser/local_discovery/wifi/wifi_manager.h (right): https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_discovery/wifi/wifi_manager.h#newcode80 chrome/browser/local_discovery/wifi/wifi_manager.h:80: // it changes. On 2014/05/16 17:56:02, Noam Samuel wrote: ...

6 years, 7 months ago (2014-05-19 16:29:51 UTC) #31

Noam Samuel

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_discovery/wifi/wifi_manager.h File chrome/browser/local_discovery/wifi/wifi_manager.h (right): https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_discovery/wifi/wifi_manager.h#newcode28 chrome/browser/local_discovery/wifi/wifi_manager.h:28: virtual void Start() = 0; On 2014/05/15 18:16:26, stevenjb ...

6 years, 7 months ago (2014-05-20 21:19:52 UTC) #32

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager.h (right):

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:28: virtual void Start() = 0;
On 2014/05/15 18:16:26, stevenjb wrote:
> Please comment.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:39: class WifiManager {
On 2014/05/15 18:16:26, stevenjb wrote:
> Please add a comment describing the purpose of this class.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:52: virtual void Start() = 0;
On 2014/05/15 18:16:26, stevenjb wrote:
> Please comment.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:59: // is initiated and may
be null.
On 2014/05/15 18:16:26, stevenjb wrote:
> Since this is just requesting a scan, does it really need a callback? Since
> there is no success or failure indication, and the scan itself is
asynchronous,
> it doesn't seem like it would be useful.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:62: // Connect to a network
with a given SSID and password. |callback| will be
On 2014/05/15 18:16:26, stevenjb wrote:
> This should probably say "Configure and connect to a network". I might also
> optionally suggest naming this ConfigureNetworkAndConnect.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:76: const
CredentialsCallback& callback) = 0;
On 2014/05/15 18:16:26, stevenjb wrote:
> My first assumption was that this was caching the credentials which seemed
bad.
> I would consider renaming this RequestNetworkCredentials, and would definitely
> put "from the system" somewhere in the comment.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:80: // it changes.
On 2014/05/19 16:29:52, stevenjb wrote:
> On 2014/05/16 17:56:02, Noam Samuel wrote:
> > On 2014/05/15 18:16:26, stevenjb wrote:
> > > It's not clear to me how this is intended to be used, and I don't see it
> > getting
> > > called in this CL. I think it might be better to leave this call out of
this
> > CL
> > > and add it when it is being used so that the reviewers can more easily
> > > understand the intention.
> > > 
> > > Usually I would expect to see an implementer inherit from
> NetworkListObserver
> > > and Add/Remove itself from the observed class (WifiManager).
> > 
> > This is an RAII observer tied to a callback. An object of some other class
> would
> > call this with a callback, and get back a scoped_ptr it can store. Then,
when
> > the object is destroyed, the callback will automatically stop being called.
> > Maybe "observer" is the wrong word, does "watcher" make more sense?
> 
> Hmm. I'm not convinced that this would be a better model here than just making
> "some other class" the observer and Add/Remove itself. Again, it would be
easier
> to judge if this were added when it is implemented. "Watcher" would be better
> than Observer, but it's really more of a "CallbackWrapper".

Named to watcher because it's in line with ServiceWatcher, used in the (somewhat
related) service discovery code.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:82: const SSIDListCallback&
callback) = 0;
On 2014/05/15 18:16:26, stevenjb wrote:
> DISALLOW_COPY_AND_ASSIGN

Causes compile errors. Also, unnecessary for abstract class?

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right):

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:13: using
wifi::WiFiService;
On 2014/05/15 18:16:26, stevenjb wrote:
> nit: ::wifi::WiFiService would make the need for this a little more clear
(since
> the rest of the file is in local_discovery::wifi)

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:56: class
WifiServiceWrapper {
On 2014/05/15 18:16:26, stevenjb wrote:
> I think this and NetworkListObserverNonChromeos would be more clear as member
> classes of WifiManagerNonChromeos (you can still forward declare them in the
> header, just do it inside the class).
> 
> This would allow you to make WifiManagerNonChromeos::Add/RemoveObserver
private
> (it is confusing to have public methods that are not actually publicly
usable),
> and would make it a little more clear that these are implementation details of
> WifiManagerNonChromeos.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:69: const
std::string& password,
On 2014/05/15 18:16:26, stevenjb wrote:
> Take Credentials or name ConnectToPskNetwork.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:106:
base::WeakPtrFactory<WifiServiceWrapper> weak_factory_;
On 2014/05/15 18:16:26, stevenjb wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:118: }
On 2014/05/15 18:16:26, stevenjb wrote:
> optional nit: Usually in chrome we don't use {} around single line if
> statements.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:126: }
On 2014/05/15 18:16:26, stevenjb wrote:
> ditto

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:173: void
WifiServiceWrapper::ConnectToNetwork(
On 2014/05/15 20:50:07, tbarzic wrote:
> how is this going to be used as opposed to ConnectToNetworkByID?

This is for connecting to networks that are not yet necessarily configured, it
configures the network and then connects to it.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:181: std::string
error_string;
On 2014/05/15 18:16:26, stevenjb wrote:
> nit: declare these closer to where they are used

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:200:
wifi_service_->SetProperties(
On 2014/05/15 20:50:07, tbarzic wrote:
> SetProperties for setting passphrase won't work on Windows.
> Use CreateNetwork instead (which could probably renamed to
CreateConfiguration)
> Note that it's ok for this to fail in case the network is already configured.

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:241:
IsConnected(connecting_network_guid_)) {
On 2014/05/15 18:16:26, stevenjb wrote:
> reverse logic and early exit

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:276: bool
connected = false;
On 2014/05/15 18:16:26, stevenjb wrote:
> declare on line 287 when first assigned/used

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:293:
base::Bind(post_callback_, base::Bind(callback, !error && connected)));
On 2014/05/15 18:16:26, stevenjb wrote:
> return;

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:294: } else {
On 2014/05/15 18:16:26, stevenjb wrote:
> no else

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:316: }
On 2014/05/15 18:16:26, stevenjb wrote:
> optional: no {}

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:336: std::string
key;
On 2014/05/15 18:16:26, stevenjb wrote:
> declare where used

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:403: i++) {
On 2014/05/15 18:16:26, stevenjb wrote:
> ++i (nit: for iterators, 'iter' or 'it' is more commonly used in Chrome)

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:418: i++) {
On 2014/05/15 18:16:26, stevenjb wrote:
> ++i

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:447:
wifi_wrapper_ = new WifiServiceWrapper(
On 2014/05/15 18:16:26, stevenjb wrote:
> It is confusing that we allocate wifi_wrapper_ on the UI thread and delete it
on
> the FILE thread. Even though this might be safe here, it's much harder to
verify
> that. If it is necessary (or much easier) to do it this way, please comment
why
> and why it is safe.
> 
> At a glance, it seems like wifi_wrapper_ should have a WeakPtr and use
> DeleteSoon on wifi_service_.

Added comment.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h (right):

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:23: 
On 2014/05/15 18:16:26, stevenjb wrote:
> nit: no WS between declarations

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:33: 
On 2014/05/15 18:16:26, stevenjb wrote:
> nit: either no WS between method declarations (my preference for overrides) or
> WS between each (i.e. be consistent).

Done.

https://codereview.chromium.org/226883002/diff/150001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:68:
base::WeakPtrFactory<WifiManagerNonChromeos> weak_factory_;
On 2014/05/15 18:16:26, stevenjb wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

stevenjb

Sorry for the delay, been a week. You should definitely wait on an OK from ...

6 years, 7 months ago (2014-05-23 16:42:39 UTC) #33

Vitaly Buka (NO REVIEWS)

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode106 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:106: std::string connected_network_guid_; // SSID of previously connected network. // ...

6 years, 7 months ago (2014-05-23 17:49:16 UTC) #34

mef

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode182 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:182: scoped_ptr<base::DictionaryValue> properties = MakeProperties(ssid, password); You also need a ...

6 years, 7 months ago (2014-05-23 18:22:49 UTC) #35

Noam Samuel

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_discovery/wifi/wifi_manager.h File chrome/browser/local_discovery/wifi/wifi_manager.h (right): https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_discovery/wifi/wifi_manager.h#newcode33 chrome/browser/local_discovery/wifi/wifi_manager.h:33: virtual void Start() = 0; On 2014/05/23 16:42:40, stevenjb ...

6 years, 7 months ago (2014-05-24 00:15:38 UTC) #36

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager.h (right):

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:33: virtual void Start() = 0;
On 2014/05/23 16:42:40, stevenjb wrote:
> Who is responsible for calling Start()?

Obsolete. See below.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:70: // password. |callback|
will be called once the network is connected or after
On 2014/05/23 16:42:40, stevenjb wrote:
> s/password/credentials

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:76: // Connect to a network
with a given network ID. |callback| will be called
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: s/Connect to a network/Connect to a configured network/

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager.h:92: const SSIDListCallback&
callback) = 0;
On 2014/05/23 16:42:40, stevenjb wrote:
> I'm still not convinced it is useful to make this part of WifiManager (vs. the
> more conventional pattern of exposing Add/RemoveObserver here and leaving
> Observer management to the implementation class), and I would encourage
leaving
> this out until it is implemented somewhere, but I won't object based on what
is
> arguably a personal preference.

Gave it some more thought and in this case RAII observers complicate the code
without adding too much benefit. Changed to normal observers.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right):

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:48: void
OnNetworkListChanged(const std::vector<NetworkProperties>& ssid_list);
On 2014/05/23 16:42:40, stevenjb wrote:
> I think this class would be better named NetworkListObserver since this *is*
an
> Observer, with OnNetworkListChanged the observer method. It took me a while to
> follow this, which is why I'm not entirely happy with the whole "Watcher"
layer;
> I think it just makes the code harder to follow.
> 
> At least add a comment here to the effect that this is the Observer method
> called by WifiManagerNonChromeos.

Obsolete, see above.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:106: std::string
connected_network_guid_;  // SSID of previously connected network.
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> // SSID of network we are connecting to.
> std::string connecting_network_guid_;

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:114: 
On 2014/05/23 16:42:40, stevenjb wrote:
> Either move the implementations just below each class, or add a comment
> separating each implementation.

Obsolete, see above.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:174:
base::Bind(post_callback_, base::Bind(callback, network_property_list)));
On 2014/05/23 16:42:40, stevenjb wrote:
> This requires copying the entire vector in the binding. If SSIDListCallback
were
> to take a scoped_vector, then we can just pass ownership of
> network_property_list to the callback using base::Passed.

I assume you mean scoped_ptr<std::vector>, and not scoped_vector? Either way,
changed internal callback structure without changing interface to achieve the
same goal.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:182:
scoped_ptr<base::DictionaryValue> properties = MakeProperties(ssid, password);
On 2014/05/23 18:22:50, mef wrote:
> You also need a to pass in wifi security type:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/wifi/wi...

Done-ish. For now this'll work since this method is going to be used mostly to
connect to unsecured networks created by devices. I'll go back soon and expand
it to add a security type (added TODO).

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:187: false,
properties.Pass(), &network_guid, &error_string);
On 2014/05/23 16:42:40, stevenjb wrote:
> Is this a no-op if a matching network is already configured? If so please add
a
> comment, otherwise we should maybe do the search for an existing configured
> network first?
> 

Added comment

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:199:
network_property_list.begin();
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> with indexes it's just one line for statement

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:202: if
(it->ssid == ssid) {
On 2014/05/23 16:42:40, stevenjb wrote:
> On Chrome OS it is possible to have more than one WiFi service with the same
> SSID if the security type is different (e.g. an open "Foo" network and a "Foo"
> network with WPA security). This is unusual, but possible. If WifService
already
> addresses that, maybe add a comment here, otherwise we should add a TODO or
> comment that we are explicitly disregarding that case.

Added TODO

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:209: error =
true;  // Could not create the network but it is not configured.
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: s/but/and

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:228:
base::Bind(post_callback_, base::Bind(callback, !error)));
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: s/!error/false /* success *//
> 

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:293:
base::Bind(post_callback_, base::Bind(callback, !error && connected)));
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: s/!error && connected/!error/

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:320:
content::BrowserThread::UI,
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> Extract following into method.
> content::BrowserThread::PostTask(
> content::BrowserThread::UI,
> FROM_HERE,
> base::Bind(post_callback_, ...
> 

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:323:
base::Bind(connect_success_callback_, connected)));
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> maybe save some callback_runner_ in constructor instead of explicit
> content::BrowserThread::UI calls

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:338:
base::Bind(callback, false, std::string(), std::string())));
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: maybe declare error, ssid, and key above the #if and move the PostTask
> outside the #if block

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:340:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> move outside DCHECK FILE?

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:413: }
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: {} unnecessary.

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:429: }
On 2014/05/23 16:42:40, stevenjb wrote:
> nit: {} unnecessary.

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h (right):

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:24: explicit
WifiManagerNonChromeos();
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> -explicit

Done.

https://codereview.chromium.org/226883002/diff/190001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:59:
WifiServiceWrapper* wifi_wrapper_;
On 2014/05/23 17:49:16, Vitaly Buka wrote:
> scoped_ptr<WifiServiceWrapper>

Deleted on a different thread. Added comment. Originally had special scoped_ptr
but removed it due to review comment.

stevenjb

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode53 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:53: ssid_list)> NetworkListCallbackCallback; nit: I think NetworkListPostCallback might be more ...

6 years, 7 months ago (2014-05-27 16:49:07 UTC) #37

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right):

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:53: ssid_list)>
NetworkListCallbackCallback;
nit: I think NetworkListPostCallback might be more clear.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:106: // SSID of
network we are connecting to.
nit: WS above, maybe put the comment for connected_network_guid_ above it also
(with WS) for consistency. (but see next comment).

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:122:
network_list_post_callback_(network_list_post_callback),
nit: This is only a suggestion, I haven't carefully mapped out any
thread-saftery issues but it seems like it would be fine: Rather than pass three
separate callbacks to the Wrapper, maybe just pass a WeakPtr to
WifiManagerNonChromeos to this class and call its methods directly. Since this
class is a member of WifiManagerNonChromeos, it can access private methods of
WifiManagerNonChromeos.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:170: 
nit: if (error_string.empty) { ConnectToNetworkByID(network_guid, callback);
return; }

It adds a call to ConnectToNetworkByID, but makes the rest of the function
easier to read.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:190: error =
true;  // Could not create the network and it is not configured.
nit: If this whole if (!error_string.empty()) {} clause were put into a helper
method, we can early return 'false' here and below, making this a little bit
more readable, e.g.

if (GetExistingNetwork(&network_guid))
  ConnectToNetworkByID(network_guid, callback);
else
  PostClosure(base::Bind(callback, false /* success */));

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:259:
wifi_service_->StartConnect(connected_network_id, &error_string);
nit: PostClosure(base::Bind(callback, false /* success */)); return;

That way we can elim 'error' and the non-trivial IsConnected() call below.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:262: bool
connected = IsConnected(network_guid);
nit: if (IsConnected(network_guid)) { PostClosure(base::Bind(callback, true /*
success */)); return; }

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:287:
wifi_service_->StartConnect(connected_network_guid_, &error_string);
We should add a comment here. 'connected' vs. 'connecting' is subtle and I was
confused for a while (again I think) as to what this was doing.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:291:
PostClosure(base::Bind(connect_success_callback_, connected));
Add /* success */ comment, or maybe just rename 'connected' -> 'success'.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:332:
PostClosure(base::Bind(callback, !error, ssid, key));
Fix indent.
nit: 'error' -> 'success' (since that's what gets passed to 'callback')

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:355: continue;
nit: In general we shouldn't handle NOTREACHED code. If network_value == NULL we
will crash, which is fine. Otherwise we should either LOG(ERROR) and continue,
or leave the NOTREACHED and add the entry with bogus properties (i.e. remove the
continue).

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h (right):

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:42: // network
list changes.
Add/RemoveNetworkListObserver are now part of the base class, so would be more
clear to group them with the other OVERRIDEs and rely on comments in the base
class. (i.e remove this comment and the next).

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:52: // Called
when the network list changes. Used for NetworkListWatcher.
'Used by WifiServiceWrapper.'

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:54:
scoped_ptr<std::vector<NetworkProperties> > ssid_list);
nit: std::vector<NetworkProperties> is used enough that we should maybe typedef
it (e.g. NetworkPropertiesVector). It's not any shorter, but is a little easier
to read, especially with scoped_ptr<>.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:68: // Owned.
Deleted on file thread.
nit: Put this comment after the variable, or a blank line above it to make it
clear what it refers to.

Noam Samuel

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode53 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:53: ssid_list)> NetworkListCallbackCallback; On 2014/05/27 16:49:08, stevenjb wrote: > nit: ...

6 years, 7 months ago (2014-05-27 22:24:07 UTC) #38

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right):

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:53: ssid_list)>
NetworkListCallbackCallback;
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: I think NetworkListPostCallback might be more clear.

Made obsolete by the next comment.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:106: // SSID of
network we are connecting to.
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: WS above, maybe put the comment for connected_network_guid_ above it also
> (with WS) for consistency. (but see next comment).

Looks like it fits next to the variable now.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:122:
network_list_post_callback_(network_list_post_callback),
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: This is only a suggestion, I haven't carefully mapped out any
> thread-saftery issues but it seems like it would be fine: Rather than pass
three
> separate callbacks to the Wrapper, maybe just pass a WeakPtr to
> WifiManagerNonChromeos to this class and call its methods directly. Since this
> class is a member of WifiManagerNonChromeos, it can access private methods of
> WifiManagerNonChromeos.

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:170: 
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: if (error_string.empty) { ConnectToNetworkByID(network_guid, callback);
> return; }
> 
> It adds a call to ConnectToNetworkByID, but makes the rest of the function
> easier to read.

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:190: error =
true;  // Could not create the network and it is not configured.
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: If this whole if (!error_string.empty()) {} clause were put into a helper
> method, we can early return 'false' here and below, making this a little bit
> more readable, e.g.
> 
> if (GetExistingNetwork(&network_guid))
>   ConnectToNetworkByID(network_guid, callback);
> else
>   PostClosure(base::Bind(callback, false /* success */));

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:259:
wifi_service_->StartConnect(connected_network_id, &error_string);
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: PostClosure(base::Bind(callback, false /* success */)); return;
> 
> That way we can elim 'error' and the non-trivial IsConnected() call below.

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:262: bool
connected = IsConnected(network_guid);
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: if (IsConnected(network_guid)) { PostClosure(base::Bind(callback, true /*
> success */)); return; }

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:287:
wifi_service_->StartConnect(connected_network_guid_, &error_string);
On 2014/05/27 16:49:08, stevenjb wrote:
> We should add a comment here. 'connected' vs. 'connecting' is subtle and I was
> confused for a while (again I think) as to what this was doing.

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:291:
PostClosure(base::Bind(connect_success_callback_, connected));
On 2014/05/27 16:49:08, stevenjb wrote:
> Add /* success */ comment, or maybe just rename 'connected' -> 'success'.

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:332:
PostClosure(base::Bind(callback, !error, ssid, key));
On 2014/05/27 16:49:08, stevenjb wrote:
> Fix indent.
> nit: 'error' -> 'success' (since that's what gets passed to 'callback')
> 

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:355: continue;
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: In general we shouldn't handle NOTREACHED code. If network_value == NULL
we
> will crash, which is fine. Otherwise we should either LOG(ERROR) and continue,
> or leave the NOTREACHED and add the entry with bogus properties (i.e. remove
the
> continue).

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h (right):

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:42: // network
list changes.
On 2014/05/27 16:49:08, stevenjb wrote:
> Add/RemoveNetworkListObserver are now part of the base class, so would be more
> clear to group them with the other OVERRIDEs and rely on comments in the base
> class. (i.e remove this comment and the next).

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:52: // Called
when the network list changes. Used for NetworkListWatcher.
On 2014/05/27 16:49:08, stevenjb wrote:
> 'Used by WifiServiceWrapper.'

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:54:
scoped_ptr<std::vector<NetworkProperties> > ssid_list);
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: std::vector<NetworkProperties> is used enough that we should maybe
typedef
> it (e.g. NetworkPropertiesVector). It's not any shorter, but is a little
easier
> to read, especially with scoped_ptr<>.
> 

Done.

https://codereview.chromium.org/226883002/diff/210001/chrome/browser/local_di...
chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.h:68: // Owned.
Deleted on file thread.
On 2014/05/27 16:49:08, stevenjb wrote:
> nit: Put this comment after the variable, or a blank line above it to make it
> clear what it refers to.

Done.

stevenjb

LGTM! https://codereview.chromium.org/226883002/diff/230001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/230001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode100 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:100: connecting_network_guid_; // SSID of network we are connecting ...

6 years, 7 months ago (2014-05-27 22:48:03 UTC) #39

Noam Samuel

mef@ and vitalybuka@, can you PTAL? https://codereview.chromium.org/226883002/diff/230001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/230001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode100 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:100: connecting_network_guid_; // SSID ...

6 years, 7 months ago (2014-05-27 23:15:09 UTC) #40

mef

https://codereview.chromium.org/226883002/diff/250001/chrome/browser/local_discovery/wifi/wifi_manager.h File chrome/browser/local_discovery/wifi/wifi_manager.h (right): https://codereview.chromium.org/226883002/diff/250001/chrome/browser/local_discovery/wifi/wifi_manager.h#newcode18 chrome/browser/local_discovery/wifi/wifi_manager.h:18: struct NetworkProperties { would it make sense to make ...

6 years, 6 months ago (2014-05-28 16:28:29 UTC) #42

Noam Samuel

https://codereview.chromium.org/226883002/diff/250001/chrome/browser/local_discovery/wifi/wifi_manager.h File chrome/browser/local_discovery/wifi/wifi_manager.h (right): https://codereview.chromium.org/226883002/diff/250001/chrome/browser/local_discovery/wifi/wifi_manager.h#newcode18 chrome/browser/local_discovery/wifi/wifi_manager.h:18: struct NetworkProperties { On 2014/05/28 16:28:30, mef wrote: > ...

6 years, 6 months ago (2014-05-28 23:04:35 UTC) #43

mef

looks pretty good. https://codereview.chromium.org/226883002/diff/270001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/270001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode131 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:131: base::Bind(&WifiServiceWrapper::OnNetworksChangedEvent, FYI, on Mac this works ...

6 years, 6 months ago (2014-05-30 20:48:32 UTC) #44

Noam Samuel

https://codereview.chromium.org/226883002/diff/270001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc File chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc (right): https://codereview.chromium.org/226883002/diff/270001/chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc#newcode131 chrome/browser/local_discovery/wifi/wifi_manager_nonchromeos.cc:131: base::Bind(&WifiServiceWrapper::OnNetworksChangedEvent, On 2014/05/30 20:48:32, mef wrote: > FYI, on ...

6 years, 6 months ago (2014-05-30 22:04:41 UTC) #45