1395615 - Implement the "file moved or missing" check in download item onChange handlers

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

In bug 1354532 the Downloads panel subview (located in the Library panel) was implemented, with the remark that the onChanged handler needs more sophisticated handling of the 'file moved or missing' check.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

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

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

https://reviewboard.mozilla.org/r/179364/#review185728

This review would have fit Paolo much better, I'm a bit rusty on how this code evolved recently :( Btw, I tried to follow most of the code and I have a doubt to be verified below.

::: browser/components/downloads/DownloadsSubview.jsm:108
(Diff revision 1)
>        waitForMs = 0;
>      }
>      // Wait a wee bit to dispatch the event, because another batch may start
>      // right away.
> -    this._batchTimeout = window.setTimeout(() =>
> -      this.panelview.dispatchEvent(new window.CustomEvent("DownloadsLoaded")), waitForMs);
> +    this._batchTimeout = window.setTimeout(() => {
> +      this._scheduleRefresh();

The method name could probably improve a bit, since refresh is really generic.
Maybe even just updateStatsFromDisk (the scheduling is an implementation detail).
Something that makes clear what is going to happen.

::: browser/components/downloads/DownloadsSubview.jsm:166
(Diff revision 1)
> +    if (this._refreshing)
> +      return;
> +
> +    this._refreshing = true;
> +    // Start with getting an idle moment to (maybe) refresh the list of downloads.
> +    await new Promise(resolve => this.window.requestIdleCallback(resolve));

Do we care about having a timeout here?
If the user opens the panel we should present a consistent situation in a meaningful time. I suppose there are the user events to handle to the idle callback may not happen "soon" enough.
What about a 500ms timeout? Thought?

::: browser/components/downloads/DownloadsSubview.jsm:172
(Diff revision 1)
> +    // In the meantime, this instance could have been destroyed, so take note.
> +    if (this.destroyed)
> +      return;
> +
> +    let count = 0;
> +    for (let button of [...this.container.childNodes]) {

I don't think you need [...] at all, childNodes is iterable and for of should not have issues with it

Actually, it could be worth filinga bug for an eslint rule that disallows spreading in for of for known iterables... not sure, just a thought.

::: browser/components/downloads/DownloadsSubview.jsm:182
(Diff revision 1)
> +
> +      await button._shell.refresh();
> +
> +      // Make sure to request a new idle moment every `kRefreshBatchSize` buttons.
> +      if (++count % kRefreshBatchSize === 0) {
> +        await new Promise(resolve => this.window.requestIdleCallback(resolve));

same question about a timeout, but if the initial batch is large enough to cover the first downloads in the view, here it's less important.

::: browser/components/downloads/DownloadsSubview.jsm:186
(Diff revision 1)
> +      if (++count % kRefreshBatchSize === 0) {
> +        await new Promise(resolve => this.window.requestIdleCallback(resolve));
> +      }
> +    }
> +
> +    this._refreshing = false;

I'd feel better if this would be unset in a finally clause.

::: browser/components/downloads/DownloadsSubview.jsm:401
(Diff revision 1)
>  
>    /**
>     * Handler method; invoked when any state attribute of a download changed.
>     */
>    onChanged() {
>      // TODO: implement "file moved or missing" check - bug 1395615.

comment to remove

::: browser/components/downloads/DownloadsSubview.jsm:410
(Diff revision 1)
>        this.onStateChanged();
>      } else {
>        this._updateState();
>      }
>  
> +    if (this.download.succeeded) {

If I'm not wrong, the above onStateChanged() will set this._targetFileChecked = false; and invoke this._updateState(); that invokes super._updateState(); that invokes this._updateProgress(); that does the same here
http://searchfox.org/mozilla-central/rev/184f0c7888dd6abb32235377693b7d1fc0b75ac1/browser/components/downloads/DownloadsViewUI.jsm#163

So looks like we are doing this twice and it's unnecessary?
Could you please check?

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

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

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

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

Comment 7

[Mike de Boer [:mikedeboer]]

Thanks Marco, for that first review pass - it was super helpful!! I've asked Paolo for the next round, if you're OK with that...

Comment 8

[:Paolo Amadini]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

https://reviewboard.mozilla.org/r/179364/#review188010

I have some general comments, aimed at trying not to regress performance when using this view.

::: browser/components/downloads/DownloadsSubview.jsm:110
(Diff revision 3)
> -      this.panelview.dispatchEvent(new window.CustomEvent("DownloadsLoaded")), waitForMs);
> +      this._updateStatsFromDisk().then(() => {
> +        this.panelview.dispatchEvent(new window.CustomEvent("DownloadsLoaded"));
> +      });

I think we shouldn't wait for the state of all downloads to be updated from disk before showing the subview, as this can take some time and make the subview feel unresponsive. There is already a noticeable delay when the view is first opened because we need to load the list of downloads from disk and query the Places database.

So, it's probably better to show the view as soon as possible, then accept a flicker in the visible state when we get the responses from the file system. Normally, in fact, the state won't change as the files would still be present.

We should also try to avoid hijacking the disk I/O by adding small delays between each access. Ideally we should also stop as soon as we reach downloads that are not currently visible, and restart when they are scrolled into view, but this can be done in a follow-up given that we limit the number of downloads that we show here.

We should also figure out if we should repeat the checks when the view is reopened, or on mouse hover, because the files may have been removed after the view has been opened once. This is what the Downloads Panel does, but the performance concerns are lessened there, because the panel is limited to 5 downloads.

Comment 9

[Mike de Boer [:mikedeboer]]

Waaaah! This is exactly what I implemented in the previous version!! LOL

Comment 10

[:Paolo Amadini]

Well, to be fair, it seems one of the concerns with the previous version was that the requestIdleCallback was unbound... but yes, just with regard to the speed of opening the subview, the previous version did it immediately, which I think is better.

Comment 11

[Mike de Boer [:mikedeboer]]

Yeah, somehow I thought that the changes I had to make between the two versions were much bigger, whereas in fact they were marginal :P
Updated patch coming up!

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179364/diff/3-4/

Comment 13

[:Paolo Amadini]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

https://reviewboard.mozilla.org/r/179364/#review188722

Looks good!

::: browser/components/downloads/DownloadsSubview.jsm:169
(Diff revision 4)
> +    let idleOptions = { timeout: kMaxWaitForIdleMs };
> +    // Start with getting an idle moment to (maybe) refresh the list of downloads.
> +    await new Promise(resolve => this.window.requestIdleCallback(resolve), idleOptions);
> +    // In the meantime, this instance could have been destroyed, so take note.
> +    if (this.destroyed)
> +      return;

Even though it's unlikely to fail, this could go inside the "try" block for correctness.

Comment 14

[:Paolo Amadini]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

https://reviewboard.mozilla.org/r/179364/#review188724

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179364/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10e6c06f04d9
Implement the "file moved or missing" check for download items in the Library Downloads subview. r=Paolo

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/10e6c06f04d9

Comment 18

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170929100122

This issue has been verified on latest Firefox Nightly Build ID: 20170929100122 on Windows 8.1 x64, Mac OS 10.12 and Ubuntu 14.04 and it is no longer reproducible. If a file was downloaded and moved to another location then, when opening Downloads section from Library the "File moved or missing" message is displayed under the item.

Comment 19

[:Gijs (he/him)]

Does this want beta 57 uplift? Does the fix for bug 1398346 actually work on beta without this bug?

Comment 20

[:Paolo Amadini]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1354532
[User impact if declined]: In the new Downloads Subview in the Library Panel, clicking on downloads whose files have been removed will have no effect.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: As per comment 18
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Limited risk
[Why is the change risky/not risky?]: Only code related to the feature is changed
[String changes made/needed]: None

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8907697 [details]
Bug 1395615 - Implement the "file moved or missing" check for download items in the Library Downloads subview.

Photon related recent regression, Beta57+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/adf452ba200a

Comment 23

[Ciprian Georgiu, Desktop QA]

This bug is also verified fixed on 57 Beta 7 (20171009192146) across platforms: Windows 10 x64, Mac OS X 10.11 and Ununtu 16.04 x64.