1229350 - Import bookmarks behavior inconsistent on Android 6.0

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: First Run, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1183559 we decided to hide "Import from Android" in settings on Android 6+. However we show the import option during first run on Android 6+ devices.

Removing the setting is mentioned here too:
https://support.mozilla.org/en-US/kb/get-started-firefox-android#w_import-your-android-bookmarks-and-history

Background: In Android M preview 2 querying the system's bookmark provider crashed the app. Later in the final version of Android M/6.0 they replaced the content provider with a dummy version:
https://android.googlesource.com/platform/packages/providers/BookmarkProvider/+/marshmallow-release/src/com/android/bookmarkstore/BookmarkProvider.java

So the system's provider does not have any function any more (They also removed all related fields in android.provider.Browser - we worked around this in bug 1183069).

However since bug 1186037 we are querying Samsung's content provider too. And there might be more content providers we can query (chrome?).

If at least Samsung's content provider works on Android 6+ then we might want to show the import setting again (for those devices?). Or we should hide the import option from first run on Android 6+ because it doesn't actually import anything (on non-Samsung devices).

Comment 2

[:Margaret Leibovic]

I must have missed this bug go by. This isn't good, we should make sure to hide that onboarding screen if it doesn't work!

Comment 3

[:Margaret Leibovic]

I'll write a patch.

Comment 4

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Review commit: https://reviewboard.mozilla.org/r/28275/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28275/

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

https://reviewboard.mozilla.org/r/28275/#review25321

::: gradle/wrapper/gradle-wrapper.properties:6
(Diff revision 1)
> -distributionUrl=https\://services.gradle.org/distributions/gradle-2.7-all.zip
> +distributionUrl=https\://services.gradle.org/distributions/gradle-2.7-bin.zip

Is this a relevant change?

::: mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPagerConfig.java:26
(Diff revision 1)
> +        if (isInExperimentLocal(context, ONBOARDING_A) || !AppConstants.Versions.preM) {

I almost feel like this kind of check should be in the ONBOARDING_B if statement, but I understand that that might mess up the A/B testing a bit.

if (ONBOARDING_B && preM)

But I'm fine with keeping it here or moving it to the ONBOARDING_B check so disabling on M is checked at the same place as adding the import.

Comment 6

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #5)
> Comment on attachment 8699551 [details]
> MozReview Request: Bug 1229350 - Disable Import first run slide on Android
> M+. r=liuche
> 
> https://reviewboard.mozilla.org/r/28275/#review25321
> 
> ::: gradle/wrapper/gradle-wrapper.properties:6
> (Diff revision 1)
> > -distributionUrl=https\://services.gradle.org/distributions/gradle-2.7-all.zip
> > +distributionUrl=https\://services.gradle.org/distributions/gradle-2.7-bin.zip
> 
> Is this a relevant change?

Er, not sure how that ended up in there.

> :::
> mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunPagerConfig.java:
> 26
> (Diff revision 1)
> > +        if (isInExperimentLocal(context, ONBOARDING_A) || !AppConstants.Versions.preM) {
> 
> I almost feel like this kind of check should be in the ONBOARDING_B if
> statement, but I understand that that might mess up the A/B testing a bit.
> 
> if (ONBOARDING_B && preM)
> 
> But I'm fine with keeping it here or moving it to the ONBOARDING_B check so
> disabling on M is checked at the same place as adding the import.

Hm... I'm trying to think of what will have a weirder affect on our data. If we put the preM check in the ONBOARDING_B check, that means that all Android M users in group A will be counted in the experiment, but none of the ones who would have been destined for group B will be counted. It seems like it would be best to just exclude those M users from the data altogether.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28275/diff/1-2/

Comment 8

[:Margaret Leibovic]

Comment on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28275/diff/2-3/

Comment 9

[:Margaret Leibovic]

I just want to double check my assumption with Alex...

Alex, "Import" doesn't work for most Android 6.0 users, so we've disabled this feature for them in settings. Given that it doesn't work, we also want to disable the first run slide that talks about this.

Should I just exclude all Android 6.0 users from the experiment? (That's what my patch here does.)

Comment 10

[Alex Davis [:adavis]]

this makes sense

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/fc825ad761f9586ccee5663c8d2219d0717afe85
Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Comment 12

[:Margaret Leibovic]

Comment on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

(Nom'ing for release in case we end up with a dot release. I don't think this bug is bad enough to merit its own dot release, but it would be a good ride-along.)

Approval Request Comment
[Feature/regressing bug #]: Bug 1183559.

[User impact if declined]: For some Android M users, we'll offer to import their bookmarks/history on first run, but then it won't work. Bad first impression.

[Describe test coverage new/current, TreeHerder]: No automated tests, tested locally (we really should fix bug 1201653).

[Risks and why]: Low-risk, excludes Android M users from first-run A/B test.

[String/UUID change made/needed]: None.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Seems like a good candidate for uplift to Beta and Aurora. Waiting for it to land on Nightly first.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc825ad761f9

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Sure, let's take in aurora and beta. Should be in 44 beta 2.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

So far this is the only nomination for android uplifts on release. I agree with Margaret, this is not enough on its own for a release driver for a new Fennec build.  If that changes please let me know.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6a7e8c9e3e2

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This isn't cleanly applying on beta, likely due to the lack of bug 1107811. Can we get a rebased patch to work around the lack of 1107811 if this really needs to go to beta?

Comment 19

[Teodora Vermesan (:TeoVermesan)]

Import first run slide does not appear anymore on Android M+, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 46.0a1 (2015-12-22)

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Import first run slide does not appear anymore on Android M+, so:
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 45.0a2 (2015-12-23)

Comment 21

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-beta/rev/c977d1b649bf

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Too late for 43.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/c977d1b649bf