Closed Bug 972069 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools Graveyard :: WebIDE, defect)

28 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: nick, Assigned: paul)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
How about a progress indicator since the app size is known in advance?
(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.)?
(In reply to J. Ryan Stinnett [:jryans] from comment #3)
> Perhaps next to the project action buttons (Update / Debug / etc.)?

Sounds perfect.
Just FYI, in bug 919502 (which I'm supposed to be working on), I introduced a spinner.
Blocks: 971719
Not working on this for now.  Seems best to address this with UI v2.
Assignee: jryans → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1021491
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8445825 - Flags: review?(jryans)
Duplicate of this bug: 1021528
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. :)
Attachment #8445825 - Flags: review?(jryans)
Attached patch v2 (obsolete) — Splinter Review
Attachment #8445825 - Attachment is obsolete: true
Attachment #8447105 - Flags: review?(jryans)
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).
Attachment #8447105 - Flags: review?(jryans) → review+
Attached patch v2Splinter Review
Attachment #8447105 - Attachment is obsolete: true
Attachment #8447960 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1384e5d7b1f3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.