Implement Android key/value backup

Implement Android key/value backup

This replaces our previous impmentation of (Dolly) Auto Backup.
Only the preferences needed to restore the user's signin atate
and sync state are backed up.

BUG=661188
Committed: https://crrev.com/d56b7ec3e6e6c3236835414584f4205283beb218
Cr-Commit-Position: refs/heads/master@{#432517}

[aberent, 2016-11-10 21:42:02]

aberent@chromium.org changed reviewers:
+ bauerb@chromium.org

[aberent, 2016-11-10 21:42:02]

Java tests to follow before the CL lands, but seems to work so an initial review
would be useful.

[aberent, 2016-11-10 21:42:28]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 21:43:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 22:23:20]

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

[commit-bot: I haz the power, 2016-11-10 22:23:21]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Bernhard Bauer, 2016-11-14 10:05:39]

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/Android...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:152: 
Remove this line again?

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(right):

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:66:
long hashCode = backupNames.hashCode() ^
Arrays.deepHashCode(backupValues.toArray());
hashCode() isn't guaranteed to avoid collisions, and skipping backup in case of
a collision would be wrong. Should we use a cryptographic hash function?

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:87:
Log.w(TAG, "Browser launch failed on backup");
Maybe log the exception as well?

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:97:
for (Boolean val : nativeBackupValues) {
Unboxed boolean?

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:98:
backupValues.add(val ? new byte[] {1} : new byte[] {0});
Extract this part into a helper method?

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupWatcher.java
(right):

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupWatcher.java:37:
// Watch the Java preferences that are backed up
Nit: Could you put periods at the end of sentences?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_agent.cc (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent.cc:57: constexpr int pref_count =
sizeof(backed_up_preferences_) / sizeof(char*);
You can use arraysize() for this.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent.cc:92: return
std::vector<std::string>(backed_up_preferences_,
For consistency, use std::begin() here as well?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_agent_unittest.cc (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:25: // since we don't
want to define anything here.
Ugh. Couldn't you just expose these functions from the chrome_backup_agent.h
(with a prefix of ...ForTesting)?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:48: virtual void SetUp()
{
override (and no virtual)?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:68: jobjectArray result =
Do you need to wrap this in a ScopedJavaLocalRef or something?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:83:
ASSERT_EQ(expected_pref_names_.size(), (size_t)size);
static_cast<size_t>()

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:115: JNIEnv* env_ =
base::android::AttachCurrentThread();
Unnecessary, I think.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:126: if (i == 3)
I would maybe examine |expected_pref_names_| instead here (and then fold the
check outside of the loop into this).

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:152: EXPECT_EQ((i == 5 ||
i == 8),
Just directly use values[i] here?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_watcher.cc (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.cc:19: }
Add an empty line after this one.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.cc:29: base::Closure callback =
base::Bind(&BackupPrefsChanged, java_watcher_);
In the interest of future-proofing the code, use a base::RepeatingClosure (with
base::BindRepeating) here?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_watcher.h (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:21: // Watch the preferences that
we backup using Android Backup, so that we can
Nit: the verb is "back up", only the noun is "backup" :)

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:25: explicit
ChromeBackupWatcher(Profile* profile);
Add an empty destructor?

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:28:
std::unique_ptr<PrefChangeRegistrar> registrar_;
Make this a direct member?

[aberent, 2016-11-14 17:23:40]

Java tests still to do.

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/Android...
File chrome/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/Android...
chrome/android/java/AndroidManifest.xml:152: 
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> Remove this line again?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(right):

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:66:
long hashCode = backupNames.hashCode() ^
Arrays.deepHashCode(backupValues.toArray());
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> hashCode() isn't guaranteed to avoid collisions, and skipping backup in case
of
> a collision would be wrong. Should we use a cryptographic hash function?

Actually we can use the whole data unmodified (just serialised). This is stored
locally and is only available to the backup service. I have changed the code to
do this.

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:87:
Log.w(TAG, "Browser launch failed on backup");
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> Maybe log the exception as well?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:97:
for (Boolean val : nativeBackupValues) {
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> Unboxed boolean?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:98:
backupValues.add(val ? new byte[] {1} : new byte[] {0});
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> Extract this part into a helper method?

Done, assuming you meant the bool to byte conversion.

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupWatcher.java
(right):

https://codereview.chromium.org/2496693002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupWatcher.java:37:
// Watch the Java preferences that are backed up
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> Nit: Could you put periods at the end of sentences?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_agent.cc (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent.cc:57: constexpr int pref_count =
sizeof(backed_up_preferences_) / sizeof(char*);
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> You can use arraysize() for this.

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent.cc:92: return
std::vector<std::string>(backed_up_preferences_,
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> For consistency, use std::begin() here as well?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_agent_unittest.cc (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:25: // since we don't
want to define anything here.
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Ugh. Couldn't you just expose these functions from the chrome_backup_agent.h
> (with a prefix of ...ForTesting)?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:48: virtual void SetUp()
{
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> override (and no virtual)?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:68: jobjectArray result =
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Do you need to wrap this in a ScopedJavaLocalRef or something?

Done. There is a conversion from a JavaRef to a bare object, but it is going
away.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:83:
ASSERT_EQ(expected_pref_names_.size(), (size_t)size);
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> static_cast<size_t>()

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:115: JNIEnv* env_ =
base::android::AttachCurrentThread();
On 2016/11/14 10:05:38, Bernhard Bauer wrote:
> Unnecessary, I think.

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:126: if (i == 3)
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> I would maybe examine |expected_pref_names_| instead here (and then fold the
> check outside of the loop into this).

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_agent_unittest.cc:152: EXPECT_EQ((i == 5 ||
i == 8),
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Just directly use values[i] here?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_watcher.cc (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.cc:19: }
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Add an empty line after this one.

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.cc:29: base::Closure callback =
base::Bind(&BackupPrefsChanged, java_watcher_);
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> In the interest of future-proofing the code, use a base::RepeatingClosure
(with
> base::BindRepeating) here?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_watcher.h (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:21: // Watch the preferences that
we backup using Android Backup, so that we can
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Nit: the verb is "back up", only the noun is "backup" :)

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:25: explicit
ChromeBackupWatcher(Profile* profile);
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Add an empty destructor?

Done.

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:28:
std::unique_ptr<PrefChangeRegistrar> registrar_;
On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> Make this a direct member?

Done.

[Bernhard Bauer, 2016-11-14 18:02:04]

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
File chrome/browser/android/chrome_backup_watcher.h (right):

https://codereview.chromium.org/2496693002/diff/1/chrome/browser/android/chro...
chrome/browser/android/chrome_backup_watcher.h:21: // Watch the preferences that
we backup using Android Backup, so that we can
On 2016/11/14 17:23:40, aberent wrote:
> On 2016/11/14 10:05:39, Bernhard Bauer wrote:
> > Nit: the verb is "back up", only the noun is "backup" :)
> 
> Done.

Thanks. The correct use of the three forms "back up", "Backup" and "backup" in a
single sentence now makes me happier than I care to admit ;-)

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(right):

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:70:
in.close();
Hrmph, technically you'd have to make sure you close the file descriptor even if
an exception occurs. Java…

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:73:
assert false;
assert and throw seems a bit redundant. And then if you're going to throw the
exception anyway, why bother?

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:92:
public int hashCode() {
I wouldn't override hashCode if you don't need it for hash tables.

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:136:
Log.w(TAG, "Browser launch failed on backup: " + e);
Add the exception as an additional parameter. Our own logging will Do The Right
Thing when the last parameter is an exception.

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:179:
// This will happen if Chrome has never written backup data, or if the backup
status is
ClassNotFoundException would *not* happen if Chrome has never written a backup,
right? Maybe we should just put an assert false into the catch block in the
constructor, or maybe even throw it as a RuntimeException (because if we really
do get a ClassNotFoundException while trying to deserialize ArrayList, something
has to be really messed up, so we might as well crash).

https://codereview.chromium.org/2496693002/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_backup_agent_unittest.cc (right):

https://codereview.chromium.org/2496693002/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_backup_agent_unittest.cc:127: jbooleanArray varray
= env_->NewBooleanArray(expected_pref_names_.size());
I think you need a ScopedJavaLocalRef here as well (roughly speaking, never hold
on to a raw Java object, because you need to tell the garbage collector when
you're done with it).

[aberent, 2016-11-15 19:30:11]

Patchset #3 (id:40001) has been deleted

[aberent, 2016-11-15 19:39:38]

Patchset #3 (id:60001) has been deleted

[aberent, 2016-11-15 19:39:51]

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(right):

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:70:
in.close();
On 2016/11/14 18:02:04, Bernhard Bauer wrote:
> Hrmph, technically you'd have to make sure you close the file descriptor even
if
> an exception occurs. Java…

Actually, on further investigation, the backup system closes these files, so I
don't need to at all. Removed.

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:73:
assert false;
On 2016/11/14 18:02:04, Bernhard Bauer wrote:
> assert and throw seems a bit redundant. And then if you're going to throw the
> exception anyway, why bother?

Done.

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:92:
public int hashCode() {
On 2016/11/14 18:02:04, Bernhard Bauer wrote:
> I wouldn't override hashCode if you don't need it for hash tables.

Done.

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:136:
Log.w(TAG, "Browser launch failed on backup: " + e);
On 2016/11/14 18:02:04, Bernhard Bauer wrote:
> Add the exception as an additional parameter. Our own logging will Do The
Right
> Thing when the last parameter is an exception.

Done.

https://codereview.chromium.org/2496693002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:179:
// This will happen if Chrome has never written backup data, or if the backup
status is
On 2016/11/14 18:02:04, Bernhard Bauer wrote:
> ClassNotFoundException would *not* happen if Chrome has never written a
backup,
> right? Maybe we should just put an assert false into the catch block in the
> constructor, or maybe even throw it as a RuntimeException (because if we
really
> do get a ClassNotFoundException while trying to deserialize ArrayList,
something
> has to be really messed up, so we might as well crash).

Done.

https://codereview.chromium.org/2496693002/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_backup_agent_unittest.cc (right):

https://codereview.chromium.org/2496693002/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_backup_agent_unittest.cc:127: jbooleanArray varray
= env_->NewBooleanArray(expected_pref_names_.size());
On 2016/11/14 18:02:04, Bernhard Bauer wrote:
> I think you need a ScopedJavaLocalRef here as well (roughly speaking, never
hold
> on to a raw Java object, because you need to tell the garbage collector when
> you're done with it).

Done.

[aberent, 2016-11-15 19:51:11]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 19:52:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 20:06:53]

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

[commit-bot: I haz the power, 2016-11-15 20:06:54]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[aberent, 2016-11-16 10:22:23]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 10:22:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aberent, 2016-11-16 10:25:22]

git cl upload problems have gone away. CL now complete, please review with view
to landing (branch point tomorrow).

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java
(left):

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
File removed. It isn't possible to do an instrumentation test of key/value
backup, since the BackupAgent API uses classes that can only be created by the
Android backup system (no visible constructors or factories) and there is no way
of forcing an immediate backup.

[commit-bot: I haz the power, 2016-11-16 11:55:18]

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

[commit-bot: I haz the power, 2016-11-16 11:55:19]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-11-16 12:29:18]

Mostly small things; Rewriting the mock could potentially be done in a followup
CL.

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(right):

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:76:
Log.e(TAG, "Unexpected ClassNotFound exception reading backup data.", e);
I don't think you need this at all -- if you propagate the exception upwards by
throwing, it will contain the relevant information anyway.

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:138:
} catch (ProcessInitException e) {
You could move the exception handling into initializeBrowser() as well.

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java
(right):

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:195:
// the two state files should contain identical data.
Nit: capitalize the beginning of the sentence.

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:281:
BackupDataInput backupData = mock(BackupDataInput.class);
Honestly, it might be easier to just write your own subclass of BackupDataInput
rather than shoehorning this into Mockito.

[aberent, 2016-11-16 14:32:53]

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(right):

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:76:
Log.e(TAG, "Unexpected ClassNotFound exception reading backup data.", e);
On 2016/11/16 12:29:18, Bernhard Bauer wrote:
> I don't think you need this at all -- if you propagate the exception upwards
by
> throwing, it will contain the relevant information anyway.

Done.

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:138:
} catch (ProcessInitException e) {
On 2016/11/16 12:29:18, Bernhard Bauer wrote:
> You could move the exception handling into initializeBrowser() as well.

Done.

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java
(right):

https://codereview.chromium.org/2496693002/diff/100001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:281:
BackupDataInput backupData = mock(BackupDataInput.class);
On 2016/11/16 12:29:18, Bernhard Bauer wrote:
> Honestly, it might be easier to just write your own subclass of
BackupDataInput
> rather than shoehorning this into Mockito.

I can't. The BackupDataInput constructor is hidden (@SystemApi, see
https://cs.corp.google.com/android/frameworks/base/core/java/android/app/back...),
which means that my code can't invoke it, so can't derive from BackupDataInput.
I could probably create something using reflection (presumably Mockito is doing
something of the sort) but this sounds worse than using Mockito.

This is the same reason I can't (without using reflection) create
instrumentation tests for this.

[aberent, 2016-11-16 14:33:29]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 14:33:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bernhard Bauer, 2016-11-16 15:24:48]

lgtm

[aberent, 2016-11-16 15:41:44]

The CQ bit was unchecked by aberent@chromium.org

[aberent, 2016-11-16 15:41:45]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2016-11-16 15:42:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 16:44:03]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-11-16 16:47:23]

Description was changed from

==========
Implement Android key/value backup

This replaces our previous impmentation of (Dolly) Auto Backup.
Only the preferences needed to restore the user's signin atate
and sync state are backed up.

BUG=661188
==========

to

==========
Implement Android key/value backup

This replaces our previous impmentation of (Dolly) Auto Backup.
Only the preferences needed to restore the user's signin atate
and sync state are backed up.

BUG=661188

Committed: https://crrev.com/d56b7ec3e6e6c3236835414584f4205283beb218
Cr-Commit-Position: refs/heads/master@{#432517}
==========

[commit-bot: I haz the power, 2016-11-16 16:47:24]

Patchset 5 (id:??) landed as
https://crrev.com/d56b7ec3e6e6c3236835414584f4205283beb218
Cr-Commit-Position: refs/heads/master@{#432517}