Add NetworkingPrivateDelegate class

Add NetworkingPrivateDelegate class

This CL introduces NetworkingPrivateDelegate and moves (almost) all extension API specific code into a common NetworkingPrivateApi class.

The ChromeOS specific Delegate is now in networking_private_api_chromeos.cc (now a profile keyed service).

The Windows/Mac specific Delegate is now in networking_private_service_client.cc. networking_private_api_nonchromeos.cc has been deleted.

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

[stevenjb, 2014-07-10 02:25:23]

Strictly re-factoring in prep for a Linux implementation, no changed
functionality.

[mef, 2014-07-10 15:09:12]

Nice, like to see some de-duplication.

https://codereview.chromium.org/378103002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/378103002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
2014?

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

https://codereview.chromium.org/378103002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:65:
VerificationProperties::FromValue(*properties);
Are |properties| just |VerificationProperties|? If so, should they be called
|verification_properties|?

[stevenjb, 2014-07-10 18:04:20]

https://codereview.chromium.org/378103002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/378103002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/07/10 15:09:11, mef wrote:
> 2014?

Done.

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

https://codereview.chromium.org/378103002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:65:
VerificationProperties::FromValue(*properties);
On 2014/07/10 15:09:11, mef wrote:
> Are |properties| just |VerificationProperties|? If so, should they be called
> |verification_properties|?

Yes. Done. (They are called 'verification_properties' in the callers).

[stevenjb, 2014-07-14 19:01:38]

ping

[pneubeck (no reviews), 2014-07-14 21:33:59]

I'm not happy about the naming, but maybe you already intended to fix that in a
follow up.
E.g.
NetworkingPrivateDelegate should be NetworkingServiceBase
NetworkingPrivateChromeOS should be NetworkingServiceChromeOS
NetworkingPrivateServiceClient should be NetworkingServiceNonChromeOS
(note that the 'Private' is not required for lower layers)

I commented at a few places on the cyclic dependency between the services and
the API. The API should depend on the service and factories but not the other
way round.
Then, all of the Service stuff should be moved out of the extension api folder.

Btw. if we had a common platform independent place for the service definition,
we could also have there some common definitions like the VerificationProperties
that could be used by both chromeos and non-chromeos.
That would be similar to how ONC is already shared across platforms.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:47:
bool NetworkingPrivateGetPropertiesFunction::RunAsync() {
Feel free to skip this comment and the next two, because this CL is more about
moving stuff. I can do a follow up an change it to the currently idiomatic
functions.

you shouldn't override RunAsync anymore, but instead Run()
and return at the end of this function: 
  return RespondLater()

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:61:
SetResult(result.release());
the new style of returning results in extension functions is defined here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

In this case, it should be:
Respond(OneArgument(result.release()))

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:66:
error_ = error;
should be
Respond(Error(error));

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:406:
verification_properties.Pass(),
The _api.cc should translate into the representation of the next lower layer.
E.g. the delegate could use a struct (like the one that is currently in
ShillManager) and you could do the translation here.
The service_client implementation wouldn't have to access the generated api code
anymore
and the chromeos implementation wouldn't have to translate anything anymore.

A base::DictionaryValue _could_ be this representation of the next layer, but
then it shouldn't depend on the keys of the API.

Accordingly for all verification properties below.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/networking_private_api.h
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.h:13:
#include "base/memory/ref_counted.h"
unused?

missing:
#include "base/macros.h"
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.h:15:
#include "chromeos/dbus/dbus_method_call_status.h"
unused?

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.h:16:
#include "components/keyed_service/core/keyed_service.h"
unused?

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
(left):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
The diff doesn't say 'D' but 'M' for this file.
Is it not removed?

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:54:
input.GetStringWithoutPathExpansion("certificate", &output.certificate);
IMO, a layer violation

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:70:
*error = extensions::networking_private::kErrorInvalidNetworkGuid;
again, I think, a layer violation.
Not so, if the error constants were declared/defined in _delegate.h/.cc

If they're also defined for the API, we would probably have to translate them
:-/

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:81:
chromeos::ProfileHelper::GetUserIdHashFromProfile(
this dependency should be in the factory, to keep the dependencies of the
service minimal.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:90:
// to avoid complexites with the policy code.
complications or complexities?

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:130:
: browser_context_(browser_context) {
as mentioned above, please get the user hash once in the factory and don't store
the BrowserContext.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:223:
failure_callback.Run(error);
optional nit:
the old comment was no so bad:
-    // Do not allow configuring a non-shared network from a non-primary user
-    // context.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:365:
chromeos::NetworkPortalDetector::Get();
should be passed in by the factory through the constructor

and yes, I suggest to do the same with all network handlers as well.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:385:
api::networking_private::NETWORK_TYPE_ETHERNET));
this translation should be in the _api.cc

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_delegate.h:35:
virtual void GetProperties(const std::string& guid,
None of the functions explain what the results should be.
E.g. a String result could be many things, but in some cases means GUID.
similarly for DictionaryValue, sometimes means ONC network configuration.

Having semantic callback definitions would give better documentation, e.g. an
GuidCallback, or an NetworkConfigCallback.

Alternatively, each function should have an explanation of the expected results.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc:17:
NetworkingPrivateDelegate* NetworkingPrivateDelegate::GetForBrowserContext(
to implement this function here is very surprising.
I would expect it either in networking_private_delegate_chromeos.cc and 
networking_private_delegate_nonchromeos.cc

or in networking_private_api_chromeos.cc and
etworking_private_api_nonchromeos.cc

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc:41:
DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
not required?
I don't find any access to the ExtensionSystem.

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

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:14:
#include
"chrome/browser/extensions/api/networking_private/networking_private_api.h"
this should be avoided, because this introduces a cyclic dependency.
api -> (delegate) service_client -> api

chrome/common/extensions/api/networking_private.h from below should be
sufficient to use the generated code.

but I would even avoid that, to get a clear layering:
networkingPrivate API
networkingService
chromeos/network/*handler*

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:51:
// CryptoVerify
seems out of place

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:234:
////////////////////////////////////////////////////////////////////////////////
?

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:10:
#include "content/public/browser/browser_context.h"
you don't access any member of BrowserContext. No need to include it.

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

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
shouldn't be changed?

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc
(right):

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:7:
#include
"chrome/browser/extensions/api/networking_private/networking_private_delegate.h"
unused?

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:10:
#include "content/public/browser/browser_context.h"
not required

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:42:
DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
should be possible to remove here as well.

[mef, 2014-07-14 21:43:26]

On 2014/07/14 21:33:59, pneubeck wrote:
> I'm not happy about the naming, but maybe you already intended to fix that in
a
> follow up.
> E.g.
> NetworkingPrivateDelegate should be NetworkingServiceBase
> NetworkingPrivateChromeOS should be NetworkingServiceChromeOS
> NetworkingPrivateServiceClient should be NetworkingServiceNonChromeOS
> (note that the 'Private' is not required for lower layers)
> 
> I commented at a few places on the cyclic dependency between the services and
> the API. The API should depend on the service and factories but not the other
> way round.
> Then, all of the Service stuff should be moved out of the extension api
folder.
> 
> Btw. if we had a common platform independent place for the service definition,
> we could also have there some common definitions like the
VerificationProperties
> that could be used by both chromeos and non-chromeos.
> That would be similar to how ONC is already shared across platforms.
BTW, davidben@ is working on https://codereview.chromium.org/396463004/ to
implement NetworkingPrivate crypto stuff using OpenSSL,
so at some point it could be shared across all platforms.

[stevenjb, 2014-07-15 00:33:40]

On 2014/07/14 21:33:59, pneubeck wrote:
> I'm not happy about the naming, but maybe you already intended to fix that in
a
> follow up.
> E.g.
> NetworkingPrivateDelegate should be NetworkingServiceBase
> NetworkingPrivateChromeOS should be NetworkingServiceChromeOS
> NetworkingPrivateServiceClient should be NetworkingServiceNonChromeOS
> (note that the 'Private' is not required for lower layers)

I think we have different perspectives on the objectives here.

My only intention is to separate the platform specific implementations of the
extension API from the Extension API glue code, so that we don't copy/paste the
glue code again for the Linux implementation. (Which btw is a good reason not to
use "nonchromeos" anywhere since it will likely have a separate implementation
than ChromeOS and Win/Mac).

While I did initially try to keep dependencies on networking_private.h to a
minimum, that was not a primary goal, and in this iteration I have actually
increased that dependency.

Thus, "Delegate", not "ServiceBase". While it might be possible to use the
Delegate implementations independently, we don't currently have any use cases
for that, and I am not strongly motivated to add duplicate structs, etc, for a
hypothetical use case. (I would not be against a future CL in that direction,
but it's a bigger step than I care to take here).

> 
> I commented at a few places on the cyclic dependency between the services and
> the API. The API should depend on the service and factories but not the other
> way round.
> Then, all of the Service stuff should be moved out of the extension api
folder.

Again, that's not really the model I was striving for here. The API depends on
the Delegate header and the delegates depend on the API header, that's not
actually circular (and is a common pattern).

I understand the temptation to jump to a separate service here, the dependencies
aren't that big, but I honestly think that might actually be more effort to
maintain, and I don't especially want to jump to that in one CL.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/networking_private_api.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:47:
bool NetworkingPrivateGetPropertiesFunction::RunAsync() {
On 2014/07/14 21:33:57, pneubeck wrote:
> Feel free to skip this comment and the next two, because this CL is more about
> moving stuff. I can do a follow up an change it to the currently idiomatic
> functions.
> 
> you shouldn't override RunAsync anymore, but instead Run()
> and return at the end of this function: 
>   return RespondLater()

I see. I think that should be done separately. At least now it only needs to be
done in one place.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:61:
SetResult(result.release());
On 2014/07/14 21:33:57, pneubeck wrote:
> the new style of returning results in extension functions is defined here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> In this case, it should be:
> Respond(OneArgument(result.release()))

Acknowledged.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:66:
error_ = error;
On 2014/07/14 21:33:57, pneubeck wrote:
> should be
> Respond(Error(error));

Acknowledged.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.cc:406:
verification_properties.Pass(),
On 2014/07/14 21:33:57, pneubeck wrote:
> The _api.cc should translate into the representation of the next lower layer.
> E.g. the delegate could use a struct (like the one that is currently in
> ShillManager) and you could do the translation here.
> The service_client implementation wouldn't have to access the generated api
code
> anymore
> and the chromeos implementation wouldn't have to translate anything anymore.
> 
> A base::DictionaryValue _could_ be this representation of the next layer, but
> then it shouldn't depend on the keys of the API.
> 
> Accordingly for all verification properties below.

Yeah, I went back and forth on this. What I think I would prefer to do is make
the keys ONC constants and not use a struct at all, same as we do for Network
properties, but I think instead, rather than creating a duplicate
VerificationProperties struct in NetworkingPrivateDelegate, I will just typedef
the api struct (see my meta-comment).

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/networking_private_api.h
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.h:13:
#include "base/memory/ref_counted.h"
On 2014/07/14 21:33:57, pneubeck wrote:
> unused?
> 
> missing:
> #include "base/macros.h"
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.h:15:
#include "chromeos/dbus/dbus_method_call_status.h"
On 2014/07/14 21:33:57, pneubeck wrote:
> unused?

Done.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api.h:16:
#include "components/keyed_service/core/keyed_service.h"
On 2014/07/14 21:33:57, pneubeck wrote:
> unused?

Done.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
(left):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/07/14 21:33:57, pneubeck wrote:
> The diff doesn't say 'D' but 'M' for this file.
> Is it not removed?

It says D now, not sure what was up before.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:54:
input.GetStringWithoutPathExpansion("certificate", &output.certificate);
On 2014/07/14 21:33:57, pneubeck wrote:
> IMO, a layer violation

Acknowledged.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:70:
*error = extensions::networking_private::kErrorInvalidNetworkGuid;
On 2014/07/14 21:33:57, pneubeck wrote:
> again, I think, a layer violation.
> Not so, if the error constants were declared/defined in _delegate.h/.cc
> 
> If they're also defined for the API, we would probably have to translate them
> :-/
See meta-comment.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:81:
chromeos::ProfileHelper::GetUserIdHashFromProfile(
On 2014/07/14 21:33:58, pneubeck wrote:
> this dependency should be in the factory, to keep the dependencies of the
> service minimal.
Hmm, I'm not convinced of the value in that. This is all "glue" code and tested
at a high level, I don't think it is a good candidate for unit testing where
tracking dependent services individually would be helpful, and I don't think
shifting the dependencies from the implementation to the factory would make this
any easier to maintain.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:90:
// to avoid complexites with the policy code.
On 2014/07/14 21:33:57, pneubeck wrote:
> complications or complexities?

Done.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:130:
: browser_context_(browser_context) {
On 2014/07/14 21:33:58, pneubeck wrote:
> as mentioned above, please get the user hash once in the factory and don't
store
> the BrowserContext.

Acknowledged.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:223:
failure_callback.Run(error);
On 2014/07/14 21:33:58, pneubeck wrote:
> optional nit:
> the old comment was no so bad:
> -    // Do not allow configuring a non-shared network from a non-primary user
> -    // context.

Done.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:365:
chromeos::NetworkPortalDetector::Get();
On 2014/07/14 21:33:57, pneubeck wrote:
> should be passed in by the factory through the constructor
> 
> and yes, I suggest to do the same with all network handlers as well.
See comment above.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:385:
api::networking_private::NETWORK_TYPE_ETHERNET));
On 2014/07/14 21:33:57, pneubeck wrote:
> this translation should be in the _api.cc

Done.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_delegate.h
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_delegate.h:35:
virtual void GetProperties(const std::string& guid,
On 2014/07/14 21:33:58, pneubeck wrote:
> None of the functions explain what the results should be.
> E.g. a String result could be many things, but in some cases means GUID.
> similarly for DictionaryValue, sometimes means ONC network configuration.
> 
> Having semantic callback definitions would give better documentation, e.g. an
> GuidCallback, or an NetworkConfigCallback.
> 
> Alternatively, each function should have an explanation of the expected
results.

It doesn't seem especially valuable to repeat the documentation in the json
file, but I added a reference to that file above.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc:17:
NetworkingPrivateDelegate* NetworkingPrivateDelegate::GetForBrowserContext(
On 2014/07/14 21:33:58, pneubeck wrote:
> to implement this function here is very surprising.
> I would expect it either in networking_private_delegate_chromeos.cc and 
> networking_private_delegate_nonchromeos.cc
> 
> or in networking_private_api_chromeos.cc and
> etworking_private_api_nonchromeos.cc

I have seen this pattern used before. I think that adding a factory dependency
to the implementation is less intuitive than having the platform-independent
getter defined here.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc:41:
DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
On 2014/07/14 21:33:58, pneubeck wrote:
> not required?
> I don't find any access to the ExtensionSystem.
I copy/pasted this, but we don't have any direct dependencies, so I will go
ahead and remove this.

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

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:14:
#include
"chrome/browser/extensions/api/networking_private/networking_private_api.h"
On 2014/07/14 21:33:58, pneubeck wrote:
> this should be avoided, because this introduces a cyclic dependency.
> api -> (delegate) service_client -> api
> 
> chrome/common/extensions/api/networking_private.h from below should be
> sufficient to use the generated code.
> 
> but I would even avoid that, to get a clear layering:
> networkingPrivate API
> networkingService
> chromeos/network/*handler*
See meta-comment

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:51:
// CryptoVerify
On 2014/07/14 21:33:58, pneubeck wrote:
> seems out of place
These are CryptoVerify overrides. I'll provide the qualified name to be more
clear (+WS).

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:234:
////////////////////////////////////////////////////////////////////////////////
On 2014/07/14 21:33:58, pneubeck wrote:
> ?

Acknowledged.

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:10:
#include "content/public/browser/browser_context.h"
On 2014/07/14 21:33:58, pneubeck wrote:
> you don't access any member of BrowserContext. No need to include it.

Done.

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

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.h:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/07/14 21:33:58, pneubeck wrote:
> shouldn't be changed?

Done.

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc
(right):

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:7:
#include
"chrome/browser/extensions/api/networking_private/networking_private_delegate.h"
On 2014/07/14 21:33:58, pneubeck wrote:
> unused?

Used for convenience getter (NetworkingPrivateDelegate::GetForBrowserContext().

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:10:
#include "content/public/browser/browser_context.h"
On 2014/07/14 21:33:58, pneubeck wrote:
> not required

Done.

https://codereview.chromium.org/378103002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_service_client_factory.cc:42:
DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
On 2014/07/14 21:33:58, pneubeck wrote:
> should be possible to remove here as well.

Done.

[pneubeck (no reviews), 2014-07-15 09:56:28]

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.cc:81:
chromeos::ProfileHelper::GetUserIdHashFromProfile(
On 2014/07/15 00:33:39, stevenjb wrote:
> On 2014/07/14 21:33:58, pneubeck wrote:
> > this dependency should be in the factory, to keep the dependencies of the
> > service minimal.
> Hmm, I'm not convinced of the value in that. This is all "glue" code and
tested
> at a high level, I don't think it is a good candidate for unit testing where
> tracking dependent services individually would be helpful, and I don't think
> shifting the dependencies from the implementation to the factory would make
this
> any easier to maintain.

Moving dependencies to a factory is not only helpful for testing. It's also
helping the reader to keep track of dependencies.
One look at the constructor of the service, and you know all dependencies of the
service.
One look at the create function in the factory and you know where the
dependencies come from.

Construction order is made more explicit/static, that's one of the main points
of the BCKS movement: Are all dependencies available before the new service is
constructed or not? A DCHECK in the factory ensures that without depending on
the actual usage of member functions of the service (e.g. if GetUserIdHash is
not called in a test, this will never crash and hide the broken construction
order).

There are many examples where this helped readability and maintainability a lot.

[pneubeck (no reviews), 2014-07-15 10:13:43]

On 2014/07/15 00:33:40, stevenjb wrote:
> On 2014/07/14 21:33:59, pneubeck wrote:
> > I'm not happy about the naming, but maybe you already intended to fix that
in
> a
> > follow up.
> > E.g.
> > NetworkingPrivateDelegate should be NetworkingServiceBase
> > NetworkingPrivateChromeOS should be NetworkingServiceChromeOS
> > NetworkingPrivateServiceClient should be NetworkingServiceNonChromeOS
> > (note that the 'Private' is not required for lower layers)
> 
> I think we have different perspectives on the objectives here.
> 
> My only intention is to separate the platform specific implementations of the
> extension API from the Extension API glue code, so that we don't copy/paste
the
> glue code again for the Linux implementation.
I fully support this intention, nonetheless we should try to make the code as
understandable as possible to readers. IMO, acyclic dependencies are always
easier to understand and maintain than cyclic ones.
By moving ShillManagerClient::VerificationProperties to a platform-independent
place, no additional struct or translation has to be added, but it reduces the
number of dependencies that are inverse to the layering (api -> service ->
handlers).

> (Which btw is a good reason not to
> use "nonchromeos" anywhere since it will likely have a separate implementation
> than ChromeOS and Win/Mac).
Agreed. That is indeed a good reason to not use nonchromeos. But it is not a
reason to use a seemingly unrelated name 'service_client'.
How about a '_winmac' suffix (I would be careful with _win_mac which might
trigger some automatic build rules)?
Of course, renamings might be better punted to a follow up CL, but I'd like to
know about the intention here.

[pneubeck (no reviews), 2014-07-15 12:06:07]

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc
(right):

https://codereview.chromium.org/378103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_factory_chromeos.cc:17:
NetworkingPrivateDelegate* NetworkingPrivateDelegate::GetForBrowserContext(
On 2014/07/15 00:33:40, stevenjb wrote:
> On 2014/07/14 21:33:58, pneubeck wrote:
> > to implement this function here is very surprising.
> > I would expect it either in networking_private_delegate_chromeos.cc and 
> > networking_private_delegate_nonchromeos.cc
> > 
> > or in networking_private_api_chromeos.cc and
> > etworking_private_api_nonchromeos.cc
> 
> I have seen this pattern used before.
You likely mean the common pattern:

abc.h:
  static f();

abc_chromeos.cc:
  f() {...}

abc_win.cc:
  f() {...}

AFAIK, we always try to keep the names of the files containing declaration and
definition as similar as possible (usually at most a suffix addition).

> I think that adding a factory dependency
> to the implementation is less intuitive than having the platform-independent
> getter defined here.

[stevenjb, 2014-07-15 19:30:57]

Re: file naming - I used networking_private_service_client.cc because it
was already there, and "service client" seemed as good or bad as any for
now. "wifi_service_client" would be better since it is an implementation
based on the WifiService class, but again, I'm not trying to get everything
perfect in a single step.

The problem with moving ShillManagerClient::VerificationProperties  is that
there aren't a lot of places in the code that are lower level than
src/chromeos. Personally I would have just used a dictionary, but once
again I am trying to limit the scope of this re-factor. I have two other
major re-factors in progress which is already one two many.

BCKS?

Please let me know what a minimal set of additional re-factoring would be
required for this CL to be acceptable. Alternately I am happy to review a
different approach.




On Tue, Jul 15, 2014 at 5:06 AM, <pneubeck@chromium.org> wrote:

>
> https://codereview.chromium.org/378103002/diff/80001/
> chrome/browser/extensions/api/networking_private/networking_
> private_factory_chromeos.cc
> File
> chrome/browser/extensions/api/networking_private/networking_
> private_factory_chromeos.cc
> (right):
>
> https://codereview.chromium.org/378103002/diff/80001/
> chrome/browser/extensions/api/networking_private/networking_
> private_factory_chromeos.cc#newcode17
> chrome/browser/extensions/api/networking_private/networking_
> private_factory_chromeos.cc:17:
> NetworkingPrivateDelegate*
> NetworkingPrivateDelegate::GetForBrowserContext(
> On 2014/07/15 00:33:40, stevenjb wrote:
>
>> On 2014/07/14 21:33:58, pneubeck wrote:
>> > to implement this function here is very surprising.
>> > I would expect it either in networking_private_delegate_chromeos.cc
>>
> and
>
>> > networking_private_delegate_nonchromeos.cc
>> >
>> > or in networking_private_api_chromeos.cc and
>> > etworking_private_api_nonchromeos.cc
>>
>
>  I have seen this pattern used before.
>>
> You likely mean the common pattern:
>
> abc.h:
>   static f();
>
> abc_chromeos.cc:
>   f() {...}
>
> abc_win.cc:
>   f() {...}
>
> AFAIK, we always try to keep the names of the files containing
> declaration and definition as similar as possible (usually at most a
> suffix addition).
>
>
>  I think that adding a factory dependency
>> to the implementation is less intuitive than having the
>>
> platform-independent
>
>> getter defined here.
>>
>
> https://codereview.chromium.org/378103002/
>

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

[pneubeck (no reviews), 2014-07-15 20:39:06]

I commented on everything that attract my attention. That doesn't mean that this
has to be all done in one go. Splitting into smaller CLs is preferable, of
course.

this part lgtm. Please consider my comments for follow ups.

[pneubeck (no reviews), 2014-07-15 20:39:44]

On 2014/07/15 20:39:06, pneubeck wrote:
> I commented on everything that attract my attention. That doesn't mean that
this
> has to be all done in one go. Splitting into smaller CLs is preferable, of
> course.
> 
> this part lgtm. Please consider my comments for follow ups.

BCKS = BrowserContextKeyedService

[tbarzic, 2014-07-16 06:38:08]

lgtm

https://codereview.chromium.org/378103002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos.h
(right):

https://codereview.chromium.org/378103002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.h:5:
// These classes implement the chrome.networkingPrivate JavaScript extension
nit: remove this comment
(it looks like copy/paste from networking_private_api.h; I don't think the
comment is needed there either)

[stevenjb, 2014-07-16 19:32:38]

On 2014/07/15 20:39:06, pneubeck wrote:
> I commented on everything that attract my attention. That doesn't mean that
this
> has to be all done in one go. Splitting into smaller CLs is preferable, of
> course.
> 
> this part lgtm. Please consider my comments for follow ups.

OK, thanks, will do. One of the objectives of this is to move the code to
src/extensions and for tracking purposes I want to keep the renaming to a
minimum until that is completed, but I agree that it can be improved.

I will create an issue to track the next level of re-factoring as I envision it.
I will cc: you please comment on anything I missed from your comments or
anything you disagree with. I don't expect to get to it especially soon, but I
do think we generally agree with how to do this "right".

[stevenjb, 2014-07-16 19:34:13]

https://codereview.chromium.org/378103002/diff/140001/chrome/browser/extensio...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos.h
(right):

https://codereview.chromium.org/378103002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/networking_private/networking_private_chromeos.h:5:
// These classes implement the chrome.networkingPrivate JavaScript extension
On 2014/07/16 06:38:08, tbarzic wrote:
> nit: remove this comment
> (it looks like copy/paste from networking_private_api.h; I don't think the
> comment is needed there either)

Done.

[stevenjb, 2014-07-16 19:34:27]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-07-16 19:37:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/378103002/160001

[pneubeck (no reviews), 2014-07-16 19:51:04]

On 2014/07/16 19:32:38, stevenjb wrote:
> On 2014/07/15 20:39:06, pneubeck wrote:
> > I commented on everything that attract my attention. That doesn't mean that
> this
> > has to be all done in one go. Splitting into smaller CLs is preferable, of
> > course.
> > 
> > this part lgtm. Please consider my comments for follow ups.
> 
> OK, thanks, will do. One of the objectives of this is to move the code to
> src/extensions and for tracking purposes I want to keep the renaming to a
> minimum until that is completed, but I agree that it can be improved.
> 
> I will create an issue to track the next level of re-factoring as I envision
it.
> I will cc: you please comment on anything I missed from your comments or
> anything you disagree with. I don't expect to get to it especially soon, but I
> do think we generally agree with how to do this "right".

sounds good.

[commit-bot: I haz the power, 2014-07-16 21:21:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-16 21:48:11]

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

[commit-bot: I haz the power, 2014-07-16 21:48:13]

Try jobs failed on following builders:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[stevenjb, 2014-07-18 02:12:09]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-07-18 02:13:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/378103002/250001

[commit-bot: I haz the power, 2014-07-18 05:59:51]

Change committed as 284019