953328 - update about:apps to work with synthetic APKs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress: fixes retrieval of biggest icon

With Synthetic APKs enabled, WebappsUI is no longer defined in browser.js, so about:apps shouldn't count on it existing.

(I'm not sure we should have about:apps at all with SynthAPKs, which use native interfaces for browsing and launching installed apps; but it's useful for testing app updates at the very least; and while we still have it, it should work.)

Here's a patch that moves WebappsUI.getBiggestIcon to ManifestHelper.biggestIconURL, where it can be accessed both with and without SynthAPKs.

Along the way I also removed an orphaned code block in aboutApps.js and fixed a non-SynthAPK-specific bug that causes Fennec to load about:blank instead of Marketplace when you tap the label of the "Browse the Firefox Marketplace" list item (because the event target points at the inner div that contains the label instead of the outer div that has the pref attribute; the fix for which is to use currentTarget, which is always the outer div).

This patch depends on the "install flow" patch in bug 934758 and lives in the apk-about-apps branch of the apks branch:

https://github.com/mykmelez/gecko-dev/tree/apk-about-apps
https://github.com/mykmelez/gecko-dev/compare/apks...apk-about-apps

I'm not sure what to do with the "add to homescreen" and "uninstall" context menu items.  Perhaps we should simply preprocess them out for SynthAPKs.

Comment 1

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #0)

> Along the way I also removed an orphaned code block in aboutApps.js and
> fixed a non-SynthAPK-specific bug that causes Fennec to load about:blank
> instead of Marketplace when you tap the label of the "Browse the Firefox
> Marketplace" list item (because the event target points at the inner div
> that contains the label instead of the outer div that has the pref
> attribute; the fix for which is to use currentTarget, which is always the
> outer div).

Sounds like you fixed bug 953048

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Sounds like you fixed bug 953048

Indeed, so I isolated that fix and landed it over in that bug!

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8351615 [details] [diff] [review]
work in progress: fixes retrieval of biggest icon

Review of attachment 8351615 [details] [diff] [review]:
-----------------------------------------------------------------

Just glancing at this, I'm not sure what this fixes that's broken currently. What do synthetic apk's lack from their manifest that the old code doesn't work?

Long term, it might be nice to fix-up moz-icon:// protocols to work for app names (we already use that in our native UI in some places to show icons). i.e.
moz-icon://org.mozilla.fennec

we'd need to build out http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#1808 for that though, and its beyond scope.

::: dom/apps/src/AppsUtils.jsm
@@ +628,5 @@
>      }
>      return {};
>    },
>  
> +  get biggestIconURL() {

We actually have the icon size we want in js now. We could probably do something better than "biggest". Different bug though...

::: mobile/android/chrome/content/browser.js
@@ -7141,5 @@
> -    let iconURI = null;
> -    try {
> -      iconURI = Services.io.newURI(biggestIcon, null, null);
> -      if (iconURI.scheme == "data") {
> -        return iconURI.spec;

Do we not want this anymore?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes about:apps

(In reply to Wesley Johnston (:wesj) from comment #3)

> Just glancing at this, I'm not sure what this fixes that's broken currently.
> What do synthetic apk's lack from their manifest that the old code doesn't
> work?

It isn't a manifest issue; rather, when synthetic APKs are enabled, then gChromeWin.WebappsUI doesn't exist, as it has been preprocessed out of browser.js, so we can't access its getBiggestIcon method.  Thus we move the method to AppsUtils, where it can be accessed by both the old and new implementations.


> Long term, it might be nice to fix-up moz-icon:// protocols to work for app
> names (we already use that in our native UI in some places to show icons).
> i.e.
> moz-icon://org.mozilla.fennec

Indeed!


> ::: mobile/android/chrome/content/browser.js
> @@ -7141,5 @@
> > -    let iconURI = null;
> > -    try {
> > -      iconURI = Services.io.newURI(biggestIcon, null, null);
> > -      if (iconURI.scheme == "data") {
> > -        return iconURI.spec;
> 
> Do we not want this anymore?

The new implementation returns this._origin.resolve(biggestIcon), where this._origin is set to Services.io.newURI(aOrigin, null, null), and *resolve* resolves a data: URL against any base URL by simply returning the data: URL, so the new implementation does the same thing in a different way.

Evaluate this expression in the Browser Console to verify:

  Services.io.newURI("https://www.mozilla.org", null, null).resolve("data:text/plain,foo")

Here's an updated patch that disables the context menu for synthetic APKs, as both Add to Homescreen and Uninstall duplicate native interfaces for adding apps to the homescreen and uninstalling them.  We could decide to provide such functionality anyway, just as the mozApps API provides "uninstall".  But this seems like a reasonable step for now and unbreaks about:apps (until we remove it or give it a new raison d'être, like being an app permissions manager).

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8361407 [details] [diff] [review]
patch v1: fixes about:apps

Review of attachment 8361407 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like its racing with the update patch :)

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8361407 [details] [diff] [review]
patch v1: fixes about:apps

(In reply to Wesley Johnston (:wesj) from comment #5)
> This looks like its racing with the update patch :)

It is indeed!  As is bug 957070 and bug 959420.  And they're all going to win!  So I'll have to rebase the update patch a bunch.  And I need Fabrice's review of the AppsUtils addition here.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8361407 [details] [diff] [review]
patch v1: fixes about:apps

Review of attachment 8361407 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/AppsUtils.jsm
@@ +630,5 @@
>    },
>  
> +  get biggestIconURL() {
> +    let icons = this._localeProp("icons");
> +    if (!icons)

nit: braces even for single line ifs.

@@ +634,5 @@
> +    if (!icons)
> +      return null;
> +
> +    let iconSizes = Object.keys(icons);
> +    if (iconSizes.length == 0)

ditto.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50af4be3734c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8970925d19

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50af4be3734c
https://hg.mozilla.org/mozilla-central/rev/6b8970925d19