New issue
Advanced search Search tips

Issue 782696 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 774564



Sign in to add a comment

Support the Event mechanism in WorkletGlobalScope

Project Member Reported by hongchan@chromium.org, Nov 8 2017

Issue description

During the implementation of MessagePort in AudioWorkletNode and AudioWorkletProcessor,
we found that V8AbstractEventListener cannot be used in WorkletGlobalScope.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp?sq=package:chromium&l=63

It is necessary to rearrange the class design so V8AbstractEventListener can support WorkerOrWorkletGlobalScope.

Related WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/747621
 
Blocking: 774564
Owner: nhiroki@chromium.org
Status: Started (was: Available)
I'll take a look.
Summary: Support the Event mechanism in WorkletGlobalScope (was: Support MessagePort in WorkletGlobalScope)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3a8139a4c20831d215c5bfcdfae38915d3dc418e

commit 3a8139a4c20831d215c5bfcdfae38915d3dc418e
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Thu Nov 09 22:18:48 2017

Worklet: Move EventListener stuff from WorkerGlobalScope to WorkerOrWorkletGlobalScope

This is a preparation CL for introducing MessagePort support in AudioWorklet.

AudioWorklet is the first worklet that will have the Event mechanism. To
implement it, this CL moves functions etc related to events from
WorkerGlobalScope to WorkerOrWorkletGlobalScope so that they are usable in both
workers and worklets.

Bug:  782696 
Change-Id: I96dbee4f58d90f71f813db42938b1b7f5277f3dd
Reviewed-on: https://chromium-review.googlesource.com/759438
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515317}
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.cpp
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/bindings/core/v8/V8AbstractEventListener.h
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkerEventQueue.h
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkerGlobalScope.h
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkerOrWorkletGlobalScope.cpp
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkerOrWorkletGlobalScope.h
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp
[modify] https://crrev.com/3a8139a4c20831d215c5bfcdfae38915d3dc418e/third_party/WebKit/Source/core/workers/WorkletGlobalScope.h

Status: Fixed (was: Started)
hongchan@: I think you can now resume your work. Please let me know if you need additional changes :)
Status: Assigned (was: Fixed)
Thanks for the fix! :)

But unfortunately I am having another issue:

1. WorkerOrWorkletGlobalScope::IsClosing() is supposed to be impl. by subclasses.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/WorkerOrWorkletGlobalScope.h?q=WorkerOrWorklet&sq=package:chromium&l=63

2. WorkletGlobalScope::IsClosing() always returns false.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/WorkletGlobalScope.h?sq=package:chromium&l=49

3. WorkerOrWorkletGlobalScope::DeregisterEventListener() fails at CHECKs:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/workers/WorkerOrWorkletGlobalScope.cpp?type=cs&sq=package:chromium&l=172

I'll add more issue as I investigate MessagePort integration on AudioWorkletGlobalScope.
Exposing MessagePort to AudioWorkletGlobalScope may implicate a bit more plumbing.

It exposes MessagePort, Event, EventTarget to AudioWorkletGlobalScope. (I changed HTML/DOM spec to reflect this.) Now it requires proper implementations of methods like Dispose(), IsClosing().

Not really familiar with the management of lifecycle or internal storage in WorkletGlobalScope, but I'll try to update this issue as much as possible.
Status: Started (was: Assigned)
I found another issue:

4. ToV8Context() (in V8BindingForCore.cpp) called from V8AbstractEventListener::handleEvent() returns an empty v8::Context when the current execution context is WorkletGlobalScope. Also, we need to replace ToWorkerGlobalScope() in V8WorkerGlobalScopeEventListener::HandleEvent() with ToWorkerOrWorkletGlobalScope().

Copied from https://chromium-review.googlesource.com/c/chromium/src/+/747621#message-c892c28b0e9511bebf678c375876a8f528f0f214
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cfad34cce2c9a4c54752bec654998a5f382f61c

commit 2cfad34cce2c9a4c54752bec654998a5f382f61c
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Wed Nov 15 08:30:41 2017

Worklet: Replace To/IsWorkerGlobalScope() with To/IsWorkerOrWorkletGlobalScope in Event code

This CL enables worklets to reuse the Event handler code for workers.

Tests will be added by a following CL:
https://chromium-review.googlesource.com/c/chromium/src/+/747621

Bug:  782696 
Change-Id: Idf5bf9553dccb9a6de4e064d4317f4d7ebdbb4e0
Reviewed-on: https://chromium-review.googlesource.com/770643
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516637}
[modify] https://crrev.com/2cfad34cce2c9a4c54752bec654998a5f382f61c/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.cpp
[modify] https://crrev.com/2cfad34cce2c9a4c54752bec654998a5f382f61c/third_party/WebKit/Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.cpp
[modify] https://crrev.com/2cfad34cce2c9a4c54752bec654998a5f382f61c/third_party/WebKit/Source/bindings/core/v8/V8WorkerGlobalScopeEventListener.h

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd5e88dc00881e051bc6cd650a493095d2a5ebb0

commit cd5e88dc00881e051bc6cd650a493095d2a5ebb0
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri Nov 17 05:10:12 2017

Worklet: Rename V8WorkerGlobalScopeEventListener to V8WorkerOrWorkletEventListener

This is a follow-up CL for https://chromium-review.googlesource.com/c/759438/

After the CL, V8WorkerGlobalScopeEventListener is used not only for Workers but
also for Worklets.

Bug:  782696 
Change-Id: Iece08a78ff0d640aca21e49bf23ecf9623204daa
Reviewed-on: https://chromium-review.googlesource.com/776207
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517312}
[modify] https://crrev.com/cd5e88dc00881e051bc6cd650a493095d2a5ebb0/third_party/WebKit/Source/bindings/bindings.gni
[modify] https://crrev.com/cd5e88dc00881e051bc6cd650a493095d2a5ebb0/third_party/WebKit/Source/bindings/core/v8/V8EventListenerHelper.cpp
[rename] https://crrev.com/cd5e88dc00881e051bc6cd650a493095d2a5ebb0/third_party/WebKit/Source/bindings/core/v8/V8WorkerOrWorkletEventListener.cpp
[rename] https://crrev.com/cd5e88dc00881e051bc6cd650a493095d2a5ebb0/third_party/WebKit/Source/bindings/core/v8/V8WorkerOrWorkletEventListener.h

Status: Fixed (was: Started)
Looks like all blockers are gone. Feel free to reopen this issue if you find another blocker.

Sign in to add a comment