899353 - Show progress while installing packaged apps or appcache

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Marco Castelluccio [:marco]]

We should show something to the user while we're downloading an app, otherwise the user could think something went wrong.
We don't have to show a real progress, but at least something that tells the user we're actually doing something.

Some options are:
1) A progress popup notification (like we do for addons)
2) A fake progress popup notification (a progress bar that keeps moving and moving until the download is finished). This could be useful because sometimes we don't have correct informations on the download size.
3) Add an entry in the downloads panel (I don't know if it's feasible, may be too difficult).

Comment 1

[Marco Castelluccio [:marco]]

Another option is not to show the UI in Firefox itself, but leave to the content the opportunity to do that (it's the current situation).

Comment 2

[Marco Castelluccio [:marco]]

Attachment: show_progress

I think an undetermined progressmeter could be a good solution while we decide.

Comment 3

[Marco Castelluccio [:marco]]

Attachment: show_progress

(After bug 747552, the WebappsInstaller.install function will return a promise)

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch

This is even simpler.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 793133 [details] [diff] [review]
Patch

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

::: browser/modules/webappsUI.jsm
@@ +27,5 @@
>    init: function webappsUI_init() {
>      Services.obs.addObserver(this, "webapps-ask-install", false);
>      Services.obs.addObserver(this, "webapps-launch", false);
>      Services.obs.addObserver(this, "webapps-uninstall", false);
> +    cpmm.addMessageListener("Webapps:OfflineCache", this);

nit: remove listener on uninit

@@ +37,5 @@
>      Services.obs.removeObserver(this, "webapps-uninstall");
>    },
>  
> +  receiveMessage: function(aMessage) {
> +    let msg = aMessage.json;

`json` still works but has been deprecated: use -> aMessage.data

also the name of this var is better as "data" rather than "msg"

@@ +147,5 @@
> +                  yield WebappsInstaller.install(aData, aManifest);
> +                  if (this.downloads[manifestURL]) {
> +                    yield this.downloads[manifestURL].promise;
> +                  }
> +                  installationSuccessNotification(aData, app, bundle);

question: will this notification automatically overwrite the progress notification?

@@ +154,4 @@
>                    // TODO: Notify user that the installation has failed
> +                } finally {
> +                  notification.remove();
> +                  if (this.downloads[manifestURL]) {

no need for the if check, you can always call delete

@@ +194,5 @@
>        }
>      }
>    };
>  
> +  let notifier;

this declaration can be inside the try block

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+.

(In reply to :Felipe Gomes from comment #5)
> @@ +147,5 @@
> > +                  yield WebappsInstaller.install(aData, aManifest);
> > +                  if (this.downloads[manifestURL]) {
> > +                    yield this.downloads[manifestURL].promise;
> > +                  }
> > +                  installationSuccessNotification(aData, app, bundle);
> 
> question: will this notification automatically overwrite the progress
> notification?

One is a popup notification, the other is a XUL alert. The popup notification is removed in the finally block.

Maybe we could get rid of the XUL alert, I think we should wait UX input about that. I'll open a new bug.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/17f932a5856d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out because one of the 4 webapps patches landed on your behalf was causing mochitest-other assertions. You really need to start running your patches through Try before requesting checkin. This is far from the first bustage we've had to backout for.
https://hg.mozilla.org/integration/fx-team/rev/3a3d9846ceca

https://tbpl.mozilla.org/php/getParsedLog.php?id=27007390&tree=Fx-Team

Comment 9

[Marco Castelluccio [:marco]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #8)
> You really need to start running yourbpatches through Try before requesting checkin.

I did: https://tbpl.mozilla.org/?tree=Try&rev=30a4e8eb935c

Comment 10

[Marco Castelluccio [:marco]]

Actually I forgot to add this patch to my try push, so this is at fault.

The assertion is:
###!!! ASSERTION: Why did this not get handled while processing mRestyleRoots?: '!element->HasFlag(collector->tracker->RootBit()) || (element->GetFlattenedTreeParent() && (!element->GetFlattenedTreeParent()->GetPrimaryFrame()|| element->GetFlattenedTreeParent()->GetPrimaryFrame()->IsLeaf())) || (aData.mChangeHint & nsChangeHint_ReconstructFrame)', file e:/builds/moz2_slave/fx-team-w32-d-0000000000000000/build/layout/base/RestyleTracker.cpp, line 87

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Interestingly, it was also causing mochitest-bc orange. Tim Taubert might be able to help if you have questions on that.
https://tbpl.mozilla.org/php/getParsedLog.php?id=27012954&tree=Fx-Team

Comment 12

[Marco Castelluccio [:marco]]

The assertion is caused by the popup notification, if I don't show it, the assertion isn't triggered.

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Patch

Could you see why the popup notification is causing the assertion?

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch

Actually I've fixed the problem by removing the old notification popup before showing the new one.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Nope.  This assertion shouldn't happen: that's why it's an assertion.  If it's happening, then something is seriously broken somewhere.

Comment 16

[Marco Castelluccio [:marco]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Nope.  This assertion shouldn't happen: that's why it's an assertion.  If
> it's happening, then something is seriously broken somewhere.

I think we can land this patch in the meantime, as the assertion isn't triggered anymore with the fix. Do you agree? Or do you want to fix the underlying problem before?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Landing sounds fine; if you have a reliable way to reproduce the assertion, please file a bug with steps to reproduce!

Comment 18

[Marco Castelluccio [:marco]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> Landing sounds fine; if you have a reliable way to reproduce the assertion,
> please file a bug with steps to reproduce!

I will!

Sadly there's still the unexpected uninterruptible reflow on Linux that prevents landing. ttaubert do you know what causes that?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

I can reproduce this every time I run browser_duplicateIDs.js before browser_tabopen_reflows.js. If I make browser_duplicateIDs.js a no-op the reflow test is working fine.

I'm not sure why but for some reason the querySelector() call or something in that test activates the progress bar and that causes reflows afterwards...

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Here's the test:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_duplicateIDs.js

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Strange. This does *not* fail:

function test() {
  document.querySelectorAll("[id]");
}

But this does:

function test() {
  Array.forEach(document.querySelectorAll("[id]"), function () {});
}

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Replacing Array.forEach() with this still fails:

for (let node of document.querySelectorAll("[id]")) {}

Is there something special that happens when we access items of a NodeList?

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Removing the id attribute from the progressmeter helps, it's unused anyway:

- <progressmeter id="webapps-progressmeter" mode="undetermined"/>
+ <progressmeter mode="undetermined"/>

No idea why it doesn't like to be retrieved via querySelectorAll(). While this is a simple workaround I feel like we should investigate the root issue here a little deeper.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Is there something special that happens when we access items of a NodeList?

Yes.  XBL bindings would get attached for any of those elements that should have them but don't have them attached yet (e.g. display:none elements).

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Okay, that makes sense. Does that mean once the progressmeter has been shown it doesn't stop requesting animation frames until it has been removed from the document? Looks a lot like it:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/progressmeter.xml#92

Also, the progressmeter should probably not start animating if it is initialized but is hidden?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Looks a lot like it:

Yes, sure does.

Comment 27

[Marco Castelluccio [:marco]]

So I should probably add the progress meter "on-the-fly" and remove it when I remove the notification.

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Patch v2

This fixes the problem: https://tbpl.mozilla.org/?tree=Try&rev=f866b97dd81f

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Patch

Updated commit message.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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