Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561433002:
Make FirstRunActivity extend AsyncInitializationActivity. (Closed)
|
Created:
4 years ago by Alexei Svitkine (slow) Modified:
3 years, 11 months ago CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake FirstRunActivity extend AsyncInitializationActivity.
This is a follow-up to https://codereview.chromium.org/2519853004/
which made it possible to show the first FRE screen before native
is initialized, to support fetching the variations seed at that
time.
This CL further builds on that to make FirstRunActivity actually
be an AsyncInitializationActivity, so that it never needs to do
the synchronous native init path - instead waiting for async
init to be done before allowing the user to proceed.
When clicking Accept on the first FRE page (ToS and UMA), this
will now show a spinner while waiting for native init and the
variations seed fetch (whose timeout has been lowered to 3s).
BUG=632199
Review-Url: https://codereview.chromium.org/2561433002
Cr-Commit-Position: refs/heads/master@{#442919}
Committed: https://chromium.googlesource.com/chromium/src/+/36860c01c1913acd255129a9024e22928f934d0e
Patch Set 1 #Patch Set 2 : Show a spinner when native isn't initialized yet instead of disabling the button. #
Total comments: 7
Patch Set 3 : Add skip logic and remove some calls to createPostNativePageSequence(). #Patch Set 4 : Rebase #Patch Set 5 : Use setUserVisibleHint() and hide UI when showing spinner. #Patch Set 6 : Rebase #Patch Set 7 : Couple small cleanups. #Patch Set 8 : Update UX. #Messages
Total messages: 53 (39 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: This issue passed the CQ dry run.
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...
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
asvitkine@chromium.org changed reviewers: + tedchoc@chromium.org, yusufo@chromium.org
Ted & Yusuf, PTAL. Some points of discussion: - I'm not sold on disabling the "next" button - as I think the user should have some indication of something happening and that it's not just broken. We could add a spinning circle or some text - WDYT? What would be the best way to go about that? (I've not done Clank UI work before.) - Unlike the behavior of my previous CL, since this blocks advancing until native init AND variations fetch are done, we now need to decide on a reasonable timeout for the variations fetch since it's blocking advancement. 75% of fetches take < 2.5s and 90% < 6s. Seems like 5s is a sweet spot, but it's a bit long from a UX point of view. We could pick something like 3s instead maybe? WDYT? - This code makes the FirstRunActivity track which fragments need to be notified of native init. I had to do this because surprisingly there's no API to get the list of fragments. We can't use the FirstRunPagerAdapter because it's getItem() function actually invokes the constructor for the page - i.e. each call creates a new instance - which is weird but seems to match Android example code.
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...
On 2016/12/12 17:38:24, Alexei Svitkine (OO from 15th) wrote: > Ted & Yusuf, PTAL. > > Some points of discussion: > - I'm not sold on disabling the "next" button - as I think the user should > have some indication of something happening and that it's not just broken. We > could add a spinning circle or some text - WDYT? What would be the best way to > go about that? (I've not done Clank UI work before.) > - Unlike the behavior of my previous CL, since this blocks advancing until > native init AND variations fetch are done, we now need to decide on a reasonable > timeout for the variations fetch since it's blocking advancement. 75% of > fetches take < 2.5s and 90% < 6s. Seems like 5s is a sweet spot, but it's a bit > long from a UX point of view. We could pick something like 3s instead maybe? > WDYT? > - This code makes the FirstRunActivity track which fragments need to be > notified of native init. I had to do this because surprisingly there's no API to > get the list of fragments. We can't use the FirstRunPagerAdapter because it's > getItem() function actually invokes the constructor for the page - i.e. each > call creates a new instance - which is weird but seems to match Android example > code. Did some searching and seems if we want to do a spinner, it's a <ProgressBar> UI element in Android: http://www.materialdoc.com/circular-progress/ Will try using this tomorrow, but I guess we need to discuss the UX of that and whether we want to do that.
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...)
I uploaded a new patchset where I switched the behavior to something in my opinion a little better: Instead of disabling the button until native is initialized (which would be confusing to a user as to why it's disabled), instead clicking the button before native will open a circular progress spinner if native hasn't been initialized yet and then transition to the next card once it has. I think this is better UX since the user has a clear indication that something is happening and they don't need to wait for anything to click the button. I am now happy with this approach % tweaking timeouts for variations seed fetch latency, which should still be done (see above for earlier discussion on that). One thing that I still need to do on this CL is to style it better - right now, the position of the spinner is not right and I'm thinking of the following options: 1. Show it vertically between the UMA checkbox and the button. 2. Show it instead of the button once the user clicks the button (hiding the button). 3. Hide all the other elements and show it as the only thing centered on the page. WDYT?
Overall, this seems reasonable. I do wish there was a cleaner way of getting the initialized fragments from the ViewPager, but you'd end up needing to do something very similar in the adapter to what you did here so I don't think there is much to be done there. The question about the UI when we need to skip states is the main thing that worries me. https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:104: private HashSet<FirstRunPage> mPagesToNotifyOfNativeInit; use generic Set for the variable declaration https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:244: skipPagesIfNecessary(); What does the UI look like now if we're supposed to skip the first page? Does it show until native is initialized? I wonder if we need to update the L&F of the first page in this case (i.e. don't show the accept buttons or any buttons for that matter, just the Chrome logo and the spinner you added?) https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java (right): https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java:61: view.findViewById(R.id.progress_spinner).setVisibility(View.VISIBLE); I would put this in an else below.
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Make FirstRunActivity extend AsyncInitializationActivity. This is a follow-up to https://codereview.chromium.org/2519853004/ which made it possible to show the first FRE screen before native is initialized, to support fetching the variations seed at that time. This CL further builds on that to make FirstRunActivity actually be an AsyncInitializationActivity, so that it never needs to do the synchronous native init path - instead waiting for async init to be done before allowing the user to proceed. BUG=632199 ========== to ========== Make FirstRunActivity extend AsyncInitializationActivity. This is a follow-up to https://codereview.chromium.org/2519853004/ which made it possible to show the first FRE screen before native is initialized, to support fetching the variations seed at that time. This CL further builds on that to make FirstRunActivity actually be an AsyncInitializationActivity, so that it never needs to do the synchronous native init path - instead waiting for async init to be done before allowing the user to proceed. When clicking Accept on the first FRE page (ToS and UMA), this will now show a spinner while waiting for native init and the variations seed fetch (whose timeout has been lowered to 3s). BUG=632199 ==========
Description was changed from ========== Make FirstRunActivity extend AsyncInitializationActivity. This is a follow-up to https://codereview.chromium.org/2519853004/ which made it possible to show the first FRE screen before native is initialized, to support fetching the variations seed at that time. This CL further builds on that to make FirstRunActivity actually be an AsyncInitializationActivity, so that it never needs to do the synchronous native init path - instead waiting for async init to be done before allowing the user to proceed. When clicking Accept on the first FRE page (ToS and UMA), this will now show a spinner while waiting for native init and the variations seed fetch (whose timeout has been lowered to 3s). BUG=632199 ========== to ========== Make FirstRunActivity extend AsyncInitializationActivity. This is a follow-up to https://codereview.chromium.org/2519853004/ which made it possible to show the first FRE screen before native is initialized, to support fetching the variations seed at that time. This CL further builds on that to make FirstRunActivity actually be an AsyncInitializationActivity, so that it never needs to do the synchronous native init path - instead waiting for async init to be done before allowing the user to proceed. When clicking Accept on the first FRE page (ToS and UMA), this will now show a spinner while waiting for native init and the variations seed fetch (whose timeout has been lowered to 3s). BUG=632199 ==========
Patchset #3 (id:120001) 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 checked by asvitkine@chromium.org to run a CQ dry run
Patchset #3 (id:140001) 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...
Thanks Ted, PTAL. I also added the timeout changes to variations seed fetching in this CL, since this CL blocks advancement past first FRE screen on the fetch and native. Also re-jiggered things a bit in the fragment to support both the skip case and the user hitting Back to go back to that page. https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:104: private HashSet<FirstRunPage> mPagesToNotifyOfNativeInit; On 2016/12/13 21:50:15, Ted C wrote: > use generic Set for the variable declaration Done. https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:244: skipPagesIfNecessary(); On 2016/12/13 21:50:15, Ted C wrote: > What does the UI look like now if we're supposed to skip the first page? Does > it show until native is initialized? > > I wonder if we need to update the L&F of the first page in this case (i.e. don't > show the accept buttons or any buttons for that matter, just the Chrome logo and > the spinner you added?) Good catch - you're right that this was not a good experience - it would still show the first page and then just advance past it after native was ready. I changed it to what you suggest - i.e. showing Chrome logo and spinner only during this time. Thanks! https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java (right): https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java:61: view.findViewById(R.id.progress_spinner).setVisibility(View.VISIBLE); On 2016/12/13 21:50:15, Ted C wrote: > I would put this in an else below. Refactored a bit. The previous code had an issue that if you would hit back on the next screen, it would come back to the screen with the spinner still showing and the UI greyed out - now the code is such that it restores things back to the original state when advancing. Note: I had to add new method to FirstRunPage to notify the page of when it's returned to via Back - as it turns out that since we use the Fragment pager adapter thing, it doesn't send the Fragment any normal events - indicating when it becomes active or inactive (I tried overriding almost everything in the Fragment API with no luck).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java (right): https://codereview.chromium.org/2561433002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/ToSAndUMAFirstRunFragment.java:61: view.findViewById(R.id.progress_spinner).setVisibility(View.VISIBLE); On 2016/12/15 01:13:24, Alexei Svitkine (OO from 15th) wrote: > On 2016/12/13 21:50:15, Ted C wrote: > > I would put this in an else below. > > Refactored a bit. The previous code had an issue that if you would hit back on > the next screen, it would come back to the screen with the spinner still showing > and the UI greyed out - now the code is such that it restores things back to the > original state when advancing. > > Note: I had to add new method to FirstRunPage to notify the page of when it's > returned to via Back - as it turns out that since we use the Fragment pager > adapter thing, it doesn't send the Fragment any normal events - indicating when > it becomes active or inactive (I tried overriding almost everything in the > Fragment API with no luck). Looking at the source code for FragmentPagerAdapter, actually it looks we can use setUserVisibleHint() here - which seems to be reliably called FragmentPagerAdapter based on the source code. I'm changing to use this since my previous custom FirstRunPage function that I called on back didn't reliably handle all the cases (I observed I could switch back to Chrome and get back to that page without the back codepath being hit...).
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...
Ok one more iteration on this - I think this is now finally in a good state. - I'm using setUserVisibleHint() to restore the UI from the spinner mode - I'm no longer disabling the button and checkbox when showing spinner and instead hiding them along with the TOS - the same thing as done when we're going to skip the page. I think this looks a bit nicer and cleaner and simplifies the code so there's just these two states. Ted, PTAL - I would like to get this landed today as I disappear for vacation as of tomorrow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Okay, still welcome comments on this - but given I've not received them yet and there are no Chrome releases until end of year anyway, maybe this will have to wait till Jan. Meanwhile, I discovered something that may be an issue with this - still investigating - but I'm observing a DCHECK in UmaEndSession() - which could be related to this because now that this is an AsyncInitializationActivity which is a ChromeActivity, it calls into those functions. So I'll investigate this more - but still welcome comments on the other parts of this CL.
On 2016/12/15 21:31:24, Alexei Svitkine (OO from 15th) wrote: > Okay, still welcome comments on this - but given I've not received them yet and > there are no Chrome releases until end of year anyway, maybe this will have to > wait till Jan. > > Meanwhile, I discovered something that may be an issue with this - still > investigating - but I'm observing a DCHECK in UmaEndSession() - which could be > related to this because now that this is an AsyncInitializationActivity which is > a ChromeActivity, it calls into those functions. So I'll investigate this more - > but still welcome comments on the other parts of this CL. I really think we should get UX approval about this change, either in parallel or while this is in review. I see the screenshots on the bug, but dont see anyone from UX commenting yet. Since we are changing the look quite a bit and the technical bits look quite good already, it would be good to hear from them before calling on when to land this.
On 2016/12/15 21:43:16, Yusuf wrote: > On 2016/12/15 21:31:24, Alexei Svitkine (OO from 15th) wrote: > > Okay, still welcome comments on this - but given I've not received them yet > and > > there are no Chrome releases until end of year anyway, maybe this will have to > > wait till Jan. > > > > Meanwhile, I discovered something that may be an issue with this - still > > investigating - but I'm observing a DCHECK in UmaEndSession() - which could be > > related to this because now that this is an AsyncInitializationActivity which > is > > a ChromeActivity, it calls into those functions. So I'll investigate this more > - > > but still welcome comments on the other parts of this CL. > > I really think we should get UX approval about this change, either in parallel > or while > this is in review. I see the screenshots on the bug, but dont see anyone from UX > commenting > yet. Since we are changing the look quite a bit and the technical bits look > quite > good already, it would be good to hear from them before calling on when to land > this. lgtm - agreed with yusufo@, we should get UX sign off before landing this (even preliminarily).
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.
Updated UX per the offline thread: - Spinner is positioned at some location as button with size 24dp - Title is hidden when showing spinner Landing this CL now. (I've also done a bunch of manual testing and haven't found any problems.)
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2561433002/#ps260001 (title: "Update UX.")
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": 260001, "attempt_start_ts": 1484149716164370,
"parent_rev": "841efc17964ac66ffa6b8b5ff63436f4ff6d52c1", "commit_rev":
"36860c01c1913acd255129a9024e22928f934d0e"}
Message was sent while issue was closed.
Description was changed from ========== Make FirstRunActivity extend AsyncInitializationActivity. This is a follow-up to https://codereview.chromium.org/2519853004/ which made it possible to show the first FRE screen before native is initialized, to support fetching the variations seed at that time. This CL further builds on that to make FirstRunActivity actually be an AsyncInitializationActivity, so that it never needs to do the synchronous native init path - instead waiting for async init to be done before allowing the user to proceed. When clicking Accept on the first FRE page (ToS and UMA), this will now show a spinner while waiting for native init and the variations seed fetch (whose timeout has been lowered to 3s). BUG=632199 ========== to ========== Make FirstRunActivity extend AsyncInitializationActivity. This is a follow-up to https://codereview.chromium.org/2519853004/ which made it possible to show the first FRE screen before native is initialized, to support fetching the variations seed at that time. This CL further builds on that to make FirstRunActivity actually be an AsyncInitializationActivity, so that it never needs to do the synchronous native init path - instead waiting for async init to be done before allowing the user to proceed. When clicking Accept on the first FRE page (ToS and UMA), this will now show a spinner while waiting for native init and the variations seed fetch (whose timeout has been lowered to 3s). BUG=632199 Review-Url: https://codereview.chromium.org/2561433002 Cr-Commit-Position: refs/heads/master@{#442919} Committed: https://chromium.googlesource.com/chromium/src/+/36860c01c1913acd255129a9024e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:260001) as https://chromium.googlesource.com/chromium/src/+/36860c01c1913acd255129a9024e...
Message was sent while issue was closed.
There's a follow-up fix for this in the cq: https://codereview.chromium.org/2628943002/ |
