🔍 More consistent first run triggering

🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

* Fixes SyncTestBase so that it clears data before creating a SharedPref.  This
  caches the SharedPrefs and makes the data clear completely useless, causing
  different test runs to affect each other.

BUG=708844, 710952, 616456
Review-Url: https://codereview.chromium.org/2833673003
Cr-Commit-Position: refs/heads/master@{#466748}
Committed: https://chromium.googlesource.com/chromium/src/+/40127a47c9fb31fbc46aff7a7ad6f5bd8ea09418

[gone, 2017-04-20 03:45:44]

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

[commit-bot: I haz the power, 2017-04-20 03:46:08]

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

[gone, 2017-04-20 05:03:18]

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

[commit-bot: I haz the power, 2017-04-20 05:03:33]

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

[gone, 2017-04-20 05:05:31]

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

[commit-bot: I haz the power, 2017-04-20 05:05:49]

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, 2017-04-20 06:38:00]

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

[commit-bot: I haz the power, 2017-04-20 06:38:01]

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

[gone, 2017-04-20 07:29:38]

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

[commit-bot: I haz the power, 2017-04-20 07:29:49]

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

[gone, 2017-04-20 07:30:31]

Description was changed from

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

BUG=708844,710952
==========

to

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Replaces a bunch of the old tests with new ones that account for redirecting.

BUG=708844,710952
==========

[gone, 2017-04-20 07:32:00]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org, yusufo@chromium.org

[gone, 2017-04-20 07:32:00]

The last suite of First Run tests is giving me some grief because I think I have
to also rewrite it from scratch (yay).  Can you guys both take a pass on this
and see if there's anything generally scary?

[commit-bot: I haz the power, 2017-04-20 08:54:20]

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

[commit-bot: I haz the power, 2017-04-20 08:54:21]

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

[Ted C, 2017-04-20 16:58:03]

Overall, this seems ok to me (didn't look at the tests though)

I thought redirectsToFirstRunIfNecessary was a bit weird where
requiresFirstRunToProceed might be clearer.  But if we make CTA handle first run
internally than it would be weird to return false from this.

Maybe shouldFinishAndStartFirstRunIfNecessary...but that might be too specific. 
Really, I'll be fine if we keep the name as is, but it did read a bit weird to
me.

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

https://codereview.chromium.org/2833673003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:332:
} else {
I guess any reason we don't do instanceof BroadcastReceiver and then assert
false in the last block?

The usage of "assume" scares me.

https://codereview.chromium.org/2833673003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:346:
* @return True if the user may not proceed (via Activity#finish or dropping the
Intent).
s/True if the user/Whether the user

[gone, 2017-04-20 17:09:15]

Renamed it to "requiresFirstRunToBeCompleted".  Better maybe?

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

https://codereview.chromium.org/2833673003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:332:
} else {
On 2017/04/20 16:58:03, Ted C wrote:
> I guess any reason we don't do instanceof BroadcastReceiver and then assert
> false in the last block?
> 
> The usage of "assume" scares me.

The context might be the application context here.  I just pass in a boolean
now.

https://codereview.chromium.org/2833673003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:346:
* @return True if the user may not proceed (via Activity#finish or dropping the
Intent).
On 2017/04/20 16:58:03, Ted C wrote:
> s/True if the user/Whether the user 

Reworded.

[Yusuf, 2017-04-20 18:19:10]

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:281:
super.onCreate(null);
should we be worried about this getting called twice? Can that happen? Is
isStartedUpCorrectly a DocumentActivity thing?

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:210:
if (FirstRunFlowSequencer.launch(context, intent, true)) return;
when we abort here and user goes through FRE, where do we land at the very end?

[gone, 2017-04-21 00:23:53]

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:281:
super.onCreate(null);
On 2017/04/20 18:19:10, Yusuf wrote:
> should we be worried about this getting called twice? Can that happen? Is
> isStartedUpCorrectly a DocumentActivity thing?

We always return after calling super.onCreate().  isStartedUpCorrectly isn't
just used for DocumentActivity; it's used for ChromeTabbedActivity, too.

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2833673003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:210:
if (FirstRunFlowSequencer.launch(context, intent, true)) return;
On 2017/04/20 18:19:10, Yusuf wrote:
> when we abort here and user goes through FRE, where do we land at the very
end?

We end up back here because the Intent that was used to start this
BroadcastReceiver gets refired at the end of First Run (whether or not it was
canceled).  The launch() call checks the result and lets us know whether we need
to stop moving forward.

[gone, 2017-04-21 00:56:37]

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

[commit-bot: I haz the power, 2017-04-21 00:56:59]

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

[gone, 2017-04-21 00:59:23]

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

[commit-bot: I haz the power, 2017-04-21 00:59:51]

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

[gone, 2017-04-21 01:04:16]

dfalcantara@chromium.org changed reviewers:
+ nyquist@chromium.org

[gone, 2017-04-21 01:04:17]

I *think* I got the sync FirstRunTest working.  I'm not happy about the way I
did it, but without someone refactoring how that test suite works this is the
best way I could come up with because the base class has setup and
initialization functions that the suite needs.

+nyquist to review/scoff at it, +pavely as FYI

[gone, 2017-04-21 01:04:23]

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

[commit-bot: I haz the power, 2017-04-21 01:04:37]

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, 2017-04-21 01:38:34]

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

[commit-bot: I haz the power, 2017-04-21 01:38:35]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-21 02:41:31]

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

[commit-bot: I haz the power, 2017-04-21 02:41:49]

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

[gone, 2017-04-21 02:42:05]

Description was changed from

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Replaces a bunch of the old tests with new ones that account for redirecting.

BUG=708844,710952
==========

to

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

BUG=708844,710952
==========

[commit-bot: I haz the power, 2017-04-21 03:18:53]

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

[commit-bot: I haz the power, 2017-04-21 03:18:54]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-21 04:41:42]

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:257:
abortLaunch();
pre-L, if you call finish in onCreate does it remove the task?  Just making sure
we don't need to do anything in onResume or anything.

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (left):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2474: <message
name="IDS_FRE_LABEL" desc="First Run Experience Strings">
what is the default label w/o this?  Just Chrome?

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProviderTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProviderTest.java:177:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
I think you could add the Add(DISABLE_FRE) at the class level and only have the
remove on this one.

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java:37:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
Hmm...we have this and yet we start first run.  Why?  Is it because
ChromeTabbedActivity forces first run to complete?  Should we just change the
logic or whether FRE needs to be completed in
AsyncInitActivity/ChromeTabbedActivity to check for this?

[gone, 2017-04-21 17:05:06]

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:257:
abortLaunch();
On 2017/04/21 04:41:42, Ted C wrote:
> pre-L, if you call finish in onCreate does it remove the task?  Just making
sure
> we don't need to do anything in onResume or anything.

Seems to.  I've been testing on a KK device.

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (left):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:2474: <message
name="IDS_FRE_LABEL" desc="First Run Experience Strings">
On 2017/04/21 04:41:42, Ted C wrote:
> what is the default label w/o this?  Just Chrome?

Yeah, it falls back to the app name.  This label isn't terrible helpful as is.

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProviderTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProviderTest.java:177:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
On 2017/04/21 04:41:42, Ted C wrote:
> I think you could add the Add(DISABLE_FRE) at the class level and only have
the
> remove on this one.

Done.

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java:37:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
On 2017/04/21 04:41:42, Ted C wrote:
> Hmm...we have this and yet we start first run.  Why?  Is it because
> ChromeTabbedActivity forces first run to complete?  Should we just change the
> logic or whether FRE needs to be completed in
> AsyncInitActivity/ChromeTabbedActivity to check for this?

Little lost... are you asking why this flag is being removed?  I was looking
into why this suite screwed up some other things, like why I have to explicitly
mark First Run as being incomplete in setUp().  That check is performed after
the command line check.

[Ted C, 2017-04-21 17:13:12]

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java:37:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
On 2017/04/21 17:05:06, slow (dfalcantara) wrote:
> On 2017/04/21 04:41:42, Ted C wrote:
> > Hmm...we have this and yet we start first run.  Why?  Is it because
> > ChromeTabbedActivity forces first run to complete?  Should we just change
the
> > logic or whether FRE needs to be completed in
> > AsyncInitActivity/ChromeTabbedActivity to check for this?
> 
> Little lost... are you asking why this flag is being removed?  I was looking
> into why this suite screwed up some other things, like why I have to
explicitly
> mark First Run as being incomplete in setUp().  That check is performed after
> the command line check.

No, I'm confused why we have this flag and then do a bunch of stuff that
seems to undo it.

I guess, what was failing in this suite?  I don't see why your changes should
have broken anything.

[gone, 2017-04-21 17:15:55]

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java:37:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
On 2017/04/21 17:13:12, Ted C wrote:
> On 2017/04/21 17:05:06, slow (dfalcantara) wrote:
> > On 2017/04/21 04:41:42, Ted C wrote:
> > > Hmm...we have this and yet we start first run.  Why?  Is it because
> > > ChromeTabbedActivity forces first run to complete?  Should we just change
> the
> > > logic or whether FRE needs to be completed in
> > > AsyncInitActivity/ChromeTabbedActivity to check for this?
> > 
> > Little lost... are you asking why this flag is being removed?  I was looking
> > into why this suite screwed up some other things, like why I have to
> explicitly
> > mark First Run as being incomplete in setUp().  That check is performed
after
> > the command line check.
> 
> No, I'm confused why we have this flag and then do a bunch of stuff that
> seems to undo it.
> 
> I guess, what was failing in this suite?  I don't see why your changes should
> have broken anything.

Oh.  It's a SyncTestBase subclass.  It extends
ChromeActivityTestCaseBase<ChromeTabbedActivity>, so it tries to start up
ChromeTabbedActivity, which automatically shuts itself back down.  It never
actually _needed_ to, though, so now I'm making it start FirstRunActivity. 
Ideally I'd make the base class run FirstRunActivity instead, but it's used
elsewhere.  The alternative is refactor the base class to pull out all of its
functions into something else.

[gone, 2017-04-21 17:25:39]

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

[commit-bot: I haz the power, 2017-04-21 17:26:14]

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, 2017-04-21 18:05:39]

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

[commit-bot: I haz the power, 2017-04-21 18:05:41]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-21 18:24:03]

Description was changed from

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

BUG=708844,710952
==========

to

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

BUG=708844,710952,616456
==========

[gone, 2017-04-21 18:34:57]

Description was changed from

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

BUG=708844,710952,616456
==========

to

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

* Fixes SyncTestBase so that it clears data before creating a SharedPref.  This
  caches the SharedPrefs and makes the data clear completely useless, causing
  different test runs to affect each other.

BUG=708844,710952,616456
==========

[gone, 2017-04-21 18:36:15]

Figured out why I had to set the SharedPreference before the test ran.  Turns
out the SharedPrefererences were getting cached before data from the previous
run was deleted.  So that's great.

[gone, 2017-04-21 18:36:25]

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

[commit-bot: I haz the power, 2017-04-21 18:36:52]

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, 2017-04-21 19:14:25]

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

[commit-bot: I haz the power, 2017-04-21 19:14:28]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-21 21:47:19]

lgtm

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java
(right):

https://codereview.chromium.org/2833673003/diff/140001/chrome/android/sync_sh...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FirstRunTest.java:37:
@CommandLineFlags.Remove(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
On 2017/04/21 17:15:55, slow (dfalcantara) wrote:
> On 2017/04/21 17:13:12, Ted C wrote:
> > On 2017/04/21 17:05:06, slow (dfalcantara) wrote:
> > > On 2017/04/21 04:41:42, Ted C wrote:
> > > > Hmm...we have this and yet we start first run.  Why?  Is it because
> > > > ChromeTabbedActivity forces first run to complete?  Should we just
change
> > the
> > > > logic or whether FRE needs to be completed in
> > > > AsyncInitActivity/ChromeTabbedActivity to check for this?
> > > 
> > > Little lost... are you asking why this flag is being removed?  I was
looking
> > > into why this suite screwed up some other things, like why I have to
> > explicitly
> > > mark First Run as being incomplete in setUp().  That check is performed
> after
> > > the command line check.
> > 
> > No, I'm confused why we have this flag and then do a bunch of stuff that
> > seems to undo it.
> > 
> > I guess, what was failing in this suite?  I don't see why your changes
should
> > have broken anything.
> 
> Oh.  It's a SyncTestBase subclass.  It extends
> ChromeActivityTestCaseBase<ChromeTabbedActivity>, so it tries to start up
> ChromeTabbedActivity, which automatically shuts itself back down.  It never
> actually _needed_ to, though, so now I'm making it start FirstRunActivity. 
> Ideally I'd make the base class run FirstRunActivity instead, but it's used
> elsewhere.  The alternative is refactor the base class to pull out all of its
> functions into something else.

BAH...I thought this file was ADDing the command line switch but it is in fact
removing it.  So previously, this worked because CTA would start an FRE on top
of itself?  But now it kills itself and triggers an FRE instead.  Gotcha.

[gone, 2017-04-21 22:51:40]

Eh heh, sorry for the churn but I've switched it to use singleInstance instead
of singleTop for the FRE.  There were weird cases where Clank would allow two
FREs to appear (if you clicked on Clank's main icon, then clicked on the widget)
in Recents.  This led to a different bug where finishing one and then clicking
the other would refire whatever PendingIntent it was holding, then start the
interrupted Activity again.  I'm straight up preventing that from happening by
hiding First Run from recents.  WDYT?

[gone, 2017-04-21 23:16:41]

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

[commit-bot: I haz the power, 2017-04-21 23:17:17]

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, 2017-04-21 23:55:12]

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

[commit-bot: I haz the power, 2017-04-21 23:55:13]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-24 02:20:55]

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

[commit-bot: I haz the power, 2017-04-24 02:20:59]

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, 2017-04-24 02:54:55]

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

[commit-bot: I haz the power, 2017-04-24 02:54:56]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-24 16:50:12]

On 2017/04/21 22:51:40, slow (dfalcantara) wrote:
> Eh heh, sorry for the churn but I've switched it to use singleInstance instead
> of singleTop for the FRE.  There were weird cases where Clank would allow two
> FREs to appear (if you clicked on Clank's main icon, then clicked on the
widget)
> in Recents.  This led to a different bug where finishing one and then clicking
> the other would refire whatever PendingIntent it was holding, then start the
> interrupted Activity again.  I'm straight up preventing that from happening by
> hiding First Run from recents.  WDYT?

For allowing multiple FREs, is that new or would that happen before this change?

For refiring a weird pendingintent, it should always have the last pending
intent it received set via onNewIntent/setIntent being called.  Was it
sending the pending intent automatically or was it holding onto a stale one?

I'm not against hiding the FRE as it isn't something that I think is super
critical, but it would be nicer to not have to (incase you started and
went to create an account or something and got sidetracked...again, I won't
lose sleep over it, so we shouldn't spend too much time on this either).

[gone, 2017-04-24 18:06:57]

On 2017/04/24 16:50:12, Ted C wrote:
> On 2017/04/21 22:51:40, slow (dfalcantara) wrote:
> > Eh heh, sorry for the churn but I've switched it to use singleInstance
instead
> > of singleTop for the FRE.  There were weird cases where Clank would allow
two
> > FREs to appear (if you clicked on Clank's main icon, then clicked on the
> widget)
> > in Recents.  This led to a different bug where finishing one and then
clicking
> > the other would refire whatever PendingIntent it was holding, then start the
> > interrupted Activity again.  I'm straight up preventing that from happening
by
> > hiding First Run from recents.  WDYT?
> 
> For allowing multiple FREs, is that new or would that happen before this
change?
> 
> For refiring a weird pendingintent, it should always have the last pending
> intent it received set via onNewIntent/setIntent being called.  Was it
> sending the pending intent automatically or was it holding onto a stale one?
> 
> I'm not against hiding the FRE as it isn't something that I think is super
> critical, but it would be nicer to not have to (incase you started and
> went to create an account or something and got sidetracked...again, I won't
> lose sleep over it, so we shouldn't spend too much time on this either).

We've had the ability to open multiple FREs: you need to start
ChromeTabbedActivity
via the icon, then let it sit in First Run, then open a Custom Tab (e.g.).

The PendingIntent was holding onto a stale one, so if you re-selected it from
Recents
it would restart the old activity.  In one case, it was restarting the
SearchActivity and triggering new voice queries.

I think I'm fine with not hiding the FRE if we keep using singleInstance, but
I don't care much either way.

[gone, 2017-04-24 18:19:28]

On 2017/04/24 18:06:57, slow (dfalcantara) wrote:
> On 2017/04/24 16:50:12, Ted C wrote:
> > On 2017/04/21 22:51:40, slow (dfalcantara) wrote:
> > > Eh heh, sorry for the churn but I've switched it to use singleInstance
> instead
> > > of singleTop for the FRE.  There were weird cases where Clank would allow
> two
> > > FREs to appear (if you clicked on Clank's main icon, then clicked on the
> > widget)
> > > in Recents.  This led to a different bug where finishing one and then
> clicking
> > > the other would refire whatever PendingIntent it was holding, then start
the
> > > interrupted Activity again.  I'm straight up preventing that from
happening
> by
> > > hiding First Run from recents.  WDYT?
> > 
> > For allowing multiple FREs, is that new or would that happen before this
> change?
> > 
> > For refiring a weird pendingintent, it should always have the last pending
> > intent it received set via onNewIntent/setIntent being called.  Was it
> > sending the pending intent automatically or was it holding onto a stale one?
> > 
> > I'm not against hiding the FRE as it isn't something that I think is super
> > critical, but it would be nicer to not have to (incase you started and
> > went to create an account or something and got sidetracked...again, I won't
> > lose sleep over it, so we shouldn't spend too much time on this either).
> 
> We've had the ability to open multiple FREs: you need to start
> ChromeTabbedActivity
> via the icon, then let it sit in First Run, then open a Custom Tab (e.g.).
> 
> The PendingIntent was holding onto a stale one, so if you re-selected it from
> Recents
> it would restart the old activity.  In one case, it was restarting the
> SearchActivity and triggering new voice queries.
> 
> I think I'm fine with not hiding the FRE if we keep using singleInstance, but
> I don't care much either way.

Talked offline:
We'll file a bug to address this like how SeparateTaskManagedCustomTabActivity
does it, where it fires an Intent back at the Intent but adds the
EXCLUDE_FROM_RECENTS flag.

[nyquist, 2017-04-24 20:27:46]

//chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync lgtm

[gone, 2017-04-24 20:29:11]

Talked with Yusuf.  Good to land.

[gone, 2017-04-24 20:29:19]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2017-04-24 20:29:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2833673003/#ps220001
(title: "Rebased")

[commit-bot: I haz the power, 2017-04-24 20:30: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, 2017-04-24 20:38:24]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493065759676260,
"parent_rev": "094047d6fd5b831902dbb9a35416634024895e95", "commit_rev":
"40127a47c9fb31fbc46aff7a7ad6f5bd8ea09418"}

[commit-bot: I haz the power, 2017-04-24 20:38:31]

Description was changed from

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

* Fixes SyncTestBase so that it clears data before creating a SharedPref.  This
  caches the SharedPrefs and makes the data clear completely useless, causing
  different test runs to affect each other.

BUG=708844,710952,616456
==========

to

==========
🔍 More consistent first run triggering

If the user tries to launch an AsyncInitializationActivity (tabbed mode or not),
but has not yet gone through first run, force them to do so by launching it in
the same way the ChromeLauncherActivity does it.  By reusing the codepath, we
reduce the number of ways we can mess up this flow.

* Pull ChromeLauncherActivity#launchFirstRunExperience out into a static
  function in FirstRunFlowSequencer.

* Simplify the logic to not care about whether it's in tabbed mode or not.

* Call that function directly from ChromeLauncherActivity for all modes (tabbed
  or otherwise)

* Also call it from AsyncInitializationActivity in the failure case.  This
  requires fixing ChromeActivity#destroy() so that it doesn't try to undo
  anything that didn't happen because the Activity was killed early.

* Makes the SearchWidgetProvider check if First Run is required before allowing
  the user into the SearchActivity.

* Fixes how PendingIntent flags are created for FirstRun.  It was, for some
  reason, using the Intent's flags instead of using PendingIntent flags.

* Take out the "First Run Experience" app label.  Going to Recents and seeing it
  is strange.  It just uses the app name, now.

* Replaces a bunch of the old tests with new ones that account for redirecting.

* Fixes SyncTestBase so that it clears data before creating a SharedPref.  This
  caches the SharedPrefs and makes the data clear completely useless, causing
  different test runs to affect each other.

BUG=708844,710952,616456

Review-Url: https://codereview.chromium.org/2833673003
Cr-Commit-Position: refs/heads/master@{#466748}
Committed:
https://chromium.googlesource.com/chromium/src/+/40127a47c9fb31fbc46aff7a7ad6...
==========

[commit-bot: I haz the power, 2017-04-24 20:38:33]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/40127a47c9fb31fbc46aff7a7ad6...