Allow DirPickerBuildFileListTasks to be cancelled, and cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory

RESOLVED FIXED in mozilla27

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
DirPickerBuildFileListTasks can be long-lived. We need to cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory and kicks off a new DirPickerBuildFileListTask for that directory.
(Assignee)

Comment 1

5 years ago
Created attachment 813950 [details] [diff] [review]
part 1 - rename DirPickerBuildFileListTasks to DirPickerFileListBuilderTask
Attachment #813950 - Flags: review?(bugs)
(Assignee)

Comment 2

5 years ago
Created attachment 813951 [details] [diff] [review]
part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory

To avoid races between the cancelling and the setting of the progress counter (i.e. having mLastFileListProgress set on the HTMLInputElement by a discarded DirPickerFileListBuilderTask) I moved the progress counter to the task object.
Attachment #813951 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Blocks: 846931
(Assignee)

Comment 3

5 years ago
Created attachment 814109 [details] [diff] [review]
part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory
Attachment #813951 - Attachment is obsolete: true
Attachment #813951 - Flags: review?(bugs)
Attachment #814109 - Flags: review?(bugs)

Updated

5 years ago
Attachment #813950 - Flags: review?(bugs) → review+

Comment 4

5 years ago
Comment on attachment 814109 [details] [diff] [review]
part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory


>     // Now back on the main thread, set the list on our HTMLInputElement:
>-    if (mFileList.IsEmpty()) {
This is old code, but I don't understand why we don't dispatch change event.
If we opened and closed the filepicker, we cleared filelist, but then
here we don't necessarily fire change event.
We need to fire change at some point.

r-, but perhaps r- should go to the patch clearing filelist, if we should actually
dispatch change event at that point. It would be at least consistent - always dispatch change
when filelist changes.
Attachment #814109 - Flags: review?(bugs) → review-
(Assignee)

Comment 5

5 years ago
(In reply to Olli Pettay [:smaug] from comment #4)
> r-, but perhaps r- should go to the patch clearing filelist

Yeah, as discussed on IRC let's r- bug 912492 instead for now.
(Assignee)

Updated

5 years ago
Attachment #814109 - Flags: review- → review?(bugs)

Comment 6

5 years ago
Comment on attachment 814109 [details] [diff] [review]
part 2 - Cancel any in-progress DirPickerBuildFileListTasks when the user picks a new directory

>   NS_IMETHOD Run() {
>     if (!NS_IsMainThread()) {
>       // Build up list of nsDOMFileFile objects on this dedicated thread:
>       nsCOMPtr<nsISimpleEnumerator> iter =
>         new DirPickerRecursiveFileEnumerator(mTopDir);
>       bool hasMore = true;
>       nsCOMPtr<nsISupports> tmp;
>       while (NS_SUCCEEDED(iter->HasMoreElements(&hasMore)) && hasMore) {
>         iter->GetNext(getter_AddRefs(tmp));
>         nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(tmp);
>         MOZ_ASSERT(domFile);
>         mFileList.AppendElement(domFile);
>-        mInput->SetFileListProgress(mFileList.Length());
>+        if (mCanceled) {
Please assert that mInput is null here

>+  void Cancel() {
{ should be in the next line

>+  uint32_t GetFileListLength() const {
ditto

>+    return mFileList.Length();
>+  }
Hmm, so you end up accessing mFileList on several threads.
Not too happy with that. May lead to some odd behavior.
Could you please use Atomic to store the length.

You should use Atomic for mCanceled too.
(Not sure we support bool, but uint32_t should work.)
Attachment #814109 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

5 years ago
Oh man, clearly I shouldn't be pushing stuff before 9am on a Monday. Just realized that comment 6 was an unconditional r-, not a conditional r+.

That said this is all disabled behind a pref, so I'll err on the side of not churning the code unnecessarily and ping you on IRC about addressing any remaining issues you might have as a follow-up, Smaug.

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/271b0129916d
https://hg.mozilla.org/mozilla-central/rev/aa9527b63f67
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Updated

5 years ago
Depends on: 926330
You need to log in before you can comment on or make changes to this bug.