856131 - Regression: No Android home-screen shortcut created on app install

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

Not sure what this regressed from, but installing any application is not creating an Android home-screen shortcut anymore on trunk (mozilla-central, Fx22).

i) http://www.testmanifest.com
ii) Click 'install'
iii) Check Android home-screens for newly added shortcut (see notification is existant and fine)

--
Nightly (03/29)
Asus Nexus 7 (Android 4.2.2)

Comment 1

[Teodora Vermesan (:TeoVermesan)]

The regression window for this issue is:
1. mozilla central
good build: 26.03.2013  
bad build:  27.03.2013 

possible pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=456cb08f8509&tochange=178a4a770bb1

2. inbound
good build: 1364240243
bad build:  1364243303

possible pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f7138044b0cf&tochange=9592881ee7fc

Comment 2

[Aaron Train [:aaronmt]]

Guessing it's bug 813736 that mangled something accidently.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

James, do you have a plan for this?

Comment 4

[Matt Basta [:basta]]

Interestingly, the Marketplace (when installed from http://inferno.paas.allizom.org/) generates an icon (albeit after a delay, see bug 862948).

The marketplace is a packaged app, though. Don't know if that makes a difference.

Comment 5

[Aaron Train [:aaronmt]]

Confirming the above application works; weird. Anything currently on Marketplace doesn't install a shortcut-icon for me; Nightly (04/17) - Nexus 4 (Android 4.2.2)

Comment 6

[James Hugman [:jhugman] [@jhugman]]

Investigating now. 

I can replicate with hosted apps, but not with packaged apps.

Comment 7

[James Hugman [:jhugman] [@jhugman]]

Attachment: Inform browser.js of download success for hosted apps.

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 739348 [details] [diff] [review]
Inform browser.js of download success for hosted apps.

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

Splinter fails miserably at showing the diffs, but from what I see:
- rename aZipDownloadSuccessCallback to aInstallSuccessCallback
- I don't like that we have to copy over the |if (!aFromSync) ...| block.
- why did you change |manifest = new ManifestHelper(jsonManifest, app.manifestURL);| to |manifest = new ManifestHelper(jsonManifest, appObject.installOrigin);| ?

Comment 9

[James Hugman [:jhugman] [@jhugman]]

Attachment: Second patch following feedback.

- rename aZipDownloadSuccessCallback to aInstallSuccessCallback
done
- I don't like that we have to copy over the |if (!aFromSync) ...| block.
I've put the 'then' block into a function called installFromMarketSuccess(). I could've put more in there, e.g. 

+ this.updateAppHandlers(…)
+          if (aInstallSuccessCallback)
+            aInstallSuccessCallback(aManifest);

or even the if (!aFromSync) {} could go in this function.

I wasn't convinced that this wouldn't change the orderings of the statements from the original.

- why did you change |manifest = new ManifestHelper(jsonManifest, app.manifestURL);| to |manifest = new ManifestHelper(jsonManifest, appObject.installOrigin);| 
To match http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1044 . I took your question as a suggestion, so have changed it back.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 740858 [details] [diff] [review]
Second patch following feedback.

r=me with the following nits addressed:
- rename installFromMarketSuccess() to something that does not mention "Market", eg postInstallTask().
- use { } for all if blocks, even single lines (I know, some were like this before your patch).

Also, please push this to try before landing.

Comment 11

[James Hugman [:jhugman] [@jhugman]]

Attachment: Feedback fixing nits in fabrice r+

Changed postInstallFromMarket to postFirstInstallTask. It's only involved in installs not from sync, hence the reference to "first install".

Pushed to try.

Comment 12

[Wesley Johnston (:wesj)]

How'd this look on try? Can we land it?

Comment 13

[James Hugman [:jhugman] [@jhugman]]

This looks fine on try. Do please land!

https://tbpl.mozilla.org/?tree=Try&rev=df73ff22de54

Comment 14

[Wesley Johnston (:wesj)]

That try push doesn't look like the right one? I pushed this again...

https://tbpl.mozilla.org/?tree=Try&rev=6bf7e59b19fd

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d11b778458e9

Comment 16

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

This can ride the trains

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[Julien Wajsberg [:julienw]]

Hey, I think this produced bug 873134.

Could you please explain the rationale behind moving the message install event broadcast inside the |if (!aData.isPackage)| block ?

(see https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2022)

Comment 19

[James Hugman [:jhugman] [@jhugman]]

This is out of date, obsoleted by synthesized APKs.