[Sync] Add keystore encryption info to about:sync

[Sync] Add keystore encryption info to about:sync

This patch adds the following fields under the encryption section of about:sync
- Has Keystore Key: whether the encryption handler has a keystore encryption key
- Migration Time: the time migration was performed, or "Not Migrated" if
  migration hasn't been performed yet
- Passphrase Type: the actual passphrase type (provides more detail than
   Is Using Explicit Passphrase, but stored at a diff layer)

Added sync/api/time.h, which just includes sync/util/time.h but is accessible
from chrome/

BUG=129665
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157499

[Nicolas Zea, 2012-09-13 22:52:19]

PTAL.

[Nicolas Zea, 2012-09-17 17:25:12]

ping!

[akalin, 2012-09-17 18:07:20]

sorry!  some comments.

https://codereview.chromium.org/10917246/diff/4001/chrome/browser/sync/about_...
File chrome/browser/sync/about_sync_util.cc (right):

https://codereview.chromium.org/10917246/diff/4001/chrome/browser/sync/about_...
chrome/browser/sync/about_sync_util.cc:302:
keystore_migration_time.SetValue(full_status.keystore_migration_time);
yeah, I think the string convs should happen here

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/public/e...
File sync/internal_api/public/engine/sync_status.h (right):

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/public/e...
sync/internal_api/public/engine/sync_status.h:88: std::string
keystore_migration_time;
can you make _time be base::Time and do the string conversion further up the
stack?

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/public/e...
sync/internal_api/public/engine/sync_status.h:89: std::string passphrase_type;
can you make passphrase_type be the enum type?  Does that introduce a weird
dependency?  If so, maybe make it type 'int'

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.h (right):

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:289: int64 migration_time_ms_;
can you change this to base::Time?

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:550: allstatus_.SetHasKeystoreKey(true);
SetHasKeystoreKey(type == KEYSTORE_BOOKSTRAP_TOKEN)?

[Nicolas Zea, 2012-09-17 21:00:24]

Comments addressed, PTAL. I also moved sync/util/time.h into
internal_api/public/util/, but I think that's a more appropriate place anyways
(we were relying on a DEPS loophole to use it in glue/)

https://codereview.chromium.org/10917246/diff/4001/chrome/browser/sync/about_...
File chrome/browser/sync/about_sync_util.cc (right):

https://codereview.chromium.org/10917246/diff/4001/chrome/browser/sync/about_...
chrome/browser/sync/about_sync_util.cc:302:
keystore_migration_time.SetValue(full_status.keystore_migration_time);
On 2012/09/17 18:07:20, akalin wrote:
> yeah, I think the string convs should happen here

Done.

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/public/e...
File sync/internal_api/public/engine/sync_status.h (right):

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/public/e...
sync/internal_api/public/engine/sync_status.h:88: std::string
keystore_migration_time;
On 2012/09/17 18:07:20, akalin wrote:
> can you make _time be base::Time and do the string conversion further up the
> stack?

Done.

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/public/e...
sync/internal_api/public/engine/sync_status.h:89: std::string passphrase_type;
On 2012/09/17 18:07:20, akalin wrote:
> can you make passphrase_type be the enum type?  Does that introduce a weird
> dependency?  If so, maybe make it type 'int'

Done.

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_enc...
File sync/internal_api/sync_encryption_handler_impl.h (right):

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_enc...
sync/internal_api/sync_encryption_handler_impl.h:289: int64 migration_time_ms_;
On 2012/09/17 18:07:20, akalin wrote:
> can you change this to base::Time?

Done.

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/10917246/diff/4001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:550: allstatus_.SetHasKeystoreKey(true);
On 2012/09/17 18:07:20, akalin wrote:
> SetHasKeystoreKey(type == KEYSTORE_BOOKSTRAP_TOKEN)?

Can't, that will set the boolean to false if the non-keystore boostrap token is
updated (they can be updated independently of each other).

[akalin, 2012-09-18 00:19:21]

I think we can't move time.h/time.cc out.

http://codereview.chromium.org/10917246/diff/2003/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

http://codereview.chromium.org/10917246/diff/2003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:725:
EXPECT_CALL(*observer(),
Looks like it's time for another mock -> fake conversion! (Not in this CL,
though)

http://codereview.chromium.org/10917246/diff/2003/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/10917246/diff/2003/sync/sync.gyp#newcode74
sync/sync.gyp:74: 'internal_api/public/util/time.cc',
unfortunately, i don't think we can do this.  It's been a long-standing bug that
the 'sync' target contains internal_api stuff at all; those should all be in
syncapi_core.

But then the 'sync' target does use time.h/time.cc, so that needs to remain in
there.

[Nicolas Zea, 2012-09-18 01:24:24]

Moved time.h back to sync/util, and added sync/api/time.h for use outside of
sync/. I believe this is what we had discussed. PTAL

[akalin, 2012-09-18 20:16:37]

lgtm after nits

also mention addition of api/time.h in CL description

http://codereview.chromium.org/10917246/diff/3037/sync/api/time.h
File sync/api/time.h (right):

http://codereview.chromium.org/10917246/diff/3037/sync/api/time.h#newcode11
sync/api/time.h:11: #include <sync/util/time.h>
use quotes here

http://codereview.chromium.org/10917246/diff/3037/sync/internal_api/public/en...
File sync/internal_api/public/engine/sync_status.cc (right):

http://codereview.chromium.org/10917246/diff/3037/sync/internal_api/public/en...
sync/internal_api/public/engine/sync_status.cc:7: #include
"sync/internal_api/public/util/sync_string_conversions.h"
don't need this anymore, right?

[commit-bot: I haz the power, 2012-09-18 22:17:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10917246/21001

[Nicolas Zea, 2012-09-18 22:17:18]

Done, committing.

http://codereview.chromium.org/10917246/diff/3037/sync/api/time.h
File sync/api/time.h (right):

http://codereview.chromium.org/10917246/diff/3037/sync/api/time.h#newcode11
sync/api/time.h:11: #include <sync/util/time.h>
On 2012/09/18 20:16:37, akalin wrote:
> use quotes here

Woops, done.

http://codereview.chromium.org/10917246/diff/3037/sync/internal_api/public/en...
File sync/internal_api/public/engine/sync_status.cc (right):

http://codereview.chromium.org/10917246/diff/3037/sync/internal_api/public/en...
sync/internal_api/public/engine/sync_status.cc:7: #include
"sync/internal_api/public/util/sync_string_conversions.h"
On 2012/09/18 20:16:37, akalin wrote:
> don't need this anymore, right?

Done.

[commit-bot: I haz the power, 2012-09-19 00:38:13]

Failed to apply patch for
sync/internal_api/sync_encryption_handler_impl_unittest.cc:
While running patch -p1 --forward --force;
patching file sync/internal_api/sync_encryption_handler_impl_unittest.cc
Hunk #1 succeeded at 32 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 385 (offset 5 lines).
Hunk #3 succeeded at 402 (offset 5 lines).
Hunk #4 succeeded at 513 (offset 10 lines).
Hunk #5 succeeded at 525 (offset 10 lines).
Hunk #6 succeeded at 569 (offset 11 lines).
Hunk #7 succeeded at 583 (offset 11 lines).
Hunk #8 succeeded at 607 (offset 11 lines).
Hunk #9 succeeded at 619 with fuzz 2 (offset 11 lines).
Hunk #10 succeeded at 645 (offset 12 lines).
Hunk #11 succeeded at 663 with fuzz 2 (offset 12 lines).
Hunk #12 succeeded at 687 (offset 13 lines).
Hunk #13 succeeded at 715 with fuzz 2 (offset 13 lines).
Hunk #14 succeeded at 737 (offset 14 lines).
Hunk #15 succeeded at 758 with fuzz 2 (offset 14 lines).
Hunk #16 FAILED at 802.
Hunk #17 succeeded at 873 (offset 17 lines).
Hunk #18 succeeded at 902 (offset 17 lines).
Hunk #19 succeeded at 952 (offset 16 lines).
Hunk #20 succeeded at 979 (offset 16 lines).
Hunk #21 succeeded at 1032 (offset 16 lines).
Hunk #22 succeeded at 1056 (offset 17 lines).
Hunk #23 succeeded at 1110 (offset 17 lines).
Hunk #24 succeeded at 1134 (offset 18 lines).
Hunk #25 succeeded at 1199 (offset 18 lines).
Hunk #26 succeeded at 1214 (offset 18 lines).
Hunk #27 succeeded at 1284 (offset 20 lines).
Hunk #28 succeeded at 1300 (offset 20 lines).
Hunk #29 succeeded at 1381 (offset 20 lines).
Hunk #30 succeeded at 1394 (offset 20 lines).
Hunk #31 succeeded at 1406 (offset 20 lines).
Hunk #32 succeeded at 1487 (offset 20 lines).
Hunk #33 succeeded at 1500 (offset 20 lines).
Hunk #34 succeeded at 1581 (offset 20 lines).
Hunk #35 succeeded at 1594 (offset 20 lines).
Hunk #36 succeeded at 1608 (offset 20 lines).
Hunk #37 succeeded at 1653 (offset 20 lines).
Hunk #38 succeeded at 1679 (offset 21 lines).
1 out of 38 hunks FAILED -- saving rejects to file
sync/internal_api/sync_encryption_handler_impl_unittest.cc.rej

[commit-bot: I haz the power, 2012-09-19 00:41:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10917246/26001

[commit-bot: I haz the power, 2012-09-19 04:12:20]

Change committed as 157499