[webkit-reviews] review denied: [Bug 65004] [V8][Chromium] Run workers in a separate v8::Isolate : [Attachment 101685] Allocates v8::isolate per worker

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Dmitry Lomov <dslomov at google.com>'s
request for review:
Bug 65004: [V8][Chromium] Run workers in a separate v8::Isolate
https://bugs.webkit.org/show_bug.cgi?id=65004

Attachment 101685: Allocates v8::isolate per worker
https://bugs.webkit.org/attachment.cgi?id=101685&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101685&action=review


> Source/WebCore/ChangeLog:9
> +	   No new tests. (OOPS!)

This should either say what test were added or why test can't be written or
what test cover it or that no new functionality is exposed so no new tests are
needed.

> Source/WebCore/bindings/v8/V8Binding.h:112
>	   DOMDataList& allStores() { return m_DOMDataList; }

Add blank line.

> Source/WebCore/bindings/v8/V8Binding.h:120
> +	       m_DOMDataList.remove(m_DOMDataList.find(domDataStore));

What happens if domDataStore isn't in there?

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:59
> +    data->allStores().append(&m_DOMDataStore);

consider swapping these two lines.

(It seems like setDOMDataStore should assert that what it is given is in the
allStores.)

> Source/WebCore/bindings/v8/WorkerScriptController.h:82
> +	   ScopedDOMDataStore m_DOMDataStore;

m_domDataStore