Closed Bug 828190 Opened 8 years ago Closed 8 years ago

Support updates of preinstalled hosted apps

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

RESOLVED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: sicking, Assigned: fabrice)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
blocking-basecamp: --- → +
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.
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.
(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?
Attached patch v1 (obsolete) — Splinter Review
Fabrice's patch from Bug 788894
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.
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.
(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
Attached patch patch v2Splinter Review
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.
Attachment #699682 - Attachment is obsolete: true
Attachment #699779 - Flags: review?(ferjmoreno)
Comment on attachment 699779 [details] [diff] [review]
patch v2

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

r=me
Attachment #699779 - Flags: review?(ferjmoreno) → review+
Marking FIXED as this is landed on inbound and b2g18.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
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).
Target Milestone: --- → mozilla21
Keywords: verifyme
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.