1350933 - Label dom/filesystem

Status: RESOLVED FIXED

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

baku, could you take a look?

Bug 1321812 has the background information.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - Runnable

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - DispatchTo{Current,Main}Thread

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 3 - Actor

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - actor

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/9d0d8e859a64
https://hg.mozilla.org/mozilla-central/rev/611184e06d80
https://hg.mozilla.org/mozilla-central/rev/a269457f31c9