Bug 100316

Summary:

[V8] We can merge DOMDataStore, ScopedDOMDataStore, and StaticDOMDataStore into one class

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, haraken, webkit.review.bot

Priority:

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

Description Adam Barth 2012-10-24 18:24:09 PDT

[V8] We can merge DOMDataStore, ScopedDOMDataStore, and StaticDOMDataStore into one class

Comment 1 Adam Barth 2012-10-24 18:24:27 PDT

Created attachment 170531 [details]
Work in progress

Comment 2 Adam Barth 2012-10-24 18:24:51 PDT

This currently crashes every worker test, but I'm hoping to improve that soon.

Comment 3 Adam Barth 2012-10-24 22:40:07 PDT

Created attachment 170552 [details]
Patch

Comment 4 Eric Seidel (no email) 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?

Comment 5 Adam Barth 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.

Comment 6 Adam Barth 2012-10-24 23:41:16 PDT

Created attachment 170563 [details]
Patch for landing

Comment 7 WebKit Review Bot 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>

Comment 8 WebKit Review Bot 2012-10-25 00:14:40 PDT

All reviewed patches have been landed.  Closing bug.