Add TOS-accepted Java pref on Android.

Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

BUG=668741
Committed: https://crrev.com/03e06cc174fe2cbba5fab1291442e470e5e94b09
Cr-Commit-Position: refs/heads/master@{#435283}

[Alexei Svitkine (slow), 2016-11-25 20:00:30]

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

[commit-bot: I haz the power, 2016-11-25 20:00:40]

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

[Alexei Svitkine (slow), 2016-11-25 20:20:41]

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

[commit-bot: I haz the power, 2016-11-25 20:20:48]

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

[Alexei Svitkine (slow), 2016-11-25 20:24:23]

asvitkine@chromium.org changed reviewers:
+ gogerald@chromium.org, tedchoc@chromium.org

[Alexei Svitkine (slow), 2016-11-25 20:24:24]

gogerald, tedchoc: PTAL

Couple questions:
  1. Is removing the glue seem fine to you? It seemed like it was just extra
unnecessary complexity since the abstraction wasn't needed.
  2. Does it make sense to call the pref sync code on startup from where I'm
currently calling it from ChromeTabbedActivity.java or is there a better place
for it?

[Alexei Svitkine (slow), 2016-11-25 21:29:01]

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

[commit-bot: I haz the power, 2016-11-25 21:29:10]

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-25 23:16:48]

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

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

Dry run: This issue passed the CQ dry run.

[gogerald1, 2016-11-28 15:50:23]

It looks better to me to keep these static interfaces in a separate file, might
remove abstraction but keep implementation (rename FirstRunGlueImpl to
FirstRunGlue).

In addition, it looks we only launch FRE before native initialization for custom
tab intents in ChromeLauncherActivity and we are discussing whether we want to
do it the same as non-custom tab intents.

[Alexei Svitkine (slow), 2016-11-28 16:21:57]

On 2016/11/28 15:50:23, gogerald1 wrote:
> It looks better to me to keep these static interfaces in a separate file,
might
> remove abstraction but keep implementation (rename FirstRunGlueImpl to
> FirstRunGlue).

Sure, I could do that. For consistency, however, I think if we do that, we
should also move some functions from FirstRunFlowSequencer to FirstRunGlue.

At the very least, the ones that touch native prefs, specifically:

- isFirstRunEulaAccepted()
- markFlowAsCompleted()
  Or a helper function can be split off from that one.

There's some other static functions in FirstRunFlowSequencer that don't have
anything to do with prefs or native code. Are you fine leaving them in the
class?
 - createLightweightFirstRunIntent() / createGenericFirstRunIntent()
 - checkIfFirstRunIsNecessary()
 - addPendingIntent()

Note: The other distinction we can make is whether functions in FirstRunGlue
should only be package protected (i.e. so that they don't get used outside of
the firstrun package). If we use this criteria, then the above functions
shouldn't go in FirstRunGlue.

What do you think?

> 
> In addition, it looks we only launch FRE before native initialization for
custom
> tab intents in ChromeLauncherActivity and we are discussing whether we want to
> do it the same as non-custom tab intents.

Interesting - so the custom tabs case is when we end up initializating the
browser synchronously from FirstRunActivity.onCreate() - I wasn't sure when that
code path would be run, but this explains it. Note: As part of this work (in a
later CL) I'm planning to refactor the code and change where that's done from
FRE code (so it will be after first card) and to launch FRE before native in all
cases.

[Ted C, 2016-11-29 00:34:46]

I'm not really against removing FirstRunGlue as just is indirection right now. 
Looking at my crystal ball though, I would expect that it was added to allow
FirstRun to be tested but it hasn't been.  If you were to get the same feeling,
then I'd lean towards adding tests instead of deleting it.

I think regardless, it seems orthogonal to this change and I'd recommend doing
it separately.  By mixing the deletion of the file, it's hard to tell where you
are changing logic and that could make tracking this down later more difficult.

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:365:
FirstRunFlowSequencer.synchronizeFirstRunPrefs();
I would probably put this in FeatureUtilities#cacheNativeFlags.  That seems to
be our more and more common pattern.

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java
(right):

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:241:
static void acceptTermsOfService(boolean allowCrashUpload) {
it seems odd to me that acceptTermsOfService requires native but didAccept does
not.  How could you get into the state where didAccept is true but the user did
not go through this path?

Is that only for the TosAckedReceiver.checkAnyUserHasSeenToS part?

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:266:
/** Synchronizes first run native and Java preferences.
nit, for multi-line javadoc, start on the line after the /**

Also, it looks like each of the following javadoc lines is indented 1 too many

[Alexei Svitkine (slow), 2016-11-29 15:51:34]

Thanks for the comment.

I'll leave out the FirstRunGlue refactor out of this CL then and land it later
as a separate CL.

This would mean introducing some inconsistency in the mean time - specifically
adding a static public method to FirstRunGlueImpl (to sync the prefs) - which
currently doesn't have any static methods - and also calling it from outside the
firstrun package (which feels like calling something internal since the class
has Impl in the name).

But both of those inconsistencies can go away with my later refactor CL.

(I'll ping this again when I'll make the above changes.)

[Alexei Svitkine (slow), 2016-11-29 16:15:08]

Description was changed from

==========
Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

Additionally, this CL removes FirstRunGlue and FirstRunGlueImpl
classes, instead folding the functionality into static package
methods in FirstRunFlowSequencer (which already had other methods
that touched prefs), to simplify the code.

BUG=668741
==========

to

==========
Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

BUG=668741
==========

[Alexei Svitkine (slow), 2016-11-29 16:17:44]

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

[commit-bot: I haz the power, 2016-11-29 16:17:52]

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

[Alexei Svitkine (slow), 2016-11-29 16:18:12]

Patchset #4 (id:60001) has been deleted

[Alexei Svitkine (slow), 2016-11-29 16:23:06]

Patchset #4 (id:60002) has been deleted

[Alexei Svitkine (slow), 2016-11-29 16:23:09]

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

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

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

[Alexei Svitkine (slow), 2016-11-29 16:27:04]

PTAL - changes above (and below) have been made.

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:365:
FirstRunFlowSequencer.synchronizeFirstRunPrefs();
On 2016/11/29 00:34:45, Ted C wrote:
> I would probably put this in FeatureUtilities#cacheNativeFlags.  That seems to
> be our more and more common pattern.

Done.

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java
(right):

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:241:
static void acceptTermsOfService(boolean allowCrashUpload) {
On 2016/11/29 00:34:46, Ted C wrote:
> it seems odd to me that acceptTermsOfService requires native but didAccept
does
> not.

I agree that it's a little weird, but in my upcoming CL to actually change when
the FRE begins to depend on native, the flow guarantees that the
acceptTermsOfService would only be called after native is initialized. So from
the point of view of the flow, it should work correctly.

Of course, there's the question of how to organize the code to make it easy to
reason about and maybe we can discuss this fact on that CL? Especially if we're
also punting the refactor of this code structure to after this CL.

(We still need the native pref because it is checked by other native code.)

> How could you get into the state where didAccept is true but the user did
> not go through this path? Is that only for the
TosAckedReceiver.checkAnyUserHasSeenToS part?

Hmm, indeed this appears to be the case and seems incorrect to me. I'll file a
bug.

https://codereview.chromium.org/2531743003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:266:
/** Synchronizes first run native and Java preferences.
On 2016/11/29 00:34:46, Ted C wrote:
> nit, for multi-line javadoc, start on the line after the /**
> 
> Also, it looks like each of the following javadoc lines is indented 1 too many

Done.

[commit-bot: I haz the power, 2016-11-29 17:03:28]

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

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

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-11-29 20:42:32]

lgtm assuming the fre question is accurate

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java
(right):

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java:39:
public boolean didAcceptTermsOfService(Context appContext) {
this is only used within the first run activity right?  just making sure this
won't force people back through FRE after this change.

[Alexei Svitkine (slow), 2016-11-29 21:01:28]

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java
(right):

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java:39:
public boolean didAcceptTermsOfService(Context appContext) {
On 2016/11/29 20:42:32, Ted C wrote:
> this is only used within the first run activity right?  just making sure this
> won't force people back through FRE after this change.

That's correct as far as I can tell.

Specifically, we check checkIfFirstRunIsNecessary() to decide if FRE should be
shown and that uses FirstRunStatus.getFirstRunFlowComplete() to determine
whether it should be shown - which is a separate Java pref.

It will cause people who have started the FRE and went past the TOS screen but
didn't complete it to have to start from the beginning, but I think that's OK.

[gogerald1, 2016-11-30 14:18:02]

lgtm

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java
(right):

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java:24:
private static final String TOS_ACCEPTED_PREF = "first_run_tos_accepted";
nit: rename to CACHED_TOS_ACCEPTED_PREF and related function name?

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java:39:
public boolean didAcceptTermsOfService(Context appContext) {
On 2016/11/29 21:01:28, Alexei Svitkine (slow) wrote:
> On 2016/11/29 20:42:32, Ted C wrote:
> > this is only used within the first run activity right?  just making sure
this
> > won't force people back through FRE after this change.
> 
> That's correct as far as I can tell.
> 
> Specifically, we check checkIfFirstRunIsNecessary() to decide if FRE should be
> shown and that uses FirstRunStatus.getFirstRunFlowComplete() to determine
> whether it should be shown - which is a separate Java pref.
> 
> It will cause people who have started the FRE and went past the TOS screen but
> didn't complete it to have to start from the beginning, but I think that's OK.

FYI, "have  to start from the beginning" looks only happen to CCT view intents
and after upgrading Chrome.

[Alexei Svitkine (slow), 2016-11-30 15:49:37]

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java
(right):

https://codereview.chromium.org/2531743003/diff/90001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunGlueImpl.java:24:
private static final String TOS_ACCEPTED_PREF = "first_run_tos_accepted";
On 2016/11/30 14:18:02, gogerald1 wrote:
> nit: rename to CACHED_TOS_ACCEPTED_PREF and related function name?

Done.

[Alexei Svitkine (slow), 2016-11-30 15:49:53]

The CQ bit was checked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-11-30 15:49:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, gogerald@chromium.org
Link to the patchset: https://codereview.chromium.org/2531743003/#ps110001
(title: "Address comment.")

[commit-bot: I haz the power, 2016-11-30 15:50:23]

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-30 16:24:03]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1480520993854360,
"parent_rev": "46a04951b8bea669fdd4387306062d790e55b72b", "commit_rev":
"5f6000800c15418cc06896ce47a2e980ca175148"}

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

Description was changed from

==========
Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

BUG=668741
==========

to

==========
Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

BUG=668741
==========

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

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2016-11-30 16:26:14]

Description was changed from

==========
Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

BUG=668741
==========

to

==========
Add TOS-accepted Java pref on Android.

This will support being able to show the first page of the
FRE without native code being initialized yet, so that we
could fetch a variations seed while it's showing, which I'm
working on under crbug.com/632199

This change adds a Java-level pref for TOS-accepted, so it
can be checked before native is initialized. Also makes the
code sync the existing native pref to the Java pref for users
migrating from old versions. (We may want to merge this CL
back ahead of the main FRE de-nativization work so that users
can migrate their prefs ahead of time.)

BUG=668741

Committed: https://crrev.com/03e06cc174fe2cbba5fab1291442e470e5e94b09
Cr-Commit-Position: refs/heads/master@{#435283}
==========

[commit-bot: I haz the power, 2016-11-30 16:26:15]

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