883959 - Show Download Progress via Circular Progressbar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Downloads, defect)

Comment 1

[Marina Samuel [:emtwo]]

Attachment: v1: Show circular download progress bar.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: v1: Rework Download Progress

Two main things in this patch:
* Added _downloadsInProgress so that _progressNotificationInfo can keep track of completed downloads until all downloads happening together are completed. Then it's cleared.
* put cancelling code after manager.cancelDownload() in the try{} block since it may fail if a download is already done

Comment 3

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 779813 [details] [diff] [review]
v1: Rework Download Progress

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

This all looks good to me so far. Lets make sure we test and handle funky cases like aborted/interrupted downloads, missing files.

::: browser/metro/base/content/downloads.js
@@ +116,5 @@
> +      this._progressNotificationInfo.delete(aDownload.guid);
> +      this._runDownloadBooleanMap.delete(aDownload.targetFile.path);
> +      this._downloadCount--;
> +      this._downloadsInProgress--;
> +      if (this._downloadsInProgress == 0) {

the pessimist in me wants this check to be against <= 0, dont ask me how you'd get there, its just doesn't hurt

Comment 4

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 779473 [details] [diff] [review]
v1: Show circular download progress bar.

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

Looks like a great start. I'd like to see this bundled up in an XBL binding or similar so it can be more easily tested in isolation. Right now you need to go find files to download to see it in action - if we can build a mochitest for it (see browser_tiles.xul, browser_tiles.js) we can iterate faster and be more assured that all bases are covered.

::: browser/metro/base/content/browser.xul
@@ +295,5 @@
>            <toolbarbutton id="star-button" class="appbar-primary" type="checkbox" oncommand="Appbar.onStarButton()"/>
>            <toolbarbutton id="pin-button" class="appbar-primary" type="checkbox" oncommand="Appbar.onPinButton()"/>
>            <toolbarbutton id="menu-button" class="appbar-primary" oncommand="Appbar.onMenuButton(event)"/>
> +
> +          <html:canvas id="downloadProgressBar" width="46" height="46"></html:canvas>

It would be nice to move this to anonymous content in a binding. That would also get positional specifics out of the CSS which will make life easier as we move things around

::: browser/metro/base/content/downloads.js
@@ +256,5 @@
>      this.showNotification("download-complete", message, buttons,
>        this._notificationBox.PRIORITY_WARNING_MEDIUM);
>    },
>  
> +  _updateCircularProgressMeter: function dv_updateCircularProgressMeter() {

We talked about possibly using a binding (extending toolbarbutton?) to get all this a bit better encapsulated. Ideally you would just set progress=n% or something like that from the download progress observer. 
We may need to animate a transition between steps, depending on the granularity of the progress data we get.

Comment 5

[Marina Samuel [:emtwo]]

Attachment: WIP v2: Show circular download progress bar.

Simply added the binding here but no other changes. Inheriting from toolbarbutton didn't quite work but I think this way is ok. Just checking to see if this is looking more like what we want. Tests to come.

Comment 6

[Marina Samuel [:emtwo]]

Attachment: v3: Show circular download progress bar.

* Added some very basic tests (open to other test ideas, just wasn't sure what else could be tested)

* Looked into reusing the page loading code but there's a lot of very specific stuff happening in that code (e.g. "Elements.progress.style.width = ...") so I think I'd want to leave recycling the code there for another bug if we still really want to do that.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 781019 [details] [diff] [review]
v3: Show circular download progress bar.

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

Broadly, looks good. The binding and the simple updateProgress API it provides mean that you can do some re-naming/refactoring here that I think will aid reuse and generally clarify what is used where & how.

In terms of actual result, it seems like the progress bounces around a lot and doesn't really correspond to anything like bytes downloaded or % remaining. I've looked at the downloads update data to know if that's an accurate representation of the data delivered or a bug, do you know?

Do follow up with shorlander for that image asset. If you create a really ugly placeholder image at the location you need and the dimensions you need then you or anyone can swap it out later. (and everyone will be clear that its a placeholder and UX will be motivated to fix :)

::: browser/metro/base/content/bindings/circularprogress.xml
@@ +10,5 @@
> +          xmlns:html="http://www.w3.org/1999/xhtml">
> +  <binding id="circular-progress-indicator">
> +    <content>
> +      <xul:stack>
> +        <xul:toolbarbutton anonid="download-button" class="appbar-secondary" xbl:inherits="oncommand"/>

this binding is all about displaying progress in a circular/ring-shaped overlay. It knows nothing of downloads, so this anonid can simply be "button".

@@ +11,5 @@
> +  <binding id="circular-progress-indicator">
> +    <content>
> +      <xul:stack>
> +        <xul:toolbarbutton anonid="download-button" class="appbar-secondary" xbl:inherits="oncommand"/>
> +        <html:canvas anonid="downloadProgressBar" width="46" height="46"></html:canvas>

Same here, maybe "progressRing", or something?

@@ +31,5 @@
> +            let startAngle = 1.5 * Math.PI;
> +            let endAngle = startAngle + (2 * Math.PI * percentComplete);
> +
> +            let ctx = this._progressCircleCtx;
> +            ctx.clearRect(0, 0, 46, 46);

Rather than hard-code these values here, we could do this.getBoundingClientRect() - perhaps in the constructor to avoid re-measuring each call.

@@ +37,5 @@
> +            // Save the state, so we can undo the clipping
> +            ctx.save();
> +
> +            ctx.beginPath();
> +            ctx.arc(23, 23, 23, startAngle, endAngle, false);

I assume 23, 23 is the center of the canvas - lets calculate that so the bound node isn't locked to a particular size

@@ +55,5 @@
> +            return [startAngle, endAngle];
> +          ]]>
> +        </body>
> +      </method>
> +      <method name="clearCanvas">

Should be "resetProgress" or just "reset" I think. Consuming code doesn't need to know we use a canvas for the progress rendering

::: browser/metro/base/content/downloads.js
@@ +197,5 @@
>          label: tryAgainButtonText,
>          accessKey: "",
>          callback: function() {
>            Downloads.manager.retryDownload(aDownload.id);
> +          Downloads._downloadProgressIndicator.clearCanvas();

See note in binding file about this method name

::: browser/metro/theme/browser.css
@@ +431,5 @@
>  #navbar[startpage] {
>    transform: none;
>  }
>  
> +[anonid="downloadProgressBar"] {

circularprogressindicator > [anonid="progressBar"] is an improvement I think.

@@ +664,5 @@
>  }
>  
>  /* Application-Specific */
>  
> +[anonid="download-button"] {

Ditto above with making selector clearer.

Comment 8

[Marina Samuel [:emtwo]]

Attachment: v4: Show circular download progress bar.

Addressed comments. In terms of the bouncing, it did not seem appropriate to always make sure the progress goes up since maybe there does exist a reason why it would not go up (e.g. adding another download makes total progress go down since there's a larger total bytes to download). Instead I looked at how desktop does it and they were using a "percentComplete" attribute from nsIDownload instead of computing the percent, so I used this. Hopefully this is more accurate and less bouncy.

In case this still fails, I filed bug 899072.

Comment 9

[Marina Samuel [:emtwo]]

Attachment: v5: Show circular download progress bar.

Forgot to remove debug statements.

Comment 10

[Marina Samuel [:emtwo]]

I also wanted to note that for the patch "v1: Rework Download Progress", I'm currently working on tests to show we are in the correct state when downloads are cancelled/aborted. However, in the interest of shipping this bug, would we be able to move over this work to bug 895053?

I've manually verified that attempting to cancel a completed download or one with a missing file works correctly. Would that be sufficient for now?

Comment 11

[Marina Samuel [:emtwo]]

Attachment: v6: Show circular download progress bar.

Added progress circle image

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 782799 [details] [diff] [review]
v6: Show circular download progress bar.

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

I'm seeing the ring progress in -metrodesktop mode, but not in immersive mode with this patch. I did spot an exception being thrown where (with a little logging) I got this: 

updateDownload, this._progressNotification: [object XULElement]
progressMeter, adding %total: 4
progressMeter, update with percentComplete %total: 4
JavaScript strict warning: chrome://browser/content/bindings/circularprogress.xml, line 54: reference to undefined property this._img
JavaScript error: chrome://browser/content/bindings/circularprogress.xml, line 5
4: Argument 1 of CanvasRenderingContext2D.drawImage could not be converted to any of: HTMLImageElement, HTMLCanvasElement, HTMLVideoElement.
updateDownload, this._progressNotification: [object XULElement]

It looks like fixing "this" in that onload callback should take care of most if not all of that, but you should verify. 
That should be it though, everything else is looking good to me.

::: browser/metro/base/content/bindings/circularprogress.xml
@@ +10,5 @@
> +          xmlns:html="http://www.w3.org/1999/xhtml">
> +  <binding id="circular-progress-indicator">
> +    <content>
> +      <xul:stack>
> +        <xul:toolbarbutton anonid="progressButton" class="circularprogrssindicator-progressButton appbar-secondary"/>

nit: not sure if "circularprogrssindicator" (vs circularprogressindicator) is a typo but we may as well spell it out.

@@ +18,5 @@
> +    <implementation>
> +      <field name="_progressCanvas">
> +          document.getAnonymousElementByAttribute(this, "anonid", "progressRing");
> +      </field>
> +      <constructor>

Can you add fields (null initial value) for _progressCircleCtx and _img.

@@ +28,5 @@
> +      <method name="updateProgress">
> +        <parameter name="percentComplete"/>
> +        <body>
> +          <![CDATA[
> +            const DOWNLOAD_RING_IMG = "chrome://browser/skin/images/progresscircle.png";

nit: PROGRESS_RING_IMG

@@ +48,5 @@
> +            ctx.closePath();
> +            ctx.clip();
> +
> +            // Draw circle image.
> +            if (!this._img.src) {

If you reversed the logic, so it goes if (this._img && this._img.src) { .. } else { ...assign .src } this would be easier to follow I think

@@ +49,5 @@
> +            ctx.clip();
> +
> +            // Draw circle image.
> +            if (!this._img.src) {
> +              this._img.onload = function() {

need to .bind(this) for this._img to resolve in the callback

::: browser/metro/base/content/downloads.js
@@ +257,5 @@
>    },
>  
> +  _updateCircularProgressMeter: function dv_updateCircularProgressMeter() {
> +    let totPercent = 0;
> +    for (let info of this._progressNotificationInfo) {

Is there any case when _progressNotificationInfo might be null? If we cant rule that out, lets check it meets our expectations and early return if not.

@@ +258,5 @@
>  
> +  _updateCircularProgressMeter: function dv_updateCircularProgressMeter() {
> +    let totPercent = 0;
> +    for (let info of this._progressNotificationInfo) {
> +      totPercent += info[1].download.percentComplete;

Nit: Can you add a comment here on what the info structure is and why we need the 1th entry?

Comment 13

[Sam Foster [:sfoster] (he/him)]

Re: test, yes lets track those in that other bug. I'm fine as long as we don't lose sight of it.

Comment 14

[Marina Samuel [:emtwo]]

Attachment: v7: Show circular download progress bar.

Addressed comments. Thanks for your patience, Sam! Somehow the progress indicator showed up for me in immersive mode and I totally missed the bind(this) issue. Very odd.

Comment 15

[Marina Samuel [:emtwo]]

Attachment: v2: Rework Download Progress

Made suggested change + added placeholder for relevant test.

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 783165 [details] [diff] [review]
v7: Show circular download progress bar.

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

::: browser/metro/base/tests/mochitest/browser_circular_progress_indicator.js
@@ +1,1 @@
> +let doc;

Please add a license header.  The standard MPL boilerplate is fine for this file.  (For web API tests that are re-usable by other browser vendors we use Public Domain, but that's not generally useful for our browser-chrome tests.)

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 783165 [details] [diff] [review]
v7: Show circular download progress bar.

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

Looks great, works well. Good to go with the nit and license header.

::: browser/metro/base/content/downloads.js
@@ +256,5 @@
>        this._notificationBox.PRIORITY_WARNING_MEDIUM);
>    },
>  
> +  _updateCircularProgressMeter: function dv_updateCircularProgressMeter() {
> +    if (this._progressNotificationInfo == null) {

nit: maybe just if (!this._progressNotificationInfo)

Comment 18

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 783178 [details] [diff] [review]
v2: Rework Download Progress

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

Looks good to me.

Comment 19

[Marina Samuel [:emtwo]]

Pushed to fx-team

https://hg.mozilla.org/integration/fx-team/rev/9ac580e78e10
https://hg.mozilla.org/integration/fx-team/rev/9aac8dc7a9b9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ac580e78e10
https://hg.mozilla.org/mozilla-central/rev/9aac8dc7a9b9

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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