[webkit-reviews] review denied: [Bug 26366] Teach V8 bindings about isolated worlds : [Attachment 31227] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Adam Barth
<abarth at webkit.org>'s request for review:
Bug 26366: Teach V8 bindings about isolated worlds
https://bugs.webkit.org/show_bug.cgi?id=26366

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Holy moly! You rewrote the world! I think I understand what the patch is doing,
especially with the Reitveld companion.

My concerns are:

* v8_isolated_world should live upstream, in bindings/v8.
* How does this affect bindings performance?
* Make sure there are no layout test regressions.

Review comments:

No ChangeLog? :)

> +    private:
> +	 DOMData* m_domData;

4 spaces?

> +    static void handleWeakObject(DOMDataStore::DOMWrapperMapType mapType,
> +				    v8::Handle<v8::Object> v8Object,
> +				    T* domObject);

No need to wrap here -- it's WebKit-land :)

> +DOMDataStoreHandle::DOMDataStoreHandle()
> +    : m_store(new ScopedDOMDataStore(DOMData::getCurrent()))  // TODO: Fix!

FIXME: and What?

> +void visitDOMNodesInCurrentThread(DOMWrapperMap<Node>::Visitor* visitor) {

Brace on new line.

> +void visitDOMObjectsInCurrentThread(DOMWrapperMap<void>::Visitor* visitor) {


Ditto.

> +void visitActiveDOMObjectsInCurrentThread(DOMWrapperMap<void>::Visitor*
visitor) {

Ditto.

> +void
visitDOMSVGElementInstancesInCurrentThread(DOMWrapperMap<SVGElementInstance>::V
isitor* visitor) {

Ditto.

> +void visitSVGObjectsInCurrentThread(DOMWrapperMap<void>::Visitor* visitor) {


Ditto.