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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

Trunk
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
Flags: qe-verify+
QA Contact: gwimberly
Whiteboard: [photon-structure] → [reserve-photon-structure]
(Assignee)

Updated

a year ago
Duplicate of this bug: 1398358
(Assignee)

Updated

a year ago
Duplicate of this bug: 1398468
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: P3 → P1
Iteration: --- → 57.3 - Sep 19

Comment 4

a year ago
mozreview-review
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?
Attachment #8907697 - Flags: review?(mak77)
Iteration: 57.3 - Sep 19 → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
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

a year ago
mozreview-review
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.
Attachment #8907697 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 9

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

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
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 hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
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.
Attachment #8907697 - Flags: review?(paolo.mozmail)

Comment 14

a year ago
mozreview-review
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
Attachment #8907697 - Flags: review+
Comment hidden (mozreview-request)

Comment 16

a year ago
Pushed by mdeboer@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10e6c06f04d9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
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.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified

Comment 19

a year ago
Does this want beta 57 uplift? Does the fix for bug 1398346 actually work on beta without this bug?
Flags: needinfo?(paolo.mozmail)

Comment 20

a year ago
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
Flags: needinfo?(paolo.mozmail)
Attachment #8907697 - Flags: approval-mozilla-beta?
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+
Attachment #8907697 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/adf452ba200a
status-firefox57: affected → fixed
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.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.