1199859 - Add two A/B testing options for Firstrun

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

1. (control) Welcome screen
2. Sync | Import
3. Import | Sync

Comment 1

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Customize: Import

Comment 2

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1199859 - Add two A/B testing options for Firstrun. r=mfinkle

Bug 1199859 - Add three A/B testing options for Onboarding.

Comment 3

[Chenxia Liu [:liuche]]

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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(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?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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;

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

No idea why the "\{" | "\}" were used instead of "{" | "}". I blame MozReview.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 8

[Chenxia Liu [:liuche]]

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.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Mock: Option B (Import | Sync)

Comment 11

[Chenxia Liu [:liuche]]

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).

Comment 12

[Chenxia Liu [:liuche]]

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?

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Mock: Option B (Import | Sync)

Mock with the updated copy.

Comment 14

[Chenxia Liu [:liuche]]

Anthony - btw, I realized that the image should be one that doesn't touch the edges (like the coffee cup one).

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Bug 1199859 - Add Import option for Firstrun.

Comment 16

[Chenxia Liu [:liuche]]

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.

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Import page

This still needs to have the title bar updated to match HomePager, but that is bug 1185002.

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: slides2B.zip

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ I updated the images! they look even better now!

Comment 20

[Chenxia Liu [:liuche]]

Actually I need some different images for the sync slide - no branding on the devices, because the branding needs to be channel-specific.

Comment 21

[Chenxia Liu [:liuche]]

Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

Bug 1199859 - Add Import option for Firstrun.

Comment 22

[Chenxia Liu [:liuche]]

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.

Comment 23

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1199859 - Add "Next" sliding. r=mfinkle

Add "Next" sliding.

Comment 24

[Chenxia Liu [:liuche]]

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.

Comment 25

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: 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!

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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?

Comment 27

[Barbara Bermes [:barbara] - NI please!]

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.

Comment 28

[Alex Davis [: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.

Comment 30

[Chenxia Liu [:liuche]]

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.

Comment 31

[Barbara Bermes [:barbara] - NI please!]

thanks Chenxia, how much effort is this to switch to 2 versions?

I'd rather test this properly sooner than later.

Thanks

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 33

[Barbara Bermes [:barbara] - NI please!]

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?

Comment 34

[Nick Alexander :nalexander [he/him]]

(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.

Comment 35

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 36

[Barbara Bermes [:barbara] - NI please!]

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?

Comment 37

[Chenxia Liu [:liuche]]

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.

Comment 38

[Barbara Bermes [:barbara] - NI please!]

 
> > - 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.

Comment 39

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 40

[Chenxia Liu [:liuche]]

New build with better timing and new images.

http://people.mozilla.org/~liuche/bug-1199859/sliding2.apk

Comment 41

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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!

Comment 42

[Barbara Bermes [:barbara] - NI please!]

(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).

Comment 43

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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

Comment 44

[Chenxia Liu [:liuche]]

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.

Comment 45

[Chenxia Liu [:liuche]]

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

Comment 46

[Chenxia Liu [: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 47

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Add "Next" sliding. r=mfinkle

Comment 48

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1199859 - Use local switchboard. r=mfinkle

Bug 1199859 - Use local switchboard. r=mfinkle

Comment 49

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1199859 - Add telemetry. r=mfinkle

Bug 1199859 - Add telemetry. r=mfinkle

Comment 50

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 51

[Chenxia Liu [: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 52

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Add "Next" sliding. r=mfinkle

Comment 53

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Use local switchboard. r=mfinkle

Comment 54

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Add telemetry. r=mfinkle

Comment 55

[Chenxia Liu [:liuche]]

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.

Comment 56

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8654443 [details]
MozReview Request: Bug 1199859 - Add Import option for Firstrun. r=mfinkle

https://reviewboard.mozilla.org/r/17697/#review17491

Comment 57

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 58

[Mark Finkle (:mfinkle) (use needinfo?)]

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 59

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 60

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 61

[Mark Finkle (:mfinkle) (use needinfo?)]

Solid work on this patch set. You are really close.

Comment 62

[Barbara Bermes [:barbara] - NI please!]

> 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?

Comment 63

[Barbara Bermes [:barbara] - NI please!]

(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

Comment 64

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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. :)

Comment 65

[Barbara Bermes [:barbara] - NI please!]

2 seconds going once.....twice...ok!

Let's keep the 2 seconds :)

Thanks

Comment 66

[Chenxia Liu [:liuche]]

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.

Comment 67

[Barbara Bermes [:barbara] - NI please!]

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

Comment 68

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 69

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 70

[Barbara Bermes [:barbara] - NI please!]

(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?

Comment 71

[Chenxia Liu [:liuche]]

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.

Comment 72

[Alex Davis [:adavis]]

(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 73

[Chenxia Liu [: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 74

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Add "Next" sliding. r=mfinkle

Comment 75

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Use local switchboard. r=mfinkle

Comment 76

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Add telemetry. r=mfinkle

Comment 77

[Chenxia Liu [:liuche]]

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.

Comment 78

[Chenxia Liu [:liuche]]

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 79

[Mark Finkle (:mfinkle) (use needinfo?)]

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

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

Comment 80

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 81

[Barbara Bermes [:barbara] - NI please!]

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 82

[Chenxia Liu [:liuche]]

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

Bug 1199859 - Add telemetry. r=mfinkle

Comment 83

[Chenxia Liu [:liuche]]

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 84

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 85

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f8c449c0dfd4
https://hg.mozilla.org/integration/fx-team/rev/3d97a673734c
https://hg.mozilla.org/integration/fx-team/rev/8349bbfd8910
https://hg.mozilla.org/integration/fx-team/rev/4eabe281eb88
https://hg.mozilla.org/integration/fx-team/rev/ab8c0ada6aa1

Comment 86

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f8c449c0dfd4
https://hg.mozilla.org/mozilla-central/rev/3d97a673734c
https://hg.mozilla.org/mozilla-central/rev/8349bbfd8910
https://hg.mozilla.org/mozilla-central/rev/4eabe281eb88
https://hg.mozilla.org/mozilla-central/rev/ab8c0ada6aa1