Bug 100175 – [V8] AbstractWeakReferenceMap is unnecessarily general and complex

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] DOMWrapperMaps are insanity in a bottle

[Adam Barth]

Created attachment 170280 [details]
work in progress

[Adam Barth]

Still working on compile and integration with V8GCController.

[Adam Barth]

Created attachment 170460 [details]
Almost works

[Adam Barth]

Created attachment 170464 [details]
Might actually work

[Adam Barth]

Created attachment 170473 [details]
Patch

[WebKit Review Bot]

Attachment 170473 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/V8DOMMap.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 170473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170473&action=review

Despite the epic ChangeLog, this change doesn't seem that complicated.

> Source/WebCore/bindings/v8/V8DOMMap.h:61
> +    DOMWrapperMap<Node>& getDOMNodeMap(v8::Isolate* = 0);
> +    DOMWrapperMap<Node>& getActiveDOMNodeMap(v8::Isolate* = 0);

Can these just require an Isolate?

[Eric Seidel (no email)]

Comment on attachment 170473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170473&action=review

This looks great.  Please do the event insanity in a separate patch, and CC inferno.

> Source/WebCore/bindings/v8/DOMWrapperMap.h:113
> +        ASSERT(it->value == wrapper);

Release will fail if this isn't ASSERT_UNUSED.

[Adam Barth]

> This looks great.  Please do the event insanity in a separate patch, and CC inferno.

Thanks.  The worker part of the patch is now in bug 100295.

[Adam Barth]

Created attachment 170493 [details]
Patch for landing

[Adam Barth]

Comment on attachment 170493 [details]
Patch for landing

Needs to wait for the dependent patch to land.

[WebKit Review Bot]

Attachment 170493 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/V8DOMMap.h:45:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 170493 [details]
Patch for landing

Clearing flags on attachment: 170493

Committed r132441: <http://trac.webkit.org/changeset/132441>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.