Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(27)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 705003002: Change the return type of WTF::bind() to |OwnPtr<Function>| (Closed)

Created:
6 years, 1 month ago by hiroshige
Modified:
6 years, 1 month ago
CC:
aandrey+blink_chromium.org, Mads Ager (chromium), blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, blink-reviews-wtf_chromium.org, Rik, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, falken, feature-media-reviews_chromium.org, gavinp+loader_chromium.org, haraken, horo+watch_chromium.org, Nate Chapin, kinuko+worker_chromium.org, kinuko+fileapi, kouhei+heap_chromium.org, Mikhail, nhiroki, philipj_slow, rwlbuis, sof, tyoshino+watch_chromium.org, tzik
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Change the return type of WTF::bind() to |OwnPtr<Function>| Previously WTF::bind() returns |Function|, but this causes implicit temporary objects and copies of |Function|, resulting potential thread-safe issues. This CL changes the return type of WTF::bind() to |OwnPtr<Function>|, thus removing such temporary objects and making the ownership of |Function| explicit. - Change the return type of WTF::bind(). - Replace |const Function&| in parameter with |PassOwnPtr<Function>|. - Replace |Function| with |OwnPtr<Function>|. - Add |.release()| and replace |f()| with |(*f)()|. - Other types of changes are in |callOnMainThread| and NetworkStateNotifierTest.cpp. (|Function| appears in typedef'ed names such as Closure.) Also this CL makes |FunctionBase| non-copyable to prevent unintended copies of |Function|. BUG=390851 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185348

Patch Set 1 #

Patch Set 2 : bug fix #

Patch Set 3 : Rebase #

Total comments: 10

Patch Set 4 : Reflect comments from yhirano #

Unified diffs Side-by-side diffs Delta from patch set Stats (+146 lines, -138 lines) Patch
M Source/core/dom/ExecutionContextTask.h View 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/dom/Microtask.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/dom/Microtask.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.cpp View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M Source/core/html/parser/HTMLParserThread.h View 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/html/parser/HTMLParserThread.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/NetworkStateNotifierTest.cpp View 1 2 chunks +4 lines, -4 lines 0 comments Download
M Source/core/workers/WorkerThread.cpp View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp View 3 chunks +9 lines, -9 lines 0 comments Download
M Source/modules/filesystem/LocalFileSystem.h View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M Source/modules/filesystem/LocalFileSystem.cpp View 1 chunk +4 lines, -4 lines 0 comments Download
M Source/platform/PermissionCallbacks.h View 2 chunks +7 lines, -6 lines 0 comments Download
M Source/platform/PermissionCallbacks.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/platform/Task.h View 1 2 3 1 chunk +5 lines, -3 lines 0 comments Download
M Source/platform/heap/HeapTest.cpp View 1 chunk +4 lines, -4 lines 0 comments Download
M Source/platform/scheduler/Scheduler.h View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M Source/platform/scheduler/Scheduler.cpp View 3 chunks +4 lines, -4 lines 0 comments Download
M Source/web/WebEmbeddedWorkerImpl.cpp View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M Source/web/WebSharedWorkerImpl.cpp View 3 chunks +5 lines, -5 lines 0 comments Download
M Source/wtf/Functional.h View 1 3 chunks +16 lines, -14 lines 0 comments Download
M Source/wtf/FunctionalTest.cpp View 7 chunks +55 lines, -54 lines 0 comments Download
M Source/wtf/MainThread.h View 1 2 3 2 chunks +3 lines, -4 lines 0 comments Download
M Source/wtf/MainThread.cpp View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 11 (3 generated)
hiroshige
PTAL. I'm still looking at trybot failures.
6 years, 1 month ago (2014-11-07 04:42:46 UTC) #2
yhirano
https://codereview.chromium.org/705003002/diff/40001/Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h (right): https://codereview.chromium.org/705003002/diff/40001/Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h#newcode75 Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h:75: OwnPtrList<Closure> m_clientTasks; Can't we use Vector<OwnPtr<Closure>>? https://codereview.chromium.org/705003002/diff/40001/Source/modules/filesystem/LocalFileSystem.h File Source/modules/filesystem/LocalFileSystem.h ...
6 years, 1 month ago (2014-11-07 06:05:28 UTC) #3
hiroshige
Thank you for reviewing! https://codereview.chromium.org/705003002/diff/40001/Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h (right): https://codereview.chromium.org/705003002/diff/40001/Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h#newcode75 Source/core/loader/WorkerLoaderClientBridgeSyncHelper.h:75: OwnPtrList<Closure> m_clientTasks; On 2014/11/07 06:05:27, ...
6 years, 1 month ago (2014-11-07 07:17:00 UTC) #4
yhirano
lgtm
6 years, 1 month ago (2014-11-07 07:44:49 UTC) #5
hiroshige
jochen@, could you take a look?
6 years, 1 month ago (2014-11-13 05:31:43 UTC) #7
jochen (gone - plz use gerrit)
lgtm
6 years, 1 month ago (2014-11-13 15:17:34 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/705003002/60001
6 years, 1 month ago (2014-11-14 04:43:54 UTC) #10
commit-bot: I haz the power
6 years, 1 month ago (2014-11-14 05:28:09 UTC) #11
Message was sent while issue was closed. 
Committed patchset #4 (id:60001) as 185348

Powered by Google App Engine
This is Rietveld 408576698