828190 - Support updates of preinstalled hosted apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since preinstalled hosted apps are updated whenever the server content is updated, we should support updating of the application manifest as well, as to enable the developer to keep the two in sync.

This applies also to non-uninstallable preinstalled apps such as the marketplace.

Comment 1

[Jason Smith [:jsmith]]

FYI - A weird case btw to handle here that Tony hit:

A while back the maps origin had origin A as the app origin. Then, we fixed some patches that changed the origin to B. When tchung updates his phone, the preloaded maps app still had origin A.

Not sure how we'll handle this.

Comment 2

[[:fabrice] Fabrice Desré]

That's because the app id has not changed, which should have happened when changing the origin. I understand why we did that but we won't try to support this kind of url change in this patch.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> That's because the app id has not changed, which should have happened when
> changing the origin. I understand why we did that but we won't try to
> support this kind of url change in this patch.

Should I file a separate bug about this edge case?

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Fabrice's patch from Bug 788894

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 699682 [details] [diff] [review]
v1

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

::: dom/apps/src/Webapps.jsm
@@ +1289,5 @@
>                                   app.localId, false, updateObserver);
>        });
> +
> +      // if the app is updatable (because it's not on the /system partition),
> +      // we proceed to try an update.

If I am not wrong, for non-removable hosted apps we already checked for update at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1267, so if the app is not in '/system' we would be calling |checkForUpdate| twice for the same app (the second call would be at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1220)

It would also be great if you could modify the comments at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1233 since that it's not true anymore.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

In fact, the condition at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1234 is not valid anymore and, if I am not wrong, we would need to check if the app is in /system instead. I mean we just check the appcache only if the app lives in /system cause we can't modify the manifest.

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> Comment on attachment 699682 [details] [diff] [review]
> v1
> 
> Review of attachment 699682 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +1289,5 @@
> >                                   app.localId, false, updateObserver);
> >        });
> > +
> > +      // if the app is updatable (because it's not on the /system partition),
> > +      // we proceed to try an update.
> 
> If I am not wrong, for non-removable hosted apps we already checked for
> update at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1267,
> so if the app is not in '/system' we would be calling |checkForUpdate| twice
> for the same app (the second call would be at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1220)

I think you're right, but that this is part of the fix to clean the appcache update story in bug 824697

> It would also be great if you could modify the comments at
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1233
> since that it's not true anymore.

Sure, good catch

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Updated patch. With that I can see updates firing to https://marketplace.firefox.com/telefonica/manifest.webapp returning 200 on first call and then 304.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 699779 [details] [diff] [review]
patch v2

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

r=me

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4e21765ed0

Comment 11

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0ee278d8174d

Comment 12

[Johnny Stenback (:jst)]

Marking FIXED as this is landed on inbound and b2g18.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a4e21765ed0

Comment 14

[Gene Lian (I already quit Mozilla)]

Sorry for bugging but this patch would destroy the first-run start-up. Fire bug 828835 (Patrick has already got the root cause and provided a solution, fortunately).