Replace Shared Prefs commit with apply.

Replace Shared Prefs commit with apply.

BUG=607761
Committed: https://crrev.com/35269b7ddd6cf1134ea4e93c9928365bca90db9c
Cr-Commit-Position: refs/heads/master@{#404653}

[Peter Wen, 2016-07-04 19:36:16]

wnwen@chromium.org changed reviewers:
+ agrieve@chromium.org

[Peter Wen, 2016-07-04 19:36:17]

🍩

[agrieve, 2016-07-04 20:21:22]

On 2016/07/04 19:36:17, Peter Wen wrote:
> 🍩

Thanks for doing this! lgtm.

[Peter Wen, 2016-07-04 20:29:18]

wnwen@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Peter Wen, 2016-07-04 20:29:18]

+tedchoc@ for chrome/android OWNERS.

One thing to note: Is there something special about ManageSpaceActivity.java
that it needs to commit right away rather than wait for Android to do so
automatically in the background? It doesn't seem too different from others even
if startup fails and falls into the catch block at the end of onCreate.

[Peter Wen, 2016-07-04 21:20:07]

wnwen@chromium.org changed reviewers:
- tedchoc@chromium.org

[Peter Wen, 2016-07-04 21:20:08]

-Ted as he's OOO

[Peter Wen, 2016-07-04 21:20:49]

wnwen@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Peter Wen, 2016-07-04 21:20:49]

+mariakhomenko@ for chrome/android OWNERS.

[Ted C, 2016-07-04 21:55:41]

On 2016/07/04 20:29:18, Peter Wen wrote:
> +tedchoc@ for chrome/android OWNERS.
> 
> One thing to note: Is there something special about ManageSpaceActivity.java
> that it needs to commit right away rather than wait for Android to do so
> automatically in the background? It doesn't seem too different from others
even
> if startup fails and falls into the catch block at the end of onCreate.

Yes, I specifically chose commit there (the code was just added last week, so
this is no legacy cruft).

This is there to catch where the native library crashes and kills the browser
process.  In that case, there is no java exception and it just dies.  If we to 
switch to apply, then there is no guarantee it will be done before a crash
kills the process.  Hence, I chose commit.  So please don't replace that.

I don't know why other places do it differently, but here we want to make
sure we don't attempt to start the browser process and have it kill chrome.

This activity is how you can clear data for the chrome app, so we want it
to be particularly error resistant.

[Peter Wen, 2016-07-05 12:21:01]

Thanks for explaining, Ted, I appreciate your detailed explanation! I've
reverted that file and added a comment for posterity. :)

[Peter Wen, 2016-07-06 15:41:05]

Description was changed from

==========
Replace Shared Prefs commit with apply.

BUG=607761
==========

to

==========
Replace Shared Prefs commit with apply.

BUG=607761
==========

[Maria, 2016-07-06 22:41:05]

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

[Maria, 2016-07-06 22:41:06]

+ Anthony to comment on ChromeBackupAgent. I want to make sure there isn't a
reason to use commit() there as well.

[aberent, 2016-07-07 09:29:01]

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

https://codereview.chromium.org/2124583003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:191:
sharedPrefs.edit().clear().apply();
See my comment below.

https://codereview.chromium.org/2124583003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:206:
editor.apply();
I am worried about using apply() here. The issue is that Android Restore closes
down the process immediately this returns, and I am not sure that it does
anything to force apply's to save to disk before it does so. It does so, I
think, though unconventional means, so I don't think the lifecycle guarantees on
apply work in this case. If you do want to change this please check with the
Dolly team whether it is safe to do so before doing so.

Note that is all runs outside a normal Chrome process or run, during device
setup, so there is no advantage in using apply().

[Peter Wen, 2016-07-08 21:30:06]

Thanks! PTAL. :)

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

https://codereview.chromium.org/2124583003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:191:
sharedPrefs.edit().clear().apply();
On 2016/07/07 09:29:01, aberent wrote:
> See my comment below.

Done.

https://codereview.chromium.org/2124583003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:206:
editor.apply();
On 2016/07/07 09:29:01, aberent wrote:
> I am worried about using apply() here. The issue is that Android Restore
closes
> down the process immediately this returns, and I am not sure that it does
> anything to force apply's to save to disk before it does so. It does so, I
> think, though unconventional means, so I don't think the lifecycle guarantees
on
> apply work in this case. If you do want to change this please check with the
> Dolly team whether it is safe to do so before doing so.
> 
> Note that is all runs outside a normal Chrome process or run, during device
> setup, so there is no advantage in using apply(). 

Makes sense, changed it back with a comment and lint suppression.

[Maria, 2016-07-08 23:43:04]

lgtm

https://codereview.chromium.org/2124583003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(left):

https://codereview.chromium.org/2124583003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:58:
FirstRunStatus.LIGHTWEIGHT_FIRST_RUN_FLOW_COMPLETE,
Can't tell if this is a merge issue or you need to rebase

[Peter Wen, 2016-07-11 13:25:16]

Thanks for the reviews!

https://codereview.chromium.org/2124583003/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java
(left):

https://codereview.chromium.org/2124583003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:58:
FirstRunStatus.LIGHTWEIGHT_FIRST_RUN_FLOW_COMPLETE,
On 2016/07/08 23:43:04, Maria (OOO July 11 - 15) wrote:
> Can't tell if this is a merge issue or you need to rebase

Thanks, I think this was a merge issue. Fixed now.

[Peter Wen, 2016-07-11 13:26:20]

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

[commit-bot: I haz the power, 2016-07-11 13:26:36]

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-07-11 14:04:18]

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

[commit-bot: I haz the power, 2016-07-11 14:04:19]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-07-11 14:24:35]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2016-07-11 14:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2124583003/#ps60001
(title: "Rebase and fix merge conflict.")

[commit-bot: I haz the power, 2016-07-11 14:24:50]

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-07-11 14:28:41]

Description was changed from

==========
Replace Shared Prefs commit with apply.

BUG=607761
==========

to

==========
Replace Shared Prefs commit with apply.

BUG=607761
==========

[commit-bot: I haz the power, 2016-07-11 14:28:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-11 14:29:03]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 14:30:09]

Description was changed from

==========
Replace Shared Prefs commit with apply.

BUG=607761
==========

to

==========
Replace Shared Prefs commit with apply.

BUG=607761

Committed: https://crrev.com/35269b7ddd6cf1134ea4e93c9928365bca90db9c
Cr-Commit-Position: refs/heads/master@{#404653}
==========

[commit-bot: I haz the power, 2016-07-11 14:30:10]

Patchset 4 (id:??) landed as
https://crrev.com/35269b7ddd6cf1134ea4e93c9928365bca90db9c
Cr-Commit-Position: refs/heads/master@{#404653}