Closed Bug 1372926 Opened 7 years ago Closed 7 years ago

[Android O] Add to Home screen doesn't work on any version of Firefox on Oreo

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Version:
55 Branch
Type:
defect

Tracking

(fennec+, firefox54 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
fennec + ---
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: marcia, Assigned: snorp)

References

Details

Attachments

(2 files)

Bug 1372926 - Support new shortcut API in Android O
59 bytes, text/x-review-board-request
cnevinchen
: review+
Sylvestre
: approval-mozilla-beta+
Details
Bug 1372926 - Revert bug 1394356, allowing "add to homescreen" again on Android O
59 bytes, text/x-review-board-request
cnevinchen
: review+
Details 
Seen while running the latest Android O beta, Version 8.0.0, running on a Google Pixel.

STR:
1. Load a web page
2. Page | Add to Home Screen

The page is not added. I confirmed that the same operation works using Google Chrome.
Blocks: android-o
QA Contact: ioana.chiorean
From https://bugzilla.mozilla.org/show_bug.cgi?id=1378348#c0 also se elink in #c1

> The com.android.launcher.action.INSTALL_SHORTCUT broadcast no longer has any 
> effect on your app, because it is now a private, implicit broadcast. Instead, 
> you should create an app shortcut by using the requestPinShortcut() method from 
> the ShortcutManager class.
(In reply to Ioana Chiorean from comment #4)
> From https://bugzilla.mozilla.org/show_bug.cgi?id=1378348#c0 also se elink
> in #c1
> 
> > The com.android.launcher.action.INSTALL_SHORTCUT broadcast no longer has any 
> > effect on your app, because it is now a private, implicit broadcast. Instead, 
> > you should create an app shortcut by using the requestPinShortcut() method from 
> > the ShortcutManager class.

sebastian: This requires us to _target_ Android 25+ and not just build with Android 25+, right?
Flags: needinfo?(s.kaspari)
The new API that the "behavior change" documentation[1] mentions is API 26:
https://developer.android.com/reference/android/content/pm/ShortcutManager.html#requestPinShortcut(android.content.pm.ShortcutInfo,%20android.content.IntentSender)

Compiling with the new SDK is enough for that. There are some other things that fall apart once we actually target 26.

[1] https://developer.android.com/preview/behavior-changes.html#as
Flags: needinfo?(s.kaspari)
we probably need to temporarily disable / hide "Add to homescreen" on Android O if a proper solution to compile with the new SDK requires gradle build and all the other dependencies
(In reply to Joe Cheng [:jcheng] (please needinfo) from comment #7)
> we probably need to temporarily disable / hide "Add to homescreen" on
> Android O if a proper solution to compile with the new SDK requires gradle
> build and all the other dependencies

That's what we did for the Focus 1.2 release too.

Although note that I do think that we could update to the new SDK without waiting for gradle first (nalexander might know more here though; I think he might even tried that locally). We did updates of the SDK in the past and while it's a bit more complicated than changing a line in gradle it's doable.
We couldn't update to the N SDK that easily because it required a new compiler. But this restriction is now gone in Android O again and an SDK update could work.

Let me know if you want to do that and whether you need my help. Also note that every past SDK update required additional work for fixing behavior changes and API deprecation too.
(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> (In reply to Joe Cheng [:jcheng] (please needinfo) from comment #7)
> > we probably need to temporarily disable / hide "Add to homescreen" on
> > Android O if a proper solution to compile with the new SDK requires gradle
> > build and all the other dependencies
> 
> That's what we did for the Focus 1.2 release too.
> 
> Although note that I do think that we could update to the new SDK without
> waiting for gradle first (nalexander might know more here though; I think he
> might even tried that locally). We did updates of the SDK in the past and
> while it's a bit more complicated than changing a line in gradle it's doable.
> We couldn't update to the N SDK that easily because it required a new
> compiler. But this restriction is now gone in Android O again and an SDK
> update could work.

Sadly, I believe there's a snag (see also https://bugzilla.mozilla.org/show_bug.cgi?id=1384312#c0).  I believe that SDK 25+ requires build-tools 25.0.3, which requires... glibc 2.14+.  Our ancient CentOS 6-based build images are pinned to glibc 2.12, and I believe it's not possible to update glibc and not feasible to have two glibc versions installed.

I got pretty far along the path of replacing the CentOS 6-based build images with Debian based build images that worked with glibc 2.14+, but haven't pushed it across the line.  Moving to CentOS 7-based build images might be quite simple, since the centos6-build Dockerfile has been improved recently.

If SDK 25+ doesn't require a more recent build-tools (perhaps that's Gradle only?), then we can absolutely push forward the SDK versions for building, etc.  We will need to fix the Proguard issue, but that's not difficult -- see https://bugzilla.mozilla.org/show_bug.cgi?id=1352599.
Blocks: progressive-apps
No longer blocks: 1310986
(In reply to Joe Cheng [:jcheng] (please needinfo) from comment #7)
> we probably need to temporarily disable / hide "Add to homescreen" on
> Android O if a proper solution to compile with the new SDK requires gradle
> build and all the other dependencies

There are online reports that Pixel devices will be updated to Android O on 8/21, not sure if that is true or not.
Our partners have started to discover this on Android O.

If we are not going to be able to fix this any time soon, we need to remove the add to home screen option for Android O.
Android O shipped to Pixels the other day - do we want to create a separate bug to track removing the home screen option?
Comment on attachment 8900448 [details]
Bug 1372926 - Support new shortcut API in Android O

https://reviewboard.mozilla.org/r/171804/#review177398

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:14
(Diff revision 2)
>  import android.app.Application;
>  import android.content.ContentResolver;
>  import android.content.Context;
>  import android.content.Intent;
> +import android.content.pm.ShortcutManager;
> +import android.content.pm.ShortcutInfo;

This is added in API leve 25

https://developer.android.com/reference/android/content/pm/ShortcutManager.html
I think we need to update our SDK before we can use it.

I believe there's a discussion between Max and Nick about what's the best way do this. AFAIK it's more complicated than previous update. But I don't know the schedule about it.
> I believe there's a discussion between Max and Nick about what's the best
> way do this. AFAIK it's more complicated than previous update. But I don't
> know the schedule about it.

The tricky thing (I think) is requiring newer build-tools, which in turn sets off a cascade.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1370119#c2.
Comment on attachment 8900448 [details]
Bug 1372926 - Support new shortcut API in Android O

https://reviewboard.mozilla.org/r/171804/#review179874

Please request review again once we've done the upgrade :)
Attachment #8900448 - Flags: review?(cnevinchen)
Let's put "Oreo" in the title to make this searchable.
Summary: [Android O] Add to Home screen doesn't work on any version of Firefox → [Android O] Add to Home screen doesn't work on any version of Firefox on Oreo
We need a patch for Firefox 56 that at least removes this menu poption for Android O.

If an OEM ships Android O, we need the functionality to not be broke.
I believe the patch to remove the menu option  is in bug 1394356 and should be in the beta 11 build that we'll do later today.  

If we need this fix for 56, then please try to get review and testing in place quickly so that we can uplift this to beta. Sebastian, Joe, what do you think? Can anyone work on this now? Or is it going to wait for Firefox 57?
status-firefox57: --- → affected
Flags: needinfo?(s.kaspari)
Flags: needinfo?(jcheng)
It would be best to get this into 56 indeed.
Flagging for the mobile triage meeting tomorrow - hopefully someone from the mobile core team can pick this up.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
agree with Andreas on comment 23.
flagging Wesly
Flags: needinfo?(jcheng) → needinfo?(wehuang)
This patch can't go in 56 because it requires build changes (I think).

For 56, we took bug 1394356 which removes add to homescreen on Android O.
I meant to apply the temporary solution bug 1394356 for 56 and it looks like we just did.
The full solution will require us to apply gradle build and SDK update first
Joe is correct, the plan is to have 1394356 (remove the menu option from UI) for 56+, and support the full "Add to Home screen" functionality after gradle is ready. Set P1 for now.

(In reply to Joe Cheng [:jcheng] (please needinfo) from comment #27)
> I meant to apply the temporary solution bug 1394356 for 56 and it looks like
> we just did.
> The full solution will require us to apply gradle build and SDK update first
Flags: needinfo?(wehuang)
Priority: -- → P1
[triage0913] + as P1.
tracking-fennec: ? → +
To be 100% clear, we're talking about temporarily disabling "add to home screen" in 56 (and 57) running on Oreo, while still keeping it working as expected in older Android versions, right?
> To be 100% clear, we're talking about temporarily disabling "add to home screen" in 56 (and 57) running on Oreo, while still keeping it working as expected in older Android versions, right?

Yes. This was done in bug 1394356.
The attached patch will allow us to restore the "add to home screen" functionality even without the SDK upgrade.
Comment on attachment 8900448 [details]
Bug 1372926 - Support new shortcut API in Android O

https://reviewboard.mozilla.org/r/171804/#review189650

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:562
(Diff revision 5)
>  
>      private static void createHomescreenIcon(final Intent shortcutIntent, final String aTitle,
>                                               final String aURI, final Bitmap aIcon) {
> +        final Context context = GeckoAppShell.getApplicationContext();
> +
> +        if (Versions.feature26Plus) {

nit: can we move this code to an utility class and let it decide the implemetation there ?
Attachment #8900448 - Flags: review?(cnevinchen) → review+
Comment on attachment 8912801 [details]
Bug 1372926 - Revert bug 1394356, allowing "add to homescreen" again on Android O

https://reviewboard.mozilla.org/r/184110/#review189652
Attachment #8912801 - Flags: review?(cnevinchen) → review+
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/787e7de1ace6
Revert bug 1394356, allowing "add to homescreen" again on Android O r=nechen
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac21f869569
Support new shortcut API in Android O r=nechen
Bugherder might butcher this due to "revert" being in one of the commit messages, but here's the first patch merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/787e7de1ace6
https://hg.mozilla.org/mozilla-central/rev/2ac21f869569
https://hg.mozilla.org/mozilla-central/rev/6bb669555e24
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
These patches conflicted with the ones from bug 1395841 when they got merged together. I think I resolved the conflicts, but it wouldn't hurt to make sure I did it right.
What is the plan wrt to 57 for this bug?
thanks
status-firefox54: affected → wontfix
status-firefox55: affected → wontfix
status-firefox56: affected → wontfix
I just tested this using the latest nightly and it works - using Page | Add Page Shortcut.
Comment on attachment 8900448 [details]
Bug 1372926 - Support new shortcut API in Android O

Approval Request Comment
[Feature/Bug causing the regression]: Android N release
[User impact if declined]: Unable to add shortcuts to homescree on Android N
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Ensure Page -> "Add to homescreen" works on an Android N device.
[List of other uplifts needed for the feature/fix]: Both patches on this bug
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only adds special path for Android N devices, others unaffected.
[String changes made/needed]: None
Attachment #8900448 - Flags: approval-mozilla-beta?
Note to relman: this is also an important partner issue. Contact Mike Kaply for details.
Assignee: nobody → snorp
Comment on attachment 8900448 [details]
Bug 1372926 - Support new shortcut API in Android O

Make senses anyway. Taking it.
Should be in 57b6
Attachment #8900448 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: