830463 - download/update notification is showing up late on a weak connection in some cases

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Related to bug 830447.

Build: B2G 18 1/14/2013
Device: Unagi

Steps:

1. Install a hosted app preloading appcache on a weak connection (Mozilla Guest) after trying an install once before in bug 830447

Expected:

Upon downloading starts, we should show a download progress for the appcache hosted app being downloaded.

Actual:

On a weak connection, a noticeable delay can be seen before the download notification shows up. Definitely not a blocker cause progress is evident eventually, but this does feel a tad awkward?

Comment 1

[Julien Wajsberg [:julienw]]

see Bug 830447 comment 4

Comment 2

[Julien Wajsberg [:julienw]]

ie we can't do anything here because we don't get any event from the platform before the first progress.

Comment 3

[Julien Wajsberg [:julienw]]

I got an idea discussing with Michal.

I believe that if gecko sends a progress event (with app.progress === 0) as soon as the download start, then gaia would display the notification ASAP.

This is imho the best solution as no solutions in gaia can cover all cases now, eg the "launch the update download from the app" case (because if gaia doesn't initiate the download, it doesn't know about it before the first progress), and the "restart a failed install" case (because we don't have an event for the resume, and therefore gaia only knows at the first progress).

I could work on a patch for this but first I'd like Fabrice's feedback.

Comment 4

[[:fabrice] Fabrice Desré]

That's an option, and that's reasonable.

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: patch v1

tested both install and update cases for both packaged and hosted+appcache apps, in both a strong and weak connection (far from my wifi router).

This works fine for me.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 712250 [details] [diff] [review]
patch v1

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

::: dom/apps/src/Webapps.jsm
@@ +2825,5 @@
>          " - " + aApp.installState);
>    this.app = aApp;
>    this.startStatus = aApp.installState;
>    this.lastProgressTime = 0;
> +  // send a first progress event to correctly set the external object's

nit: s/external/DOM

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: patch v2

fixed nits, carrying r=fabrice

Comment 9

[Julien Wajsberg [:julienw]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -

User impact if declined: the primary bug is that the notification is showing late when downloading (installing/updating) on weak connection (that is: in our target market). Bug this triggers things like Bug 839715, that are especially important if we're not uplifting Bug 838561.

Testing completed: yes, I tested virtually all code paths modified by this patch. Of course we could wait that this lands in central and that Jason tests this before upliting. Or we can uplift this and test the resulting build.

Risk to taking this patch (and alternatives if risky): very low as we're not modifying any logic here, but merely sending an already existing event.

String or UUID changes made by this patch: none

I believe we need either this or fix in Bug 838561, without one of them the user can put the phone in a strange state that would need a reboot to recover correctly. I also believe this fix is safer (no change in the branching and the code path, contrary to fix in Bug 838561).

So I'd really want to see this in v1.0.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d23d959dcf6

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d23d959dcf6

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Approving this lower risk option, bug Bug 838561 is being held for approval until we're landing to v1.1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ca24d7b9e3c9