Add two A/B testing options for Firstrun

RESOLVED FIXED in Firefox 43

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 43
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 1 obsolete attachment)

40 bytes, text/x-review-board-request
mfinkle
: review+
Details | Review
40 bytes, text/x-review-board-request
mfinkle
: review+
Details | Review
156.01 KB, image/png
Details
141.57 KB, image/png
Details
186.44 KB, application/zip
Details
40 bytes, text/x-review-board-request
mfinkle
: review+
Details | Review
126.79 KB, application/zip
Details
40 bytes, text/x-review-board-request
mfinkle
: review+
Details | Review
40 bytes, text/x-review-board-request
mfinkle
: review+
Details | Review
1. (control) Welcome screen
2. Sync | Import
3. Import | Sync
Created attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Customize: Import
Created attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - Add three A/B testing options for Onboarding.
Need to make the 'isActive' attribute of the payload actually return true. For some reason, the Switchboard init code is not generating that id, or passing it to the server.
(In reply to Chenxia Liu [:liuche] from comment #3)
> Need to make the 'isActive' attribute of the payload actually return true.
> For some reason, the Switchboard init code is not generating that id, or
> passing it to the server.

Where is the code for your "test" server?
https://reviewboard.mozilla.org/r/17699/#review15825

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:22
(Diff revision 1)
> +        if (SwitchBoard.isInExperiment(context, ONBOARDING_A)) {

You're not using the AppConstants.MOZ_SWITCHBOARD test here. I think the code might end up looking like:

if (AppConstants.MOZ_SWITCHBOARD) \{
    if (SwitchBoard.isInExperiment(context, ONBOARDING_B)) \{
        panels.add(new FirstrunPanel(CustomizePanel.class.getName(), CustomizePanel.TITLE_RES));
        panels.add(new FirstrunPanel(WelcomePanel.class.getName(), WelcomePanel.TITLE_RES));
    \} else if (SwitchBoard.isInExperiment(context, ONBOARDING_C)) \{
        panels.add(new FirstrunPanel(WelcomePanel.class.getName(), WelcomePanel.TITLE_RES));
        panels.add(new FirstrunPanel(CustomizePanel.class.getName(), CustomizePanel.TITLE_RES));
    \}
    return panels;
\}

// Fallback for onboarding-a OR no Switchboard
panels.add(new FirstrunPanel(WelcomePanel.class.getName(), WelcomePanel.TITLE_RES));
return panels;
No idea why the "\{" | "\}" were used instead of "{" | "}". I blame MozReview.
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Chenxia Liu [:liuche] from comment #3)
> > Need to make the 'isActive' attribute of the payload actually return true.
> > For some reason, the Switchboard init code is not generating that id, or
> > passing it to the server.
> 
> Where is the code for your "test" server?

I found the code here: https://github.com/liuche/switchboard-server

I see "This branch is 1 commit behind mfinkle:master". Update the code and you'll get the right JSON.

Updated

3 years ago
Depends on: 1200277
We have to make a call to get our bucket from the server on first run and while it'd be great to do this synchronously, it takes a bit of time (about 2 extra seconds from my not-very scientific testing). Working on getting our Firstrun to wait until AsyncTask is done, or until we hit some "this is too long" threshold. Point being, this will definitely increase the time for first run to get shown.

It's also possible that hitting the server for our first run experience is not a great idea.
(In reply to Chenxia Liu [:liuche] from comment #8)
> We have to make a call to get our bucket from the server on first run and
> while it'd be great to do this synchronously, it takes a bit of time (about
> 2 extra seconds from my not-very scientific testing). Working on getting our
> Firstrun to wait until AsyncTask is done, or until we hit some "this is too
> long" threshold. Point being, this will definitely increase the time for
> first run to get shown.

Thoughts:
1. Make sure you are using the mozilla-switchboard end point. Your test endpoint is likely on a "free" instance and is slower.
2. We should modify the Switchboard code to allow for NOT pinging the server to update the URLs. It's probably an easy win and given our 6 week cycle + uplifts, we may not run into issues.

> It's also possible that hitting the server for our first run experience is
> not a great idea.

Agreed. Other thought:
* We could add Switchboard.isInBucket(minInclusive, maxExclusive) to the client code. This would make onboarding a purely client-side system. We would not need to ping the server at all. The downside is that we lose all dynamic capability. We'd rely on code changes + uplifts for any rollout.
Assignee: nobody → liuche
Comment hidden (typo)
Attachment #8656323 - Attachment description: Mock: Option B (Import → Mock: Option B (Import | Sync)
We are dropping the 3rd option (Sync | Import) because adding the Sync flow into this firstrun A/B test adds too many differences (increases the number screens).
Anthony, I thought that I had the resources for the Sync panel background (the one with the set of devices: laptop, phone, tablet), but I realized that I actually have the version that omits the phone in the center. Can you include the set of "Sync" panel background that includes all three devices?
Flags: needinfo?(alam)
Summary: Add three A/B testing options for Onboarding → Add two A/B testing options for Firstrun
Created attachment 8656335 [details]
Mock: Option B (Import | Sync)

Mock with the updated copy.
Attachment #8656323 - Attachment is obsolete: true
Anthony - btw, I realized that the image should be one that doesn't touch the edges (like the coffee cup one).
Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Bug 1199859 - Add Import option for Firstrun.
Attachment #8654443 - Attachment description: MozReview Request: Customize: Import → MozReview Request: Bug 1199859 - Add Import option for Firstrun.
Comment on attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - WIP Add two A/B testing options for Firstrun.
Attachment #8654444 - Attachment description: MozReview Request: Bug 1199859 - Add three A/B testing options for Onboarding. → MozReview Request: Bug 1199859 - WIP Add two A/B testing options for Firstrun.
Created attachment 8656348 [details]
Screenshot: Import page

This still needs to have the title bar updated to match HomePager, but that is bug 1185002.
Created attachment 8656678 [details]
slides2B.zip
Flags: needinfo?(alam) → needinfo?(liuche)
^ I updated the images! they look even better now!
Actually I need some different images for the sync slide - no branding on the devices, because the branding needs to be channel-specific.
Flags: needinfo?(liuche)
Flags: needinfo?(alam)
Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Bug 1199859 - Add Import option for Firstrun.
Comment on attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - WIP Add two A/B testing options for Firstrun.
Created attachment 8658997 [details]
MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle

Add "Next" sliding.
Here's the apk for the first run:
http://people.mozilla.org/~liuche/bug-1199859/sliding-no-telemetry.apk

The first run experience is randomized and still pulling from the server. 2/3 of the time, it shows the control version, so you'll have to reset your app profile a few times (to simulate "first run") and hope that the odds are ever in your favor. (I've found that resetting the profile and then immediately re-opening Fennec seems to have more than a 2/3 control, but maybe this is statistics. In any case, maybe it's caching based on time or something, so I'd wait a few seconds before restarting fennec if you keep seeing the same first run version all the time).

Anthony, what do you think about the delay between importing and sliding? I can tweak that - it's currently at 500ms.
Created attachment 8659388 [details]
slides2C.zip

(In reply to Chenxia Liu [:liuche] from comment #20)
> Actually I need some different images for the sync slide - no branding on
> the devices, because the branding needs to be channel-specific.

Here!
Flags: needinfo?(alam) → needinfo?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #24)
> Here's the apk for the first run:
> http://people.mozilla.org/~liuche/bug-1199859/sliding-no-telemetry.apk
> 
> The first run experience is randomized and still pulling from the server.
> 2/3 of the time, it shows the control version, so you'll have to reset your
> app profile a few times (to simulate "first run") and hope that the odds are
> ever in your favor. (I've found that resetting the profile and then
> immediately re-opening Fennec seems to have more than a 2/3 control, but
> maybe this is statistics. In any case, maybe it's caching based on time or
> something, so I'd wait a few seconds before restarting fennec if you keep
> seeing the same first run version all the time).
> 
> Anthony, what do you think about the delay between importing and sliding? I
> can tweak that - it's currently at 500ms.

Oh man, if I were in the Hunger Games, the odds would not be in my favor. It took me a while to "roll" the right FRUE. 

In any case! 500ms seems too quick to me. Can we go to 1 full second and see how that feels?
I haven't tested it yet but I was wondering why we would want 2/3 for control? I thought we'd do 50/50, i.e. 100% / how many variations (2) = 50% each.
Flags: needinfo?(adavis)
(In reply to Barbara Bermes [:bbermes] from comment #27)
> I haven't tested it yet but I was wondering why we would want 2/3 for
> control? I thought we'd do 50/50, i.e. 100% / how many variations (2) = 50%
> each.

+1

Since volume will be smaller on Nightly, splitting 50/50 will help us reach statistical significance as quickly as possible.

The test will run as long as we don't have statistical significance.

If we want to put variation B at a lower rate the first day or 2 to make sure there are no bugs, we can do that but then ideally for the period of the test, set it to 50/50.
Flags: needinfo?(adavis)
Duplicate of this bug: 1075643
Don't worry about the 2/3 thing - it's something that we need to change server side that's a throwback from when we initially wanted 3 versions. Basically the server is still serving up 3 versions, so the version that isn't being handled is getting turned into the control, so we don't drop it on the floor.
Flags: needinfo?(liuche)
thanks Chenxia, how much effort is this to switch to 2 versions?

I'd rather test this properly sooner than later.

Thanks
Flags: needinfo?(cliu)
(In reply to Barbara Bermes [:bbermes] from comment #31)
> thanks Chenxia, how much effort is this to switch to 2 versions?
> 
> I'd rather test this properly sooner than later.
> 
> Thanks

A few minutes. I did it last night.
Flags: needinfo?(cliu)
Alright, some initial feedback, sorry it took me so long -- Good stuff! Thanks Chenxia

- I really noticed the long wait time the server-side implementation of Switchboard caused when getting variation B.
- Some copy suggestion (I know those are not finalized): "Transfer to Fennec" button: we need better copy there, we need to tell the user from where their booksmarks are being imported, Chrome, Stock etc.?
- Screen that shows after the import was successful, is only briefly shown, and goes automatically to Sync slide, is that intentional?
- Broken experience for current Sync screen --> tabs at the top are gone (I understand why but... could we load this into the tabs when we move to web-based login)?
- After I finished singing in, it just show the top sites screen after I hit "Back to Browsing" --> it didn't show me that I finished the onboarding, can we pass in something that knows we came to the sign-up
page from the onboarding screens and show them instead of "Back to Browsing" something more like "you've successfully setup your Fennec experienc"? I assume this is nalexander pending?
Flags: needinfo?(nalexander)
Flags: needinfo?(liuche)
(In reply to Barbara Bermes [:bbermes] from comment #33)
> Alright, some initial feedback, sorry it took me so long -- Good stuff!
> Thanks Chenxia
> 
> - I really noticed the long wait time the server-side implementation of
> Switchboard caused when getting variation B.
> - Some copy suggestion (I know those are not finalized): "Transfer to
> Fennec" button: we need better copy there, we need to tell the user from
> where their booksmarks are being imported, Chrome, Stock etc.?
> - Screen that shows after the import was successful, is only briefly shown,
> and goes automatically to Sync slide, is that intentional?
> - Broken experience for current Sync screen --> tabs at the top are gone (I
> understand why but... could we load this into the tabs when we move to
> web-based login)?

Yes.  In fact, we have to :)  I will screen-cap this transition shortly.

> - After I finished singing in, it just show the top sites screen after I hit
> "Back to Browsing" --> it didn't show me that I finished the onboarding, can
> we pass in something that knows we came to the sign-up
> page from the onboarding screens and show them instead of "Back to Browsing"
> something more like "you've successfully setup your Fennec experienc"? I
> assume this is nalexander pending?

There are two points here:

* right now, with the native flow, we could definitely pass in a "first run" switch that turns "Back to Browsing" into something more appropriate to first run ("Start browsing with your new Firefox").  If we could live with the current for now, it would save some throw away work, but if we want it let's schedule it.

* after the web flow is live, we'll need to work with the fxa-content-server team to finalize what messages we want.  Right now, I've mooted the three screens in https://www.lucidchart.com/documents/edit/92d0ec74-a2af-40b8-b714-6db99149e39c/1 (the badly-named *Choose what to Sync Android* tab) to show after sign up, sign in, and re-connect.  We can add another screen after completing the first run experience.
Flags: needinfo?(nalexander)
See Also: → bug 1204905
(In reply to Barbara Bermes [:bbermes] from comment #33)
> Alright, some initial feedback, sorry it took me so long -- Good stuff!
> Thanks Chenxia
> 
> - I really noticed the long wait time the server-side implementation of
> Switchboard caused when getting variation B.

This is no longer an issue, and Chenxia can move away from waiting for the server-side config.
mfinkle -> good news!

nalexander, I guess not really a good idea to add this to code that will be replaced soon. Can you please confirm again when the native flow will switch to web-based?
Flags: needinfo?(nalexander)
Thanks for the feedback Barbara! Was out of commission for a few days because my computer broke, but I can include these suggestions now.

> - I really noticed the long wait time the server-side implementation of
> Switchboard caused when getting variation B.

Now that bug 1201384 landed, I'll update this.

> - Some copy suggestion (I know those are not finalized): "Transfer to
> Fennec" button: we need better copy there, we need to tell the user from
> where their booksmarks are being imported, Chrome, Stock etc.?

Actually, this the final copy that we decided on in bug 1200277 - please reopen that bug if you want to make changes!

> - Screen that shows after the import was successful, is only briefly shown,
> and goes automatically to Sync slide, is that intentional?

I'll be increasing this to 1s, instead of 500ms.

> - After I finished singing in, it just show the top sites screen after I hit
> "Back to Browsing" --> it didn't show me that I finished the onboarding, can
> we pass in something that knows we came to the sign-up
> page from the onboarding screens and show them instead of "Back to Browsing"
> something more like "you've successfully setup your Fennec experienc"? I
> assume this is nalexander pending?

Anthony, do you have thoughts on this? It's not in our original flow.

I'll also update the images for the Sync first run panel, and I still need to fix bug 1185002 for the pager titles in first run.
Flags: needinfo?(liuche) → needinfo?(alam)
 
> > - Some copy suggestion (I know those are not finalized): "Transfer to
> > Fennec" button: we need better copy there, we need to tell the user from
> > where their booksmarks are being imported, Chrome, Stock etc.?
> 
> Actually, this the final copy that we decided on in bug 1200277 - please
> reopen that bug if you want to make changes!

I did add a comment but I thought we normally don't re-open bugs?!?! Rather opening a new one?

> 
> > - Screen that shows after the import was successful, is only briefly shown,
> > and goes automatically to Sync slide, is that intentional?
> 
> I'll be increasing this to 1s, instead of 500ms.
Awesome, let me know once you have another patch and I can try it out again.
(In reply to Chenxia Liu [:liuche] from comment #37)
> > - After I finished singing in, it just show the top sites screen after I hit
> > "Back to Browsing" --> it didn't show me that I finished the onboarding, can
> > we pass in something that knows we came to the sign-up
> > page from the onboarding screens and show them instead of "Back to Browsing"
> > something more like "you've successfully setup your Fennec experienc"? I
> > assume this is nalexander pending?
> 
> Anthony, do you have thoughts on this? It's not in our original flow.

No, this wasn't part of the original design. Though it could be something we think about for next versions as we add more slides.

Currently, there's just not a lot of things to "confirm". Also, you already get a "confirm" after you've successfully imported. I know nalexander has been talking about adding something similar to the web-flow so we can see where that goes first as well. 

ATM, I'd like to hold off on adding more to this experience. At least until we complete our testing and get some time to think about this a bit more.
Flags: needinfo?(alam)
New build with better timing and new images.

http://people.mozilla.org/~liuche/bug-1199859/sliding2.apk
Flags: needinfo?(bbermes)
Flags: needinfo?(alam)
(In reply to Chenxia Liu [:liuche] from comment #40)
> New build with better timing and new images.
> 
> http://people.mozilla.org/~liuche/bug-1199859/sliding2.apk

Nice!

Some feedback:
 - 1 second delay feels great!
 - images look nice and centered
 - Header navigation, can we use the fixed tabs UI like in the Mock? this should help show users "what's left?"
 - Checkbox confirmation looks great, but I'd like it to maintain it's position (centered within the space where the button used to be) instead of shifting up and thereby shifting the "next" button up as well.

Thanks Chenxia!
Flags: needinfo?(alam) → needinfo?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #40)
> New build with better timing and new images.
> 
> http://people.mozilla.org/~liuche/bug-1199859/sliding2.apk

Thanks :)

As for the devices image on sync: it shows the fennec icon, will this be replaced by the Firefox icon later?

Also to be honest, I don't think 1s is still enough. Could we make it 2s? I barely can read what i says there (Assuming this is the first time I see the screen).
Flags: needinfo?(bbermes) → needinfo?(alam)
(In reply to Barbara Bermes [:barbara] from comment #42)
> (In reply to Chenxia Liu [:liuche] from comment #40)
> > New build with better timing and new images.
> > 
> > http://people.mozilla.org/~liuche/bug-1199859/sliding2.apk
> 
> Thanks :)
> 
> As for the devices image on sync: it shows the fennec icon, will this be
> replaced by the Firefox icon later?

Yep!

> Also to be honest, I don't think 1s is still enough. Could we make it 2s? I
> barely can read what i says there (Assuming this is the first time I see the
> screen).

Wouldn't reading happen before you get the feedback? and therefore trigger the 1 or 2 second countdown? After the user initiates the import, all that's left to do is really to move to the next slide..

But, we can get a build to test 2 seconds as well! :D
Flags: needinfo?(alam)
Okay, here's the 2sec apk, with the new sizing for the green check so that the "Next" doesn't shift.

http://people.mozilla.org/~liuche/bug-1199859/sliding3-2sec.apk

I also added telemetry (although you can't see it, unless you open logcat).
- Sessions for the a/b test (appended as a 0 or 1 to the telemetry probe, corresponding to the A or B version)
- When the user manually swipes to a different firstrun page (an event, not a session)
- When a user toggles a checkbox in the "Transfer" dialog - doesn't save what the value is
- When a user confirms the "Transfer" in the dialog, and what values have been checked
- When a user clicks the "Next"

Already in the product are the Sync telemetry probes:
- When a user clicks on "Set up Sync"
- When a user clicks "Start browsing"

I didn't add any more probes for the actual Sync setup process because afaik, that's changing in 43, and if it's not, we can uplift probes to aurora after merge.
Flags: needinfo?(liuche)
Flags: needinfo?(bbermes)
Flags: needinfo?(alam)
Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Bug 1199859 - Add Import option for Firstrun. r=mfinkle
Attachment #8654443 - Attachment description: MozReview Request: Bug 1199859 - Add Import option for Firstrun. → MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle
Attachment #8654443 - Flags: review?(mark.finkle)
Attachment #8654444 - Attachment description: MozReview Request: Bug 1199859 - WIP Add two A/B testing options for Firstrun. → MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle
Attachment #8654444 - Flags: review?(mark.finkle)
Comment on attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle
Comment on attachment 8658997 [details]
MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle

Bug 1199859 - Add "Next" sliding. r=mfinkle
Attachment #8658997 - Attachment description: MozReview Request: Add "Next" sliding. → MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle
Attachment #8658997 - Flags: review?(mark.finkle)
Created attachment 8662107 [details]
MozReview Request: Bug 1199859 - Use local switchboard. r=mfinkle

Bug 1199859 - Use local switchboard. r=mfinkle
Attachment #8662107 - Flags: review?(mark.finkle)
Created attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

Bug 1199859 - Add telemetry. r=mfinkle
Attachment #8662108 - Flags: review?(mark.finkle)
(In reply to Chenxia Liu [:liuche] from comment #44)
> Okay, here's the 2sec apk, with the new sizing for the green check so that
> the "Next" doesn't shift.
> 
> http://people.mozilla.org/~liuche/bug-1199859/sliding3-2sec.apk
> 
> I also added telemetry (although you can't see it, unless you open logcat).
> - Sessions for the a/b test (appended as a 0 or 1 to the telemetry probe,
> corresponding to the A or B version)
> - When the user manually swipes to a different firstrun page (an event, not
> a session)
> - When a user toggles a checkbox in the "Transfer" dialog - doesn't save
> what the value is
> - When a user confirms the "Transfer" in the dialog, and what values have
> been checked
> - When a user clicks the "Next"
> 
> Already in the product are the Sync telemetry probes:
> - When a user clicks on "Set up Sync"
> - When a user clicks "Start browsing"
> 
> I didn't add any more probes for the actual Sync setup process because
> afaik, that's changing in 43, and if it's not, we can uplift probes to
> aurora after merge.

Awesome, that check box looks so much better!

2 seconds feels a bit long to me. Maybe 1.5?

Since none of the content actually changes after the user finishes the "transfer", I don't think this needs to stick around long enough for someone to read the copy. For me, it's a transition from the first slide to the second and this delay remedies the animation from being too jarring after the user comes out of the "Importing bookmarks and history from Android” dialog spinner.

Speaking of the dialog, let's change it to say "Transferring" when it's spinning. This is a more natural jump.
Flags: needinfo?(alam) → needinfo?(liuche)
Comment on attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle
Comment on attachment 8658997 [details]
MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle

Bug 1199859 - Add "Next" sliding. r=mfinkle
Comment on attachment 8662107 [details]
MozReview Request: Bug 1199859 - Use local switchboard. r=mfinkle

Bug 1199859 - Use local switchboard. r=mfinkle
Comment on attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

Bug 1199859 - Add telemetry. r=mfinkle
I talked with Anthony on Vidyo, and I think that we should keep the slide delay at 1sec. Since the only thing that is changing is the orange button getting changed to a green checkbox, 2sec is really long.

As for the progress dialog, we don't want to make that hang around artificially longer that it it takes to import. Changing the string of the progress dialog to "Transferring" to echo the "Transfer from Firefox" string should ease that transition, so hopefully the user is just happy that importing doesn't take that much time.
Flags: needinfo?(liuche)
Attachment #8654443 - Flags: review?(mark.finkle) → review+
Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

https://reviewboard.mozilla.org/r/17697/#review17491
Comment on attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

https://reviewboard.mozilla.org/r/17699/#review17495

::: mobile/android/base/BrowserApp.java:750
(Diff revision 5)
> +            SwitchBoard.updateConfigServerUrl(this);

I know you remove this in a following patch

::: mobile/android/base/BrowserApp.java:756
(Diff revision 5)
> -            new AsyncConfigLoader(this, AsyncConfigLoader.CONFIG_SERVER).execute();
> +            // TODO: Make this NON-SYNCHRONOUS

I know you remove this in a following patch

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:23
(Diff revision 5)
> +        if (AppConstants.MOZ_SWITCHBOARD) {

if you move this MOZ_SWITCHBOARD check into isBucket, you don't need the redundant else block here.

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:51
(Diff revision 5)
> +    public static List<FirstrunPanelConfig> getRestricted() {

Add a blank line between the methods

::: mobile/android/base/firstrun/ImportPanel.java:100
(Diff revision 5)
> +        Log.e(LOGTAG, "Importing Android history/bookmarks");

Log.e ?

::: mobile/android/base/firstrun/ImportPanel.java:131
(Diff revision 5)
> +                // Constructing AndroidImport may need finding the profile,

Indented too much?
Attachment #8654444 - Flags: review?(mark.finkle) → review+
Attachment #8658997 - Flags: review?(mark.finkle) → review+
Comment on attachment 8658997 [details]
MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle

https://reviewboard.mozilla.org/r/18735/#review17499

::: mobile/android/base/firstrun/FirstrunPager.java:59
(Diff revision 3)
> +                onFinishListener.onFinish();

Should we check for null here?

::: mobile/android/base/firstrun/FirstrunPanel.java
(Diff revision 3)
> -        if (pagerNavigation != null) {

Should we remove the null check?

::: mobile/android/base/firstrun/ImportPanel.java:30
(Diff revision 3)
> +    private static final int AUTOADVANCE_DELAY_MS = 2000;

It seems that this could change, but that can be a separate bug/patch

Figure out if we need/want the null checks
Comment on attachment 8662107 [details]
MozReview Request: Bug 1199859 - Use local switchboard. r=mfinkle

https://reviewboard.mozilla.org/r/19503/#review17501

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:44
(Diff revision 2)
> +    private static boolean isInExperimentLocal(Context context, String name) {

isInExperimentLocal is probably overkill. You could just move the | SwitchBoard.isInBucket(context, min, max) | into isBucket.

Actually, isn't isInExperimentLocal good enough to just call directly? You could probably just remove isInBucket and call isInExperimentLocal.
Attachment #8662107 - Flags: review?(mark.finkle)
Attachment #8662108 - Flags: review?(mark.finkle)
Comment on attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

https://reviewboard.mozilla.org/r/19505/#review17503

::: mobile/android/base/TelemetryContract.java:213
(Diff revision 2)
> +        // Firstrun A/B testing.

I might be evil, but I don't think we should add these permanently to the Session enum.

::: mobile/android/base/firstrun/FirstrunPager.java:70
(Diff revision 2)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.SHOW, TelemetryContract.Method.HOMESCREEN, "firstrun-panel-" + i);

For other events, we use the position as an Extra.

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:30
(Diff revision 2)
> +                Telemetry.startUISession(TelemetryContract.Session.FIRSTRUN_A);

I would use the ONBOARDING_A and ONBOARDING_B strings since those match the A/B experiment names.

Yes, we would not have the ".1" suffix, but that is only for versioning and these Sessions are not long term.

::: mobile/android/base/firstrun/ImportPanel.java:78
(Diff revision 2)
>                                 runImport(importBookmarks, importHistory);

Does the runImport code, or the real importer actually, support any telemetry for knowing if we actually had records to import? If not, we could add some in a separate bug. The Session would be tagged in those Events so we could tell if the import happened from settings or firstrun.

::: mobile/android/base/firstrun/ImportPanel.java:94
(Diff revision 2)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.BUTTON, "firstrun-import-next");

What about swiping between panels? I suppose we have three ways to move between pages: Next, Swipe, Pager strip. Can we cover those with telemetry?
Solid work on this patch set. You are really close.
> 2 seconds feels a bit long to me. Maybe 1.5?
> 
> Since none of the content actually changes after the user finishes the
> "transfer", I don't think this needs to stick around long enough for someone
> to read the copy. For me, it's a transition from the first slide to the
> second and this delay remedies the animation from being too jarring after
> the user comes out of the "Importing bookmarks and history from Android”
> dialog spinner.

I like 2 seconds :) but if Anthony really wants 1.5, I'd go with that.

After import was successful, the page also includes a "Next", however this almost seems redundant because you are being auto-redirect to the next slide anyways. Thoughts? What would be the use case for clicking on next?
Flags: needinfo?(bbermes) → needinfo?(alam)
(In reply to Chenxia Liu [:liuche] from comment #44)
> Okay, here's the 2sec apk, with the new sizing for the green check so that
> the "Next" doesn't shift.
> 
> http://people.mozilla.org/~liuche/bug-1199859/sliding3-2sec.apk
> 
> I also added telemetry (although you can't see it, unless you open logcat).
> - Sessions for the a/b test (appended as a 0 or 1 to the telemetry probe,
> corresponding to the A or B version)
> - When the user manually swipes to a different firstrun page (an event, not
> a session)
> - When a user toggles a checkbox in the "Transfer" dialog - doesn't save
> what the value is
> - When a user confirms the "Transfer" in the dialog, and what values have
> been checked
> - When a user clicks the "Next"
> 
> Already in the product are the Sync telemetry probes:
> - When a user clicks on "Set up Sync"
> - When a user clicks "Start browsing"
> 
> I didn't add any more probes for the actual Sync setup process because
> afaik, that's changing in 43, and if it's not, we can uplift probes to
> aurora after merge.

That is awesome, thanks Chenxia
(In reply to Barbara Bermes [:barbara] from comment #62)
> > 2 seconds feels a bit long to me. Maybe 1.5?
> > 
> > Since none of the content actually changes after the user finishes the
> > "transfer", I don't think this needs to stick around long enough for someone
> > to read the copy. For me, it's a transition from the first slide to the
> > second and this delay remedies the animation from being too jarring after
> > the user comes out of the "Importing bookmarks and history from Android”
> > dialog spinner.
> 
> I like 2 seconds :) but if Anthony really wants 1.5, I'd go with that.
> 
> After import was successful, the page also includes a "Next", however this
> almost seems redundant because you are being auto-redirect to the next slide
> anyways. Thoughts? What would be the use case for clicking on next?

Sort of, since the user _can_ swipe back and forth, it's a nice fall back to return to the final slide. This can be especially useful as we add more slides. But at this point, I'm not too strongly inclined either way here. :)
Flags: needinfo?(alam)
2 seconds going once.....twice...ok!

Let's keep the 2 seconds :)

Thanks
I'd actually prefer to keep it at 1.5s, or even 1s. 2sec seems much too long, and it feels abrupt when the screen suddenly slides because it feels like a different, unexpected task has started.
Hey Chenxia, can you just a quick look at https://bugzilla.mozilla.org/show_bug.cgi?id=1205824
and see if this is something we could do (now or later), just getting an estimate if that's even possible. Thanks
Depends on: 1205824
Flags: needinfo?(liuche)
Flags: needinfo?(alam)
(In reply to Barbara Bermes [:barbara] from comment #67)
> Hey Chenxia, can you just a quick look at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1205824
> and see if this is something we could do (now or later), just getting an
> estimate if that's even possible. Thanks

Changes to the importer are a bit outside the scope of the onboarding work.
(In reply to Barbara Bermes [:barbara] from comment #65)
> 2 seconds going once.....twice...ok!
> 
> Let's keep the 2 seconds :)
> 
> Thanks

There seems to be some confusion here. I think we may have gotten our wires crossed a bit. Again, I think 2 seconds is too long. Let's go with 1.5 for reasons mentioned in comment 50. 

For clarity, this delay is meant to improve the UX, not to be long enough so the user can read the slide.
Flags: needinfo?(alam)
(In reply to Mark Finkle (:mfinkle) from comment #68)
> (In reply to Barbara Bermes [:barbara] from comment #67)
> > Hey Chenxia, can you just a quick look at
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1205824
> > and see if this is something we could do (now or later), just getting an
> > estimate if that's even possible. Thanks
> 
> Changes to the importer are a bit outside the scope of the onboarding work.

I agree I'm not saying this has to be done for the on-boarding (this ticket), I just wanted to know if it's even possible, that's all. Does that make sense? I removed the dependency.

Mark, also do you think we might annoy the user if they organized everything in many folders in their stock browser and now they import and all is mixed up?
No longer depends on: 1205824
Hey Barbara, I commented on the bug that you linked to already - it's possible, but it might not be worth maintaining because we will have to special case some devices, and the import could go wrong if we try to be too specific with guessing versions and we guess the schema incorrectly.
Flags: needinfo?(liuche)
(In reply to Chenxia Liu [:liuche] from comment #71)
> Hey Barbara, I commented on the bug that you linked to already - it's
> possible, but it might not be worth maintaining because we will have to
> special case some devices, and the import could go wrong if we try to be too
> specific with guessing versions and we guess the schema incorrectly.

I unfortunately haven't had the chance to test yet so sorry for this question. Do we put their bookmarks in a "Imported" folder so it doesn't look like too much of a mess?

Since keeping the tree structure doesn't seem easy/possible, it might be best to avoid making a mess of their bookmarks.

You might be thinking, "How is this related to the test?" Our hypothesis was "If we add data import to the onboarding funnel, it will increase engagement and retention because it will improve the user experience by immediately getting them started with their personal data (history, bookmarks)".

If we make a mess of their bookmarks, it might not reduce friction very much (perhaps create instead).
Comment on attachment 8654444 [details]
MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle
Comment on attachment 8658997 [details]
MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle

Bug 1199859 - Add "Next" sliding. r=mfinkle
Comment on attachment 8662107 [details]
MozReview Request: Bug 1199859 - Use local switchboard. r=mfinkle

Bug 1199859 - Use local switchboard. r=mfinkle
Attachment #8662107 - Flags: review?(mark.finkle)
Attachment #8662108 - Flags: review?(mark.finkle)
Comment on attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

Bug 1199859 - Add telemetry. r=mfinkle
https://reviewboard.mozilla.org/r/18735/#review17621

::: mobile/android/base/firstrun/FirstrunPanel.java
(Diff revision 3)
> -        if (pagerNavigation != null) {

Yeah, good call - to be properly Java, we should keep these null checks.
https://reviewboard.mozilla.org/r/19505/#review17629

::: mobile/android/base/TelemetryContract.java:213
(Diff revision 2)
> +        // Firstrun A/B testing.

Currently, the code requires that the sessions get started as enums, and we don't allow them to be made from strings, probably because we want to ensure that there's a single place where you can find all the probes without needing to mxr 'Telemetry.startUISession' or garbage like that.

If we wanted to allow starting sessions from strings, we would need to expose that. I'd rather not allow that, because currently it keeps people from being lazy and making a session probe without adding it TelemetryContract - unless we explicitly do not mind having sessions floating around that aren't in TelemetryContract.

We can always remove them from here!

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:30
(Diff revision 2)
> +                Telemetry.startUISession(TelemetryContract.Session.FIRSTRUN_A);

I can update the enum names and use the strings directly from FirstrunPagerConfig. That's better than opening the floodgates of hell to everyone who wants to write Terrible Telemetry Probes that just use strings.

::: mobile/android/base/firstrun/ImportPanel.java:78
(Diff revision 2)
>                                 runImport(importBookmarks, importHistory);

Filed bug 1205884.

::: mobile/android/base/firstrun/ImportPanel.java:94
(Diff revision 2)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.BUTTON, "firstrun-import-next");

This is the "Next" link and has telemetry to handle clicks. As for moving between pages, all of that (including clicking next, or auto-advancing) is covered by the telemetry probe in the onPageSelected listener.

We can probably add a listener to clicks on the scrolling tabs header for clicks. I looked at adding a listener to scrolling, but it's hard to tell reliably determine when a user has switched pages because of scrolling, or if the user just mostly scrolled the page but it bounced back.
Comment on attachment 8662107 [details]
MozReview Request: Bug 1199859 - Use local switchboard. r=mfinkle

https://reviewboard.mozilla.org/r/19503/#review17633
Attachment #8662107 - Flags: review?(mark.finkle) → review+
Comment on attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

https://reviewboard.mozilla.org/r/19505/#review17635

::: mobile/android/base/TelemetryContract.java:9
(Diff revision 3)
> +import org.mozilla.gecko.firstrun.FirstrunPager;

No need for these if we change to EXPERIMENT(...)

::: mobile/android/base/TelemetryContract.java:215
(Diff revision 3)
> +        // Firstrun A/B testing.

Let's go with:

// A/B test experiments
EXPERIMENT("experiment.1").

::: mobile/android/base/firstrun/FirstrunPager.java:73
(Diff revision 3)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.SHOW, TelemetryContract.Method.HOMESCREEN, i);

HOMESCREEN is really for Android homescreen shortcuts, even though the Search Activity might be bastardizing it.

What about adding a new method: PANE or PAGE ?

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:20
(Diff revision 3)
> -    private static final String ONBOARDING_A = "onboarding-a";
> +    public static final String ONBOARDING_A = "onboarding-a";

These can probably stay private

::: mobile/android/base/firstrun/FirstrunPagerConfig.java:27
(Diff revision 3)
> +            Telemetry.startUISession(TelemetryContract.Session.ONBOARDING_A);

Use the suffix version:
Telemetry.startUISession(TelemetryContract.Session.EXPERIMENT, ONBOARDING_A);

::: mobile/android/base/firstrun/FirstrunPane.java:64
(Diff revision 3)
> +        Telemetry.stopUISession(TelemetryContract.Session.ONBOARDING_A);

Same here
Attachment #8662108 - Flags: review?(mark.finkle)
I think we need to make a decision soon if we want this to ride the trains, as far as I understand September 21 is our go/no go decision, correct?

Alex/Mark/Chenxia/Antlam, should we just push this as is to get it into 43 due to scope creep or will the test be obsolete then (based on what Alex said)
Comment on attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

Bug 1199859 - Add telemetry. r=mfinkle
Attachment #8662108 - Flags: review?(mark.finkle)
I added the "panel" prefix back in for the panel change to be more explicit. These are just events, not sessions, which is different from the HomePager panel sessions - we don't need sessions for the duration of each panel because each action that can be taken within a panel is already unique.
Comment on attachment 8662108 [details]
MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

https://reviewboard.mozilla.org/r/19505/#review17737

::: mobile/android/base/TelemetryContract.java:9
(Diff revision 4)
> +import org.mozilla.gecko.firstrun.FirstrunPager;

Remove these

::: mobile/android/base/firstrun/FirstrunPager.java:73
(Diff revision 4)
> +                Telemetry.sendUIEvent(TelemetryContract.Event.SHOW, TelemetryContract.Method.PANEL, "panel" + i);

"panel" is too general for the Extra, especially since it's the Method too. Let's use "firstrun." or "onboarding."

Yes the trailing '.' is there on purpose. We use the '.' for "engine.#" and "history.#" Extras.
Attachment #8662108 - Flags: review?(mark.finkle) → review+
Duplicate of this bug: 1190989
Flags: needinfo?(nalexander)
Depends on: 1215604
Duplicate of this bug: 1199448
You need to log in before you can comment on or make changes to this bug.