Closed Bug 802228 Opened 9 years ago Closed 9 years ago

Check for app updates when we check for system update

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

VERIFIED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: etienne, Assigned: fabrice)

References

Details

Attachments

(1 file, 1 obsolete file)

The current app updates specs specifies that when gecko's update manager checks for system update we should check the app update status too.

We can do it in gecko, iterating on the apps and calling .checkForUpdate

or

we can do it from gaia, but in this case gaia needs to receive a mozChromeEvent when the update manager checks for system update.

I'm fine with both, but we should probably decide quickly.
The spec'ed out UI does not have any other way to check for app updates, so probably a blocker.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Blocks: 799888
I talked with marshall. What we need to do here is to hook in to Checker.checkForUpdates function at [1]

If we make that function call something which triggers a mozIDOMApplication.checkForUpdate call on all installed applications, then we will automatically check for application updates any time we do a system update check. I.e. when an update check is triggered from the timer, from the "online" notification and from the force-update-check in the settings UI.

That leaves figuring out how to make the settings app wait with telling the user that we're done checking for updates until we've both checked for system updates and for app updates. One solution would be to delay the _callback.onCheckComplete(...) callback at [2] until we're done checking for app updates as well.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2736

[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2875
If gaia is calling checkUpdate() for each app, it can also wait for the DOMRequest to return and do the joining of app updates and gecko updates into a global "Updates are available" state.

Etienne, would it be enough to send a chromeEvent saying "it's time to check for app updates" ?
Fabrice: The problem is that if we add need to add code to [1] to check for updates for all apps in order to have app updates happen automatically. Once we do that it will also happen when the user presses the "force update check" button in the settings app.

We could have the settings app also do an app update check and wait for the DOMRequests to finish, but that would mean that we check all apps for update twice when the "force update check" button is pressed.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2736
Jonas, I think I agree but I was not clear about what I had in mind:
1) at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#2736, add an observer notification.
2) observe that in shell.js
3) fire a mozChromeEvent when that happens
4) the system app in gaia then do apps.forEach().checkUpdate()
5) the system app waits for all apps updates to return and also for the gecko update

So when we force an update from the settings app, we go through the same code path.
How would we get the information back to the settings app that we are done checking for system and app updates? I.e. how do we enable the settings app UI which contain the "force update check" button to display to the user that the updatecheck is done?

Right now the information from nsUpdateService to the settings app flows using a complicated setup involving nsUpdateService causing a mozChromeEvent fired at the system app, and then the system app setting a setting which is observed by the settings app.


My suggestion was that we make nsUpdateService do the apps.forEach().checkForUpdate() calls and then using the returned requests to figure out when to do the callback which starts the already-existing chain of callbacks which eventually reach the settings app.
I'm fine with both solutions.

It's more a product decision about the precision of the status displayed in the Settings app while forcing the checkForUpdates.

And in both cases gaia will wait a bit in order to display only 1 notification to the user with the total number of updates available.
Assignee: nobody → fabrice
Attached patch wip (obsolete) — Splinter Review
Etienne,

With this patch, we trigger update checks for all apps when a system update check occurs. Once we have done apps update checks, a mozChromeEvent is sent to gaia, called "update-apps". The payload is : { apps: ["manifest1", "manifest2", ...] }

Let me know if you prefer that I coalesce this notification with the "update-available" one.
(In reply to Fabrice Desré [:fabrice] from comment #8)
> Created attachment 676810 [details] [diff] [review]
> wip
> 
> Etienne,
> 
> With this patch, we trigger update checks for all apps when a system update
> check occurs. Once we have done apps update checks, a mozChromeEvent is sent
> to gaia, called "update-apps". The payload is : { apps: ["manifest1",
> "manifest2", ...] }
> 
> Let me know if you prefer that I coalesce this notification with the
> "update-available" one.

Do I even need this if I'm registered to each app |ondownloadavailable|?
(In reply to Etienne Segonzac (:etienne) from comment #9)
> 
> Do I even need this if I'm registered to each app |ondownloadavailable|?

It saves you from doing some housekeeping, like knowing for sure when we started the batch update.
(In reply to Fabrice Desré [:fabrice] from comment #10)
> (In reply to Etienne Segonzac (:etienne) from comment #9)
> > 
> > Do I even need this if I'm registered to each app |ondownloadavailable|?
> 
> It saves you from doing some housekeeping, like knowing for sure when we
> started the batch update.

The batch checkForUpdate right?

Concerning the (hum) mozSetting use to tell the Settings app that the check was done, any changes?
(In reply to Etienne Segonzac (:etienne) from comment #11)
> (In reply to Fabrice Desré [:fabrice] from comment #10)
> > (In reply to Etienne Segonzac (:etienne) from comment #9)
> > > 
> > > Do I even need this if I'm registered to each app |ondownloadavailable|?
> > 
> > It saves you from doing some housekeeping, like knowing for sure when we
> > started the batch update.
> 
> The batch checkForUpdate right?

yes

> Concerning the (hum) mozSetting use to tell the Settings app that the check
> was done, any changes?

No change here for now. Not sure what we can do cleanly...
Okay, here's the plan.

Currently system updates are launched by the timer or by the user using the force-check button in the settings app. We should check for apps updates at those time too.

Gaia will be able to know the status of the check by observing an app.updateStatus setting the same we do it right now with gecko.updateStatus.
Attached patch patchSplinter Review
This patch implements Etienne's request.

It sets the "apps.updateStatus" setting to "check-complete" and sends the list of apps to update (their manifest URL) in a "apps-update-check" mozChromeEvent.
Attachment #676810 - Attachment is obsolete: true
Attachment #679772 - Flags: review?(marshall)
Comment on attachment 679772 [details] [diff] [review]
patch

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

Looks good. Flagging bbondy for review of nsUpdateService.js

::: b2g/components/UpdatePrompt.js
@@ +388,5 @@
> +      this.result.forEach(function updateApp(aApp) {
> +        let update = aApp.checkForUpdate();
> +        update.onsuccess = function() {
> +          appsChecked += 1;
> +          appsToUpdate.push(aApp.manifestURL);

I don't know if the app update API currently provides this, but should we try to proactively include the kind of information we know we'll need? Things like:
- app update size (if it exists)
- app version
- release notes

@@ +394,5 @@
> +            self.appsUpdated(appsToUpdate);
> +          }
> +        }
> +        update.onerror = function() {
> +          appsChecked += 1;

Should we be communicating individual app errors up to gaia somehow? Also: holy nested function scope, batman!

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2807,5 @@
>    checkForUpdates: function UC_checkForUpdates(listener, force) {
>      if (!listener)
>        throw Cr.NS_ERROR_NULL_POINTER;
>  
> +    Services.obs.notifyObservers(null, "update-check-start", null);

This is probably ok, but we'll want someone with AUS ownership to review this just to be safe..
Attachment #679772 - Flags: review?(netzen)
Attachment #679772 - Flags: review?(marshall)
Attachment #679772 - Flags: review+
QA Contact: jsmith
Comment on attachment 679772 [details] [diff] [review]
patch

That's fine for the update service change, just canceling the review since I didn't review the other changes in the patch.
Attachment #679772 - Flags: review?(netzen)
https://hg.mozilla.org/mozilla-central/rev/52596d2fa339
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Keywords: verifyme
Marking as verified - I am able to get app updates when checking for a system update at a sanity level. Obviously there's more bugs to be found, but this at least notes it works at a smoke test level.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.