chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java - Issue 2519853004: Allow fetching variations while FRE is showing on Android first run.

Unified Diff: chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

Issue 2519853004: Allow fetching variations while FRE is showing on Android first run. (Closed)

Patch Set: Fix unit tests. Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java ('k') | chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java » ('j') | chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

index 2a8bbbc7d112ab5fa67f635f74152bf5068cc710..a2bbef179d142f361bd24fee3fc7b7c520972b6b 100644

--- a/chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

+++ b/chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

@@ -59,6 +59,8 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

static final String SHOW_SIGNIN_PAGE = "ShowSignIn";

static final String SHOW_DATA_REDUCTION_PAGE = "ShowDataReduction";

+ static final String POST_NATIVE_SETUP_NEEDED = "PostNativeSetupNeeded";

// Outgoing results:

public static final String RESULT_CLOSE_APP = "Close App";

public static final String RESULT_SIGNIN_ACCOUNT_NAME = "ResultSignInTo";

@@ -98,6 +100,7 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

private String mResultSignInAccountName;

private boolean mResultShowSignInSettings;

+ private boolean mPostNativePageSequenceCreated;

private boolean mNativeSideIsInitialized;

private ProfileDataCache mProfileDataCache;

@@ -105,6 +108,8 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

protected Bundle mFreProperties;

+ private Runnable mSetUpPropertiesPostNative;

private List<Callable<FirstRunPage>> mPages;

private List<Integer> mFreProgressStates;

@@ -125,27 +130,49 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

if (mShowWelcomePage) {

mPages.add(pageOf(ToSAndUMAFirstRunFragment.class));

mFreProgressStates.add(FRE_PROGRESS_WELCOME_SHOWN);

+ } else {

+ // Otherwise, if we're skipping past the welcome page, then init the

+ // native process and determine if data reduction proxy and signin

+ // pages should be shown - which both depend on native code.

+ createPostNativePageSequence();

}

+ }

+ private void createPostNativePageSequence() {

+ if (mPostNativePageSequenceCreated) return;

+ ensureBrowserProcessInitialized();

+ if (mFreProperties.getBoolean(POST_NATIVE_SETUP_NEEDED)

+ && mSetUpPropertiesPostNative != null) {

+ mSetUpPropertiesPostNative.run();

+ mFreProperties.remove(POST_NATIVE_SETUP_NEEDED);

+ }

+ boolean notifyAdapter = false;

// An optional Data Saver page.

if (mFreProperties.getBoolean(SHOW_DATA_REDUCTION_PAGE)) {

mPages.add(pageOf(DataReductionProxyFirstRunFragment.class));

mFreProgressStates.add(FRE_PROGRESS_DATA_SAVER_SHOWN);

+ notifyAdapter = true;

}

// An optional sign-in page.

if (mFreProperties.getBoolean(SHOW_SIGNIN_PAGE)) {

mPages.add(pageOf(AccountFirstRunFragment.class));

mFreProgressStates.add(FRE_PROGRESS_SIGNIN_SHOWN);

+ notifyAdapter = true;

+ }

+ if (notifyAdapter && mPagerAdapter != null) {

+ mPagerAdapter.notifyDataSetChanged();

}

+ mPostNativePageSequenceCreated = true;

}

// Activity:

@Override

protected void onCreate(Bundle savedInstanceState) {

- initializeBrowserProcess();

super.onCreate(savedInstanceState);

if (savedInstanceState != null) {

@@ -170,13 +197,14 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

new FirstRunFlowSequencer(this, mFreProperties) {

@Override

- public void onFlowIsKnown(Bundle freProperties) {

+ public void onFlowIsKnown(Bundle freProperties, Runnable setUpPropertiesPostNative) {

if (freProperties == null) {

completeFirstRunExperience();

return;

}

mFreProperties = freProperties;

+ mSetUpPropertiesPostNative = setUpPropertiesPostNative;

mShowWelcomePage = mFreProperties.getBoolean(SHOW_WELCOME_PAGE);

createPageSequence();

@@ -226,6 +254,12 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

@Override

protected void onStart() {

super.onStart();

+ // Since the FRE may be shown before any tab is shown, mark that this is the point at

+ // which Chrome went to foreground. This is needed as otherwise an assert will be hit

+ // in UmaUtils.getForegroundStartTime() when recording the time taken to load the first

+ // page (which happens after native has been initialized possibly while FRE is still

+ // active).

+ UmaUtils.recordForegroundStartTime();

stopProgressionIfNotAcceptedTermsOfService();

if (!mFreProperties.getBoolean(EXTRA_USE_FRE_FLOW_SEQUENCER)) {

if (FirstRunStatus.getFirstRunFlowComplete(this)) {

@@ -290,6 +324,7 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

@Override

public void completeFirstRunExperience() {

+ ensureBrowserProcessInitialized();

if (!TextUtils.isEmpty(mResultSignInAccountName)) {

boolean defaultAccountName =

sGlue.isDefaultAccountName(getApplicationContext(), mResultSignInAccountName);

@@ -360,6 +395,13 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

@Override

public void acceptTermsOfService(boolean allowCrashUpload) {

+ // At this point, we're advancing past the first page, which has no native

+ // dependencies to further pages in the sequence, if any. These require

+ // native to be initialized and will be added by the call below if needed.

+ // Additionally, the calls later in this function also require native

+ // to be initialized.

+ createPostNativePageSequence();

// If default is true then it corresponds to opt-out and false corresponds to opt-in.

UmaUtils.recordMetricsReportingDefaultOptIn(!DEFAULT_METRICS_AND_CRASH_REPORTING);

sGlue.acceptTermsOfService(allowCrashUpload);

@@ -461,17 +503,17 @@ public class FirstRunActivity extends AppCompatActivity implements FirstRunPageD

while (currentPageIndex < mPagerAdapter.getCount()) {

FirstRunPage currentPage = (FirstRunPage) mPagerAdapter.getItem(currentPageIndex);

if (!currentPage.shouldSkipPageOnCreate(getApplicationContext())) return;

+ // If we're advancing to the next page, ensure to init the post native page

+ // sequence - as every page except the first requires that to be initialized.

+ // This is a no-op if it has already been done.

+ createPostNativePageSequence();

if (!jumpToPage(currentPageIndex + 1)) return;

currentPageIndex = mPager.getCurrentItem();

}

- private void initializeBrowserProcess() {

- // The Chrome browser process must be started here because this Activity

- // may be started explicitly for tests cases, from Android notifications or

- // when the application is restoring a FRE fragment after Chrome being killed.

- // This should happen before super.onCreate() because it might recreate a fragment,

- // and a fragment might depend on the native library.

+ private void ensureBrowserProcessInitialized() {

+ if (mNativeSideIsInitialized) return;

Ted C 2016/12/01 21:59:06 Should we just make this Activity an AsyncInitiali

Alexei Svitkine (slow) 2016/12/02 18:34:54 This seems like a reasonable thing to do, but it s

This seems like a reasonable thing to do, but it seems like it's expanding the scope of this CL. In the current world, the CCT case still requires synchronous native initialization - and this CL doesn't change that (except of *when* that happens - i.e. previously before the first FRE screen, now before the second FRE screen). Since this CL is not regressing anything on this front, I'm thinking maybe we can do the conversion to AsyncInitializationActivity in a follow-up CL? (I'm happy to work on it afterwards for sure). I also have separate concerns about the UI changes you propose - it doesn't seem ideal to disable the button without making it clear to the user why's it's disabled. I'd rather we have some kind of progress UI like a spinning circle to indicate something is happening. Which to me seems more of a reason to do this in a separate CL - since that would be more things to change when we do that. WDYT?

Ted C 2016/12/02 21:34:52 I'm not 100% dead set on making it an AsyncInitial

On 2016/12/02 18:34:54, Alexei Svitkine wrote: > On 2016/12/01 21:59:06, Ted C wrote: > > Should we just make this Activity an AsyncInitializationActivity? > > > > If you launch first run via a CCT intent, then this will not have been > triggered > > in the background and will be slow when you hit accept on TOS. > > > > In general, I think each fragment should have more understanding of the native > > loaded-ness. > > > > Instead of blocking on the button click, I think the button should be disabled > > until native has been initialized (potentially we shouldn't show the button at > > all and instead an "Initializing . .. ..." message in its place). > > > > While CCTs were paying this cost already, it was happening at first load, > which > > I think is less painful than when the user is attempting to interact with the > > UI. > > > > My "general" thinking is that: > > - This class would become an AsyncInitializationActivity > > - FirstRunFlowSequencer would have a new method startWithNative that does the > > native initailization (onFlowKnown would be broken into onPreNativeFlowKnown, > > onPostNativeFlowKnown) > > - Maybe within this class, jumpToPage would track whether native is > initialized. > > If not, it would cache the page value (assuming greater than 0) and > > onResumeWithNative, it would call jumpToPage with that value. > > - FirstRunPage would get onNativeInitialized that would do all the heavy > lifting > > for each of the native dependent pages (i.e. you'd basically move > onViewCreated > > to onNativeInitialized for the pages that matter, tos acking would enable the > > button there). > > This seems like a reasonable thing to do, but it seems like it's expanding the > scope of this CL. In the current world, the CCT case still requires synchronous > native initialization - and this CL doesn't change that (except of *when* that > happens - i.e. previously before the first FRE screen, now before the second FRE > screen). > > Since this CL is not regressing anything on this front, I'm thinking maybe we > can do the conversion to AsyncInitializationActivity in a follow-up CL? (I'm > happy to work on it afterwards for sure). > > I also have separate concerns about the UI changes you propose - it doesn't seem > ideal to disable the button without making it clear to the user why's it's > disabled. I'd rather we have some kind of progress UI like a spinning circle to > indicate something is happening. Which to me seems more of a reason to do this > in a separate CL - since that would be more things to change when we do that. > WDYT?

I'm not 100% dead set on making it an AsyncInitialization activity in this change, but I still think this makes the CCT case worse as I outlined in my initial comment. The change in timing is actually a very bad thing IMO in terms of usability. Blocking before any UI is shown is annoying, but I think quite a bit less annoying than hanging when you click a button. I think we could also get away with smaller UI changes initially. I think we could block jumpToPage past 0 if native is not loaded, we could also set the pager visibility to GONE until we go to any page (that would handle the case where we jump past welcome on startup). Or as a much shorter term work around, kick off native initialization when we trigger first run not through ChromeTabbedActivity to hopefully mitigate the wait on blocking here. I think we should do "something" here before we launch this, so I would consider it a very high priority if we lande these separately.

Alexei Svitkine (slow) 2016/12/02 22:42:32 In my local testing, I did not see a considerable

On 2016/12/02 21:34:52, Ted C wrote: > On 2016/12/02 18:34:54, Alexei Svitkine wrote: > > On 2016/12/01 21:59:06, Ted C wrote: > > > Should we just make this Activity an AsyncInitializationActivity? > > > > > > If you launch first run via a CCT intent, then this will not have been > > triggered > > > in the background and will be slow when you hit accept on TOS. > > > > > > In general, I think each fragment should have more understanding of the > native > > > loaded-ness. > > > > > > Instead of blocking on the button click, I think the button should be > disabled > > > until native has been initialized (potentially we shouldn't show the button > at > > > all and instead an "Initializing . .. ..." message in its place). > > > > > > While CCTs were paying this cost already, it was happening at first load, > > which > > > I think is less painful than when the user is attempting to interact with > the > > > UI. > > > > > > My "general" thinking is that: > > > - This class would become an AsyncInitializationActivity > > > - FirstRunFlowSequencer would have a new method startWithNative that does > the > > > native initailization (onFlowKnown would be broken into > onPreNativeFlowKnown, > > > onPostNativeFlowKnown) > > > - Maybe within this class, jumpToPage would track whether native is > > initialized. > > > If not, it would cache the page value (assuming greater than 0) and > > > onResumeWithNative, it would call jumpToPage with that value. > > > - FirstRunPage would get onNativeInitialized that would do all the heavy > > lifting > > > for each of the native dependent pages (i.e. you'd basically move > > onViewCreated > > > to onNativeInitialized for the pages that matter, tos acking would enable > the > > > button there). > > > > This seems like a reasonable thing to do, but it seems like it's expanding the > > scope of this CL. In the current world, the CCT case still requires > synchronous > > native initialization - and this CL doesn't change that (except of *when* that > > happens - i.e. previously before the first FRE screen, now before the second > FRE > > screen). > > > > Since this CL is not regressing anything on this front, I'm thinking maybe we > > can do the conversion to AsyncInitializationActivity in a follow-up CL? (I'm > > happy to work on it afterwards for sure). > > > > I also have separate concerns about the UI changes you propose - it doesn't > seem > > ideal to disable the button without making it clear to the user why's it's > > disabled. I'd rather we have some kind of progress UI like a spinning circle > to > > indicate something is happening. Which to me seems more of a reason to do this > > in a separate CL - since that would be more things to change when we do that. > > WDYT? > > I'm not 100% dead set on making it an AsyncInitialization activity in this > change, but I still think this makes the CCT case worse as I outlined in my > initial comment. > > The change in timing is actually a very bad thing IMO in terms of usability. > Blocking before any UI is shown is annoying, but I think quite a bit less > annoying than hanging when you click a button.

In my local testing, I did not see a considerable delay when clicking the button - and this is a debug build. However, I suppose things could be very different on lower end devices - do we have any stats about how long it takes? Also, I'm happy to work on the next bit directly after this CL so hopefully it won't be more than a week delay in this intermediate state.

Yusuf 2016/12/02 23:31:23 This is turning more into a UX discussion at this

On 2016/12/02 22:42:32, Alexei Svitkine wrote: > On 2016/12/02 21:34:52, Ted C wrote: > > On 2016/12/02 18:34:54, Alexei Svitkine wrote: > > > On 2016/12/01 21:59:06, Ted C wrote: > > > > Should we just make this Activity an AsyncInitializationActivity? > > > > > > > > If you launch first run via a CCT intent, then this will not have been > > > triggered > > > > in the background and will be slow when you hit accept on TOS. > > > > > > > > In general, I think each fragment should have more understanding of the > > native > > > > loaded-ness. > > > > > > > > Instead of blocking on the button click, I think the button should be > > disabled > > > > until native has been initialized (potentially we shouldn't show the > button > > at > > > > all and instead an "Initializing . .. ..." message in its place). > > > > > > > > While CCTs were paying this cost already, it was happening at first load, > > > which > > > > I think is less painful than when the user is attempting to interact with > > the > > > > UI. > > > > > > > > My "general" thinking is that: > > > > - This class would become an AsyncInitializationActivity > > > > - FirstRunFlowSequencer would have a new method startWithNative that does > > the > > > > native initailization (onFlowKnown would be broken into > > onPreNativeFlowKnown, > > > > onPostNativeFlowKnown) > > > > - Maybe within this class, jumpToPage would track whether native is > > > initialized. > > > > If not, it would cache the page value (assuming greater than 0) and > > > > onResumeWithNative, it would call jumpToPage with that value. > > > > - FirstRunPage would get onNativeInitialized that would do all the heavy > > > lifting > > > > for each of the native dependent pages (i.e. you'd basically move > > > onViewCreated > > > > to onNativeInitialized for the pages that matter, tos acking would enable > > the > > > > button there). > > > > > > This seems like a reasonable thing to do, but it seems like it's expanding > the > > > scope of this CL. In the current world, the CCT case still requires > > synchronous > > > native initialization - and this CL doesn't change that (except of *when* > that > > > happens - i.e. previously before the first FRE screen, now before the second > > FRE > > > screen). > > > > > > Since this CL is not regressing anything on this front, I'm thinking maybe > we > > > can do the conversion to AsyncInitializationActivity in a follow-up CL? (I'm > > > happy to work on it afterwards for sure). > > > > > > I also have separate concerns about the UI changes you propose - it doesn't > > seem > > > ideal to disable the button without making it clear to the user why's it's > > > disabled. I'd rather we have some kind of progress UI like a spinning circle > > to > > > indicate something is happening. Which to me seems more of a reason to do > this > > > in a separate CL - since that would be more things to change when we do > that. > > > WDYT? > > > > I'm not 100% dead set on making it an AsyncInitialization activity in this > > change, but I still think this makes the CCT case worse as I outlined in my > > initial comment. > > > > The change in timing is actually a very bad thing IMO in terms of usability. > > Blocking before any UI is shown is annoying, but I think quite a bit less > > annoying than hanging when you click a button. > > In my local testing, I did not see a considerable delay when clicking the button > - and this is a debug build. However, I suppose things could be very different > on lower end devices - do we have any stats about how long it takes? Also, I'm > happy to work on the next bit directly after this CL so hopefully it won't be > more than a week delay in this intermediate state. > > > > > I think we could also get away with smaller UI changes initially. > > > > I think we could block jumpToPage past 0 if native is not loaded, we could > > also set the pager visibility to GONE until we go to any page (that would > > handle the case where we jump past welcome on startup). > > > > Or as a much shorter term work around, kick off native initialization > > when we trigger first run not through ChromeTabbedActivity to hopefully > > mitigate the wait on blocking here. > > > > I think we should do "something" here before we launch this, so I would > > consider it a very high priority if we lande these separately. >

This is turning more into a UX discussion at this point, but to the discussion of showing UI vs avoiding a delayed activation button, I say why not have both? We show a text saying "acquiring login information..." which gets replaced with the button when everything is ready. The user knows they are waiting for something and they dont get to feel the UI is not responsive. And +1 to making this an AsyncInitializationActivity at a very near time.

Ted C 2016/12/02 23:47:33 Lower end devices are very, very slow at loading l

Lower end devices are very, very slow at loading libraries. In my on and off testing, I've seen it easily exceed the ANR limit on Android. Especially on the very first cold start (i.e. the one that would trigger first run), as it has to run through the additional ResourceExtraction logic where it pulls out icu data, pak files, etc...

try {

ChromeBrowserInitializer.getInstance(this).handleSynchronousStartup();

mNativeSideIsInitialized = true;