972069 - Large Packaged Apps don't provide feedback, seem to hang

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

There's no feedback about how long a packaged app takes to install, so sideloading an app ~80 MB seems to just hang, as you have no idea how long to wait for it to load (90s+ by my count).  This would be super helpful.  Unfortunately, I cannot attach my test case.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yes, it's quite slow right now.  Bug 924574 will make it significantly faster, but even then that will only help future OS releases.  We should at least log some progress in the mini-console.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

How about a progress indicator since the app size is known in advance?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Panos Astithas [:past] from comment #2)
> How about a progress indicator since the app size is known in advance?

Yeah, probably a more reasonable idea. :) Any location sound good?  Perhaps next to the project action buttons (Update / Debug / etc.)?

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to J. Ryan Stinnett [:jryans] from comment #3)
> Perhaps next to the project action buttons (Update / Debug / etc.)?

Sounds perfect.

Comment 5

[Paul Rouget [:paul]]

Just FYI, in bug 919502 (which I'm supposed to be working on), I introduced a spinner.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Not working on this for now.  Seems best to address this with UI v2.

Comment 9

[Paul Rouget [:paul]]

Attachment: v1

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

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

Overall it looks great! :D Just a few style / API nits.

::: browser/devtools/webide/content/webide.js
@@ +182,5 @@
> +    document.querySelector("#action-busy-determined").value = percent;
> +    this.postponeBusyTimeout();
> +  },
> +
> +  busy: function(showProgressBar) {

This should either be a new function name (maybe |busyWithProgress|), or use an object so there's a name for the value when you call |busy({progress: true})|.

Unnamed boolean arguments are confusing and we should avoid them if we can.

@@ +187,5 @@
>      this.hidePanels();
> +    let win = document.querySelector("window");
> +    win.classList.add("busy")
> +    if (showProgressBar) {
> +      win.classList.add("busy-determined");

When you first display a progress bar, it's probably best to force the value to 0.  I was seeing a number of progress bars that appeared full from the start and then went away, so making sure the value is reset first should help I think.

@@ +212,5 @@
> +      this._busyPromise.reject("promise timeout: " + this._busyOperationDescription);
> +    }, 30000);
> +  },
> +
> +  postponeBusyTimeout: function() {

This seems redundant since |setupBusyTimeout| also cancels first.

@@ +221,5 @@
> +  cancelBusyTimeout: function() {
> +    clearTimeout(this._busyTimeout);
> +  },
> +
> +  busyUntil: function(promise, operationDescription, showProgressBar) {

Same thing here as with |busy|, either a new function name, or use a object so the boolean argument can be named.

::: browser/devtools/webide/modules/app-manager.js
@@ +113,5 @@
>      this.update("connection");
>    },
>  
> +  onInstallProgress: function(event, bytesSent, totalBytes) {
> +    this.update("install-progress", {bytesSent: bytesSent, totalBytes: totalBytes});

I've changed my patch to emit this object directly, instead of two separate number arguments.  The object form seems like a better way to me.  So, you can just pass the object straight through now.

::: browser/devtools/webide/themes/webide.css
@@ +27,5 @@
>  }
>  
>  window.busy .action-button,
> +window:not(.busy) #action-busy,
> +window.busy.busy-undetermined #action-busy-determined,

The |.busy| part of this line and the next seems technically unnecessary, since you have |.busy-(un)determined| also.  But if it makes you happier this way, keep it. :)

Comment 12

[Paul Rouget [:paul]]

Attachment: v2

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8447105 [details] [diff] [review]
v2

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

::: browser/devtools/webide/content/webide.js
@@ +184,5 @@
> +    progress.value = percent;
> +    this.setupBusyTimeout();
> +  },
> +
> +  busy: function(showProgressBar) {

Seems like you can remove this parameter now.

@@ +863,5 @@
>    play: function() {
>      switch(AppManager.selectedProject.type) {
>        case "packaged":
>        case "hosted":
> +        return UI.busyWithProgressUntil(AppManager.installAndRunProject(), "installing and running app");

At the moment, there are actually only progress events for packaged apps (not hosted).

Comment 14

[Paul Rouget [:paul]]

Attachment: v2

Comment 15

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/1384e5d7b1f3

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1384e5d7b1f3