Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519853004:
Allow fetching variations while FRE is showing on Android first run. (Closed)
|
Created:
4 years, 1 month ago by Alexei Svitkine (slow) Modified:
4 years ago CC:
chromium-reviews, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow fetching variations while FRE is showing on Android first run.
This CL makes initialization of freProperties happen in two stages - the parts
that don't have native dependencies can happen as they do now, while the parts
that require native are postponed to be done only after proceeding after the
first FRE screen.
This allows to run the FRE code before native is initialized and this CL does so
by moving the code to launch FRE to preInflationStartup(). This way, the FRE
flow starts while the rest of browser startup is still happening including
async native initialization.
With this CL, the variations fetch and native async initialization has a chance
to complete while the user is still on first FRE page. If instead they click
through before that completes, native will be initialized synchronously and so
the variations fetch will arrive later than native init.
I think this is OK for now and we can look at the stats of how often we lose
this race to decide whether we need to block on it before advancing to next FRE
page (e.g. with a progress spinner UI).
BUG=632199
Committed: https://crrev.com/a95e2463c072ace6fdb45f948dc24e2de4e781b9
Cr-Commit-Position: refs/heads/master@{#436530}
Patch Set 1 #Patch Set 2 : Revert tag change. #Patch Set 3 : Fix unit tests. #
Total comments: 12
Patch Set 4 : Replace runnable with a sequencer method. #
Total comments: 18
Patch Set 5 : Add initializeSharedState(). #Patch Set 6 : Moar comments. #Patch Set 7 : Fix recordForegroundStartTime(). #
Messages
Total messages: 74 (54 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Patchset #6 (id:140001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Refactor FRE code to move native deps to after first screen. This paves way to showing the first FRE screen before the native code is initialized and in parallel with fetching the variations seed. After this change, still more work is needed to achieve the above: - Remove the native dep on "TOS acked" - Change FRE to actually be shown before native is initialized BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a runnable. With the above, this allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Description was changed from ========== Allow fetching variations while FRE is showing on first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a runnable. With the above, this allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a runnable. With the above, this allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a runnable. With the above, this allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This allows the FRE flow to start while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and the fetch will not happen in time before native initialization. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ==========
Patchset #3 (id:120001) has been deleted
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to consider whether we make advancing from the first FRE card to be blocked on async initialization completing (with a spinner UI) - as opposed to doing synchronous init at that point as the CL does now. BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 ==========
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #1 (id:40001) has been deleted
asvitkine@chromium.org changed reviewers: + gogerald@chromium.org, tedchoc@chromium.org, yusufo@chromium.org
PTAL! Apologies for the large CL, but I think this is the last part of this work (% us deciding on whether we need to block advancement with a spinner). I've tested this locally and have confirmed that things work fine both if async initialization completes before going past first FRE card or if we click too quickly before it completes (by introducing an artificial delay for testing). Appreciate quick review turn around time on this (even if there's lots of comments) as I only have just next week and a couple of days before my vacation for the rest of the year and would hopefully like to land this before then and confirm it's working in the wild.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There was some /FirstRunFlowSequencerTest failures in the earlier patch set because the CL moved some logic from start() to processFreEnvironment() and also due to the extra POST_NATIVE_SETUP_NEEDED property. These should now be fixed with the latest patch set.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You've opened a lovely can of worms here I see. Let's dig in! https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:516: if (mNativeSideIsInitialized) return; 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). https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:53: * freProperties after native initialization. should be aligned with "Runnable that..." https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:155: Runnable setUpPropertiesPostNative = new Runnable() { instead of passing a Runnable, I think this should stuff a new boolean into the launch properties that is something like EXTRA_FRE_FLOW_SEQUENCER_NATIVE_INIT_PENDING. Then on native init, you'd check that boolean and call into this class again. I think that is more useful because EXTRA_USE_FRE_FLOW_SEQUENCER no longer indicates that the sequencer is complete like it did before. So, I think we want a new boolean that can be used in combo with the existing one to know if it really is complete. Ooooh, you already have POST_NATIVE_SETUP_NEEDED. I think we should just use that and not pass the runnable. Have FirstRunActivity keep a reference to the sequencer and call onNativeInitialized or something on it to do the rest. https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:158: // We show the sign-in page if sync is allowed, and not signed in, and this is not should this remove POST_NATIVE_SETUP_NEEDED?
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Thanks. I changed the code to remove the Runnable as you suggest, but didn't do the AsyncInitializationActivity bit yet - see discussion below and let me know what you think! https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:516: if (mNativeSideIsInitialized) return; 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? https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:53: * freProperties after native initialization. On 2016/12/01 21:59:06, Ted C wrote: > should be aligned with "Runnable that..." Removed param. https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:155: Runnable setUpPropertiesPostNative = new Runnable() { On 2016/12/01 21:59:06, Ted C wrote: > instead of passing a Runnable, I think this should stuff a new boolean into the > launch properties that is something like > EXTRA_FRE_FLOW_SEQUENCER_NATIVE_INIT_PENDING. Then on native init, you'd check > that boolean and call into this class again. > > I think that is more useful because EXTRA_USE_FRE_FLOW_SEQUENCER no longer > indicates that the sequencer is complete like it did before. So, I think we > want a new boolean that can be used in combo with the existing one to know if it > really is complete. > > Ooooh, you already have POST_NATIVE_SETUP_NEEDED. I think we should just use > that and not pass the runnable. Have FirstRunActivity keep a reference to the > sequencer and call onNativeInitialized or something on it to do the rest. Done. Compared to the previous approach, this required adding mIsAndroidEduDevice and mHasChildAccount members so we can later use them in onNativeInitialized() as well as some duplication of logic (e.g. onlyOneAccount) - which previously we could re-use by having them be bound to the Runnable. However, I agree that the new approach is a bit cleaner. https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:158: // We show the sign-in page if sync is allowed, and not signed in, and this is not On 2016/12/01 21:59:06, Ted C wrote: > should this remove POST_NATIVE_SETUP_NEEDED? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:516: if (mNativeSideIsInitialized) return; 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. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:103: private boolean mPostNativePageSequenceCreated; Would we be able to use POST_NATIVE_SETUP_NEEDED instead of this? https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:113: private Runnable mSetUpPropertiesPostNative; unneeded https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:145: final Account[] googleAccounts = getGoogleAccounts(); should these lines go below: if (!mLaunchProperties.getBoolean(FirstRunActivity.EXTRA_USE_FRE_FLOW_SEQUENCER)) { They doesn't seem to be used. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:184: public void onNativeInitialized(Bundle freProperties) { javadoc https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:188: final boolean onlyOneAccount = googleAccounts.length == 1; I would cache onlyOneAccount and forceEduSignIn above. I don't know if this class would behave nicely if these changed between the two, so probably better to cache and ensure they're in sync. And I guess if we want to keep them in sync, we should keep them where they are in the above method to ensure they are calculated before the early return. Or maybe we should move all the variable caching to "public void onParametersReady() {" or even add a new initializeSharedState or something before processFreEnvironment is called. Maybe we should rename processFreEnvironment to include "PreNative" or something to make it very clear. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:35: if (sForegroundStartTimeMs == 0) sForegroundStartTimeMs = SystemClock.uptimeMillis(); Hmm...this is a change in behavior. Previously this value would get updated if you backgrounded chrome and brought it back to the foreground. Now it will always be the initial time of the process.
Thanks for the comments - some discussion below. https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:516: if (mNativeSideIsInitialized) return; 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. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:188: final boolean onlyOneAccount = googleAccounts.length == 1; On 2016/12/02 21:34:52, Ted C wrote: > I would cache onlyOneAccount and forceEduSignIn above. I don't know if this > class would behave nicely if these changed between the two, so probably better > to cache and ensure they're in sync. > > And I guess if we want to keep them in sync, we should keep them where they are > in the above method to ensure they are calculated before the early return. So one other point of consideration is that I believe it's possible for the two functions to be called in different sessions. For example, if Chrome is killed before the native code runs and we end up restoring freProperties. This would be the case where we early return from processFreEnvironment() when EXTRA_USE_FRE_FLOW_SEQUENCER is not set. So caching locally in this class doesn't seem useful. Maybe we should cache all these things in freProperties instead - wdyt? (We wouldn't be able to cache getGoogleAccounts() but we can cache anything computed from it.) > > Or maybe we should move all the variable caching to "public void > onParametersReady() {" or even add a new initializeSharedState or something > before processFreEnvironment is called. Maybe we should rename > processFreEnvironment to include "PreNative" or something to make it very clear. We can't do it in onParametersReady() - I wanted to do this initially - because the test code relies on passing different values to the params of this function. We could make an initializeSharedState() function with the parameters that the test code could use.
https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:516: if (mNativeSideIsInitialized) return; 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.
https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:516: if (mNativeSideIsInitialized) return; 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. 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... > > > > > 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. > https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:188: final boolean onlyOneAccount = googleAccounts.length == 1; On 2016/12/02 22:42:32, Alexei Svitkine wrote: > On 2016/12/02 21:34:52, Ted C wrote: > > I would cache onlyOneAccount and forceEduSignIn above. I don't know if this > > class would behave nicely if these changed between the two, so probably better > > to cache and ensure they're in sync. > > > > And I guess if we want to keep them in sync, we should keep them where they > are > > in the above method to ensure they are calculated before the early return. > > So one other point of consideration is that I believe it's possible for the two > functions to be called in different sessions. For example, if Chrome is killed > before the native code runs and we end up restoring freProperties. Indeed that is true, but that is why I suggested caching above: if (!mLaunchProperties.getBoolean(FirstRunActivity.EXTRA_USE_FRE_FLOW_SEQUENCER)) { Even across sessions that bit of code will be run (as that is the early exit condition). > > This would be the case where we early return from processFreEnvironment() when > EXTRA_USE_FRE_FLOW_SEQUENCER is not set. > > So caching locally in this class doesn't seem useful. Maybe we should cache all > these things in freProperties instead - wdyt? (We wouldn't be able to cache > getGoogleAccounts() but we can cache anything computed from it.) > > > > > Or maybe we should move all the variable caching to "public void > > onParametersReady() {" or even add a new initializeSharedState or something > > before processFreEnvironment is called. Maybe we should rename > > processFreEnvironment to include "PreNative" or something to make it very > clear. > > We can't do it in onParametersReady() - I wanted to do this initially - because > the test code relies on passing different values to the params of this function. > We could make an initializeSharedState() function with the parameters that the > test code could use. > I think that sounds find to me. Having an initializeSharedState that is called normally from onParametersReady for production code, but called from test code differently sounds reasonable to me.
Added initializeSharedState() PTAL.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/05 16:48:51, Alexei Svitkine wrote: > Added initializeSharedState() PTAL. Overall, that seems good to me. I still had comments on the change to UmaUtils.java from a previous patch set. Also, whether we could replace FirstRunActivity#mPostNativePageSequenceCreated with checks for the existence of POST_NATIVE_SETUP_NEEDED.
Thanks - sorry forgot to address those other comments - addressed now. Also let me know if you feel strongly that I should work on the AsyncInitializationActivity in this CL instead of in a follow up CL. (My preference would be to land this first and then right after start work on the AsyncInitializationActivity CL.) https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:103: private boolean mPostNativePageSequenceCreated; On 2016/12/02 21:34:52, Ted C wrote: > Would we be able to use POST_NATIVE_SETUP_NEEDED instead of this? I don't think so, because POST_NATIVE_SETUP_NEEDED could have been addressed in a previous session and the properties restored - whereas createPostNativePageSequence() actually needs to add things to mPages. Added a comment about it to the place where this is checked. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:113: private Runnable mSetUpPropertiesPostNative; On 2016/12/02 21:34:52, Ted C wrote: > unneeded Removed. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:145: final Account[] googleAccounts = getGoogleAccounts(); On 2016/12/02 21:34:53, Ted C wrote: > should these lines go below: > > if > (!mLaunchProperties.getBoolean(FirstRunActivity.EXTRA_USE_FRE_FLOW_SEQUENCER)) { > > They doesn't seem to be used. Now in initializeSharedState(). https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:184: public void onNativeInitialized(Bundle freProperties) { On 2016/12/02 21:34:53, Ted C wrote: > javadoc Done. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java:188: final boolean onlyOneAccount = googleAccounts.length == 1; On 2016/12/02 23:47:33, Ted C wrote: > On 2016/12/02 22:42:32, Alexei Svitkine wrote: > > On 2016/12/02 21:34:52, Ted C wrote: > > > I would cache onlyOneAccount and forceEduSignIn above. I don't know if this > > > class would behave nicely if these changed between the two, so probably > better > > > to cache and ensure they're in sync. > > > > > > And I guess if we want to keep them in sync, we should keep them where they > > are > > > in the above method to ensure they are calculated before the early return. > > > > So one other point of consideration is that I believe it's possible for the > two > > functions to be called in different sessions. For example, if Chrome is killed > > before the native code runs and we end up restoring freProperties. > > Indeed that is true, but that is why I suggested caching above: > if > (!mLaunchProperties.getBoolean(FirstRunActivity.EXTRA_USE_FRE_FLOW_SEQUENCER)) { > > Even across sessions that bit of code will be run (as that is the early > exit condition). > > > > > This would be the case where we early return from processFreEnvironment() when > > EXTRA_USE_FRE_FLOW_SEQUENCER is not set. > > > > So caching locally in this class doesn't seem useful. Maybe we should cache > all > > these things in freProperties instead - wdyt? (We wouldn't be able to cache > > getGoogleAccounts() but we can cache anything computed from it.) > > > > > > > > Or maybe we should move all the variable caching to "public void > > > onParametersReady() {" or even add a new initializeSharedState or something > > > before processFreEnvironment is called. Maybe we should rename > > > processFreEnvironment to include "PreNative" or something to make it very > > clear. > > > > We can't do it in onParametersReady() - I wanted to do this initially - > because > > the test code relies on passing different values to the params of this > function. > > We could make an initializeSharedState() function with the parameters that the > > test code could use. > > > > I think that sounds find to me. Having an initializeSharedState that is called > normally from onParametersReady for production code, but called from test > code differently sounds reasonable to me. > Done. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:35: if (sForegroundStartTimeMs == 0) sForegroundStartTimeMs = SystemClock.uptimeMillis(); On 2016/12/02 21:34:53, Ted C wrote: > Hmm...this is a change in behavior. > > Previously this value would get updated if you backgrounded chrome and brought > it back to the foreground. Now it will always be the initial time of the > process. So, this change is needed because I'm now calling recordForegroundStartTime() from the FRE code - which was not done before. That's needed because without it we hit an assert in getForegroundStartTime(), because with the refactoring, we end up loading a page (the new tab page) while the ChromeTabbedActivity is in background - whereas before we would always load the NTP before showing the FRE. Looking at the previous call site of this - ChromeActivitySessionTracker.onForegroundSessionStart(), it says the following: /** * Called when a top-level Chrome activity (ChromeTabbedActivity, FullscreenActivity) is * started in foreground. It will not be called again when other Chrome activities take over * (see onStart()), that is, when correct activity calls startActivity() for another Chrome * activity. */ The comment is a little confusing, but I *think* it says that it will only ever be called once for the first time the Chrome activity is foreground. Indeed, that seems to match my reading of the code - as ChromeActivitySessionTracker is a singletone (sInstance) and it has a boolean (mIsStarted) to ensure it doesn't call onForegroundSessionStart() more than once. So based on the above, this should be safe to do without any behavior change. Do you agree?
https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:35: if (sForegroundStartTimeMs == 0) sForegroundStartTimeMs = SystemClock.uptimeMillis(); On 2016/12/05 20:09:03, Alexei Svitkine wrote: > On 2016/12/02 21:34:53, Ted C wrote: > > Hmm...this is a change in behavior. > > > > Previously this value would get updated if you backgrounded chrome and brought > > it back to the foreground. Now it will always be the initial time of the > > process. > > So, this change is needed because I'm now calling recordForegroundStartTime() > from the FRE code - which was not done before. > > That's needed because without it we hit an assert in getForegroundStartTime(), > because with the refactoring, we end up loading a page (the new tab page) while > the ChromeTabbedActivity is in background - whereas before we would always load > the NTP before showing the FRE. > > Looking at the previous call site of this - > ChromeActivitySessionTracker.onForegroundSessionStart(), it says the following: > > /** > * Called when a top-level Chrome activity (ChromeTabbedActivity, > FullscreenActivity) is > * started in foreground. It will not be called again when other Chrome > activities take over > * (see onStart()), that is, when correct activity calls startActivity() for > another Chrome > * activity. > */ > > The comment is a little confusing, but I *think* it says that it will only ever > be called once for the first time the Chrome activity is foreground. Indeed, > that seems to match my reading of the code - as ChromeActivitySessionTracker is > a singletone (sInstance) and it has a boolean (mIsStarted) to ensure it doesn't > call onForegroundSessionStart() more than once. > > So based on the above, this should be safe to do without any behavior change. Do > you agree? Sorry, forgot to reply to your actual concern: "Previously this value would get updated if you backgrounded chrome and brought it back to the foreground. Now it will always be the initial time of the process." I don't think that's true - but please correct if I'm wrong. The check for 0 shouldn't change the background behavior and my new call site from FirstRunActivity.onStart() I don't think should be called on sessions that don't trigger FRE.
https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:103: private boolean mPostNativePageSequenceCreated; On 2016/12/05 20:09:03, Alexei Svitkine wrote: > On 2016/12/02 21:34:52, Ted C wrote: > > Would we be able to use POST_NATIVE_SETUP_NEEDED instead of this? > > I don't think so, because POST_NATIVE_SETUP_NEEDED could have been addressed in > a previous session and the properties restored - whereas > createPostNativePageSequence() actually needs to add things to mPages. > > Added a comment about it to the place where this is checked. oooh, indeed, thanks for the comment. https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:35: if (sForegroundStartTimeMs == 0) sForegroundStartTimeMs = SystemClock.uptimeMillis(); On 2016/12/05 20:15:15, Alexei Svitkine wrote: > On 2016/12/05 20:09:03, Alexei Svitkine wrote: > > On 2016/12/02 21:34:53, Ted C wrote: > > > Hmm...this is a change in behavior. > > > > > > Previously this value would get updated if you backgrounded chrome and > brought > > > it back to the foreground. Now it will always be the initial time of the > > > process. > > > > So, this change is needed because I'm now calling recordForegroundStartTime() > > from the FRE code - which was not done before. > > > > That's needed because without it we hit an assert in getForegroundStartTime(), > > because with the refactoring, we end up loading a page (the new tab page) > while > > the ChromeTabbedActivity is in background - whereas before we would always > load > > the NTP before showing the FRE. > > > > Looking at the previous call site of this - > > ChromeActivitySessionTracker.onForegroundSessionStart(), it says the > following: > > > > /** > > * Called when a top-level Chrome activity (ChromeTabbedActivity, > > FullscreenActivity) is > > * started in foreground. It will not be called again when other Chrome > > activities take over > > * (see onStart()), that is, when correct activity calls startActivity() > for > > another Chrome > > * activity. > > */ > > > > The comment is a little confusing, but I *think* it says that it will only > ever > > be called once for the first time the Chrome activity is foreground. Indeed, > > that seems to match my reading of the code - as ChromeActivitySessionTracker > is > > a singletone (sInstance) and it has a boolean (mIsStarted) to ensure it > doesn't > > call onForegroundSessionStart() more than once. > > > > So based on the above, this should be safe to do without any behavior change. > Do > > you agree? > > Sorry, forgot to reply to your actual concern: > > "Previously this value would get updated if you backgrounded chrome and brought > it back to the foreground. Now it will always be the initial time of the > process." > > I don't think that's true - but please correct if I'm wrong. The check for 0 > shouldn't change the background behavior and my new call site from > FirstRunActivity.onStart() I don't think should be called on sessions that don't > trigger FRE. Sorry, but nope. mIsStarted is set back to false when all Chrome activities are backgrounded. If you add logging to recordForegroundStartTime, you'll see it is called when you leave Chrome and come back each time. So this definitely is a change in behavior. We could potentially reset the value back to 0 in ChromeActivitySessionTracker#onForegroundSessionEnd, but I have no idea if that would cause similar assets to what you were already seeing.
On 2016/12/05 21:51:11, Ted C wrote: > https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java > (right): > > https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:103: > private boolean mPostNativePageSequenceCreated; > On 2016/12/05 20:09:03, Alexei Svitkine wrote: > > On 2016/12/02 21:34:52, Ted C wrote: > > > Would we be able to use POST_NATIVE_SETUP_NEEDED instead of this? > > > > I don't think so, because POST_NATIVE_SETUP_NEEDED could have been addressed > in > > a previous session and the properties restored - whereas > > createPostNativePageSequence() actually needs to add things to mPages. > > > > Added a comment about it to the place where this is checked. > > oooh, indeed, thanks for the comment. > > https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... > File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java > (right): > > https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:35: if > (sForegroundStartTimeMs == 0) sForegroundStartTimeMs = > SystemClock.uptimeMillis(); > On 2016/12/05 20:15:15, Alexei Svitkine wrote: > > On 2016/12/05 20:09:03, Alexei Svitkine wrote: > > > On 2016/12/02 21:34:53, Ted C wrote: > > > > Hmm...this is a change in behavior. > > > > > > > > Previously this value would get updated if you backgrounded chrome and > > brought > > > > it back to the foreground. Now it will always be the initial time of the > > > > process. > > > > > > So, this change is needed because I'm now calling > recordForegroundStartTime() > > > from the FRE code - which was not done before. > > > > > > That's needed because without it we hit an assert in > getForegroundStartTime(), > > > because with the refactoring, we end up loading a page (the new tab page) > > while > > > the ChromeTabbedActivity is in background - whereas before we would always > > load > > > the NTP before showing the FRE. > > > > > > Looking at the previous call site of this - > > > ChromeActivitySessionTracker.onForegroundSessionStart(), it says the > > following: > > > > > > /** > > > * Called when a top-level Chrome activity (ChromeTabbedActivity, > > > FullscreenActivity) is > > > * started in foreground. It will not be called again when other Chrome > > > activities take over > > > * (see onStart()), that is, when correct activity calls startActivity() > > for > > > another Chrome > > > * activity. > > > */ > > > > > > The comment is a little confusing, but I *think* it says that it will only > > ever > > > be called once for the first time the Chrome activity is foreground. Indeed, > > > that seems to match my reading of the code - as ChromeActivitySessionTracker > > is > > > a singletone (sInstance) and it has a boolean (mIsStarted) to ensure it > > doesn't > > > call onForegroundSessionStart() more than once. > > > > > > So based on the above, this should be safe to do without any behavior > change. > > Do > > > you agree? > > > > Sorry, forgot to reply to your actual concern: > > > > "Previously this value would get updated if you backgrounded chrome and > brought > > it back to the foreground. Now it will always be the initial time of the > > process." > > > > I don't think that's true - but please correct if I'm wrong. The check for 0 > > shouldn't change the background behavior and my new call site from > > FirstRunActivity.onStart() I don't think should be called on sessions that > don't > > trigger FRE. > > Sorry, but nope. > > mIsStarted is set back to false when all Chrome activities are backgrounded. > > If you add logging to recordForegroundStartTime, you'll see it is called > when you leave Chrome and come back each time. So this definitely is a > change in behavior. > > We could potentially reset the value back to 0 in > ChromeActivitySessionTracker#onForegroundSessionEnd, but I have no idea > if that would cause similar assets to what you were already seeing. Also, doing the async initialization in a follow sgtm.
The CQ bit was checked by asvitkine@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java (right): https://codereview.chromium.org/2519853004/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/metrics/UmaUtils.java:35: if (sForegroundStartTimeMs == 0) sForegroundStartTimeMs = SystemClock.uptimeMillis(); On 2016/12/05 21:51:11, Ted C wrote: > On 2016/12/05 20:15:15, Alexei Svitkine wrote: > > On 2016/12/05 20:09:03, Alexei Svitkine wrote: > > > On 2016/12/02 21:34:53, Ted C wrote: > > > > Hmm...this is a change in behavior. > > > > > > > > Previously this value would get updated if you backgrounded chrome and > > brought > > > > it back to the foreground. Now it will always be the initial time of the > > > > process. > > > > > > So, this change is needed because I'm now calling > recordForegroundStartTime() > > > from the FRE code - which was not done before. > > > > > > That's needed because without it we hit an assert in > getForegroundStartTime(), > > > because with the refactoring, we end up loading a page (the new tab page) > > while > > > the ChromeTabbedActivity is in background - whereas before we would always > > load > > > the NTP before showing the FRE. > > > > > > Looking at the previous call site of this - > > > ChromeActivitySessionTracker.onForegroundSessionStart(), it says the > > following: > > > > > > /** > > > * Called when a top-level Chrome activity (ChromeTabbedActivity, > > > FullscreenActivity) is > > > * started in foreground. It will not be called again when other Chrome > > > activities take over > > > * (see onStart()), that is, when correct activity calls startActivity() > > for > > > another Chrome > > > * activity. > > > */ > > > > > > The comment is a little confusing, but I *think* it says that it will only > > ever > > > be called once for the first time the Chrome activity is foreground. Indeed, > > > that seems to match my reading of the code - as ChromeActivitySessionTracker > > is > > > a singletone (sInstance) and it has a boolean (mIsStarted) to ensure it > > doesn't > > > call onForegroundSessionStart() more than once. > > > > > > So based on the above, this should be safe to do without any behavior > change. > > Do > > > you agree? > > > > Sorry, forgot to reply to your actual concern: > > > > "Previously this value would get updated if you backgrounded chrome and > brought > > it back to the foreground. Now it will always be the initial time of the > > process." > > > > I don't think that's true - but please correct if I'm wrong. The check for 0 > > shouldn't change the background behavior and my new call site from > > FirstRunActivity.onStart() I don't think should be called on sessions that > don't > > trigger FRE. > > Sorry, but nope. > > mIsStarted is set back to false when all Chrome activities are backgrounded. > > If you add logging to recordForegroundStartTime, you'll see it is called > when you leave Chrome and come back each time. So this definitely is a > change in behavior. > > We could potentially reset the value back to 0 in > ChromeActivitySessionTracker#onForegroundSessionEnd, but I have no idea > if that would cause similar assets to what you were already seeing. Ah - you're right, I missed that! Done now, per offline discussion, by keeping a sBackgroundTimeMs and setting this if either not set or background time is after last fg time.
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen, via a Runnable. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thanks, submitting.
The CQ bit was checked by asvitkine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1480999981480460,
"parent_rev": "76c3f4459b01911667a29b9dccf5d574c4f48c60", "commit_rev":
"a2e74a6a07a3a50d79be1ff6b4900aa82184955d"}
Message was sent while issue was closed.
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 ========== to ========== Allow fetching variations while FRE is showing on Android first run. This CL makes initialization of freProperties happen in two stages - the parts that don't have native dependencies can happen as they do now, while the parts that require native are postponed to be done only after proceeding after the first FRE screen. This allows to run the FRE code before native is initialized and this CL does so by moving the code to launch FRE to preInflationStartup(). This way, the FRE flow starts while the rest of browser startup is still happening including async native initialization. With this CL, the variations fetch and native async initialization has a chance to complete while the user is still on first FRE page. If instead they click through before that completes, native will be initialized synchronously and so the variations fetch will arrive later than native init. I think this is OK for now and we can look at the stats of how often we lose this race to decide whether we need to block on it before advancing to next FRE page (e.g. with a progress spinner UI). BUG=632199 Committed: https://crrev.com/a95e2463c072ace6fdb45f948dc24e2de4e781b9 Cr-Commit-Position: refs/heads/master@{#436530} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a95e2463c072ace6fdb45f948dc24e2de4e781b9 Cr-Commit-Position: refs/heads/master@{#436530} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
