Closed Bug 1229350 Opened 4 years ago Closed 4 years ago

Import bookmarks behavior inconsistent on Android 6.0

Categories

(Firefox for Android :: First Run, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- verified
firefox46 --- verified
b2g-v2.5 --- fixed
fennec 43+ ---

People

(Reporter: sebastian, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
Flags: needinfo?(liuche)
Duplicate of this bug: 1232971
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!
Assignee: nobody → liuche
tracking-fennec: --- → ?
tracking-fennec: ? → 43+
I'll write a patch.
Assignee: liuche → margaret.leibovic
Flags: needinfo?(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.
Attachment #8699551 - Flags: review?(liuche) → review+
(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 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 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/
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.)
Flags: needinfo?(adavis)
this makes sense
Flags: needinfo?(adavis)
https://hg.mozilla.org/integration/fx-team/rev/fc825ad761f9586ccee5663c8d2219d0717afe85
Bug 1229350 - Disable Import first run slide on Android M+. r=liuche
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.
Attachment #8699551 - Flags: approval-mozilla-release?
Attachment #8699551 - Flags: approval-mozilla-beta?
Attachment #8699551 - Flags: approval-mozilla-aurora?
Seems like a good candidate for uplift to Beta and Aurora. Waiting for it to land on Nightly first.
https://hg.mozilla.org/mozilla-central/rev/fc825ad761f9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
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.
Attachment #8699551 - Flags: approval-mozilla-beta?
Attachment #8699551 - Flags: approval-mozilla-beta+
Attachment #8699551 - Flags: approval-mozilla-aurora?
Attachment #8699551 - Flags: approval-mozilla-aurora+
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.
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?
Flags: needinfo?(margaret.leibovic)
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)
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 on attachment 8699551 [details]
MozReview Request: Bug 1229350 - Disable Import first run slide on Android M+. r=liuche

Too late for 43.
Attachment #8699551 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.