Bug 100316

Summary: [V8] We can merge DOMDataStore, ScopedDOMDataStore, and StaticDOMDataStore into one class
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, haraken, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 100333    
Attachments:
Description Flags
Work in progress
none
Patch
none
Patch for landing none

Adam Barth
Reported 2012-10-24 18:24:09 PDT
[V8] We can merge DOMDataStore, ScopedDOMDataStore, and StaticDOMDataStore into one class
Attachments
Work in progress (25.11 KB, patch)
2012-10-24 18:24 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (26.50 KB, patch)
2012-10-24 22:40 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (25.68 KB, patch)
2012-10-24 23:41 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-10-24 18:24:27 PDT
Created attachment 170531 [details] Work in progress
Adam Barth
Comment 2 2012-10-24 18:24:51 PDT
This currently crashes every worker test, but I'm hoping to improve that soon.
Adam Barth
Comment 3 2012-10-24 22:40:07 PDT
Created attachment 170552 [details] Patch
Eric Seidel (no email)
Comment 4 2012-10-24 22:57:37 PDT
Comment on attachment 170552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170552&action=review LGTM. > Source/WebCore/ChangeLog:11 > + unifies all these classes into DOMDataStore itself. This makese this makese! > Source/WebCore/bindings/v8/DOMDataStore.cpp:57 > + m_domObjectMap = adoptPtr(new DOMWrapperHashMap<void>); > + m_activeDomObjectMap = adoptPtr(new DOMWrapperHashMap<void>); These are always the same for all 3, no? It seems this whole "Switch" is clearer as an if. But presumably you wanted the "forgot to add handling of the new enum value" help from the compiler?
Adam Barth
Comment 5 2012-10-24 23:06:14 PDT
(In reply to comment #4) > (From update of attachment 170552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=170552&action=review > > > Source/WebCore/bindings/v8/DOMDataStore.cpp:57 > > + m_domObjectMap = adoptPtr(new DOMWrapperHashMap<void>); > > + m_activeDomObjectMap = adoptPtr(new DOMWrapperHashMap<void>); > > These are always the same for all 3, no? It seems this whole "Switch" is clearer as an if. But presumably you wanted the "forgot to add handling of the new enum value" help from the compiler? Yeah, I thought they were going to be more different. I'll change it to an if.
Adam Barth
Comment 6 2012-10-24 23:41:16 PDT
Created attachment 170563 [details] Patch for landing
WebKit Review Bot
Comment 7 2012-10-25 00:14:36 PDT
Comment on attachment 170563 [details] Patch for landing Clearing flags on attachment: 170563 Committed r132454: <http://trac.webkit.org/changeset/132454>
WebKit Review Bot
Comment 8 2012-10-25 00:14:40 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.