1353374 - Download progress bar is not showing on the Taskbar button

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Alice0775 White]

STR:
Download something (e.g http://archive.mozilla.org/pub/firefox/nightly/2017/04/2017-04-04-03-02-04-mozilla-central/firefox-55.0a1.en-US.win32.zip )

AR:
Download progress bar is not showing on the Taskbar button

ER:
Download progress bar should be showing on the Taskbar button

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=922e2f268ab3f18834a38c41fe55b10ed1fc6250&tochange=ba78fb79365f3bcff0575568846fde62f0e0b8a0

Regressed by: Bug 1264206

Comment 1

[Mark Banner (:standard8)]

Unfortunately I don't have a Windows environment to look at this at the moment, and there's no errors on the console. My suspicions would be:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba78fb79365f#l4.64

or maybe 

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba78fb79365f#l5.24

or 

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba78fb79365f#l5.45

Most of the rest of the patch is whitespace changes.

Comment 2

[Alice0775 White]

Reverting this change fixes the problem.

@@ -106,17 +104,16 @@ this.DownloadsTaskbar = {
     if (!this._summary) {
       Downloads.getSummary(Downloads.ALL).then(summary => {
         // In case the method is re-entered, we simply ignore redundant
         // invocations of the callback, instead of keeping separate state.
         if (this._summary) {
           return;
         }
         this._summary = summary;
-        return this._summary.addView(this);
       }).then(null, Cu.reportError);
     }
   },



(BTW, Mozilla should not change the codes without automated test!)

Comment 3

[Marco Bonardo [:mak]]

it makes sense, the review comment there was to "drop the return part", that should have only removed the "return", not the whole line.

Comment 4

[Mark Banner (:standard8)]

I'll fix this in a bit. Obviously I missed that wasn't addressed correctly.

(In reply to Alice0775 White from comment #2)
> (BTW, Mozilla should not change the codes without automated test!)

Unfortunately when doing large clean-ups like this, it isn't always possible to know what is and isn't tested. There's still a large amount of untested code, and whilst we try to add new tests whenever possible this isn't always practical on the larger refactors/improvements.

Comment 5

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #3)
> it makes sense, the review comment there was to "drop the return part", that
> should have only removed the "return", not the whole line.

And the "return" is needed, so the line should just be restored as it was.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1353374 - Fix regression where the taskbar button wasn't showing the download progress.

Put back in line that was incorrectly removed previously, and add a test.

Review commit: https://reviewboard.mozilla.org/r/126826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126826/

Comment 7

[:Paolo Amadini]

Comment on attachment 8854878 [details]
Bug 1353374 - Fix regression where the taskbar button wasn't showing the download progress.

r=me for landing the DownloadsTaskbar.jsm changes, but the rest has to wait until tomorrow at least... thanks for adding the test!

Comment 8

[Mark Banner (:standard8)]

(In reply to :Paolo Amadini from comment #7)
> Comment on attachment 8854878 [details]
> Bug 1353374 - Fix regression where the taskbar button wasn't showing the
> download progress.
> 
> r=me for landing the DownloadsTaskbar.jsm changes, but the rest has to wait
> until tomorrow at least... thanks for adding the test!

I think it can wait a day. For now I've pushed to try as I want to make sure the test is stable.

Comment 9

[Marco Bonardo [:mak]]

(In reply to :Paolo Amadini from comment #5)
> And the "return" is needed, so the line should just be restored as it was.

you mean for eslint? the return value doesn't seem to be used. I mean, inverting the previous if would probably avoid both returns and make eslint happy.  Btw, this is really NOT IMPORTANT! I am just asking out of curiosity.

Comment 10

[:Paolo Amadini]

No, it's needed because addView returns a Promise, and we need to return it from the then() handler function in order for it to be chained correctly. If addView fails and the Promise is not returned, this would lead to an unhandled rejection.

Comment 11

[Marco Bonardo [:mak]]

ah, right, that makes sense. Thank you.

Comment 12

[:Paolo Amadini]

Comment on attachment 8854878 [details]
Bug 1353374 - Fix regression where the taskbar button wasn't showing the download progress.

https://reviewboard.mozilla.org/r/126826/#review130370

The Downloads API makes mo guarantees that progress methods are called in a certain order, or even called at all. For example, if a download is really fast then we might only notify the final state, and if it is the same as the initial state, we might not send any notification at all to the views. Any test relying on current internal timing or on assumptions based on the current implementation of the API might become intermittent at any time.

Unfortunately, this means that the test has to be rewritten. (It's fine if you don't want to do this now, you can land the fix without the test.)

This should be written using the model of the xpcshell tests that check progress reporting in the "jsdownloads" folder:

- Register an interruptible HTTP response handler
- Start the download
  - No need to use the drag-and-drop code to do this, by the way
- Wait until the midway notification of progress
  - Don't use waitForCondition, but have the mocked function resolve a Promise when called with the right conditions
  - This is guaranteed to happen because the response will not continue past this midway point
- Unblock the response so the download can finish
- Wait until both of these happen:
  - The final notification of progress
  - The download has completed
- Clean up

As usual, you have to set up the listeners before triggering the event that will cause them to be invoked.

This likely means porting some of the support functions from "jsdownloads" to here. You may also consider moving the test set-up helper functions to a test-only "DownloadsTestUtils.jsm" in the "jsdownloads" folder, that can easily be referenced from here.

Comment 13

[Mark Banner (:standard8)]

Ok, thank you for the comments. Given my current workloads, I've split this out into bug 1353374, and I'll get the regression fix landed.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8854878 [details]
Bug 1353374 - Fix regression where the taskbar button wasn't showing the download progress.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126826/diff/1-2/

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8854878 [details]
Bug 1353374 - Fix regression where the taskbar button wasn't showing the download progress.

https://reviewboard.mozilla.org/r/126826/#review130852

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bad5caedbb0
Fix regression where the taskbar button wasn't showing the download progress. r=mak

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/6bad5caedbb0

Comment 18

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 55.0a1 (Build ID: 20170404030204) on 
Windows 10, 64-bit.

Verified as fixed with Latest Firefox Nightly 55.0a1 (Build ID: 20170421030241)
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 19

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx55.0a1, build ID: 20170404030204, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx55.0b3, on Windows 10 x64.

Cheers!