Bug 31877 – [v8] Do not check the thread when accessing DOMDataStore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[anton muhin]

As of now, V8 DOM bindings are single threaded and execute everything on the main thread, so do not do isMainThread checks.

[anton muhin]

Created attachment 43853 [details]
First take

[anton muhin]

Created attachment 43854 [details]
Cosmetic improvements

[Darin Fisher (:fishd, Google)]

Comment on attachment 43854 [details]
Cosmetic improvements

> Index: WebCore/bindings/v8/V8DOMMap.cpp
...
> +namespace {
> +
> +static inline DOMDataStore& getDOMDataStore()
> +{
> +    ASSERT(WTF::isMainThread());  // As of now, we must be always on the main thread.
> +    return MainThreadDOMData::getCurrentMainThreadStore();
> +}
> +
> +}

nit: The anonymous namespace is redundant.  A static function already has
internal linkage.  I would just delete the anonymous namespace.

[Adam Barth]

This change is otherwise correct.

[anton muhin]

Created attachment 43860 [details]
Addressing Darin's concerns

[anton muhin]

(In reply to comment #3)
> (From update of attachment 43854 [details])
> > Index: WebCore/bindings/v8/V8DOMMap.cpp
> ...
> > +namespace {
> > +
> > +static inline DOMDataStore& getDOMDataStore()
> > +{
> > +    ASSERT(WTF::isMainThread());  // As of now, we must be always on the main thread.
> > +    return MainThreadDOMData::getCurrentMainThreadStore();
> > +}
> > +
> > +}
> 
> nit: The anonymous namespace is redundant.  A static function already has
> internal linkage.  I would just delete the anonymous namespace.

Oh, sure, sorry, those styles got mixed in my head.

Thanks a lot for a comments/review, Darin and Adam.

[Adam Barth]

Comment on attachment 43860 [details]
Addressing Darin's concerns

Technically, I think we wrap ChangeLog lines at 80 characters, but this LGTM.

[anton muhin]

(In reply to comment #7)
> (From update of attachment 43860 [details])
> Technically, I think we wrap ChangeLog lines at 80 characters, but this LGTM.

I'll fix and commit-queue+ tomorrow when east coast is asleep.

Thanks all for review.

[WebKit Commit Bot]

Comment on attachment 43860 [details]
Addressing Darin's concerns

Clearing flags on attachment: 43860

Committed r51413: <http://trac.webkit.org/changeset/51413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shinichiro Hamaji]

Committed r51454: <http://trac.webkit.org/changeset/51454>

[anton muhin]

Created attachment 44089 [details]
Solution for the problem with ui_tests

The problem was with shared workers which could run in several threads, so let's enable this explicitly only for single threaded usage of V8.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8DOMMap.h:143:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/bindings/v8/V8DOMMap.cpp:56:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2

[anton muhin]

Created attachment 44090 [details]
Address one (of two!) style comments

I'd ask to allow me to keep changes in V8DOMMap.h intact to follow the style of the file which keeps all the declarations indented.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8DOMMap.h:143:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Adam Barth]

Comment on attachment 44090 [details]
Address one (of two!) style comments

Looks good.  Sorry about the style-queue false positive.  I followed up with a question to webkit-dev.

[anton muhin]

Adam, thanks a lot for review.

I'm submitting w/o changing indentation for now.  Would change when we converge on the approach.

[anton muhin]

Just to enable (hopefully) commit queue for it

[WebKit Commit Bot]

Comment on attachment 44090 [details]
Address one (of two!) style comments

Clearing flags on attachment: 44090

Committed r51599: <http://trac.webkit.org/changeset/51599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.