Bug 31877 - [v8] Do not check the thread when accessing DOMDataStore
Summary: [v8] Do not check the thread when accessing DOMDataStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-25 08:44 PST by anton muhin
Modified: 2009-12-02 06:52 PST (History)
5 users (show)

See Also:


Attachments
First take (2.26 KB, patch)
2009-11-25 09:25 PST, anton muhin
no flags Details | Formatted Diff | Diff
Cosmetic improvements (2.26 KB, patch)
2009-11-25 09:28 PST, anton muhin
fishd: review-
Details | Formatted Diff | Diff
Addressing Darin's concerns (2.24 KB, patch)
2009-11-25 11:41 PST, anton muhin
no flags Details | Formatted Diff | Diff
Solution for the problem with ui_tests (4.19 KB, patch)
2009-12-01 11:35 PST, anton muhin
no flags Details | Formatted Diff | Diff
Address one (of two!) style comments (4.19 KB, patch)
2009-12-01 11:39 PST, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2009-11-25 08:44:47 PST
As of now, V8 DOM bindings are single threaded and execute everything on the main thread, so do not do isMainThread checks.
Comment 1 anton muhin 2009-11-25 09:25:17 PST
Created attachment 43853 [details]
First take
Comment 2 anton muhin 2009-11-25 09:28:57 PST
Created attachment 43854 [details]
Cosmetic improvements
Comment 3 Darin Fisher (:fishd, Google) 2009-11-25 10:34:26 PST
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.
Comment 4 Adam Barth 2009-11-25 11:24:20 PST
This change is otherwise correct.
Comment 5 anton muhin 2009-11-25 11:41:29 PST
Created attachment 43860 [details]
Addressing Darin's concerns
Comment 6 anton muhin 2009-11-25 11:42:31 PST
(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.
Comment 7 Adam Barth 2009-11-25 12:19:36 PST
Comment on attachment 43860 [details]
Addressing Darin's concerns

Technically, I think we wrap ChangeLog lines at 80 characters, but this LGTM.
Comment 8 anton muhin 2009-11-25 15:27:36 PST
(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.
Comment 9 WebKit Commit Bot 2009-11-26 05:20:37 PST
Comment on attachment 43860 [details]
Addressing Darin's concerns

Clearing flags on attachment: 43860

Committed r51413: <http://trac.webkit.org/changeset/51413>
Comment 10 WebKit Commit Bot 2009-11-26 05:20:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Shinichiro Hamaji 2009-11-27 21:34:05 PST
Committed r51454: <http://trac.webkit.org/changeset/51454>
Comment 12 anton muhin 2009-12-01 11:35:33 PST
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.
Comment 13 WebKit Review Bot 2009-12-01 11:36:13 PST
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
Comment 14 anton muhin 2009-12-01 11:39:56 PST
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.
Comment 15 WebKit Review Bot 2009-12-01 11:41:35 PST
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
Comment 16 Adam Barth 2009-12-01 12:46:11 PST
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.
Comment 17 anton muhin 2009-12-02 05:49:17 PST
Adam, thanks a lot for review.

I'm submitting w/o changing indentation for now.  Would change when we converge on the approach.
Comment 18 anton muhin 2009-12-02 06:42:15 PST
Just to enable (hopefully) commit queue for it
Comment 19 WebKit Commit Bot 2009-12-02 06:52:26 PST
Comment on attachment 44090 [details]
Address one (of two!) style comments

Clearing flags on attachment: 44090

Committed r51599: <http://trac.webkit.org/changeset/51599>
Comment 20 WebKit Commit Bot 2009-12-02 06:52:32 PST
All reviewed patches have been landed.  Closing bug.