774426 - Nothing shown on about:apps

Status: VERIFIED FIXED

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

Description

[Wesley Johnston (:wesj)]

Broken by bug 768276. We're now using the UNIX code here:
http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#724

which throws because there's no Home dir on Android.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

There's also an out given in Webapps.jsm, so this is us taking it. Alternatively, we could implement our own isLaunchable implementation?

Comment 2

[Wesley Johnston (:wesj)]

Filed bug 774422 and bug 774426. I'll mark them as blocking this.

Comment 3

[Wesley Johnston (:wesj)]

Dang it. Wrong bug.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

If the existence of the app in the registry (webapps.json) means that the app can always be launchable (i.e., there is no way for the user to remove an app from the OS without Firefox noticing), then yeah this allAppsLaunchable = true is the right thing to use.
But you should probably set it to true on the main place where you use Webapps.jsm, which is here if I understand it: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5878

(because getSelf/getInstalled/getAll all use that information, and if they happen to be used outside of about:apps they will return empty)

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Thanks felipe! This moves this to browser.js. Works to show apps. There's actually some problems launching them too. Digging in more...

Comment 6

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

Comment on attachment 642695 [details] [diff] [review]
Patch

I agree with Felipe, we should set this in WebappsUI.init so we get all the code paths covered.

I would also accept a patch to add a simple | return true; | to DOMApplicationRegistry._isLaunchable which has the added benefit of shoving some ANDROID into that file. Maybe we'd get broken less if we made our presence known.

Comment 7

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

Comment on attachment 642714 [details] [diff] [review]
Patch v2

Good enough I guess

Comment 8

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

Comment on attachment 642714 [details] [diff] [review]
Patch v2

I see this was not ready to review when I collided

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Threw in a fix for "Add to homescreen here". The other part I was more worried about (launching from an icon) is already fixed on mc apparently. (yay!)

Comment 10

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

Comment on attachment 642734 [details] [diff] [review]
Patch v3

looks ok to me

Comment 11

[:Felipe Gomes (needinfo for replies!)]

I think I slightly prefer the fix using allAppsLaunchable = true. _isLaunchable should only be used when you implement a proper way to check if an app has been removed from the home screen (and if you want to stop it from showing in about:apps in that case).

there's a `return true` as fallback at the end, so the ifdef fix should be to skip the UNIX part if ANDROID is defined, instead of adding an explicit return true


If the motivation to use the ifdef was just to make Android present in that file, don't worry, it won't happen again. And I'll move ifdefs all out of Webapps.jsm soon anyway into WebappOSUtils.jsm which will help with that

Comment 12

[Wesley Johnston (:wesj)]

I'm more worried about the use of XP_UNIX to determine what to do here. Doesn't seem specific enough. XP_UNIX is apparently defined for anything that isn't Windows or OS/2?

http://mxr.mozilla.org/mozilla-central/source/configure.in#8681

Comment 13

[Wesley Johnston (:wesj)]

Talked to felipe. Since we'd like to eventually remove the #ifdef's here, it seems best just to use the second patch here (with the additional icon fix from the third one). Since mfinkle r+ that originally, I'm just going to push the two and keep talking to felipe about ways to combine/reuse desktop+mobile webapps code as we move forward.

Comment 14

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/915fabca7973
https://hg.mozilla.org/integration/mozilla-inbound/rev/caadf910b246

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/915fabca7973
https://hg.mozilla.org/mozilla-central/rev/caadf910b246

Comment 16

[Aaron Train [:aaronmt]]

Looks good. V-F on M-C (Nightly 07/17), GN 4.1.1

Does this need to be uplifted to Aurora?

Comment 17

[Aaron Train [:aaronmt]]

Ping ^

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 642734 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): api spec change done in bug 768276
User impact if declined: fennec can't launch apps from the about:apps dashboard
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): none, straightforward
String or UUID changes made by this patch: none

note: what landed is a combination of this patch and patch 1. see comment 15 for the cset

Comment 19

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac2fbc3fc256
https://hg.mozilla.org/releases/mozilla-aurora/rev/f54f0b37b9e4