Bug 100175 - [V8] AbstractWeakReferenceMap is unnecessarily general and complex
Summary: [V8] AbstractWeakReferenceMap is unnecessarily general and complex
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 100208 100295
Blocks: 100313
  Show dependency tree / graph
 
Reported: 2012-10-23 18:03 PDT by Adam Barth
Modified: 2012-10-24 20:07 PDT (History)
4 users (show)

See Also:

Attachments
work in progress (46.13 KB, patch)
2012-10-23 18:03 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Almost works (43.22 KB, patch)
2012-10-24 13:28 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Might actually work (43.73 KB, patch)
2012-10-24 13:47 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (51.82 KB, patch)
2012-10-24 14:37 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (47.47 KB, patch)
2012-10-24 15:46 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-10-23 18:03:00 PDT 
[V8] DOMWrapperMaps are insanity in a bottle
Comment 1 Adam Barth 2012-10-23 18:03:12 PDT 
Created attachment 170280 [details]
work in progress
Comment 2 Adam Barth 2012-10-23 18:03:30 PDT 
Still working on compile and integration with V8GCController.
Comment 3 Adam Barth 2012-10-24 13:28:35 PDT 
Created attachment 170460 [details]
Almost works
Comment 4 Adam Barth 2012-10-24 13:47:05 PDT 
Created attachment 170464 [details]
Might actually work
Comment 5 Adam Barth 2012-10-24 14:37:12 PDT 
Created attachment 170473 [details]
Patch
Comment 6 WebKit Review Bot 2012-10-24 14:40:02 PDT 
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.
Comment 7 Eric Seidel (no email) 2012-10-24 14:50:00 PDT 
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?
Comment 8 Eric Seidel (no email) 2012-10-24 15:06:50 PDT 
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.
Comment 9 Adam Barth 2012-10-24 15:34:46 PDT 
> 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.
Comment 10 Adam Barth 2012-10-24 15:46:32 PDT 
Created attachment 170493 [details]
Patch for landing
Comment 11 Adam Barth 2012-10-24 15:46:55 PDT 
Comment on attachment 170493 [details]
Patch for landing

Needs to wait for the dependent patch to land.
Comment 12 WebKit Review Bot 2012-10-24 16:33:52 PDT 
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.
Comment 13 Adam Barth 2012-10-24 20:07:46 PDT 
Comment on attachment 170493 [details]
Patch for landing

Clearing flags on attachment: 170493

Committed r132441: <http://trac.webkit.org/changeset/132441>
Comment 14 Adam Barth 2012-10-24 20:07:56 PDT 
All reviewed patches have been landed.  Closing bug.