OwnerKeyUtil is moved to components/ownership.

OwnerKeyUtil is moved to components/ownership.

BUG=398856
TEST=existing browser_tests

Committed: https://crrev.com/39e36781429195b553416fe35e2eb989c593a5f9
Cr-Commit-Position: refs/heads/master@{#292616}

[ygorshenin1, 2014-08-21 15:31:11]

Nikita, Julian - could you please take a quick look at the CL to check
soundness?

[Nikita (slow), 2014-08-21 16:25:46]

lgtm

[pastarmovj, 2014-08-25 09:54:02]

lgtm

[ygorshenin1, 2014-08-25 10:23:31]

+ wtc@ for crypto/* and net/*
+ stevenjb@ for chromeos/*
+ erikwright@ for components/*

[erikwright (departed), 2014-08-25 15:51:07]

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:59:
ownership::OwnerKeyUtil::Create();
Ideally you would avoid looking up the path again here, perhaps by passing the
OwnerKeyUtil from chrome_browser_main_chromeos. Or maybe by doing the ownership
verification in chrome_browser_main_... and just passing that status into
KioskAppManager.

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:190:
ownership::OwnerKeyUtil::Create());
This is the file where things should be configured with platform specific
dependencies, parameters, etc. So look up the key path here.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DEPS
File components/ownership/DEPS (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DE...
components/ownership/DEPS:2: "+chromeos",
This dependency seems unnecessary (see other comments).

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util.cc (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.cc:45: base::FilePath owner_key_path;
Accept this path as an input from the caller.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util_chromeos.h (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util_chromeos.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Remove the 'chromeos' from this filename. There is nothing chromeos specific
about it. If no non-chromeos code depends on this component it will not be
compiled anyway.

[stevenjb, 2014-08-25 15:57:30]

lgtm w/nits

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util.cc (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.cc:46:
CHECK(PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path));
nit: I am not a fan of assertions with side effects. I think the following
pattern is more clear:
if (!Get(...))
  LOG(FATAL) ...

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.cc:47: return new
OwnerKeyUtilChromeOS(owner_key_path);
nit: You could #ifdef out all of Create for OS_CHROMEOS and remove the chromeos
dependencies in this file, then provide an OwnerKeyUtil::Create implementation
in the _chromeos.cc file. Since we only have non-chromeos and chromeos, this is
fine as-is, but if we add other OS specific implementations the #ifdefs here
will become complicated.

[wtc, 2014-08-25 19:27:24]

On 2014/08/25 10:23:31, ygorshenin1 wrote:
> + wtc@ for crypto/* and net/*

Patch set 5 doesn't change any files in crypto/* and net/*.

Which files should I review?

[wtc, 2014-08-25 19:41:51]

Review comments on patch set 5:

I think I figured out how to review the CL. I believe the dependency on 'net' is
unnecessary. Please see detailed comments below.

https://codereview.chromium.org/494093002/diff/100001/components/ownership.gypi
File components/ownership.gypi (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership.gy...
components/ownership.gypi:24: '<(DEPTH)/net/net.gyp:net',

This dependency on <(DEPTH)/net/net.gyp:net doesn't seem necessary.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/BU...
File components/ownership/BUILD.gn (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/BU...
components/ownership/BUILD.gn:25: "//net",

The dependency on "//net" doesn't seem necessary.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DEPS
File components/ownership/DEPS (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DE...
components/ownership/DEPS:4: "+net",

"+net" doesn't seem necessary.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.h:17: #include "net/cert/x509_util_nss.h"

This file doesn't contain "net::", so I believe this file doesn't need to
include "net/cert/x509_util_nss.h".

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util_chromeos_unittest.cc (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util_chromeos_unittest.cc:16: #include
"crypto/rsa_private_key.h"

This file doesn't contain "crypto::", so I believe this file doesn't need to
include "crypto/rsa_private_key.h".

[ygorshenin1, 2014-08-26 13:56:55]

Patchset #6 (id:120001) has been deleted

[ygorshenin1, 2014-08-26 14:53:36]

PTAL

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_manager.cc (right):

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_manager.cc:59:
ownership::OwnerKeyUtil::Create();
On 2014/08/25 15:51:06, erikwright wrote:
> Ideally you would avoid looking up the path again here, perhaps by passing the
> OwnerKeyUtil from chrome_browser_main_chromeos. Or maybe by doing the
ownership
> verification in chrome_browser_main_... and just passing that status into
> KioskAppManager.

Done.

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/494093002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:190:
ownership::OwnerKeyUtil::Create());
On 2014/08/25 15:51:06, erikwright wrote:
> This is the file where things should be configured with platform specific
> dependencies, parameters, etc. So look up the key path here.

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership.gypi
File components/ownership.gypi (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership.gy...
components/ownership.gypi:24: '<(DEPTH)/net/net.gyp:net',
On 2014/08/25 19:41:50, wtc wrote:
> 
> This dependency on <(DEPTH)/net/net.gyp:net doesn't seem necessary.

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/BU...
File components/ownership/BUILD.gn (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/BU...
components/ownership/BUILD.gn:25: "//net",
On 2014/08/25 19:41:50, wtc wrote:
> 
> The dependency on "//net" doesn't seem necessary.

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DEPS
File components/ownership/DEPS (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DE...
components/ownership/DEPS:2: "+chromeos",
On 2014/08/25 15:51:07, erikwright wrote:
> This dependency seems unnecessary (see other comments).

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/DE...
components/ownership/DEPS:4: "+net",
I'd prefer to leave "+net" as x509_util_nss.h is useful.

On 2014/08/25 19:41:51, wtc wrote:
> 
> "+net" doesn't seem necessary.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util.cc (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.cc:45: base::FilePath owner_key_path;
On 2014/08/25 15:51:07, erikwright wrote:
> Accept this path as an input from the caller.

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.cc:46:
CHECK(PathService::Get(chromeos::FILE_OWNER_KEY, &owner_key_path));
On 2014/08/25 15:57:30, stevenjb wrote:
> nit: I am not a fan of assertions with side effects. I think the following
> pattern is more clear:
> if (!Get(...))
>   LOG(FATAL) ...

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.cc:47: return new
OwnerKeyUtilChromeOS(owner_key_path);
On 2014/08/25 15:57:29, stevenjb wrote:
> nit: You could #ifdef out all of Create for OS_CHROMEOS and remove the
chromeos
> dependencies in this file, then provide an OwnerKeyUtil::Create implementation
> in the _chromeos.cc file. Since we only have non-chromeos and chromeos, this
is
> fine as-is, but if we add other OS specific implementations the #ifdefs here
> will become complicated.

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util.h:17: #include "net/cert/x509_util_nss.h"
x509_util_nss.h contains very useful typedef of PK11SlotInfo
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_util...)
and all necessary inclusions.

On 2014/08/25 19:41:51, wtc wrote:
> 
> This file doesn't contain "net::", so I believe this file doesn't need to
> include "net/cert/x509_util_nss.h".

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util_chromeos.h (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util_chromeos.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/08/25 15:51:07, erikwright wrote:
> Remove the 'chromeos' from this filename. There is nothing chromeos specific
> about it. If no non-chromeos code depends on this component it will not be
> compiled anyway.

Done.

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
File components/ownership/owner_key_util_chromeos_unittest.cc (right):

https://codereview.chromium.org/494093002/diff/100001/components/ownership/ow...
components/ownership/owner_key_util_chromeos_unittest.cc:16: #include
"crypto/rsa_private_key.h"
On 2014/08/25 19:41:51, wtc wrote:
> 
> This file doesn't contain "crypto::", so I believe this file doesn't need to
> include "crypto/rsa_private_key.h".

Done.

[stevenjb, 2014-08-26 16:02:13]

https://codereview.chromium.org/494093002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service.cc (right):

https://codereview.chromium.org/494093002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service.cc:53: }
So, while I do like removing the reference to chromeos::FILE_KEY_OWNER from the
component, we now appear to have this copy/pasted several times in the Chrome
code. Could we perhaps make this public, or create a public helper method that
is used throughout the chrome code?

[ygorshenin1, 2014-08-26 16:31:16]

PTAL

https://codereview.chromium.org/494093002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service.cc (right):

https://codereview.chromium.org/494093002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service.cc:53: }
On 2014/08/26 16:02:12, stevenjb wrote:
> So, while I do like removing the reference to chromeos::FILE_KEY_OWNER from
the
> component, we now appear to have this copy/pasted several times in the Chrome
> code. Could we perhaps make this public, or create a public helper method that
> is used throughout the chrome code?

Done.

[stevenjb, 2014-08-26 16:36:44]

Cheers. LGTM

https://codereview.chromium.org/494093002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/494093002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service.h:84: static
scoped_refptr<ownership::OwnerKeyUtil> MakeOwnerKeyUtil();
nit: Non ForTesting method before testing methods.

[erikwright (departed), 2014-08-26 16:49:55]

It would be nice to not have to construct the OwnerKeyUtil in so many places. If
you can't actually share an instance, why not make a single static
CreateOwnerKeyUtil somewhere in chrome/browser/chromeos/.

https://codereview.chromium.org/494093002/diff/140001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/140001/components/ownership/ow...
components/ownership/owner_key_util.h:72: class OWNERSHIP_EXPORT OwnerKeyUtil
It may still make sense to have a pure-virtual API for testing purposes, but I
think you can probably get rid of Create and allow clients to directly
instantiate OwnerKeyUtilImpl.

I would also suggest that you could just add some constructors that allow you to
supply the key data directly (vs. from a file) and then have a single
non-virtual implementation for test/production.

[wtc, 2014-08-26 18:00:30]

Review comments on patch set 7:

https://codereview.chromium.org/494093002/diff/160001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/160001/components/ownership/ow...
components/ownership/owner_key_util.h:17: #include "net/cert/x509_util_nss.h"

On 2014/08/26 14:53:35, ygorshenin1 wrote:
> x509_util_nss.h contains very useful typedef of PK11SlotInfo
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_util...)
> and all necessary inclusions.

It seems wrong to include a header just for a forward declaration in that
header. If you really want to do this, "crypto/rsa_private_key.h" is a better
choice -- it'll allow you to get rid of the forward declaration of RSAPrivateKey
on lines 23-25.

I think it is better to add a forward declaration of PK11SlotInfo to this file.

[ygorshenin1, 2014-08-27 14:14:15]

Patchset #9 (id:200001) has been deleted

[ygorshenin1, 2014-08-27 20:39:12]

PTAL

https://codereview.chromium.org/494093002/diff/140001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/140001/components/ownership/ow...
components/ownership/owner_key_util.h:72: class OWNERSHIP_EXPORT OwnerKeyUtil
It seems that there're no need in pure virtual base, so I made a single virtual
implementation + moved to components/ownership MockOwnerKeyUtil which is
designed only for testing.

I've also make a static method in chrome/* for OwnerKeyUtil instantiation - it's
an OwnerSettingsService::MakeOwnerKeyUtil().

On 2014/08/26 16:49:54, erikwright wrote:
> It may still make sense to have a pure-virtual API for testing purposes, but I
> think you can probably get rid of Create and allow clients to directly
> instantiate OwnerKeyUtilImpl.
> 
> I would also suggest that you could just add some constructors that allow you
to
> supply the key data directly (vs. from a file) and then have a single
> non-virtual implementation for test/production.

https://codereview.chromium.org/494093002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/ownership/owner_settings_service.h (right):

https://codereview.chromium.org/494093002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/ownership/owner_settings_service.h:84: static
scoped_refptr<ownership::OwnerKeyUtil> MakeOwnerKeyUtil();
On 2014/08/26 16:36:43, stevenjb wrote:
> nit: Non ForTesting method before testing methods.

Done.

https://codereview.chromium.org/494093002/diff/160001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/160001/components/ownership/ow...
components/ownership/owner_key_util.h:17: #include "net/cert/x509_util_nss.h"
On 2014/08/26 18:00:30, wtc wrote:
> 
> On 2014/08/26 14:53:35, ygorshenin1 wrote:
> > x509_util_nss.h contains very useful typedef of PK11SlotInfo
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/x509_util...)
> > and all necessary inclusions.
> 
> It seems wrong to include a header just for a forward declaration in that
> header. If you really want to do this, "crypto/rsa_private_key.h" is a better
> choice -- it'll allow you to get rid of the forward declaration of
RSAPrivateKey
> on lines 23-25.
> 
> I think it is better to add a forward declaration of PK11SlotInfo to this
file.

Done.

[wtc, 2014-08-27 22:12:53]

Patch set 9 LGTM.

https://codereview.chromium.org/494093002/diff/220001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/220001/components/ownership/ow...
components/ownership/owner_key_util.h:20: #include "crypto/rsa_private_key.h"

Just wanted to clarify that I would use forward declarations of class
RSAPrivateKey and the NSS PK11SlotInfo struct.

[ygorshenin1, 2014-08-28 15:02:58]

On 2014/08/27 22:12:53, wtc wrote:
> Patch set 9 LGTM.
> 
>
https://codereview.chromium.org/494093002/diff/220001/components/ownership/ow...
> File components/ownership/owner_key_util.h (right):
> 
>
https://codereview.chromium.org/494093002/diff/220001/components/ownership/ow...
> components/ownership/owner_key_util.h:20: #include "crypto/rsa_private_key.h"
> 
> Just wanted to clarify that I would use forward declarations of class
> RSAPrivateKey and the NSS PK11SlotInfo struct.

[ygorshenin1, 2014-08-28 15:03:21]

Erik, could you please take another look?

[erikwright (departed), 2014-08-28 16:31:50]

(a) Instead of having the OwnerKeyUtil statically constructed in
OwnerSettingsService::OwnerSettingsService, make it a constructor argument. In
the factory you pass in the result of MakeOwnerKeyUtil. In the test, you pass in
your test object. This will get rid of a bunch of "for test" code. This is the
recommend way of passing dependencies to a service.
(b) Instead of using the factory infrastructure to create your service for
testing, just instantiate the service. For tests of other code that access it di

I have two concerns.

(1) Mock by override is brittle. If you really think you need a mock for the
OwnerKeyUtil, just put back in the base pure-virtual.
(2) Your mock does not actually seem to obey the contract of the real object.
The real object appears to generally static. Its data remains unchanged, if I
understand, from the time it is constructed. In your tests, you pass an
OwnerKeyUtil which changes during the lifetime of its dependents.
(3) There's just no need for a mock object here. Some very small changes to use
better practices for designing and implementing the various classes involved
could simplify things quite a bit.

https://codereview.chromium.org/494093002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/494093002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:158:
OwnerSettingsService::SetOwnerKeyUtilForTesting(owner_key_util_);
It's a pain that the other code under test statically accesses the
OwnerSettingsService. Instead of SetOwnerKeyUtilForTesting, follow my
recommendation elsewhere to make this a constructor argument.

Call OwnerSettingsServiceFactory::SetTestingFactory and pass a local free
function. You'll need to use a global OwnerKeyUtil* to make |owner_key_util_|
available to this free function.

This would not be required if code under test accepted its dependencies as
constructor arguments. Its factory could look up the dependency via other
factories while the test code would construct instances directly.

https://codereview.chromium.org/494093002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:360:
owner_key_util_->SetPublicKey(GetOwnerPublicKey());
For these two tests that want to use a different key:

If you are confident that the OwnerSettingsService is not instantiated before
the test is invoked (i.e. in SetUp) you could just replace the static
OwnerKeyUtil that will be used by the testing factory (before it is called).

Otherwise, add a virtual GetKeyData or whatever method to your test fixture.
Subclass the test fixture and override this method for your "DifferentOwner"
test cases.

https://codereview.chromium.org/494093002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
(right):

https://codereview.chromium.org/494093002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation_unittest.cc:49:
OwnerSettingsService::SetOwnerKeyUtilForTesting(owner_key_util_);
Your pattern is:

Constructor: Create MockOwnerKeyUtil
SetUp: Pass MockOwnerKeyUtil to service
Test: Mutate MockOwnerKeyUtil
Test: Execute test

Also, you use the whole factory infrastructure instead of just constructing your
service. Just do this:

TEST_F(...) {
  OwnerSettingsService service(new OwnerKeyUtil(static_key_data));
  ...
}

Get rid of MockOwnerKeyUtil, and add an alternative constructor to OwnerKeyUtil
that can be initialized with static data instead of a path.

[ygorshenin1, 2014-08-28 18:17:20]

After discussion with Erik pure base OwnerKeyUtil was restored. PTAL.

[erikwright (departed), 2014-08-28 18:39:58]

https://codereview.chromium.org/494093002/diff/260001/components/ownership/mo...
File components/ownership/mock_owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/mo...
components/ownership/mock_owner_key_util.h:23: // OwnerKeyUtil:
OwnerKeyUtil implementation

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:5: #ifndef
COMPONENTS_OWNERSHIP_OWNER_KEY_UTIL_H_
Please add class comments for each of the classes in this file.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:92: OwnerKeyUtil();
This is not required, since you will remove DISALLOW...

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:94: virtual ~OwnerKeyUtil();
This can be inline defined. Although, in this case, you will have an
implementation file for Public/PrivateKey, for consistency with other
pure-virtual classes inline it.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:99:
DISALLOW_COPY_AND_ASSIGN(OwnerKeyUtil);
This is not required, because this class is pure-virtual. Ensure it is in both
implementations, though.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
File components/ownership/owner_key_util_impl.cc (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.cc:28: LOG_IF(ERROR,
base::SysInfo::IsRunningOnChromeOS())
Is this LOG_IF actually required, given the "#if defined"?

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
File components/ownership/owner_key_util_impl.h (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:8: #include "base/basictypes.h"
what do you need from basictypes that's not in macros?

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:12: #include
"base/memory/ref_counted.h"
not required.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:23: // returns true and populates
|output|.  False on failure.
These comments should be in the pure-virtual file.

In this implementation, you should have:

// OwnerKeyUtil implementation
virtual bool ... OVERRIDE;
virtual ...
etc.

A class comment, and possibly a constructor comment, should explain any
implementation-specific details such as the fact that this class is
filesystem-backed.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:39: virtual ~OwnerKeyUtilImpl();
Since you don't have any 'friends' I guess this can be private.

[ygorshenin1, 2014-08-28 18:59:05]

PTAL

https://codereview.chromium.org/494093002/diff/260001/components/ownership/mo...
File components/ownership/mock_owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/mo...
components/ownership/mock_owner_key_util.h:23: // OwnerKeyUtil:
On 2014/08/28 18:39:57, erikwright wrote:
> OwnerKeyUtil implementation

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:5: #ifndef
COMPONENTS_OWNERSHIP_OWNER_KEY_UTIL_H_
On 2014/08/28 18:39:57, erikwright wrote:
> Please add class comments for each of the classes in this file.

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:92: OwnerKeyUtil();
On 2014/08/28 18:39:57, erikwright wrote:
> This is not required, since you will remove DISALLOW...

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:94: virtual ~OwnerKeyUtil();
On 2014/08/28 18:39:57, erikwright wrote:
> This can be inline defined. Although, in this case, you will have an
> implementation file for Public/PrivateKey, for consistency with other
> pure-virtual classes inline it.

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util.h:99:
DISALLOW_COPY_AND_ASSIGN(OwnerKeyUtil);
On 2014/08/28 18:39:57, erikwright wrote:
> This is not required, because this class is pure-virtual. Ensure it is in both
> implementations, though.

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
File components/ownership/owner_key_util_impl.cc (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.cc:28: LOG_IF(ERROR,
base::SysInfo::IsRunningOnChromeOS())
On 2014/08/28 18:39:58, erikwright wrote:
> Is this LOG_IF actually required, given the "#if defined"?

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
File components/ownership/owner_key_util_impl.h (right):

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:8: #include "base/basictypes.h"
Nothing, I forget to remove it, sorry.

On 2014/08/28 18:39:58, erikwright wrote:
> what do you need from basictypes that's not in macros?

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:12: #include
"base/memory/ref_counted.h"
On 2014/08/28 18:39:58, erikwright wrote:
> not required.

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:23: // returns true and populates
|output|.  False on failure.
On 2014/08/28 18:39:58, erikwright wrote:
> These comments should be in the pure-virtual file.
> 
> In this implementation, you should have:
> 
> // OwnerKeyUtil implementation
> virtual bool ... OVERRIDE;
> virtual ...
> etc.
> 
> A class comment, and possibly a constructor comment, should explain any
> implementation-specific details such as the fact that this class is
> filesystem-backed.

Done.

https://codereview.chromium.org/494093002/diff/260001/components/ownership/ow...
components/ownership/owner_key_util_impl.h:39: virtual ~OwnerKeyUtilImpl();
On 2014/08/28 18:39:58, erikwright wrote:
> Since you don't have any 'friends' I guess this can be private.

Done.

[erikwright (departed), 2014-08-28 19:41:38]

components/ LGTM with nits.

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
components/ownership/owner_key_util.h:11: #include "base/basictypes.h"
same comment.

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
components/ownership/owner_key_util.h:31: // This class is a ref-counted wrapper
around plain public key.
_a_ plain public key.

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
components/ownership/owner_key_util.h:76: class OWNERSHIP_EXPORT OwnerKeyUtil
Do pure-virtual classes need EXPORT? Check for consistency with other code.

[ygorshenin1, 2014-08-29 10:28:58]

Many thanks!

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
File components/ownership/owner_key_util.h (right):

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
components/ownership/owner_key_util.h:11: #include "base/basictypes.h"
I need basictypes.h for the definition of uint8.

On 2014/08/28 19:41:38, erikwright wrote:
> same comment.

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
components/ownership/owner_key_util.h:31: // This class is a ref-counted wrapper
around plain public key.
On 2014/08/28 19:41:38, erikwright wrote:
> _a_ plain public key.

Done.

https://codereview.chromium.org/494093002/diff/280001/components/ownership/ow...
components/ownership/owner_key_util.h:76: class OWNERSHIP_EXPORT OwnerKeyUtil
On 2014/08/28 19:41:38, erikwright wrote:
> Do pure-virtual classes need EXPORT? Check for consistency with other code.

Done.

[ygorshenin1, 2014-08-29 10:29:20]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-08-29 10:30:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/494093002/300001

[ygorshenin1, 2014-08-29 10:45:37]

The CQ bit was unchecked by ygorshenin@chromium.org

[ygorshenin1, 2014-08-29 10:47:12]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-08-29 10:47:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/494093002/320001

[ygorshenin1, 2014-08-29 11:42:15]

The CQ bit was unchecked by ygorshenin@chromium.org

[ygorshenin1, 2014-08-29 12:01:55]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-08-29 12:02:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/494093002/340001

[commit-bot: I haz the power, 2014-08-29 12:59:47]

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

[commit-bot: I haz the power, 2014-08-29 13:09:58]

Committed patchset #15 (id:340001) as 7ea6e9bbe3fb1e8fd091f6b592dc48bd47f5d946

[commit-bot: I haz the power, 2014-09-10 03:08:02]

Patchset 15 (id:??) landed as
https://crrev.com/39e36781429195b553416fe35e2eb989c593a5f9
Cr-Commit-Position: refs/heads/master@{#292616}