Separate CryptoVerifyImpl into its own file.

Separate CryptoVerifyImpl into its own file.

This eliminates some dependencies from NetworkingPrivateServiceClient
that will allow it to be moved to src/extensions.

It also changes NetworkingPrivatCrypto from a class to a namespace since that is how we use it.

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

[stevenjb, 2014-08-05 23:32:21]

I have a CL that is almost ready that moves the majority of the
networkingPrivate implementation to src/extensions. 

The only part that can't be moved is the cert verification code on win/mac since
the Windows implementation runs in the chrome utility process. With this
separation we can move NetworkingPrivateServiceClient and leave the cert
verification implementation in Chrome.

[pneubeck (no reviews), 2014-08-06 16:01:39]

lgtm

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

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/crypto_verify_impl.cc:14: bool
VerifyCredentials(const CryptoVerifyImpl::Credentials& credentials) {
nit: indentation is off

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/crypto_verify_impl.cc:27:
NetworkingPrivateServiceClient::CryptoVerify::Create() {
Couldn't the caller as well just call 'new CryptoVerifyImpl'?

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/crypto_verify_impl.h
(right):

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/crypto_verify_impl.h:8:
#include
"chrome/browser/extensions/api/networking_private/networking_private_service_client.h"
#include "base/compiler_specific.h
#include "base/macros.h"

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

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:71:
// verification is unavailable, in which case those functions will fail.
Sorry, I don't understand the second half of the second sentence.

If this returns NULL, which functions will fail? Do you mean
NetworkingPrivateServiceClient::Verify* ?
Should this comment be at the c'tor of NetworkingPrivateServiceClient, when it
gets a NULL ptr for |crypto_verify|?

https://codereview.chromium.org/442073003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private/networking_private_crypto.h
(right):

https://codereview.chromium.org/442073003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/networking_private/networking_private_crypto.h:47:
// The trusted public key as a DER-encoded PKCS#1 RSAPublicKey
nit: fits on one line?

What means "The" here? What is this specific public key used for / what's its
purpose?
I know you didn't change this part, but if you happen to know, please make this
comment a bit more verbose.

[tbarzic, 2014-08-06 16:18:41]

lgtm

[stevenjb, 2014-08-06 16:52:14]

mef@ - ptal when you have a chance, thanks

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

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/crypto_verify_impl.cc:14: bool
VerifyCredentials(const CryptoVerifyImpl::Credentials& credentials) {
On 2014/08/06 16:01:38, pneubeck wrote:
> nit: indentation is off

Done.

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/crypto_verify_impl.cc:27:
NetworkingPrivateServiceClient::CryptoVerify::Create() {
On 2014/08/06 16:01:38, pneubeck wrote:
> Couldn't the caller as well just call 'new CryptoVerifyImpl'?
NetworkingPrivateServiceClient will be getting moved to src/extensions and will
not have access to CryptoVerifyImpl. At that point I will need to move this to a
delegate to avoid linker dependencies, but I wanted to enforce that separation
here (networking_private_service_client.cc does not include
crypto_verify_impl.h).

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/networking_private/crypto_verify_impl.h
(right):

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/crypto_verify_impl.h:8:
#include
"chrome/browser/extensions/api/networking_private/networking_private_service_client.h"
On 2014/08/06 16:01:38, pneubeck wrote:
> #include "base/compiler_specific.h
> #include "base/macros.h"

Done.

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

https://codereview.chromium.org/442073003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:71:
// verification is unavailable, in which case those functions will fail.
On 2014/08/06 16:01:38, pneubeck wrote:
> Sorry, I don't understand the second half of the second sentence.
> 
> If this returns NULL, which functions will fail? Do you mean
> NetworkingPrivateServiceClient::Verify* ?
> Should this comment be at the c'tor of NetworkingPrivateServiceClient, when it
> gets a NULL ptr for |crypto_verify|?

Yes, will clarify and add a similar comment to the constructor.

https://codereview.chromium.org/442073003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private/networking_private_crypto.h
(right):

https://codereview.chromium.org/442073003/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/networking_private/networking_private_crypto.h:47:
// The trusted public key as a DER-encoded PKCS#1 RSAPublicKey
On 2014/08/06 16:01:38, pneubeck wrote:
> nit: fits on one line?
> 
> What means "The" here? What is this specific public key used for / what's its
> purpose?
> I know you didn't change this part, but if you happen to know, please make
this
> comment a bit more verbose.

Made one line. I don't know how to clarify this comment.

[stevenjb, 2014-08-07 22:34:39]

ping

[mef, 2014-08-07 22:57:39]

lgtm

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

https://codereview.chromium.org/442073003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/networking_private/networking_private_service_client.h:110:
// which case Verify* will return a 'not implemented' error.
nit: It returns kErrorNotSupported.

https://codereview.chromium.org/442073003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private/networking_private_crypto.h
(right):

https://codereview.chromium.org/442073003/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/networking_private/networking_private_crypto.h:15:
namespace networking_private_crypto {
What's the reason to switch from class to namespace?

[stevenjb, 2014-08-08 00:02:30]

https://codereview.chromium.org/442073003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/api/networking_private/networking_private_crypto.h
(right):

https://codereview.chromium.org/442073003/diff/60001/chrome/common/extensions...
chrome/common/extensions/api/networking_private/networking_private_crypto.h:15:
namespace networking_private_crypto {
On 2014/08/07 22:57:39, mef wrote:
> What's the reason to switch from class to namespace?

It's confusing to use a class just to namespace functions (it certainly confused
me). It implies that it carries state, etc. A namespace makes it clear these are
just utility methods. (Additional clarity is especially helpful when there is a
complex class dependency tree like there is here).

[stevenjb, 2014-08-08 00:02:53]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-08-08 00:54:52]

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

[commit-bot: I haz the power, 2014-08-08 05:50:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-08 05:55:38]

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

[commit-bot: I haz the power, 2014-08-08 05:55:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2014-08-08 16:38:05]

+asargent@ for c/utility/extensions OWNER

[asargent_no_longer_on_chrome, 2014-08-08 18:13:48]

chrome/utility/extensions/extensions_handler.cc lgtm

[stevenjb, 2014-08-08 18:15:49]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-08-08 18:19:08]

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

[commit-bot: I haz the power, 2014-08-08 18:26:41]

Change committed as 288396