[webkit-reviews] review granted: [Bug 25009] Upstream changes to WorkerContextExecutionProxy for V8 bindings in order to use V8EventListenerList as container. : [Attachment 29203] Proposed Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Jian Li
<jianli at chromium.org>'s request for review:
Bug 25009: Upstream changes to WorkerContextExecutionProxy for V8 bindings in
order to use V8EventListenerList as container.
https://bugs.webkit.org/show_bug.cgi?id=25009

Attachment 29203: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=29203&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp
> @@ -62,6 +62,9 @@
WorkerContextExecutionProxy::WorkerContextExecutionProxy(WorkerContext*
workerCo
>      : m_workerContext(workerContext)
>      , m_recursion(0)
>  {
> +    // Need to use lock since V8EventListenerList constructor creates
HandleScope.
> +    v8::Locker locker;
> +    m_listeners.set(new V8EventListenerList("m_listeners"));

Why not put the v8::Locker closer to where the HandleScope is used?

As is, if someone changes V8EventListenerList to no longer use a HandleScope,
it
won't be obvious that they should also fix this code to no longer use a locker.



> +    {
> +	   // Need to use lock since V8EventListenerList::clear() creates
HandleScope.
> +	   v8::Locker locker;
> +	   m_listeners->clear();
> +    }

Same issue here.  Maybe the locker should move into the clear method?


> +	   // Need to use lock since V8EventListenerList::add() creates
HandleScope.
> +	   v8::Locker locker;
> +	   m_listeners->add(newListener.get());
> +    }

ditto


> +    // Need to use lock since V8EventListenerList::remove() creates
HandleScope.
> +    v8::Locker locker;
> +    m_listeners->remove(listener);

ditto

Or, perhaps the issue is that when V8EventListenerList is used on the main
thread of WebKit, we don't want to have it use v8::Locker?  If that is the
case, then perhaps it would be wise to create a wrapper class for
V8EventlistenerList that encapsulates the locker usage?  Or maybe that is
just plain overkill?