Chromium Code Reviews
Help | Chromium Project | Sign in
(16)

Issue 2680843006: Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 5 days ago by sof
Modified:
1 week, 3 days ago
CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews-bindings_chromium.org, blink-reviews, kinuko+watch, kouhei+heap_chromium.org, blink-reviews-wtf_chromium.org, Mikhail
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Tidy DEFINE_(THREAD_SAFE_)STATIC_LOCAL() implementations. Move the handling of static local singletons into the WTF::StaticSingleton<T> wrapper class, including the "same thread" debug verification. Use it to also implement the "thread safe" variant also, which can now be done in a straightforward manner after issue 686866 enabled C++ thread safe statics. R=kinuko BUG=686866 Review-Url: https://codereview.chromium.org/2680843006 Cr-Commit-Position: refs/heads/master@{#451305} Committed: https://chromium.googlesource.com/chromium/src/+/03165229c01b328598a7ba00bfd2d4ab6fa79f0a

Patch Set 1 #

Patch Set 2 : remove loose ends, tidying #

Patch Set 3 : static assert for toplevel Persistent<T>s #

Patch Set 4 : comment update #

Patch Set 5 : rebased and made dependent #

Patch Set 6 : explain safety of HTMLTableSectionElement singletons #

Total comments: 10

Patch Set 7 : drop no-ScriptWrappable dcheck support #

Total comments: 2

Patch Set 8 : simplify DEFINE_THREAD_SAFE_STATIC_LOCAL() #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -131 lines) Patch
M third_party/WebKit/Source/wtf/StdLibExtras.h View 1 2 3 4 5 6 7 1 chunk +98 lines, -70 lines 1 comment Download
M third_party/WebKit/Source/wtf/Threading.h View 1 2 3 4 5 6 7 2 chunks +0 lines, -23 lines 0 comments Download
M third_party/WebKit/Source/wtf/ThreadingPthreads.cpp View 1 2 3 4 5 6 7 2 chunks +0 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/wtf/ThreadingWin.cpp View 1 2 3 4 5 6 7 3 chunks +0 lines, -19 lines 0 comments Download
Commit queue not available (can’t edit this change).

Depends on Patchset:

Messages

Total messages: 102 (69 generated)
sof
please take a look.
2 weeks, 2 days ago (2017-02-10 21:55:09 UTC) #53
haraken
This verification looks complicated but yeah I don't have any better idea... https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp ...
2 weeks, 2 days ago (2017-02-11 10:25:21 UTC) #56
sof
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp#newcode129 third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. ...
2 weeks, 2 days ago (2017-02-11 12:09:11 UTC) #57
dcheng
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp#newcode129 third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. ...
2 weeks, 1 day ago (2017-02-12 09:33:45 UTC) #58
sof
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp#newcode129 third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. ...
2 weeks ago (2017-02-12 21:52:28 UTC) #59
haraken
https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp File third_party/WebKit/Source/core/frame/Frame.cpp (right): https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp#newcode129 third_party/WebKit/Source/core/frame/Frame.cpp:129: // purposes and will not accidentally leak between contexts. ...
2 weeks ago (2017-02-12 23:56:47 UTC) #60
dcheng
On 2017/02/12 23:56:47, haraken wrote: > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp > File third_party/WebKit/Source/core/frame/Frame.cpp (right): > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp#newcode129 > ...
2 weeks ago (2017-02-13 01:27:37 UTC) #61
haraken
On 2017/02/13 01:27:37, dcheng wrote: > On 2017/02/12 23:56:47, haraken wrote: > > > https://codereview.chromium.org/2680843006/diff/260001/third_party/WebKit/Source/core/frame/Frame.cpp ...
2 weeks ago (2017-02-13 01:38:04 UTC) #62
dcheng
On 2017/02/13 01:38:04, haraken wrote: > On 2017/02/13 01:27:37, dcheng wrote: > > On 2017/02/12 ...
2 weeks ago (2017-02-13 01:42:27 UTC) #63
haraken
On 2017/02/13 01:42:27, dcheng wrote: > On 2017/02/13 01:38:04, haraken wrote: > > On 2017/02/13 ...
2 weeks ago (2017-02-13 01:45:57 UTC) #64
esprehn
Worlds are just a special case of contexts. Leaks between worlds can be a security ...
1 week, 6 days ago (2017-02-14 01:59:09 UTC) #65
sof
https://codereview.chromium.org/2696713003/ is the WIP for GC plugin verification; it is complete, but needs more polish ...
1 week, 6 days ago (2017-02-14 16:01:17 UTC) #66
sof
On 2017/02/14 16:01:17, sof wrote: > https://codereview.chromium.org/2696713003/ is the WIP for GC plugin > verification; ...
1 week, 5 days ago (2017-02-14 21:50:39 UTC) #71
sof
ptal - narrowed focus and purpose of this one.
1 week, 5 days ago (2017-02-15 08:19:59 UTC) #73
dcheng
https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h File third_party/WebKit/Source/wtf/StdLibExtras.h (right): https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h#newcode126 third_party/WebKit/Source/wtf/StdLibExtras.h:126: WTF::isAtomicallyInitializedStaticMutexLockHeld(); Since we have threadsafe function-local statics by default ...
1 week, 5 days ago (2017-02-15 08:29:56 UTC) #74
sof
https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h File third_party/WebKit/Source/wtf/StdLibExtras.h (right): https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h#newcode126 third_party/WebKit/Source/wtf/StdLibExtras.h:126: WTF::isAtomicallyInitializedStaticMutexLockHeld(); On 2017/02/15 08:29:56, dcheng wrote: > Since we ...
1 week, 5 days ago (2017-02-15 08:44:57 UTC) #75
dcheng
On 2017/02/15 08:44:57, sof wrote: > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h > File third_party/WebKit/Source/wtf/StdLibExtras.h (right): > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h#newcode126 > ...
1 week, 5 days ago (2017-02-15 08:49:22 UTC) #76
sof
On 2017/02/15 08:49:22, dcheng wrote: > On 2017/02/15 08:44:57, sof wrote: > > > https://codereview.chromium.org/2680843006/diff/280001/third_party/WebKit/Source/wtf/StdLibExtras.h ...
1 week, 5 days ago (2017-02-15 08:53:42 UTC) #77
dcheng
On 2017/02/15 08:53:42, sof wrote: > On 2017/02/15 08:49:22, dcheng wrote: > > On 2017/02/15 ...
1 week, 5 days ago (2017-02-15 08:55:58 UTC) #79
esprehn
On 2017/02/15 at 08:55:58, dcheng wrote: > On 2017/02/15 08:53:42, sof wrote: > > On ...
1 week, 4 days ago (2017-02-15 18:39:42 UTC) #80
jbroman
On Wed, Feb 15, 2017 at 1:39 PM, <esprehn@chromium.org> wrote: > On 2017/02/15 at 08:55:58, ...
1 week, 4 days ago (2017-02-15 18:45:19 UTC) #81
jbroman
On Wed, Feb 15, 2017 at 1:39 PM, <esprehn@chromium.org> wrote: > On 2017/02/15 at 08:55:58, ...
1 week, 4 days ago (2017-02-15 18:45:19 UTC) #82
sof
On 2017/02/15 08:55:58, dcheng wrote: > On 2017/02/15 08:53:42, sof wrote: > > On 2017/02/15 ...
1 week, 4 days ago (2017-02-15 19:11:19 UTC) #83
sof
On 2017/02/15 19:11:19, sof wrote: > On 2017/02/15 08:55:58, dcheng wrote: > > On 2017/02/15 ...
1 week, 4 days ago (2017-02-15 19:22:19 UTC) #84
kinuko
On 2017/02/15 19:22:19, sof wrote: > On 2017/02/15 19:11:19, sof wrote: > > On 2017/02/15 ...
1 week, 4 days ago (2017-02-16 03:44:13 UTC) #85
sof
On 2017/02/16 03:44:13, kinuko wrote: > On 2017/02/15 19:22:19, sof wrote: > > On 2017/02/15 ...
1 week, 4 days ago (2017-02-16 06:29:36 UTC) #86
kinuko
On 2017/02/16 06:29:36, sof wrote: > On 2017/02/16 03:44:13, kinuko wrote: > > On 2017/02/15 ...
1 week, 4 days ago (2017-02-16 07:12:57 UTC) #87
sof
On 2017/02/16 07:12:57, kinuko wrote: > On 2017/02/16 06:29:36, sof wrote: > > On 2017/02/16 ...
1 week, 4 days ago (2017-02-16 07:30:58 UTC) #88
sof
Updated to handle the "threadsafe" case; ptal.
1 week, 3 days ago (2017-02-16 19:44:00 UTC) #92
sof
https://codereview.chromium.org/2680843006/diff/300001/third_party/WebKit/Source/wtf/StdLibExtras.h File third_party/WebKit/Source/wtf/StdLibExtras.h (right): https://codereview.chromium.org/2680843006/diff/300001/third_party/WebKit/Source/wtf/StdLibExtras.h#newcode137 third_party/WebKit/Source/wtf/StdLibExtras.h:137: return allowCrossThreadUse || m_safelyInitialized || fyi, https://codereview.chromium.org/2698953002/ has some ...
1 week, 3 days ago (2017-02-17 06:37:01 UTC) #95
kinuko
lgtm
1 week, 3 days ago (2017-02-17 13:40:49 UTC) #96
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2680843006/300001
1 week, 3 days ago (2017-02-17 15:09:00 UTC) #99
commit-bot: I haz the power
1 week, 3 days ago (2017-02-17 15:13:16 UTC) #102
Message was sent while issue was closed.
Committed patchset #8 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/03165229c01b328598a7ba00bfd2...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd