Closed Bug 1350933 Opened 3 years ago Closed 3 years ago

Label dom/filesystem

Categories

(Core :: DOM: Core & HTML, enhancement)

50 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: smaug, Assigned: baku)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
baku, could you take a look?

Bug 1321812 has the background information.
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8852042 - Flags: review?(wmccloskey)
Attachment #8852044 - Flags: review?(wmccloskey)
Attached patch part 3 - Actor (obsolete) — Splinter Review
Attachment #8852046 - Flags: review?(wmccloskey)
Attached patch part 3 - actorSplinter Review
Note that I cannot call SetEventTargetForActor() if the API is used in workers otherwise I receive a MOZ_CRASH() complaining that the workerID doesn't match with the ID of the IPC message.
Attachment #8852046 - Attachment is obsolete: true
Attachment #8852046 - Flags: review?(wmccloskey)
Attachment #8852050 - Flags: review?(wmccloskey)
See Also: → 1331568
See Also: 1331568
Attachment #8852042 - Flags: review?(wmccloskey) → review+
Comment on attachment 8852044 [details] [diff] [review]
part 2 - DispatchTo{Current,Main}Thread

Review of attachment 8852044 [details] [diff] [review]:
-----------------------------------------------------------------

There's a lot of duplication of the GetParentObject/EventTargetFor/Dispatch code. Can you instead factor it into a common function or something? Or maybe a method on FileSystemEntry (although that wouldn't cover everything)?

::: dom/filesystem/compat/FileSystemDirectoryReader.cpp
@@ +103,5 @@
> +      nsIEventTarget* target =
> +        mParentEntry->GetParentObject()->EventTargetFor(TaskCategory::Other);
> +      MOZ_ASSERT(target);
> +
> +      target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

You're missing the NS_WARNING_ASSERTION here.

@@ +167,5 @@
> +    nsIEventTarget* target =
> +      GetParentObject()->EventTargetFor(TaskCategory::Other);
> +    MOZ_ASSERT(target);
> +
> +    target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

Same.

::: dom/filesystem/compat/FileSystemRootDirectoryEntry.cpp
@@ +118,5 @@
> +      nsIEventTarget* target =
> +        GetParentObject()->EventTargetFor(TaskCategory::Other);
> +      MOZ_ASSERT(target);
> +
> +      target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

You're missing the NS_WARNING_ASSERTION here.
Attachment #8852044 - Flags: review?(wmccloskey) → review+
Comment on attachment 8852050 [details] [diff] [review]
part 3 - actor

Review of attachment 8852050 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/filesystem/FileSystemTaskBase.cpp
@@ +167,4 @@
>    if (HasError()) {
>      // In this case we don't want to use IPC at all.
>      RefPtr<ErrorRunnable> runnable = new ErrorRunnable(this);
> +    target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL);

NS_WARNING_ASSERTION

@@ +194,5 @@
>      mozilla::ipc::BackgroundChild::GetForCurrentThread();
>    MOZ_ASSERT(actor);
>  
> +  if (NS_IsMainThread()) {
> +    aActor->SetEventTargetForActor(this, target);

This confused me. Is |aActor| always equal to |actor|? Can we just get rid of |actor|?

::: dom/filesystem/GetFileOrDirectoryTask.cpp
@@ +51,5 @@
>  
>    return task.forget();
>  }
>  
> +GetFileOrDirectoryTaskChild::GetFileOrDirectoryTaskChild(nsIGlobalObject* aGlobalObject, 

Extra space at the end of the line.
Attachment #8852050 - Flags: review?(wmccloskey) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0d8e859a64
Labeling dom/filesystem - part 1 - Runnable, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/611184e06d80
Labeling dom/filesystem - part 2 - DispatchTo{Current,Main}Thread, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a269457f31c9
Labeling dom/filesystem - part 3 - IPC actors, r=billm
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.