Base infrastructure for Networking Private API on Windows and Mac.

Base infrastructure for Networking Private API on Windows and Mac.

Based on https://codereview.chromium.org/22295002/, 
but moves WiFiService to components/ and runs it in browser process instead of utility process.

Windows implementation is in http://crrev.com/68503019. 
 

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

[mef, 2013-11-01 20:47:47]

Hi, this CL is based on https://codereview.chromium.org/22295002/.

I've changed WiFiService to run in browser process instead of utility process.
I've also moved it from chrome/utility/wifi to components/wifi.

I've created a separate CL for easier review, but will be happy to merge it back
into this one if that makes more sense.

I haven't yet updated windows-specific implementation to match this.

thanks,
-m

[cbentzel, 2013-11-02 13:58:21]

It looks like this is a diff relative to the "Base Infrastructure" CL rather
than relative to trunk. Intended?

[mef, 2013-11-02 14:23:53]

On 2013/11/02 13:58:21, cbentzel wrote:
> It looks like this is a diff relative to the "Base Infrastructure" CL rather
> than relative to trunk. Intended?

Yes, I thought it would be easier to review this way. 
I could also create a CL combining "base infrastructure" and this if necessary.

I also came full circle on the notion of Utility Process being separate from
password slurping.
If jorgelo agrees that we can spawn utility process under UAC, then it would
make sense to keep "Base Infrastructure",
even though we'll launch separate instance of utility process elevated for short
time.

[mef, 2013-11-05 21:03:02]

On 2013/11/02 14:23:53, mef wrote:
> On 2013/11/02 13:58:21, cbentzel wrote:
> > It looks like this is a diff relative to the "Base Infrastructure" CL rather
> > than relative to trunk. Intended?
> 
> Yes, I thought it would be easier to review this way. 
> I could also create a CL combining "base infrastructure" and this if
necessary.
> 
> I also came full circle on the notion of Utility Process being separate from
> password slurping.
> If jorgelo agrees that we can spawn utility process under UAC, then it would
> make sense to keep "Base Infrastructure",
> even though we'll launch separate instance of utility process elevated for
short
> time.

ping.

[tbarzic, 2013-11-05 21:24:34]

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:241:
wifi_service->GetVisibleNetworks(
why do you need to get visible networks here (a comment would be nice)?

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:309:
void NetworkingPrivateServiceClient::Start() {
wifi_service_ is never set (except in test)

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:313:
BrowserThread::IO,
You don't need this anymore, do you? :)

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:344:
profile_->RemoveUserData(kNetworkingPrivateServiceClient);
Now that there's no utility process bound to the lifetime of |this|, what's the
reason to remove the service client when the last observer is removed (also,
this is inconsistent with the behaviour for the service client method calls,
where the service client will be kept in the user data until shutdown (or the
last observer gets removed))?

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:361:
base::Unretained(wifi_service_.get()),
this is not safe as all references for |this| are on UI thread (in profile's
user data) and |wifi_Service_| is owned by |this|.
On shutdown, the GetProperties task could be run after the profile is destructed
(and thus the NetworkPrivateServiceClient and the wifi_service_ passed here).

Also, I think the ExtensionFunction referenced in |callback| would be leaked if
shutdown occurs while |RunWork| is in progress (as the ExtensionFunction will be
attempt to post deletion task to the UI thread, and this would fail as the UI
thread stopped running).

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:48:
: public RefcountedBrowserContextKeyedService {
It doesn't look like you're actually using this a BrowserContextKeyedService..

[mef, 2013-11-07 16:06:07]

Hi guys, PTAL. I've addressed Toni's comments. 

Unfortunately I think at this point code complexity getting close to one using
utility process without providing benefits of process isolation.

I'm still looking for good opportunity to release wifi_service_ prior to profile
shutdown (maybe when there are no observers and no tasks in tracker).

thanks,
-m

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:241:
wifi_service->GetVisibleNetworks(
On 2013/11/05 21:24:34, tbarzic wrote:
> why do you need to get visible networks here (a comment would be nice)?

Done.

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:309:
void NetworkingPrivateServiceClient::Start() {
On 2013/11/05 21:24:34, tbarzic wrote:
> wifi_service_ is never set (except in test)

Done.

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:313:
BrowserThread::IO,
On 2013/11/05 21:24:34, tbarzic wrote:
> You don't need this anymore, do you? :)

Done.

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:344:
profile_->RemoveUserData(kNetworkingPrivateServiceClient);
On 2013/11/05 21:24:34, tbarzic wrote:
> Now that there's no utility process bound to the lifetime of |this|, what's
the
> reason to remove the service client when the last observer is removed (also,
> this is inconsistent with the behaviour for the service client method calls,
> where the service client will be kept in the user data until shutdown (or the
> last observer gets removed))?

Done.

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:361:
base::Unretained(wifi_service_.get()),
On 2013/11/05 21:24:34, tbarzic wrote:
> this is not safe as all references for |this| are on UI thread (in profile's
> user data) and |wifi_Service_| is owned by |this|.
> On shutdown, the GetProperties task could be run after the profile is
destructed
> (and thus the NetworkPrivateServiceClient and the wifi_service_ passed here).
> 
> Also, I think the ExtensionFunction referenced in |callback| would be leaked
if
> shutdown occurs while |RunWork| is in progress (as the ExtensionFunction will
be
> attempt to post deletion task to the UI thread, and this would fail as the UI
> thread stopped running).

Done. Added scoped_refptr<NetworkingPrivateServiceClient> to WiFiServiceHelper
to keep it around while tasks are running.

Also added CancellableTaskTracker to track in-flight tasks and cancel them from
ShutdownOnUIThread to release ExtensionFunction referenced in |callback|.

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:48:
: public RefcountedBrowserContextKeyedService {
On 2013/11/05 21:24:34, tbarzic wrote:
> It doesn't look like you're actually using this a BrowserContextKeyedService..

Done. It now uses NetworkingPrivateServiceClientFactory to get instance for
profile and ShutdownOnUIThread to cancel all pending tasks.

[Jorge Lucangeli Obes, 2013-11-08 22:25:37]

Why do you think this is more complicated than before?

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:21:
using content::UtilityProcessHost;
Still need this?

[mef, 2013-11-08 22:35:56]

jorgelo@ - I'm not saying that it is more complicated than with utility process,
but it seems that some of the extra complexity is added to cover async execution
and memory management. Removing utility process resulted in adding similar
complexity in different form.

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:21:
using content::UtilityProcessHost;
On 2013/11/08 22:25:38, Jorge Lucangeli Obes wrote:
> Still need this?

Done.

[tbarzic, 2013-11-09 03:09:40]

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:249:
// Get current list of Visible Networks, so |NetworkScanReply| would emit
Why would we need to emit the event? If the network list changes during scan,
the event would be emitted regardless.
And if the list does not change, I don't think any event should be emitted.

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h:18:
: public RefcountedBrowserContextKeyedServiceFactory {
you should make NetworkingPrivateEventRouter service depend on this one, as it
depends on ExtensionSystem

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:114:
class WiFiServiceHelper {
I really don't see a benefit of having these helper classes.
Wouldn't it be simpler just to have this logic in
NetworkingPrivateServiceClient?

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:302:
task_tracker_->TryCancelAll();
If you added this to prevent ExtensionFunctions from leaking (if there are
wifi_service_ requests in progress during shutdown), I am not sure this helps.
This will not actually release callbacks, it will just prevent them from
running.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:306:
wifi_service_.reset(wifi::WiFiService::CreateService());
Should WifiService::CreateService be called on the worker thread?

The same question for ~WiFiService (there's no guarantee on which thread
NetworkingPrivateServiceClient will get destructed)

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:310:
wifi_service_->SetNetworkListChangedObserver(
Comment why Unretained is used (and why it's safe).

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:340:
task_tracker_->PostTaskAndReply(
Hm, as far as I can see, wifi service methods are async, which means that it's
not guaranteed that the helper will be done before Reply is called.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:450:
base::Unretained(crypto_verify_.get()),
you have a race here as well.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:493:
void NetworkingPrivateServiceClient::OnNetworksChangedEvent(
you should ensure this is called on the UI thread.

[cbentzel, 2013-11-09 16:43:43]

I think a lot of the complexity revolves around posting callbacks that refer to
the ExtensionFunction's on worker threads and not being able to clean them up
easily.

Here's a sketch of what I think we should do.

- Make NetworkingPrivateServiceClient a non-refcounted BrowserContextService
that lives only on the UI thread. It will maintain a map of ids to the callbacks
which are passed in from the ExtensionFunction implementations. What's nice
about this is that when the NetworkingPrivateServiceClient is destroyed during
BrowserContext shutdown, it will also release the only references to the
ExtensionFunctions and make sure they are deleted as well.

- You can still have the WiFiService class which also only really lives on the
UI thread and is owned by the NetworkingPrivateServiceClient (and has
per-platform implementations). When work needs to take place on a background
thread, it can be straight function calls that do not refer to WiFiService - any
state that is interesting needs to be handled as bound arguments to the
functions running on the background thread. When these complete on the
background thread, the reply happens on the UI thread but uses a WeakPtr to
either the WiFiService or the NetworkingPrivateServiceClient, depending on how
you want the boundaries between them.

So, let's say we have something like a GetProperties extension function call. A
GetPropertiesExtensionFunction is created. It will also instantiate a
NetworkingPrivateServiceClient for the profile if one does not already exist.
The GetPropertiesExtensionFunction calls
NetworkingPrivateServiceClient::GetProperties() which takes a callback that
holds a reference to the GetPropertieExtensionFunction (it may actually take two
for the success/error path, but I think this would be simplified if we had one
callback which could handle both). NetworkingPrivateServiceClient will then
create a per-NetworkingPrivateServiceClient unique ID and insert a mapping from
the ID to the passed-in callback in the map that it owns. It creates a new
callback which has the unique ID bound and a weak pointer to the
NetworkingPrivateServiceClient, and passes this into WiFiService::GetProperties.
On Windows, this implementation will PostTask to a worker thread to
GetProperties, and pass in the callback from the NetworkingPrivateServiceClient.
When the worker thread task finishes, it replies back to the UI thread with the
NetworkingPrivateServiceClient callback. If the NetworkingPrivateServiceClient
hasn't been destroyed yet, it will look up the original
GetPropertiesExtensionFunction callback based on the bound ID, remove it from
the map, and invoke it with the result of the WiFiService one. If the
NetworkingPrivateServiceClient is destroyed while work is going on in the
background thread, it will also destroy the map of callbacks that references the
GetPropertiesExtensionFunction and delete as well - and since the callbacks
passed into the WiFiService have a weak pointer to
NetworkingPrivateServiceClient they will be effectively canceled.

Although this adds a bit of complexity due to the need for the callback map in
the NetworkingPrivateServiceClient, I think it makes reasoning through
thread-safety and object lifetimes much easier.

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:34:
// NetworkingPrivanteProcessClient::Observer overrides:
NetworkingPrivateServiceClient

[cbentzel, 2013-11-11 16:34:39]

We chatted about this at the whiteboard which is way easier than over email...

On Sat, Nov 9, 2013 at 11:43 AM,  <cbentzel@chromium.org> wrote:
>
> I think a lot of the complexity revolves around posting callbacks that refer
> to
> the ExtensionFunction's on worker threads and not being able to clean them
> up
> easily.
>
> Here's a sketch of what I think we should do.
>
> - Make NetworkingPrivateServiceClient a non-refcounted BrowserContextService
> that lives only on the UI thread. It will maintain a map of ids to the
> callbacks
> which are passed in from the ExtensionFunction implementations. What's nice
> about this is that when the NetworkingPrivateServiceClient is destroyed
> during
> BrowserContext shutdown, it will also release the only references to the
> ExtensionFunctions and make sure they are deleted as well.
>
> - You can still have the WiFiService class which also only really lives on
> the
> UI thread and is owned by the NetworkingPrivateServiceClient (and has
> per-platform implementations). When work needs to take place on a background
> thread, it can be straight function calls that do not refer to WiFiService -
> any
> state that is interesting needs to be handled as bound arguments to the
> functions running on the background thread. When these complete on the
> background thread, the reply happens on the UI thread but uses a WeakPtr to
> either the WiFiService or the NetworkingPrivateServiceClient, depending on
> how
> you want the boundaries between them.
>
> So, let's say we have something like a GetProperties extension function
> call. A
> GetPropertiesExtensionFunction is created. It will also instantiate a
> NetworkingPrivateServiceClient for the profile if one does not already
> exist.
> The GetPropertiesExtensionFunction calls
> NetworkingPrivateServiceClient::GetProperties() which takes a callback that
> holds a reference to the GetPropertieExtensionFunction (it may actually take
> two
> for the success/error path, but I think this would be simplified if we had
> one
> callback which could handle both). NetworkingPrivateServiceClient will then
> create a per-NetworkingPrivateServiceClient unique ID and insert a mapping
> from
> the ID to the passed-in callback in the map that it owns. It creates a new
> callback which has the unique ID bound and a weak pointer to the
> NetworkingPrivateServiceClient, and passes this into
> WiFiService::GetProperties.
> On Windows, this implementation will PostTask to a worker thread to
> GetProperties, and pass in the callback from the
> NetworkingPrivateServiceClient.
> When the worker thread task finishes, it replies back to the UI thread with
> the
> NetworkingPrivateServiceClient callback. If the
> NetworkingPrivateServiceClient
> hasn't been destroyed yet, it will look up the original
> GetPropertiesExtensionFunction callback based on the bound ID, remove it
> from
> the map, and invoke it with the result of the WiFiService one. If the
> NetworkingPrivateServiceClient is destroyed while work is going on in the
> background thread, it will also destroy the map of callbacks that references
> the
> GetPropertiesExtensionFunction and delete as well - and since the callbacks
> passed into the WiFiService have a weak pointer to
> NetworkingPrivateServiceClient they will be effectively canceled.
>
> Although this adds a bit of complexity due to the need for the callback map
> in
> the NetworkingPrivateServiceClient, I think it makes reasoning through
> thread-safety and object lifetimes much easier.
>
>
>
https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
> (right):
>
>
https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:34:
> // NetworkingPrivanteProcessClient::Observer overrides:
> NetworkingPrivateServiceClient
>
> https://codereview.chromium.org/54323003/

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

[mef, 2013-11-11 22:02:53]

Hi guys, PTAL.

Per discussion with Chris I've done the following changes to
NetworkingPrivateServiceClient:

- wifi_service_ and crypto_verify_ are now allocated, accessed and deleted only
on worker thread.
- callbacks from extension api are stored in callbacks_map_ and cleared in
ShutdownOnUIThread.
- callbacks wrappers are called from Replies of WiFiService calls and they use
callbacks_map_ to look up appropriate api callback. 

Please let me know whether this makes sense and if there any other comments.

thanks,
-m

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:34:
// NetworkingPrivanteProcessClient::Observer overrides:
On 2013/11/09 16:43:44, cbentzel wrote:
> NetworkingPrivateServiceClient

Done.

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:249:
// Get current list of Visible Networks, so |NetworkScanReply| would emit
On 2013/11/09 03:09:41, tbarzic wrote:
> Why would we need to emit the event? If the network list changes during scan,
> the event would be emitted regardless.
> And if the list does not change, I don't think any event should be emitted.

Done.

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h
(right):

https://codereview.chromium.org/54323003/diff/300001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h:18:
: public RefcountedBrowserContextKeyedServiceFactory {
On 2013/11/09 03:09:41, tbarzic wrote:
> you should make NetworkingPrivateEventRouter service depend on this one, as it
> depends on ExtensionSystem

Done.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:114:
class WiFiServiceHelper {
On 2013/11/09 03:09:41, tbarzic wrote:
> I really don't see a benefit of having these helper classes.
> Wouldn't it be simpler just to have this logic in
> NetworkingPrivateServiceClient?

They provide storage for call-specific parameters that cannot be trivially bound
(e.g. const WiFiService::NetworkProperties&).

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:302:
task_tracker_->TryCancelAll();
On 2013/11/09 03:09:41, tbarzic wrote:
> If you added this to prevent ExtensionFunctions from leaking (if there are
> wifi_service_ requests in progress during shutdown), I am not sure this helps.
> This will not actually release callbacks, it will just prevent them from
> running.

Done.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:306:
wifi_service_.reset(wifi::WiFiService::CreateService());
On 2013/11/09 03:09:41, tbarzic wrote:
> Should WifiService::CreateService be called on the worker thread?
> 
> The same question for ~WiFiService (there's no guarantee on which thread
> NetworkingPrivateServiceClient will get destructed)

Done.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:310:
wifi_service_->SetNetworkListChangedObserver(
On 2013/11/09 03:09:41, tbarzic wrote:
> Comment why Unretained is used (and why it's safe).
It isn't safe, so I've removed Unretained.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:340:
task_tracker_->PostTaskAndReply(
On 2013/11/09 03:09:41, tbarzic wrote:
> Hm, as far as I can see, wifi service methods are async, which means that it's
> not guaranteed that the helper will be done before Reply is called. 
Wifi service methods are actually synchronous.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:450:
base::Unretained(crypto_verify_.get()),
On 2013/11/09 03:09:41, tbarzic wrote:
> you have a race here as well.

Done. I think. I'm not sure what race did you mean.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:493:
void NetworkingPrivateServiceClient::OnNetworksChangedEvent(
On 2013/11/09 03:09:41, tbarzic wrote:
> you should ensure this is called on the UI thread.

Done.

[cbentzel, 2013-11-12 01:12:25]

I'll do a review in the early morning.

https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:49:
: public RefcountedBrowserContextKeyedService {
This shouldn't need to be RefCounted based on our earlier discussion.

[tbarzic, 2013-11-12 04:02:22]

Hm, I think this could be simplified even more if the way callbacks from
wifi_service are handled is changed.
Instead of saving wifi_service results in a helper class and then invoking the
callback using PostTaskAndReply,
it would be easier to wrap callback in a function that posts it to the right
thread,a and pass that as a callback to the wifi_service. Something like the
pseudocodish example for GetProperties here.

Also, I think it would be ok to leave the NetworkingPrivateServiceClient
refcounted (and used on both UI and worker thread), but be strict on which
thread parts of it can be used.
(e.g. user callbacks map only on UI thread, use wifi_service_ and crypto_verify_
only on the worker thread).
Note that, if you make NetworkingPrivateServiceClient UI-thread only, you'll
probably have to initialize wifi_service in two steps: Construct it on UI
thread, and then post initialization task to the 
worker thread (assuming that the Initialization cannot be run on the UI thread).
You'll also need to wrap the listener to the wifi service to hop to UI thread.

namespace {

/* Instead of this you could have
NetworkingPrivateServiceClient::GetPropertiesOnWorkerThread
  which will do the same (but it would post the callback to BrowserThread::UI)
*/
void RelayGetPropertiesCallback(
     const scoped_refptr<base::MessageLoopProxy>& proxy,
     const DictionaryResultCallback& callback,
     const std::string& network_guid,
     const WiFiService::NetworkProperties& properties) {
  scoped_ptr<base::DictionaryValue> properties_dict = properties->ToValue();
  proxy->PostTask(base::Bind(callback, network_guid,
base::Passed(properties_dict)));
}

void RelayErrorCallback(
     const scoped_refptr<base::MesasgeLoopProxy>& proxy,
     const ErrorCallback& callback,
     const std::string& error_code,
     scoped_ptr<base::DictionaryValue> error_data) {
  proxy->PostTask(base::Bind(callback, error_code, base::Passed(error_data)));
}

}  // namespace

NetworkingPrivateServiceClient::GetProperties(network_guid, callback,
error_callback) {
  callbacks_id = RegisterCallbacks(callback, error_callback);
  
  on_success = 
      base::Bind(
           &RelayGetPropertiesCallback, 
           base::MessageLoopProxy::current(),
           base::Bind(&NetworkingPrivateServiceClient::OnGetProperties,
                           this,
                           callbacks_id));
  on_error =
      base::Bind(
          &RelayErrorCallback,
          base::MessageLoopProxy::current(),
          base::Bind(&NetworkingPrivateServiceClient::OnApiError,
                          this,
                          callbacks_id));

  task_runner_->PostTask(
        &NetworkingPrivateServiceClient::GetPropertiesOnWorkerThread,
        this,
        network_guid,
        on_success,
        on_error);

  /* or maybe, if you want to make NetworkingPrivateServiceClient UI thread-only
object:
    task_runner_->PostTask(&WifiService::GetProperties,
                                                     
base::Unretained(wifi_service_),
                                                      network_guid,
                                                      on_success,
                                                      on_error);
    note that in that case you'd have to bind UI-thread callbacks to a weak_ptr
    and you'd have to make sure that the wifi_service_ is set prior to this.
  */
}

NetworkingPrivateServiceclient::GetPropertiesOnWorkerThread(
    network_guid, callback, error_callback) {
  /* if you plan to reset wifi_service when it's idle, you could have
    if (!wifi_service_)
      wifi_service_ = CreateWifiService();
  */
  /* you could also do base::Bind(&RelayGetPropertiesCallback,
                                              
BrowserThread::GetMessageLoopProxyForThread(BrowserThreadUI),
                                                callback );
      instead of doing this in GetProperties.
  */
  wifi_service_->GetProperties(network_guid, callback, error_callback);
}

NetworkingPrivateServiceClient::OnGetProperties(
    ServiceCallbackID callbacks_id,
    const std::string& network_guid,
    scoped_ptr<base::DictionaryValue> properties) {
  callbacks = LookupCallbacks(callbacks_id);

  if (callbacks && !callbacks->get_properties_callback.is_null())
    callbacks->get_properties_callback->Run(network_guid, *properties);
  RemoveCallbacks(callbacks_id);
}

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:340:
task_tracker_->PostTaskAndReply(
On 2013/11/11 22:02:54, mef wrote:
> On 2013/11/09 03:09:41, tbarzic wrote:
> > Hm, as far as I can see, wifi service methods are async, which means that
it's
> > not guaranteed that the helper will be done before Reply is called. 
> Wifi service methods are actually synchronous.

OK.
I'm a bit concerned that we assume that methods that have async interface are
always synchronous.
It wouldn't be too hard to imagine a refactoring that would make some of these
asynchronous :/

But still I don't think PostTaskAndReply is the way to go here. Wouldn't it just
be easier to post task to the right thread from the WifiService callbacks?

https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:112:
process_client_ =
you don't have to cache the process_client_, you can just use
NetworkingPrivateServiceClientFactory::GetForProfile() when you need it.

[tbarzic, 2013-11-12 04:09:00]

Though, PostTaskAndReply makes more sense with crypto_verify tasks :/

[cbentzel, 2013-11-12 10:52:10]

On 2013/11/12 04:02:22, tbarzic wrote:
> Hm, I think this could be simplified even more if the way callbacks from
> wifi_service are handled is changed.
> Instead of saving wifi_service results in a helper class and then invoking the
> callback using PostTaskAndReply,
> it would be easier to wrap callback in a function that posts it to the right
> thread,a and pass that as a callback to the wifi_service. Something like the
> pseudocodish example for GetProperties here.
> 
> Also, I think it would be ok to leave the NetworkingPrivateServiceClient
> refcounted (and used on both UI and worker thread), but be strict on which
> thread parts of it can be used.
> (e.g. user callbacks map only on UI thread, use wifi_service_ and
crypto_verify_
> only on the worker thread).
> Note that, if you make NetworkingPrivateServiceClient UI-thread only, you'll
> probably have to initialize wifi_service in two steps: Construct it on UI
> thread, and then post initialization task to the 
> worker thread (assuming that the Initialization cannot be run on the UI
thread).
> You'll also need to wrap the listener to the wifi service to hop to UI thread.

The problem with making it RefCounted and on multiple threads is that the
current model relies on the deletion of the NetworkingPrivateServiceClient to
also delete any outstanding ExtensionFunctions at the time. If we don't do that
we'll get potential leaks because worker threads don't complete by the time the
UI thread is destroyed. We'd also want to make sure to destroy the
NetworkingPrivateServiceClient on UI thread only.
> 
> namespace {
> 
> /* Instead of this you could have
> NetworkingPrivateServiceClient::GetPropertiesOnWorkerThread
>   which will do the same (but it would post the callback to BrowserThread::UI)
> */
> void RelayGetPropertiesCallback(
>      const scoped_refptr<base::MessageLoopProxy>& proxy,
>      const DictionaryResultCallback& callback,
>      const std::string& network_guid,
>      const WiFiService::NetworkProperties& properties) {
>   scoped_ptr<base::DictionaryValue> properties_dict = properties->ToValue();
>   proxy->PostTask(base::Bind(callback, network_guid,
> base::Passed(properties_dict)));
> }
> 
> void RelayErrorCallback(
>      const scoped_refptr<base::MesasgeLoopProxy>& proxy,
>      const ErrorCallback& callback,
>      const std::string& error_code,
>      scoped_ptr<base::DictionaryValue> error_data) {
>   proxy->PostTask(base::Bind(callback, error_code, base::Passed(error_data)));
> }
> 
> }  // namespace
> 
> NetworkingPrivateServiceClient::GetProperties(network_guid, callback,
> error_callback) {
>   callbacks_id = RegisterCallbacks(callback, error_callback);
>   
>   on_success = 
>       base::Bind(
>            &RelayGetPropertiesCallback, 
>            base::MessageLoopProxy::current(),
>            base::Bind(&NetworkingPrivateServiceClient::OnGetProperties,
>                            this,
>                            callbacks_id));
>   on_error =
>       base::Bind(
>           &RelayErrorCallback,
>           base::MessageLoopProxy::current(),
>           base::Bind(&NetworkingPrivateServiceClient::OnApiError,
>                           this,
>                           callbacks_id));
> 
>   task_runner_->PostTask(
>         &NetworkingPrivateServiceClient::GetPropertiesOnWorkerThread,
>         this,
>         network_guid,
>         on_success,
>         on_error);
> 
>   /* or maybe, if you want to make NetworkingPrivateServiceClient UI
thread-only
> object:
>     task_runner_->PostTask(&WifiService::GetProperties,
>                                                      
> base::Unretained(wifi_service_),
>                                                       network_guid,
>                                                       on_success,
>                                                       on_error);
>     note that in that case you'd have to bind UI-thread callbacks to a
weak_ptr
>     and you'd have to make sure that the wifi_service_ is set prior to this.
>   */
> }
> 
> NetworkingPrivateServiceclient::GetPropertiesOnWorkerThread(
>     network_guid, callback, error_callback) {
>   /* if you plan to reset wifi_service when it's idle, you could have
>     if (!wifi_service_)
>       wifi_service_ = CreateWifiService();
>   */
>   /* you could also do base::Bind(&RelayGetPropertiesCallback,
>                                               
> BrowserThread::GetMessageLoopProxyForThread(BrowserThreadUI),
>                                                 callback );
>       instead of doing this in GetProperties.
>   */
>   wifi_service_->GetProperties(network_guid, callback, error_callback);
> }
> 
> NetworkingPrivateServiceClient::OnGetProperties(
>     ServiceCallbackID callbacks_id,
>     const std::string& network_guid,
>     scoped_ptr<base::DictionaryValue> properties) {
>   callbacks = LookupCallbacks(callbacks_id);
> 
>   if (callbacks && !callbacks->get_properties_callback.is_null())
>     callbacks->get_properties_callback->Run(network_guid, *properties);
>   RemoveCallbacks(callbacks_id);
> }
> 
>
https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
> (right):
> 
>
https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:340:
> task_tracker_->PostTaskAndReply(
> On 2013/11/11 22:02:54, mef wrote:
> > On 2013/11/09 03:09:41, tbarzic wrote:
> > > Hm, as far as I can see, wifi service methods are async, which means that
> it's
> > > not guaranteed that the helper will be done before Reply is called. 
> > Wifi service methods are actually synchronous.
> 
> OK.
> I'm a bit concerned that we assume that methods that have async interface are
> always synchronous.
> It wouldn't be too hard to imagine a refactoring that would make some of these
> asynchronous :/
> 
> But still I don't think PostTaskAndReply is the way to go here. Wouldn't it
just
> be easier to post task to the right thread from the WifiService callbacks?
> 
>
https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
> (right):
> 
>
https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:112:
> process_client_ =
> you don't have to cache the process_client_, you can just use
> NetworkingPrivateServiceClientFactory::GetForProfile() when you need it.

[tbarzic, 2013-11-12 18:14:46]

The problem with leaking ExtensionFunctions could be solved by overriding
ShutdownOnUIThread to clear the callbacks map (together with the rest of UI
thread specific state).
I don't have a problem with making NetworkingPrivateServiceClient single (UI)
threaded; I'm just saying that, at least as far as I'm concerned, it's not a
strong requirement..

[mef, 2013-11-12 19:51:16]

Hi, PTAL. I bit the bullet and changed WiFiService interface to be explicitly
synchronous, which allowed to get rid of 2 layers of callbacks and all
WiFiServiceHelpers.

I'm going to look into making NetworkingPrivateServiceClient not refcounted, but
yes, all extension functions callbacks are released from ShutdownOnUIThread.

I'm also going to look into changing CryptoVerify interface to better map into
PostTaskAndReply mechanism.

thanks,
-m

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:114:
class WiFiServiceHelper {
On 2013/11/11 22:02:54, mef wrote:
> On 2013/11/09 03:09:41, tbarzic wrote:
> > I really don't see a benefit of having these helper classes.
> > Wouldn't it be simpler just to have this logic in
> > NetworkingPrivateServiceClient?
> 
> They provide storage for call-specific parameters that cannot be trivially
bound
> (e.g. const WiFiService::NetworkProperties&).

Done.

https://codereview.chromium.org/54323003/diff/380001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:340:
task_tracker_->PostTaskAndReply(
On 2013/11/12 04:02:23, tbarzic wrote:
> On 2013/11/11 22:02:54, mef wrote:
> > On 2013/11/09 03:09:41, tbarzic wrote:
> > > Hm, as far as I can see, wifi service methods are async, which means that
> it's
> > > not guaranteed that the helper will be done before Reply is called. 
> > Wifi service methods are actually synchronous.
> 
> OK.
> I'm a bit concerned that we assume that methods that have async interface are
> always synchronous.
> It wouldn't be too hard to imagine a refactoring that would make some of these
> asynchronous :/
> 
> But still I don't think PostTaskAndReply is the way to go here. Wouldn't it
just
> be easier to post task to the right thread from the WifiService callbacks?

Come to think about it, maybe I should make WiFiService interface functions
explicitly synchronous,
and that would simplify the whole PostTaskAndReply layer.

Also, with callback map I could revisit the need of WiFiServiceHelper objects as
there is already an extra layer.

https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/450001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:112:
process_client_ =
On 2013/11/12 04:02:23, tbarzic wrote:
> you don't have to cache the process_client_, you can just use
> NetworkingPrivateServiceClientFactory::GetForProfile() when you need it.

Done.

[tbarzic, 2013-11-12 20:14:59]

cool, this looks much nicer :)

I'll take a closer look when you finish it up..

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:153:
wifi_service_ = wifi::WiFiService::CreateService();
you'll have to create wifi_service_ on UI thread (without doing anything
expensive), and initialize it here.
(otherwise there is a race between StartOnWorkerThread and GetProperties which
is called on UI thread and passes wifi_service_ to the worker thread task)

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:324:
base::Bind(&NetworkingPrivateServiceClient::OnVerifyDestination,
nit: indent += 2
(same in VerifyAndEncryptData)

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:501:
ServiceCallbacksID callback_id,
indent += 2

[mef, 2013-11-13 00:07:36]

Hi guys, PTAL.

I've tweaked CryptoVerify interface and got rid of CryptoVerifyHelper. 

I've also tried to make NetworkingPrivateServiceClient support weak pointer
instead of refcount. 

My concern is that there is no ShutdownOnUIThread method to override there, 
so there doesn't seem to be a deterministic point to release extension functions
callbacks during profile shutdown.

Also, browser_tests gets failures like these, even though it appears to be
happening on correct (UI) thread :

[7584:6628:1112/185430:14446113:FATAL:weak_ptr.cc(26)] Check failed:
sequence_checker_.CalledOnValidSequencedThread(). W
eakPtrs must be checked on the same sequenced thread.
Backtrace:
        base::debug::StackTrace::StackTrace [0x10089461+33]
(s:\chrome\src\base\debug\stack_trace_win.cc:207)
        logging::LogMessage::~LogMessage [0x10119E7E+94]
(s:\chrome\src\base\logging.cc:566)
        base::internal::WeakReference::Flag::IsValid [0x1012E2E0+256]
(s:\chrome\src\base\memory\weak_ptr.cc:28)
        base::internal::WeakReference::is_valid [0x1012E4A2+50]
(s:\chrome\src\base\memory\weak_ptr.cc:43)
        base::WeakPtr<extensions::NetworkingPrivateServiceClient>::get
[0x03EEECA3+35] (s:\chrome\src\base\memory\weak_p
tr.h:203)
...
       base::MessageLoop::DoWork [0x10133C36+262]
(s:\chrome\src\base\message_loop\message_loop.cc:617)
       base::MessagePumpForUI::DoRunLoop [0x101425C4+84]
(s:\chrome\src\base\message_loop\message_pump_win.cc:243)
       base::MessagePumpWin::RunWithDispatcher [0x10141702+130]
(s:\chrome\src\base\message_loop\message_pump_win.cc

So, I've stashed the WeakPtr changes as separate CL:
https://codereview.chromium.org/68183010/, and would appreciate any comments.

thanks,
-m

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:324:
base::Bind(&NetworkingPrivateServiceClient::OnVerifyDestination,
On 2013/11/12 20:14:59, tbarzic wrote:
> nit: indent += 2
> (same in VerifyAndEncryptData)

Done.

https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:501:
ServiceCallbacksID callback_id,
On 2013/11/12 20:14:59, tbarzic wrote:
> indent += 2

Done.

[tbarzic, 2013-11-13 01:27:10]

replied inline

On 2013/11/13 00:07:36, mef wrote:
> Hi guys, PTAL.
> 
> I've tweaked CryptoVerify interface and got rid of CryptoVerifyHelper. 
> 
> I've also tried to make NetworkingPrivateServiceClient support weak pointer
> instead of refcount. 
> 
> My concern is that there is no ShutdownOnUIThread method to override there, 
> so there doesn't seem to be a deterministic point to release extension
functions
> callbacks during profile shutdown.

What's wrong with NetworkingPrivateServiceClient dtor :)
or overriding BrowserContextKeyedService::Shutdown (which is the same as
ShutdownOnUIThread for RefcounterBrowserContextKeyedService)

> 
> Also, browser_tests gets failures like these, even though it appears to be
> happening on correct (UI) thread :

Actually, you have two places where you're misusing weak ptrs:
- in ctor you create a weak ptr on UI thread and later check it on the worker
thread (by binding StartOnWorkerThread to it)
- in StartOnWorkerThread you create a weak ptr on the worker thread, and later
use it on UI thread (in the listener callback)

btw. you should not have StartOnWorkerThread thread (the point is to make
NPServiceClient live solely on UI thread, right)
Also, I'd prefer using base::WeakPtrFactory over making NPServiceClient
base::SupportsWeakPtr

> 
> [7584:6628:1112/185430:14446113:FATAL:weak_ptr.cc(26)] Check failed:
> sequence_checker_.CalledOnValidSequencedThread(). W
> eakPtrs must be checked on the same sequenced thread.
> Backtrace:
>         base::debug::StackTrace::StackTrace [0x10089461+33]
> (s:\chrome\src\base\debug\stack_trace_win.cc:207)
>         logging::LogMessage::~LogMessage [0x10119E7E+94]
> (s:\chrome\src\base\logging.cc:566)
>         base::internal::WeakReference::Flag::IsValid [0x1012E2E0+256]
> (s:\chrome\src\base\memory\weak_ptr.cc:28)
>         base::internal::WeakReference::is_valid [0x1012E4A2+50]
> (s:\chrome\src\base\memory\weak_ptr.cc:43)
>         base::WeakPtr<extensions::NetworkingPrivateServiceClient>::get
> [0x03EEECA3+35] (s:\chrome\src\base\memory\weak_p
> tr.h:203)
> ...
>        base::MessageLoop::DoWork [0x10133C36+262]
> (s:\chrome\src\base\message_loop\message_loop.cc:617)
>        base::MessagePumpForUI::DoRunLoop [0x101425C4+84]
> (s:\chrome\src\base\message_loop\message_pump_win.cc:243)
>        base::MessagePumpWin::RunWithDispatcher [0x10141702+130]
> (s:\chrome\src\base\message_loop\message_pump_win.cc
> 
> So, I've stashed the WeakPtr changes as separate CL:
> https://codereview.chromium.org/68183010/, and would appreciate any comments.
> 
> thanks,
> -m
> 
>
https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
> (right):
> 
>
https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:324:
> base::Bind(&NetworkingPrivateServiceClient::OnVerifyDestination,
> On 2013/11/12 20:14:59, tbarzic wrote:
> > nit: indent += 2
> > (same in VerifyAndEncryptData)
> 
> Done.
> 
>
https://codereview.chromium.org/54323003/diff/600001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:501:
> ServiceCallbacksID callback_id,
> On 2013/11/12 20:14:59, tbarzic wrote:
> > indent += 2
> 
> Done.

[mef, 2013-11-13 03:44:36]

Thanks a lot, Toni! 
That have fixed the issue. 
PTAL, I believe all outstanding comments are addressed at this point.

thanks,
-m

[cbentzel, 2013-11-13 11:34:39]

High-level object/threading architecture looks nice now, thanks for going
through so many iterations!

Toni - would you prefer finishing the review with the current diff basis (the
NetworkingProcessServiceClient approach) or changing the diff basis to be trunk?

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:133:
void NetworkingPrivateServiceClient::ShutdownOnWorkerThread(
Should this be a non-class method (just a normal function)?

[mef, 2013-11-13 15:51:04]

If this looks OK, then I think it would make sense for me to merge this CL back
into https://codereview.chromium.org/22295002/ and do one more review iteration
there.

thanks,
-m

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:124:
task_runner_->PostTask(
I'm wondering what happens to posted tasks after |task_runner_| is destructed?
Do they still run?

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:133:
void NetworkingPrivateServiceClient::ShutdownOnWorkerThread(
On 2013/11/13 11:34:39, cbentzel wrote:
> Should this be a non-class method (just a normal function)?

Done. It was a static method, but I don't mind to make it a stand-alone
function.

[tbarzic, 2013-11-14 05:28:36]

I think it would be reasonable to continue with the diff from ToT (but I'd just
change diff base for this cl; not merge it to 22295002)

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/730001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:124:
task_runner_->PostTask(
On 2013/11/13 15:51:04, mef wrote:
> I'm wondering what happens to posted tasks after |task_runner_| is destructed?
> Do they still run?

They should, as long as the underlying blocking pool is around.
task_runner_->PostTask will just hand the task over to the pool
(https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...),
so even if the task runner gets destroyed, that should not influence the
behaviour of the task.

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:103:
crypto_verify_(new CryptoVerifyImpl()),
I'd pass crypto_verify and wifi_service through the ctor.
That way you could get rod of SetupForTest (you'd inject stub versions using
BrowserContextKeyedServiceFactory::SetTestingFactory).

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:352:
void NetworkingPrivateServiceClient::TearDownForTest() {
you should remove this

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:415:
if (!CheckServiceError(service_callbacks, *error) &&
CheckServiceError does not really match its behavior, you should rename it.
But honestly, I'm not sure that the method is really needed;
the following looks just fine (imho more readable):

if (error->empty()) {
  DCHECK(!service_callbacks->error_callback.is_null());
  service_callbacks->error_callback.Run(*error,
scoped_ptr<base::DictionaryValue>());
} else {
  // it should be safe to dcheck the callback now, as the callback id is set by
|this|.
  DCHECK(!service_callbacks->start_connect_callback.is_null());
  service_callbacks->start_connect_callback.Run();
}

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:426:
ServiceCallbacks* service_callbacks = callbacks_map_.Lookup(callback_id);
there's a lot of boilerplate code here, maybe in future, when you're bored, you
can try to generalize ServiceCallbacks a bit :)

[mef, 2013-11-14 16:15:31]

Thanks, Toni! PTAL, I've addressed your comments and changed this CL to diff
against ToT.

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:103:
crypto_verify_(new CryptoVerifyImpl()),
On 2013/11/14 05:28:36, tbarzic wrote:
> I'd pass crypto_verify and wifi_service through the ctor.
> That way you could get rod of SetupForTest (you'd inject stub versions using
> BrowserContextKeyedServiceFactory::SetTestingFactory).

Done.

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:352:
void NetworkingPrivateServiceClient::TearDownForTest() {
On 2013/11/14 05:28:36, tbarzic wrote:
> you should remove this

Done.

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:415:
if (!CheckServiceError(service_callbacks, *error) &&
On 2013/11/14 05:28:36, tbarzic wrote:
> CheckServiceError does not really match its behavior, you should rename it.
> But honestly, I'm not sure that the method is really needed;
> the following looks just fine (imho more readable):
> 
> if (error->empty()) {
>   DCHECK(!service_callbacks->error_callback.is_null());
>   service_callbacks->error_callback.Run(*error,
> scoped_ptr<base::DictionaryValue>());
> } else {
>   // it should be safe to dcheck the callback now, as the callback id is set
by
> |this|.
>   DCHECK(!service_callbacks->start_connect_callback.is_null());
>   service_callbacks->start_connect_callback.Run();
> }

Done.

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:426:
ServiceCallbacks* service_callbacks = callbacks_map_.Lookup(callback_id);
On 2013/11/14 05:28:36, tbarzic wrote:
> there's a lot of boilerplate code here, maybe in future, when you're bored,
you
> can try to generalize ServiceCallbacks a bit :)

I have a nagging suspicion that with some creative use of Binding and callbacks
all 'AfterXXX' methods could be combined into one, but I'm not sufficiently
proficient with those yet. Now that there is no IPC to inject invalid message
maybe I should make ServiceCallbacks a template with success_callback and
error_callback and go from there?

[mef, 2013-11-14 16:24:37]

On 2013/11/14 16:15:31, mef wrote:
> Thanks, Toni! PTAL, I've addressed your comments and changed this CL to diff
> against ToT.
> 
>
https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
> File
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
> (right):
> 
>
https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:103:
> crypto_verify_(new CryptoVerifyImpl()),
> On 2013/11/14 05:28:36, tbarzic wrote:
> > I'd pass crypto_verify and wifi_service through the ctor.
> > That way you could get rod of SetupForTest (you'd inject stub versions using
> > BrowserContextKeyedServiceFactory::SetTestingFactory).
> 
> Done.
> 
>
https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:352:
> void NetworkingPrivateServiceClient::TearDownForTest() {
> On 2013/11/14 05:28:36, tbarzic wrote:
> > you should remove this
> 
> Done.
> 
>
https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:415:
> if (!CheckServiceError(service_callbacks, *error) &&
> On 2013/11/14 05:28:36, tbarzic wrote:
> > CheckServiceError does not really match its behavior, you should rename it.
> > But honestly, I'm not sure that the method is really needed;
> > the following looks just fine (imho more readable):
> > 
> > if (error->empty()) {
> >   DCHECK(!service_callbacks->error_callback.is_null());
> >   service_callbacks->error_callback.Run(*error,
> > scoped_ptr<base::DictionaryValue>());
> > } else {
> >   // it should be safe to dcheck the callback now, as the callback id is set
> by
> > |this|.
> >   DCHECK(!service_callbacks->start_connect_callback.is_null());
> >   service_callbacks->start_connect_callback.Run();
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
>
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:426:
> ServiceCallbacks* service_callbacks = callbacks_map_.Lookup(callback_id);
> On 2013/11/14 05:28:36, tbarzic wrote:
> > there's a lot of boilerplate code here, maybe in future, when you're bored,
> you
> > can try to generalize ServiceCallbacks a bit :)
> 
> I have a nagging suspicion that with some creative use of Binding and
callbacks
> all 'AfterXXX' methods could be combined into one, but I'm not sufficiently
> proficient with those yet. Now that there is no IPC to inject invalid message
> maybe I should make ServiceCallbacks a template with success_callback and
> error_callback and go from there?

I'm also wondering whether components/ is the right place for wifi/* or I should
move that into chrome/browser/extensions/api/networking_private/...

[mef, 2013-11-15 16:41:04]

Ping. 

Toni, could you please take a look and let me know whether this puppy is ready
for OWNERS.

thanks,
-m

[tbarzic, 2013-11-15 22:56:29]

looks good. Mostly minor (nitpicking) comments

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/780001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:426:
ServiceCallbacks* service_callbacks = callbacks_map_.Lookup(callback_id);
On 2013/11/14 16:15:32, mef wrote:
> On 2013/11/14 05:28:36, tbarzic wrote:
> > there's a lot of boilerplate code here, maybe in future, when you're bored,
> you
> > can try to generalize ServiceCallbacks a bit :)
> 
> I have a nagging suspicion that with some creative use of Binding and
callbacks
> all 'AfterXXX' methods could be combined into one, but I'm not sufficiently
> proficient with those yet. Now that there is no IPC to inject invalid message
> maybe I should make ServiceCallbacks a template with success_callback and
> error_callback and go from there?

yeah, I think with some magic it could be done (for example making
ServiceCallback an interface {RunErrorCallback(string
error);RunCallback(base::Value result)}, and implement it for different request
types)
But I would not force it .. It would be nice to give it some thought, but for
now, this looks ok.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:35:
using chromeos::DBusMethodCallStatus;
group includes and using parts together.

i.e.
#if defined(OS_CHROMEOS)
#include ...
#else
#include ...
#endif

#if defined(OS_CHROMEOS)
using....
#else
using....
#endif

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:64:
class CryptoVerifyMock
rename to CryptoVerifyStub (as it's a stub, not a mock)

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:17:
// |NetworkListChanged| events. Shutdown NetworkingPrivateServiceClient if there
update comment in regard to shutting down the service client

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:116:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
you can use base::MessageLoopProxy::current()
But, I'd prefer to do thread hopping here instead of in the wifi_service_ (by
wrapping listeners callbacks with a function that posts them to the UI thread
message loop).

Also, I think this would be less fragile if SetEventListeners was set on the
worker thread (depending on wifi service implementation, it may even be
necessary; I could image that we'd want to do something expensive in wifi
service to start listening to network changes)

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:342:
ServiceCallbacks* service_callbacks,
you can remove this now

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:20:
#include "chrome/common/cancelable_task_tracker.h"
do you still need this?

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:27:
class Profile;
add an empty line after here

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:68:
public:
add an empty virtual dtor here and in CryptoVerify

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:107:
NetworkingPrivateServiceClient(Profile* profile,
use content::BrowserContext* instead of Profile* (to reduce dependency on
chrome/)

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:188:
bool CheckServiceError(ServiceCallbacks* callbacks, const std::string& error);
add trailing .

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:227:
CryptoVerify* crypto_verify_;
I'd make crypto_verify_ and wifi_service_ scoped_ptr (to make it clear they are
owned by this)

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
File components/wifi/wifi_service.h (right):

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service.h:20: // WiFiService interface used by
implementation of chrome.networkingPrivate
Add a comment about threading restrictions

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service.h:66: static WiFiService* CreateService();
I'd rename these two to
Create()
CreateForTest()

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service.h:76: kFrequency2400 = 2400,
fyi, cros implementation does not normalize the frequency list, so it would be
ok if win implementation didn't either

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
File components/wifi/wifi_service_mock.cc (right):

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service_mock.cc:13: class WiFiServiceMock : public
WiFiService {
This isn't actually a mock.. I think TestWifiService/FakeWifiService would be
more suitable names (it shouldn't be too much of a hustle).

https://codereview.chromium.org/54323003/diff/1170001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:12: "platforms":
["chromeos", "win"],
I'd wait with this until the wifi_service changes land

[cbentzel, 2013-11-16 02:28:04]

LGTM

I just have nits at this point. The design and implementation look good at this
point. Thanks!

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router.h
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router.h:26:
// BrowserContextKeyedService overrides:
Nit: I don't know if you need to have additional declarations of pure virtual
functions here as the base classes should suffice. The derived classes of
NetworkingPrivateEventRouter would still need to declare the concrete OVERRIDE
versions.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc:34:
#if defined(OS_WIN)
should this be for all non-chromeos builds

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:60:
EventRouter* event_router = ExtensionSystem::Get(profile_)->event_router();
This code is shared with the chromeos implementation. Should it move in the base
class? I guess one concern is that StartOrStopListeningForNetworkChanges is
implemented differently for the two and we don't want to call a virtual method
in the base class constructor.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:60:
EventRouter* event_router = ExtensionSystem::Get(profile_)->event_router();
Nit: probably should have a helper method to get event_router just to minimize
code duplication.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:78:
EventRouter* event_router = ExtensionSystem::Get(profile_)->event_router();
Should you be checking for NULL event_router in some of the other methods (like
OnNetworksChangedEvent)

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:85:
NetworkingPrivateServiceClientFactory::GetForProfile(profile_);
Does the dependency expressed in the factory guarantee that the
NetworkingPrivateServiceClient will be shutdown after this object is? I think
so, just checking.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:91:
void ShutdownWiFiServiceOnWorkerThread(
Nit: ShutdownOnWorkerThread since both WiFiService and CryptoVerify.

Since in this pretty tight namespace I don't mind such a generic name.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:115:
wifi_service_->SetEventObservers(
this should preferably  be done on worker thread

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:105:
typedef base::Callback<void()> VoidResultCallback;
Nit: Closure is already defined the same way, not sure if you just want to use
that definition instead of this one.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:107:
NetworkingPrivateServiceClient(Profile* profile,
Nit: Needs documentation. In particular, comment on lifetime expectations for
wifi_service and crypto_verify

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:172:
DictionaryResultCallback get_properties_callback;
Could this all be a union with the exception of error_callback?

I'm not sure we want to do that - but raising it as a possibility.

https://codereview.chromium.org/54323003/diff/1170001/components/wifi/wifi_se...
File components/wifi/wifi_service.cc (right):

https://codereview.chromium.org/54323003/diff/1170001/components/wifi/wifi_se...
components/wifi/wifi_service.cc:14:
WiFiService::NetworkProperties::NetworkProperties()
Reminder: REview NetworkProperties.

[mef, 2013-11-17 20:05:16]

Hi guys, thanks for your comments, PTAL.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:35:
using chromeos::DBusMethodCallStatus;
On 2013/11/15 22:56:30, tbarzic wrote:
> group includes and using parts together.
> 
> i.e.
> #if defined(OS_CHROMEOS)
> #include ...
> #else
> #include ...
> #endif
> 
> #if defined(OS_CHROMEOS)
> using....
> #else
> using....
> #endif

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:64:
class CryptoVerifyMock
On 2013/11/15 22:56:30, tbarzic wrote:
> rename to CryptoVerifyStub (as it's a stub, not a mock)

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:17:
// |NetworkListChanged| events. Shutdown NetworkingPrivateServiceClient if there
On 2013/11/15 22:56:30, tbarzic wrote:
> update comment in regard to shutting down the service client

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:116:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
On 2013/11/15 22:56:30, tbarzic wrote:
> you can use base::MessageLoopProxy::current()
> But, I'd prefer to do thread hopping here instead of in the wifi_service_ (by
> wrapping listeners callbacks with a function that posts them to the UI thread
> message loop).
Wouldn't doing thread hoping here break agreement to access this from UI thread
only?
> 
> Also, I think this would be less fragile if SetEventListeners was set on the
> worker thread (depending on wifi service implementation, it may even be
> necessary; I could image that we'd want to do something expensive in wifi
> service to start listening to network changes)
Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:342:
ServiceCallbacks* service_callbacks,
On 2013/11/15 22:56:30, tbarzic wrote:
> you can remove this now

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:20:
#include "chrome/common/cancelable_task_tracker.h"
On 2013/11/15 22:56:30, tbarzic wrote:
> do you still need this?

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:27:
class Profile;
On 2013/11/15 22:56:30, tbarzic wrote:
> add an empty line after here

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:68:
public:
On 2013/11/15 22:56:30, tbarzic wrote:
> add an empty virtual dtor here and in CryptoVerify

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:107:
NetworkingPrivateServiceClient(Profile* profile,
On 2013/11/15 22:56:30, tbarzic wrote:
> use content::BrowserContext* instead of Profile* (to reduce dependency on
> chrome/)

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:188:
bool CheckServiceError(ServiceCallbacks* callbacks, const std::string& error);
On 2013/11/15 22:56:30, tbarzic wrote:
> add trailing .

Done.

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:227:
CryptoVerify* crypto_verify_;
On 2013/11/15 22:56:30, tbarzic wrote:
> I'd make crypto_verify_ and wifi_service_ scoped_ptr (to make it clear they
are
> owned by this)

Done. Although IMHO it is more confusing because it implies that they are
deleted in destructor of |this|, which they are not.

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
File components/wifi/wifi_service.h (right):

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service.h:20: // WiFiService interface used by
implementation of chrome.networkingPrivate
On 2013/11/15 22:56:30, tbarzic wrote:
> Add a comment about threading restrictions

Done.

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service.h:66: static WiFiService* CreateService();
On 2013/11/15 22:56:30, tbarzic wrote:
> I'd rename these two to
> Create()
> CreateForTest()

Done.

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service.h:76: kFrequency2400 = 2400,
On 2013/11/15 22:56:30, tbarzic wrote:
> fyi, cros implementation does not normalize the frequency list, so it would be
> ok if win implementation didn't either
Noted.

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
File components/wifi/wifi_service_mock.cc (right):

https://codereview.chromium.org/54323003/diff/850001/components/wifi/wifi_ser...
components/wifi/wifi_service_mock.cc:13: class WiFiServiceMock : public
WiFiService {
On 2013/11/15 22:56:30, tbarzic wrote:
> This isn't actually a mock.. I think TestWifiService/FakeWifiService would be
> more suitable names (it shouldn't be too much of a hustle).

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router.h
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router.h:26:
// BrowserContextKeyedService overrides:
On 2013/11/16 02:28:05, cbentzel wrote:
> Nit: I don't know if you need to have additional declarations of pure virtual
> functions here as the base classes should suffice. The derived classes of
> NetworkingPrivateEventRouter would still need to declare the concrete OVERRIDE
> versions.

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc:34:
#if defined(OS_WIN)
On 2013/11/16 02:28:05, cbentzel wrote:
> should this be for all non-chromeos builds

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:60:
EventRouter* event_router = ExtensionSystem::Get(profile_)->event_router();
On 2013/11/16 02:28:05, cbentzel wrote:
> This code is shared with the chromeos implementation. Should it move in the
base
> class? I guess one concern is that StartOrStopListeningForNetworkChanges is
> implemented differently for the two and we don't want to call a virtual method
> in the base class constructor.
Yes. Also I thought (probably incorrectly) that meaningful base classes are
frowned upon.
I'm not sure I understand what would helper method do.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:78:
EventRouter* event_router = ExtensionSystem::Get(profile_)->event_router();
On 2013/11/16 02:28:05, cbentzel wrote:
> Should you be checking for NULL event_router in some of the other methods
(like
> OnNetworksChangedEvent)

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:85:
NetworkingPrivateServiceClientFactory::GetForProfile(profile_);
On 2013/11/16 02:28:05, cbentzel wrote:
> Does the dependency expressed in the factory guarantee that the
> NetworkingPrivateServiceClient will be shutdown after this object is? I think
> so, just checking.
Yes.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:91:
void ShutdownWiFiServiceOnWorkerThread(
On 2013/11/16 02:28:05, cbentzel wrote:
> Nit: ShutdownOnWorkerThread since both WiFiService and CryptoVerify.
> 
> Since in this pretty tight namespace I don't mind such a generic name.

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:115:
wifi_service_->SetEventObservers(
On 2013/11/16 02:28:05, cbentzel wrote:
> this should preferably  be done on worker thread

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:105:
typedef base::Callback<void()> VoidResultCallback;
On 2013/11/16 02:28:05, cbentzel wrote:
> Nit: Closure is already defined the same way, not sure if you just want to use
> that definition instead of this one.

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:107:
NetworkingPrivateServiceClient(Profile* profile,
On 2013/11/16 02:28:05, cbentzel wrote:
> Nit: Needs documentation. In particular, comment on lifetime expectations for
> wifi_service and crypto_verify

Done.

https://codereview.chromium.org/54323003/diff/1170001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:172:
DictionaryResultCallback get_properties_callback;
On 2013/11/16 02:28:05, cbentzel wrote:
> Could this all be a union with the exception of error_callback?
> 
> I'm not sure we want to do that - but raising it as a possibility.

I was thinking about making this a template based on callback type,
but I'm not sure it is worth it.

https://codereview.chromium.org/54323003/diff/1170001/chrome/common/extension...
File chrome/common/extensions/api/networking_private.json (right):

https://codereview.chromium.org/54323003/diff/1170001/chrome/common/extension...
chrome/common/extensions/api/networking_private.json:12: "platforms":
["chromeos", "win"],
On 2013/11/15 22:56:30, tbarzic wrote:
> I'd wait with this until the wifi_service changes land
Without it browser_tests can't access networkingPrivateApi on windows.

https://codereview.chromium.org/54323003/diff/1170001/components/wifi/wifi_se...
File components/wifi/wifi_service.cc (right):

https://codereview.chromium.org/54323003/diff/1170001/components/wifi/wifi_se...
components/wifi/wifi_service.cc:14:
WiFiService::NetworkProperties::NetworkProperties()
On 2013/11/16 02:28:05, cbentzel wrote:
> Reminder: REview NetworkProperties.

I'm considering replacing NetworkProperties with DictionaryValue even
internally.
Any opinion?

[jam, 2013-11-18 05:52:30]

On 2013/11/17 20:05:16, mef wrote:
> Hi guys, thanks for your comments, PTAL.

which parts do you want me to look at?

[tbarzic, 2013-11-19 00:05:43]

lgtm with few nits

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/850001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:116:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
On 2013/11/17 20:05:18, mef wrote:
> On 2013/11/15 22:56:30, tbarzic wrote:
> > you can use base::MessageLoopProxy::current()
> > But, I'd prefer to do thread hopping here instead of in the wifi_service_
(by
> > wrapping listeners callbacks with a function that posts them to the UI
thread
> > message loop).
> Wouldn't doing thread hoping here break agreement to access this from UI
thread
> only?
> > 
> > Also, I think this would be less fragile if SetEventListeners was set on the
> > worker thread (depending on wifi service implementation, it may even be
> > necessary; I could image that we'd want to do something expensive in wifi
> > service to start listening to network changes)
> Done.
> 

Not necessarily. You'd have to wrap the callback with a standalone function
(note I'm fine with leaving this as it is; your choice):
void RelayCallback(
    scoped_refptr<base::MessageLoopProxy> loop,
    const base::Callback<void(
        const std::vector<std::string>& networks)>& callback,
    const std::vector<std::string>& networks) {
  loop->PostTask(FROM_HERE, base::Bind(callback, networks));
}

and
// You have to do this binding on UI thread:
base::Callback<...> on_networks_changed = base::Bind(
    &RelayCallback,
    base::MessageLoopProxy::current(),
    base::Bind(&NPSC::OnNetworksChanged, weak_factory_.GetWeakPtr()));
base::Callback<...> on_network_list_changed = base::Bind(
    &RelayCallback,
    base::MessageLoopProxy::current(),
    base::Bind(&NPSC::OnNetworkListChanged, weak_factory_.GetWeakPtr()));

// On the Worker thread:
wifi_service_->SetEventObservers(on_networks_changed, on_network_list_changed);

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:153:

nuke one line

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:95:
}
nit: new line here

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:104:
sequence_token_ = BrowserThread::GetBlockingPool()->
you could extract this to a Init method (if you want)

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:144:
Passed(&wifi_service_),
do you need base:: here?

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:45:
// This class is the client wrapper for the WiFiService and CryptoVerify
nit: I'd drop "This class is" part of the comment.

add a note that it can only be used on the UI thread.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:61:
virtual ~CryptoVerify() { }
nit: nuke the space inside {}
and move this before VerifyDestination

and move Create() after dtor

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:106:
// on worker thread and deleted from worker thread after all tasks are done.
nit: "after all tasks are done" is not completely precise.
Maybe something more like:
"They are deleted on the worker thread. The deletion task is posted during the
NetworkingPrivateServiceClient shutdown."

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:107:
NetworkingPrivateServiceClient(wifi::WiFiService* wifi_service,
theoretically, could wifi service be shared across profiles (in which case
service_factory could own it and pass it around)?
note: just curious, I'm completely fine with leaving this alone :)

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:159:
// BrowserContextKeyedServices method override.
nit: I'd move this after ctor

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:211:
virtual ~NetworkingPrivateServiceClient();
move this after ServiceCallbacksMap typedef

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:224:
// Interface to WiFiService. Lives on the worker thread.
s/Lives on/Used on/

https://codereview.chromium.org/54323003/diff/1320001/components/wifi/wifi_se...
File components/wifi/wifi_service.h (right):

https://codereview.chromium.org/54323003/diff/1320001/components/wifi/wifi_se...
components/wifi/wifi_service.h:21: // JavaScript extension API. All methods
should be called on worker thread.
Comment that it may be created on any (the UI) thread, so nothing expensive
should be done in the ctor.

https://codereview.chromium.org/54323003/diff/1320001/components/wifi/wifi_se...
components/wifi/wifi_service.h:65: // Create instance of |WiFiService| for
normal use.
I'd move this after dtor

[mef, 2013-11-19 17:37:02]

Hi guys, 

I would like to humbly ask for OWNER review of these changes:

erg@ - chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(extensions::NetworkingPrivateEventRouterFactory is now available on OS_WIN)

finnur@ - chrome/common/extensions/api/networking_private.json (make it
available on "win").

jam@ - chrome/*.gyp? , overall change as to get rid of utility process per your
suggestion. 

joi@ - components/* - Per your suggestions in http://crrev.com/27722003
I've moved wifi/ from chrome/utility/ into components/ and changed WiFiService
interface to be synchronous instead of async. Please let me know if
components/wifi
should have local OWNERS.

stevenjb@ - chrome/browser/extensions/api/networking_private/

thanks,
-m

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:153:

On 2013/11/19 00:05:44, tbarzic wrote:
> nuke one line

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:95:
}
On 2013/11/19 00:05:44, tbarzic wrote:
> nit: new line here

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:104:
sequence_token_ = BrowserThread::GetBlockingPool()->
On 2013/11/19 00:05:44, tbarzic wrote:
> you could extract this to a Init method (if you want)
I could, but there doesn't seem to be a meaningful error to return,
so what would be the benefit?

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:144:
Passed(&wifi_service_),
On 2013/11/19 00:05:44, tbarzic wrote:
> do you need base:: here?

Done. Passed didn't need base::, but I agree that base::Passed makes it more
obvious.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:45:
// This class is the client wrapper for the WiFiService and CryptoVerify
On 2013/11/19 00:05:44, tbarzic wrote:
> nit: I'd drop "This class is" part of the comment.
> 
> add a note that it can only be used on the UI thread.

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:61:
virtual ~CryptoVerify() { }
On 2013/11/19 00:05:44, tbarzic wrote:
> nit: nuke the space inside {}
> and move this before VerifyDestination
> 
> and move Create() after dtor

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:106:
// on worker thread and deleted from worker thread after all tasks are done.
On 2013/11/19 00:05:44, tbarzic wrote:
> nit: "after all tasks are done" is not completely precise.
> Maybe something more like:
> "They are deleted on the worker thread. The deletion task is posted during the
> NetworkingPrivateServiceClient shutdown."

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:107:
NetworkingPrivateServiceClient(wifi::WiFiService* wifi_service,
On 2013/11/19 00:05:44, tbarzic wrote:
> theoretically, could wifi service be shared across profiles (in which case
> service_factory could own it and pass it around)?
> note: just curious, I'm completely fine with leaving this alone :) 
I think it could, but that would complicate its lifespan, especially if we'll
have to delete it prior to profile shutdown (e.g. when extension is unloaded).

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:159:
// BrowserContextKeyedServices method override.
On 2013/11/19 00:05:44, tbarzic wrote:
> nit: I'd move this after ctor

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:211:
virtual ~NetworkingPrivateServiceClient();
On 2013/11/19 00:05:44, tbarzic wrote:
> move this after ServiceCallbacksMap typedef

Done.

https://codereview.chromium.org/54323003/diff/1320001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:224:
// Interface to WiFiService. Lives on the worker thread.
On 2013/11/19 00:05:44, tbarzic wrote:
> s/Lives on/Used on/

Done.

https://codereview.chromium.org/54323003/diff/1320001/components/wifi/wifi_se...
File components/wifi/wifi_service.h (right):

https://codereview.chromium.org/54323003/diff/1320001/components/wifi/wifi_se...
components/wifi/wifi_service.h:21: // JavaScript extension API. All methods
should be called on worker thread.
On 2013/11/19 00:05:44, tbarzic wrote:
> Comment that it may be created on any (the UI) thread, so nothing expensive
> should be done in the ctor.

Done.

https://codereview.chromium.org/54323003/diff/1320001/components/wifi/wifi_se...
components/wifi/wifi_service.h:65: // Create instance of |WiFiService| for
normal use.
On 2013/11/19 00:05:44, tbarzic wrote:
> I'd move this after dtor

Done.

[Jói, 2013-11-20 11:04:16]

LGTM for //components, just a couple of nits.

Also, you should add a //components/wifi/OWNERS file and add per-file lines to
//components/OWNERS for the wifi.gypi file.

Note that I only reviewed the code at a high level, mostly focused on .gyp files
and such, since I'm assuming you already got an overall review.

Cheers,
Jói

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
File components/wifi/wifi_service.cc (right):

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
components/wifi/wifi_service.cc:104: }  // namespace wifi
nit: blank line before

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
File components/wifi/wifi_service.h (right):

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
components/wifi/wifi_service.h:116: }  // namespace wifi
nit: blank line before and after

[Finnur, 2013-11-20 13:57:29]

> finnur@ - chrome/common/extensions/api/networking_private.json (make it
> available on "win").

You mean 'ChromeOS' an 'win'.

LGTM.

[mef, 2013-11-20 16:19:34]

Thanks Joi and Finnur! 

Joi, I've added you as an owner to //components/wifi.

I would like to humbly ask for OWNER review of these changes:

erg@ - chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(extensions::NetworkingPrivateEventRouterFactory is now available on OS_WIN)

jam@ - chrome/*.gyp? , overall change to get rid of utility process per your
suggestion. 

stevenjb@ - chrome/browser/extensions/api/networking_private/*

thanks,
-m

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
File components/wifi/wifi_service.cc (right):

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
components/wifi/wifi_service.cc:104: }  // namespace wifi
On 2013/11/20 11:04:18, Jói wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
File components/wifi/wifi_service.h (right):

https://codereview.chromium.org/54323003/diff/1450001/components/wifi/wifi_se...
components/wifi/wifi_service.h:116: }  // namespace wifi
On 2013/11/20 11:04:18, Jói wrote:
> nit: blank line before and after

Done.

[Jói, 2013-11-20 16:21:35]

https://codereview.chromium.org/54323003/diff/1710001/components/wifi/OWNERS
File components/wifi/OWNERS (right):

https://codereview.chromium.org/54323003/diff/1710001/components/wifi/OWNERS#...
components/wifi/OWNERS:1: joi@chromium.org
You should add yourself and perhaps one or two other folks who know this code,
not me.

I'm already a top-level owner for //components and I don't really know the code
in //components/wifi especially well.

Same comment goes for the per-file OWNERS in //components/OWNERS.

[stevenjb, 2013-11-20 18:55:38]

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:81:
if (listening_) {
nit: early exit

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:104:
if (event_router) {
early exit

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:130:
extensions::ExtensionSystem::Get(profile_)->event_router();
nit: early exit
Also, no extensions::

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:144:
if (event_router) {
nit: early exit. No extensions::

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:115:
base::Unretained(wifi_service_.get()),
Is this safe? task_runner_ is a RefPtr, so couldn't it still be running after
this is destroyed? Same with other PostTask calls below.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:126:
NetworkingPrivateServiceClient::~NetworkingPrivateServiceClient() { }
{}

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:40:
}  // namespace api
nit: trailing } comment not necessary (and somewhat noisy) for namespaced header
declarations

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:48:
: public BrowserContextKeyedService {
One line?

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:53:
virtual ~CryptoVerify() {}
protect destructor (or use DISALLOW_COPY_AND_ASSIGN)

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:73:
virtual ~Observer() { }
protect, no ' ' between {}

[mef, 2013-11-20 20:33:25]

Thanks Joi and Steven, PTAL! 

I would like to humbly ask for OWNER review of these changes:

erg@ - chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
(extensions::NetworkingPrivateEventRouterFactory is now available on OS_WIN)

jam@ - chrome/chrome.gyp? , overall change to get rid of utility process per
your
suggestion. 

thanks,
-m

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:81:
if (listening_) {
On 2013/11/20 18:55:39, stevenjb wrote:
> nit: early exit

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:104:
if (event_router) {
On 2013/11/20 18:55:39, stevenjb wrote:
> early exit

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:130:
extensions::ExtensionSystem::Get(profile_)->event_router();
On 2013/11/20 18:55:39, stevenjb wrote:
> nit: early exit
> Also, no extensions::

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:144:
if (event_router) {
On 2013/11/20 18:55:39, stevenjb wrote:
> nit: early exit. No extensions::

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:115:
base::Unretained(wifi_service_.get()),
On 2013/11/20 18:55:39, stevenjb wrote:
> Is this safe? task_runner_ is a RefPtr, so couldn't it still be running after
> this is destroyed? Same with other PostTask calls below.
Yes, it could still be running, however |wifi_service_| is not deleted when
|this| is destroyed.
Instead it is passed to ShutdownServicesOnWorkerThread posted to the same
sequential task runner.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:126:
NetworkingPrivateServiceClient::~NetworkingPrivateServiceClient() { }
On 2013/11/20 18:55:39, stevenjb wrote:
> {}

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.h
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:48:
: public BrowserContextKeyedService {
On 2013/11/20 18:55:39, stevenjb wrote:
> One line?

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:53:
virtual ~CryptoVerify() {}
On 2013/11/20 18:55:39, stevenjb wrote:
> protect destructor (or use DISALLOW_COPY_AND_ASSIGN)

Done.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:73:
virtual ~Observer() { }
On 2013/11/20 18:55:39, stevenjb wrote:
> protect, no ' ' between {}

Done.

https://codereview.chromium.org/54323003/diff/1710001/components/wifi/OWNERS
File components/wifi/OWNERS (right):

https://codereview.chromium.org/54323003/diff/1710001/components/wifi/OWNERS#...
components/wifi/OWNERS:1: joi@chromium.org
On 2013/11/20 16:21:36, Jói wrote:
> You should add yourself and perhaps one or two other folks who know this code,
> not me.
> 
> I'm already a top-level owner for //components and I don't really know the
code
> in //components/wifi especially well.
> 
> Same comment goes for the per-file OWNERS in //components/OWNERS.

Done. I've explicitly added you because you've reviewed the original WiFiService
Windows implementation (https://codereview.chromium.org/27722003/). I would add
afontan@, who has also reviewed the code, but he doesn't have chromium account.
I'm open for suggestions in regards to additional owners.

[Jói, 2013-11-20 20:34:54]

Thanks. //components LGTM.

[Elliot Glaysher, 2013-11-20 20:55:08]

profiles lgtm

[stevenjb, 2013-11-20 22:01:11]

chrome/browser/extensions/api/networking_private/ lgtm w/ added dcheck +
comment.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:115:
base::Unretained(wifi_service_.get()),
On 2013/11/20 20:33:26, mef wrote:
> On 2013/11/20 18:55:39, stevenjb wrote:
> > Is this safe? task_runner_ is a RefPtr, so couldn't it still be running
after
> > this is destroyed? Same with other PostTask calls below.
> Yes, it could still be running, however |wifi_service_| is not deleted when
> |this| is destroyed.
> Instead it is passed to ShutdownServicesOnWorkerThread posted to the same
> sequential task runner.

I see. Can we ensure that Shutdown() was called (maybe dcheck that wifi_service_
and crypto_verify_ are NULL, i.e. Passed to ShutdownServicesOnWorkerThread) and
comment those members? This is a new pattern to me and seems somewhat subtle.

[jam, 2013-11-21 19:01:29]

On 2013/11/20 20:33:25, mef wrote:
> Thanks Joi and Steven, PTAL! 
> 
> I would like to humbly ask for OWNER review of these changes:
> 
> erg@ - chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
> (extensions::NetworkingPrivateEventRouterFactory is now available on OS_WIN)
> 
> jam@ - chrome/chrome.gyp? , overall change to get rid of utility process per
> your
> suggestion. 

gypi lgtm with nits below

as for the overall change, is there something specific you want me to look at? I
didn't look at all the new code, I left that for the existing more specific
reviewers. I see that you're not using a separate process anymore, and that part
lg :)

https://codereview.chromium.org/54323003/diff/1800001/chrome/chrome_browser_e...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/54323003/diff/1800001/chrome/chrome_browser_e...
chrome/chrome_browser_extensions.gypi:1057: ['OS=="win"', {
i don't think two OS=="win" sections is going to work? even if it does, there
should only be one.

Also, the convention for gypi files is that all files are listed in the main
section, and then files should be excluded per OS in the OS-specific conditions

[mef, 2013-11-21 21:15:44]

Steven, thanks! 

John, thanks! Overall CL was FYI to demonstrate that I've heeded your advice to
use WLAN Api directly from browser process.

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc
(right):

https://codereview.chromium.org/54323003/diff/1710001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:115:
base::Unretained(wifi_service_.get()),
On 2013/11/20 22:01:12, stevenjb wrote:
> On 2013/11/20 20:33:26, mef wrote:
> > On 2013/11/20 18:55:39, stevenjb wrote:
> > > Is this safe? task_runner_ is a RefPtr, so couldn't it still be running
> after
> > > this is destroyed? Same with other PostTask calls below.
> > Yes, it could still be running, however |wifi_service_| is not deleted when
> > |this| is destroyed.
> > Instead it is passed to ShutdownServicesOnWorkerThread posted to the same
> > sequential task runner.
> 
> I see. Can we ensure that Shutdown() was called (maybe dcheck that
wifi_service_
> and crypto_verify_ are NULL, i.e. Passed to ShutdownServicesOnWorkerThread)
and
> comment those members? This is a new pattern to me and seems somewhat subtle.

Done.

https://codereview.chromium.org/54323003/diff/1800001/chrome/chrome_browser_e...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/54323003/diff/1800001/chrome/chrome_browser_e...
chrome/chrome_browser_extensions.gypi:1057: ['OS=="win"', {
On 2013/11/21 19:01:30, jam wrote:
> i don't think two OS=="win" sections is going to work? even if it does, there
should only be one.

It appears that there could be multiple sections with same condition, but the CL
with mac implementation changes this section to be 'OS=="win" or OS=="mac"':
https://codereview.chromium.org/64683014/diff/50001/chrome/chrome_browser_ext...

> Also, the convention for gypi files is that all files are listed in the main
> section, and then files should be excluded per OS in the OS-specific
conditions
Done.

[Jorge Lucangeli Obes, 2013-11-21 21:53:38]

lgtm with the #include nit.

https://codereview.chromium.org/54323003/diff/1970002/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
(right):

https://codereview.chromium.org/54323003/diff/1970002/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:12:
#include "base/strings/stringprintf.h"
Are you using stringprintf?

[commit-bot: I haz the power, 2013-11-21 23:07:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/54323003/2230001

[commit-bot: I haz the power, 2013-11-21 23:08:03]

Failed to apply patch for
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
  Hunk #1 FAILED at 10.
  1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc.rej

Patch:      
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
Index:
chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
diff --git
a/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
b/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
index
03fcd4cbb93025ccdb8f98f84d374cd60dce0b81..44b1f9fe240b0d5111b85d342c8dba3131b542db
100644
---
a/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
+++
b/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc
@@ -10,29 +10,22 @@
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "chrome/browser/browser_process.h"
+#include
"chrome/browser/extensions/api/networking_private/networking_private_service_client.h"
+#include
"chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h"
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/browser/extensions/extension_function_registry.h"
 #include "chrome/browser/extensions/extension_system.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/api/networking_private.h"
+#include "components/onc/onc_constants.h"
 
-using extensions::EventRouter;
-using extensions::ExtensionSystem;
+using extensions::NetworkingPrivateServiceClient;
+using extensions::NetworkingPrivateServiceClientFactory;
 namespace api = extensions::api::networking_private;
 

////////////////////////////////////////////////////////////////////////////////
 // NetworkingPrivateGetPropertiesFunction
 
-
-const char kNetworkingPrivateProperties[] = "NetworkingPrivateProperties";
-
-struct NetworkingPrivatePropertiesData : base::SupportsUserData::Data {
-  explicit NetworkingPrivatePropertiesData(const base::DictionaryValue* prop) :
-    properties_(prop->DeepCopy()) { }
-  scoped_ptr<base::DictionaryValue> properties_;
-};
-
 NetworkingPrivateGetPropertiesFunction::
   ~NetworkingPrivateGetPropertiesFunction() {
 }
@@ -42,38 +35,32 @@ bool NetworkingPrivateGetPropertiesFunction::RunImpl() {
       api::GetProperties::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
 
-  // If there are properties set by SetProperties function, use those.
-  NetworkingPrivatePropertiesData* stored_properties =
-      static_cast<NetworkingPrivatePropertiesData*>(
-          GetProfile()->GetUserData(kNetworkingPrivateProperties));
-  if (stored_properties != NULL) {
-    SetResult(stored_properties->properties_.release());
-    SendResponse(true);
-    return true;
-  }
+  NetworkingPrivateServiceClient* process_client =
+      NetworkingPrivateServiceClientFactory::GetForProfile(GetProfile());
 
-  const std::string network_properties =
-    "{\"ConnectionState\":\"NotConnected\","
-     "\"GUID\":\"stub_wifi2\","
-     "\"Name\":\"wifi2_PSK\","
-     "\"Type\":\"WiFi\","
-     "\"WiFi\":{"
-       "\"Frequency\":5000,"
-       "\"FrequencyList\":[2400,5000],"
-       "\"SSID\":\"wifi2_PSK\","
-       "\"Security\":\"WPA-PSK\","
-       "\"SignalStrength\":80}}";
-
-  if (params->network_guid == "nonexistent_path") {
-    error_ = "Error.DBusFailed";
-    SendResponse(false);
-  } else {
-    SetResult(base::JSONReader::Read(network_properties));
-    SendResponse(true);
-  }
+  process_client->GetProperties(
+      params->network_guid,
+      base::Bind(&NetworkingPrivateGetPropertiesFunction::GetPropertiesSuccess,
+                 this),
+      base::Bind(&NetworkingPrivateGetPropertiesFunction::GetPropertiesFailed,
+                 this));
   return true;
 }
 
+void NetworkingPrivateGetPropertiesFunction::GetPropertiesSuccess(
+    const std::string& network_guid,
+    const base::DictionaryValue& dictionary) {
+  SetResult(dictionary.DeepCopy());
+  SendResponse(true);
+}
+
+void NetworkingPrivateGetPropertiesFunction::GetPropertiesFailed(
+    const std::string& error_name,
+    scoped_ptr<base::DictionaryValue> error_data) {
+  error_ = error_name;
+  SendResponse(false);
+}
+

////////////////////////////////////////////////////////////////////////////////
 // NetworkingPrivateGetManagedPropertiesFunction
 
@@ -184,14 +171,28 @@ bool NetworkingPrivateSetPropertiesFunction::RunImpl() {
   scoped_ptr<base::DictionaryValue> properties_dict(
       params->properties.ToValue());
 
-  // Store properties_dict in profile to return from GetProperties.
-  GetProfile()->SetUserData(
-      kNetworkingPrivateProperties,
-      new NetworkingPrivatePropertiesData(properties_dict.get()));
-  SendResponse(true);
+  NetworkingPrivateServiceClient* process_client =
+      NetworkingPrivateServiceClientFactory::GetForProfile(GetProfile());
+
+  process_client->SetProperties(
+      params->network_guid,
+      *properties_dict,
+      base::Bind(&NetworkingPrivateSetPropertiesFunction::ResultCallback,
this),
+      base::Bind(&NetworkingPrivateSetPropertiesFunction::ErrorCallback,
this));
   return true;
 }
 
+void NetworkingPrivateSetPropertiesFunction::ResultCallback() {
+  SendResponse(true);
+}
+
+void NetworkingPrivateSetPropertiesFunction::ErrorCallback(
+    const std::string& error_name,
+    const scoped_ptr<base::DictionaryValue> error_data) {
+  error_ = error_name;
+  SendResponse(false);
+}
+

////////////////////////////////////////////////////////////////////////////////
 // NetworkingPrivateCreateNetworkFunction
 
@@ -204,14 +205,6 @@ bool NetworkingPrivateCreateNetworkFunction::RunImpl() {
       api::CreateNetwork::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
 
-  // Store properties_dict in profile to return from GetProperties.
-  scoped_ptr<base::DictionaryValue> properties_dict(
-      params->properties.ToValue());
-  properties_dict->SetString("GUID", "fake_guid");
-  GetProfile()->SetUserData(
-      kNetworkingPrivateProperties,
-      new NetworkingPrivatePropertiesData(properties_dict.get()));
-
   results_ = api::CreateNetwork::Results::Create("fake_guid");
   SendResponse(true);
   return true;
@@ -228,66 +221,46 @@ bool
NetworkingPrivateGetVisibleNetworksFunction::RunImpl() {
   scoped_ptr<api::GetVisibleNetworks::Params> params =
       api::GetVisibleNetworks::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params);
-  const std::string networks_json =
-      "[{"
-      "    \"ConnectionState\": \"Connected\","
-      "    \"GUID\": \"stub_ethernet\","
-      "    \"Name\": \"eth0\","
-      "    \"Type\": \"Ethernet\","
-      "    \"Ethernet\": {"
-      "      \"Authentication\": \"None\""
-      "    }"
-      "  },"
-      "  {"
-      "    \"ConnectionState\": \"Connected\","
-      "    \"GUID\": \"stub_wifi1\","
-      "    \"Name\": \"wifi1\","
-      "    \"Type\": \"WiFi\","
-      "    \"WiFi\": {"
-      "      \"Security\": \"WEP-PSK\","
-      "      \"SignalStrength\": 0"
-      "    }"
-      "  },"
-      "  {"
-      "    \"ConnectionState\": \"Connected\","
-      "    \"GUID\": \"stub_vpn1\","
-      "    \"Name\": \"vpn1\","
-      "    \"Type\": \"VPN\""
-      "  },"
-      "  {"
-      "    \"ConnectionState\": \"NotConnected\","
-      "    \"GUID\": \"stub_wifi2\","
-      "    \"Name\": \"wifi2_PSK\","
-      "    \"Type\": \"WiFi\","
-      "    \"WiFi\": {"
-      "      \"Security\": \"WPA-PSK\","
-      "      \"SignalStrength\": 80"
-      "    }"
-      "  },"
-      "  {"
-      "    \"Cellular\": {"
-      "      \"ActivateOverNonCellularNetwork\": false,"
-      "      \"ActivationState\": \"not-activated\","
-      "      \"NetworkTechnology\": \"GSM\","
-      "      \"RoamingState\": \"home\""
-      "    },"
-      "    \"ConnectionState\": \"NotConnected\","
-      "    \"GUID\": \"stub_cellular1\","
-      "    \"Name\": \"cellular1\","
-      "    \"Type\": \"Cellular\""
-      "  }]";
-  ListValue* visible_networks =
-      static_cast<ListValue*>(base::JSONReader::Read(networks_json));
-  // If caller only needs WiFi networks, then remove all other networks.
-  if (params->type == api::GetVisibleNetworks::Params::TYPE_WIFI) {
-    visible_networks->Remove(4, NULL);
-    visible_networks->Remove(2, NULL);
-    visible_networks->Remove(0, NULL);
+
+  NetworkingPrivateServiceClient* process_client =
+      NetworkingPrivateServiceClientFactory::GetForProfile(GetProfile());
+
+  process_client->GetVisibleNetworks(base::Bind(
+      &NetworkingPrivateGetVisibleNetworksFunction::ResultCallback, this));
+
+  return true;
+}
+
+void NetworkingPrivateGetVisibleNetworksFunction::ResultCallback(
+    const base::ListValue& network_list) {
+  scoped_ptr<api::GetVisibleNetworks::Params> params =
+      api::GetVisibleNetworks::Params::Create(*args_);
+  ListValue* result = new ListValue();
+  std::string params_type =
+      api::GetVisibleNetworks::Params::ToString(params->type);
+  bool request_all = params->type == api::GetVisibleNetworks::Params::TYPE_ALL;
+
+  // Copy networks of requested type;
+  for (base::ListValue::const_iterator it = network_list.begin();
+       it != network_list.end();
+       ++it) {
+    const base::Value* network_value = *it;
+    const DictionaryValue* network_dict = NULL;
+    if (!network_value->GetAsDictionary(&network_dict)) {
+      LOG(ERROR) << "Value is not a dictionary";
+      continue;
+    }
+    if (!request_all) {
+      std::string network_type;
+      network_dict->GetString(onc::network_config::kType, &netw…
(message too large)

[commit-bot: I haz the power, 2013-11-22 01:07:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/54323003/2230002

[commit-bot: I haz the power, 2013-11-22 03:05:30]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-22 03:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/54323003/2230002

[commit-bot: I haz the power, 2013-11-22 05:28:24]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-22 12:01:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/54323003/2230002

[commit-bot: I haz the power, 2013-11-22 12:44:44]

Change committed as 236752

[tfarina, 2014-04-18 00:46:53]

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi
File components/wifi.gypi (right):

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:24: 'wifi/fake_wifi_service.cc',
this is including test code in production code.

[mef, 2014-04-18 14:57:39]

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi
File components/wifi.gypi (right):

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:24: 'wifi/fake_wifi_service.cc',
On 2014/04/18 00:46:54, tfarina wrote:
> this is including test code in production code.

Should I move it to //src/chrome/browser/extensions/api/networking_private as it
is only used in
//src/chrome/browser/extensions/api/networking_private/networking_private_apitest.cc?

[tfarina, 2014-04-18 15:28:06]

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi
File components/wifi.gypi (right):

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:24: 'wifi/fake_wifi_service.cc',
On 2014/04/18 14:57:40, mef wrote:
> On 2014/04/18 00:46:54, tfarina wrote:
> > this is including test code in production code.
> 
> Should I move it to //src/chrome/browser/extensions/api/networking_private as
it
> is only used in
>
//src/chrome/browser/extensions/api/networking_private/networking_private_apitest.cc?

either that or what I have initially thought in doing, create a
wifi_test_support target.

[tfarina, 2014-04-18 15:30:36]

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi
File components/wifi.gypi (right):

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:12: '../components/components.gyp:onc_component',
also, this path is "wrong", you don't need to go up one directory level.

something like the following should work:

'components.gyp:onc_component'

[tfarina, 2014-04-18 15:35:16]

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi
File components/wifi.gypi (right):

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:12: '../components/components.gyp:onc_component',
On 2014/04/18 15:30:37, tfarina wrote:
> also, this path is "wrong", you don't need to go up one directory level.
> 
> something like the following should work:
> 
> 'components.gyp:onc_component'

just 'onc_component' also works, because you are included by components.gyp
already.

[mef, 2014-04-18 15:38:24]

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi
File components/wifi.gypi (right):

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:12: '../components/components.gyp:onc_component',
On 2014/04/18 15:35:17, tfarina wrote:
> On 2014/04/18 15:30:37, tfarina wrote:
> > also, this path is "wrong", you don't need to go up one directory level.
> > 
> > something like the following should work:
> > 
> > 'components.gyp:onc_component'
> 
> just 'onc_component' also works, because you are included by components.gyp
> already.
sgtm, I'll do that.

https://codereview.chromium.org/54323003/diff/2230002/components/wifi.gypi#ne...
components/wifi.gypi:24: 'wifi/fake_wifi_service.cc',
On 2014/04/18 15:28:07, tfarina wrote:
> On 2014/04/18 14:57:40, mef wrote:
> > On 2014/04/18 00:46:54, tfarina wrote:
> > > this is including test code in production code.
> > 
> > Should I move it to //src/chrome/browser/extensions/api/networking_private
as
> it
> > is only used in
> >
>
//src/chrome/browser/extensions/api/networking_private/networking_private_apitest.cc?
> 
> either that or what I have initially thought in doing, create a
> wifi_test_support target.
Sounds good, I'll do that