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 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 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.

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.

* 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 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.

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.

Forgot to remove debug statements.

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?

Added progress circle image

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?

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

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.

Made suggested change + added placeholder for relevant test.

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 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 on attachment 783178 [details] [diff] [review] v2: Rework Download Progress Review of attachment 783178 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.

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

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