Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919723002:
Introduce WTF::unretained() and WTF::crossThreadUnretained() (Closed)
|
Created:
4 years, 8 months ago by hiroshige Modified:
4 years, 6 months ago CC:
chromium-reviews, blink-reviews-platform-graphics_chromium.org, dshwang, kinuko+watch, blink-reviews-wtf_chromium.org, rwlbuis, krit, drott+blinkwatch_chromium.org, Justin Novosad, hongchan, Rik, blink-reviews, Mads Ager (chromium), jbroman, Raymond Toy, pdr+graphicswatchlist_chromium.org, Stephen Chennney, ajuma+watch-canvas_chromium.org, f(malita), oilpan-reviews, danakj+watch_chromium.org, kouhei+heap_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@Kuroneko_4b_ACTA_WeakPtr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce WTF::unretained() and WTF::crossThreadUnretained()
unretained() and crossThreadUnretained are used to annotate unretained raw
pointers bound by WTF::bind() and forbid raw pointers in WTF::bind().
crossThreadUnretained() is used for annotating pointers passed across threads
and corresponds to previous AllowCrossThreadAccess().
BUG=597856
Committed: https://crrev.com/5639ac31ebb1996a1cdcb3480471b0e65a1ea94c
Cr-Commit-Position: refs/heads/master@{#401571}
Patch Set 1 #Patch Set 2 : Apply unretained() #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : auto-Rebase #Patch Set 6 #
Total comments: 2
Patch Set 7 : Rebase, add tests. #Patch Set 8 : Rebase #Patch Set 9 : Reflect comment #
Total comments: 2
Patch Set 10 : sort #Patch Set 11 : Rebase. #
Dependent Patchsets: Messages
Total messages: 51 (27 generated)
Description was changed from ========== Introduce WTF::unretained() and apply it to all AllowCrossThreadAccess() BUG= ========== to ========== Introduce WTF::unretained() and apply it to all AllowCrossThreadAccess() BUG= ==========
Description was changed from ========== Introduce WTF::unretained() and apply it to all AllowCrossThreadAccess() BUG= ========== to ========== Replace AllowCrossThreadAccess() + non-GCed pointers with AllowCrossThreadAccess(unretained()) BUG= ==========
Description was changed from ========== Replace AllowCrossThreadAccess() + non-GCed pointers with AllowCrossThreadAccess(unretained()) BUG= ========== to ========== Introduce WTF::unretained() and WTF::unretainedUnsafe() BUG= ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919723002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919723002/100001
Description was changed from ========== Introduce WTF::unretained() and WTF::unretainedUnsafe() BUG= ========== to ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() BUG=597856 ==========
hiroshige@chromium.org changed reviewers: + tzik@chromium.org, yutak@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Let's add a unit test so we can check whether this would compile or not. What's going to be the differentiator between unretained and crossThreadUnretained? At this point they are the same except for their types. More information in the change description about the context of this patch would be desirable. https://codereview.chromium.org/1919723002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1919723002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Functional.h:265: static StorageType wrap(UnretainedWrapper<T, threadAffinity>&& value) { return std::move(value); } rvalue reference + std::move() sounds like an overkill since UnretainedWrapper can be copied safely. Just taking a const lvalue reference would be good.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/140001
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/160001
Description was changed from ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() BUG=597856 ========== to ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds (some of) previous AllowCrossThreadAccess(). BUG=597856 ==========
Added unretained() to unit tests and updated the CL description. https://codereview.chromium.org/1919723002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1919723002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Functional.h:265: static StorageType wrap(UnretainedWrapper<T, threadAffinity>&& value) { return std::move(value); } On 2016/05/17 09:37:04, Yuta Kitamura wrote: > rvalue reference + std::move() sounds like an overkill since > UnretainedWrapper can be copied safely. Just taking a const > lvalue reference would be good. Done.
lgtm https://codereview.chromium.org/1919723002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1919723002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Functional.h:417: using WTF::CrossThreadClosure; Could you sort them?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1919723002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1919723002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/Functional.h:417: using WTF::CrossThreadClosure; On 2016/06/09 09:38:29, tzik wrote: > Could you sort them? I prefer grouping by functionality (unless this list grows much longer) rather than sorting alphabetically. How about Patch Set 10?
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds (some of) previous AllowCrossThreadAccess(). BUG=597856 ========== to ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds to previous AllowCrossThreadAccess(). BUG=597856 ==========
yutak@, could you take a look?
LGTM on my side. Not related, but I'd suggest renaming threadSafeBind to crossThreadBind for consinstency with crossThreadXXX pointer types.
lgtm
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org Link to the patchset: https://codereview.chromium.org/1919723002/#ps180001 (title: "sort")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yutak@chromium.org, tzik@chromium.org Link to the patchset: https://codereview.chromium.org/1919723002/#ps200001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919723002/200001
Message was sent while issue was closed.
Description was changed from ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds to previous AllowCrossThreadAccess(). BUG=597856 ========== to ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds to previous AllowCrossThreadAccess(). BUG=597856 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds to previous AllowCrossThreadAccess(). BUG=597856 ========== to ========== Introduce WTF::unretained() and WTF::crossThreadUnretained() unretained() and crossThreadUnretained are used to annotate unretained raw pointers bound by WTF::bind() and forbid raw pointers in WTF::bind(). crossThreadUnretained() is used for annotating pointers passed across threads and corresponds to previous AllowCrossThreadAccess(). BUG=597856 Committed: https://crrev.com/5639ac31ebb1996a1cdcb3480471b0e65a1ea94c Cr-Commit-Position: refs/heads/master@{#401571} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5639ac31ebb1996a1cdcb3480471b0e65a1ea94c Cr-Commit-Position: refs/heads/master@{#401571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
