Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2741643003:
Replace WorkerGlobalScope::postTask with WorkerThread::postTask (Closed)
|
Created:
3 years, 9 months ago by yuryu Modified:
3 years, 9 months ago CC:
chromium-reviews, shimazu+worker_chromium.org, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace WorkerGlobalScope::postTask with WorkerThread::postTask
ExecutionContextTask and ExecutionContext::postTask are deprecated and
will be removed. This patch changes WorkerEventQueue to post tasks
directly through an underlying thread of WorkerGlobalScope as well as
adding a single thread version of WorkerThread::postTask. The behavior
remains the same.
BUG=625927
Review-Url: https://codereview.chromium.org/2741643003
Cr-Commit-Position: refs/heads/master@{#456337}
Committed: https://chromium.googlesource.com/chromium/src/+/4e533644399b42111d85ab607219494ed719f0cb
Patch Set 1 #
Total comments: 2
Patch Set 2 : implement same thread version of WorkerThread::postTask #
Total comments: 10
Patch Set 3 : missppelling; include cleanup #
Messages
Total messages: 26 (17 generated)
Description was changed from ========== Replace WorkerGlobalScope::postTask with WorkerThread::postTask ExecutionContextTask and ExecutionContext::postTask are deprecated and will be removed. This patch changes WorkerEventQueue to post tasks directly through an underlying thread of WorkerGlobalScope. The behaviour remains the same. BUG=625927 ========== to ========== Replace WorkerGlobalScope::postTask with WorkerThread::postTask ExecutionContextTask and ExecutionContext::postTask are deprecated and will be removed. This patch changes WorkerEventQueue to post tasks directly through an underlying thread of WorkerGlobalScope. The behavior remains the same. BUG=625927 ==========
yuryu@chromium.org changed reviewers: + nhiroki@google.com, tzik@chromium.org
ptal, thanks
nhiroki@chromium.org changed reviewers: + nhiroki@chromium.org
https://codereview.chromium.org/2741643003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right): https://codereview.chromium.org/2741643003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:63: BLINK_FROM_HERE, crossThreadBind(&WorkerEventQueue::dispatchEvent, Can you add WorkerThread::postTask() for the same thread task?
nhiroki@chromium.org changed reviewers: - nhiroki@google.com
The CQ bit was checked by yuryu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2741643003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right): https://codereview.chromium.org/2741643003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:63: BLINK_FROM_HERE, crossThreadBind(&WorkerEventQueue::dispatchEvent, Added, but it doesn't look very nice. What do you think?
lgtm w/ nits https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right): https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:29: #include "core/dom/TaskRunnerHelper.h" Is this no longer necessary? https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.cpp:566: template <WTF::FunctionThreadAffinity threadAfinity> s/threadAfinity/threadAffinity https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.cpp:568: std::unique_ptr<Function<void(), threadAfinity>> task) { ditto. https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:255: template <WTF::FunctionThreadAffinity threadAfinity> s/threadAfinity/threadAffinity https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:257: std::unique_ptr<Function<void(), threadAfinity>> task); ditto
On 2017/03/09 13:03:36, yuryu wrote: > https://codereview.chromium.org/2741643003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right): > > https://codereview.chromium.org/2741643003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:63: BLINK_FROM_HERE, > crossThreadBind(&WorkerEventQueue::dispatchEvent, > Added, but it doesn't look very nice. What do you think? Yeah, using templates etc is a bit unfortunate, but separating functions by thread affinity would be beneficial in terms of readability and code hygiene.
https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right): https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:29: #include "core/dom/TaskRunnerHelper.h" On 2017/03/10 03:19:27, nhiroki (slow) wrote: > Is this no longer necessary? Done. https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.cpp:566: template <WTF::FunctionThreadAffinity threadAfinity> On 2017/03/10 03:19:27, nhiroki (slow) wrote: > s/threadAfinity/threadAffinity Done. https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.cpp:568: std::unique_ptr<Function<void(), threadAfinity>> task) { On 2017/03/10 03:19:27, nhiroki (slow) wrote: > ditto. Done. https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:255: template <WTF::FunctionThreadAffinity threadAfinity> On 2017/03/10 03:19:27, nhiroki (slow) wrote: > s/threadAfinity/threadAffinity Done. https://codereview.chromium.org/2741643003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThread.h:257: std::unique_ptr<Function<void(), threadAfinity>> task); On 2017/03/10 03:19:27, nhiroki (slow) wrote: > ditto Done.
Description was changed from ========== Replace WorkerGlobalScope::postTask with WorkerThread::postTask ExecutionContextTask and ExecutionContext::postTask are deprecated and will be removed. This patch changes WorkerEventQueue to post tasks directly through an underlying thread of WorkerGlobalScope. The behavior remains the same. BUG=625927 ========== to ========== Replace WorkerGlobalScope::postTask with WorkerThread::postTask ExecutionContextTask and ExecutionContext::postTask are deprecated and will be removed. This patch changes WorkerEventQueue to post tasks directly through an underlying thread of WorkerGlobalScope as well as adding a single thread version of WorkerThread::postTask. The behavior remains the same. BUG=625927 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
lgtm
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 yuryu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2741643003/#ps40001 (title: "missppelling; include cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489393962751060,
"parent_rev": "5c3da10abd95b5410a7673503a9c6fde91b4fe96", "commit_rev":
"4e533644399b42111d85ab607219494ed719f0cb"}
Message was sent while issue was closed.
Description was changed from ========== Replace WorkerGlobalScope::postTask with WorkerThread::postTask ExecutionContextTask and ExecutionContext::postTask are deprecated and will be removed. This patch changes WorkerEventQueue to post tasks directly through an underlying thread of WorkerGlobalScope as well as adding a single thread version of WorkerThread::postTask. The behavior remains the same. BUG=625927 ========== to ========== Replace WorkerGlobalScope::postTask with WorkerThread::postTask ExecutionContextTask and ExecutionContext::postTask are deprecated and will be removed. This patch changes WorkerEventQueue to post tasks directly through an underlying thread of WorkerGlobalScope as well as adding a single thread version of WorkerThread::postTask. The behavior remains the same. BUG=625927 Review-Url: https://codereview.chromium.org/2741643003 Cr-Commit-Position: refs/heads/master@{#456337} Committed: https://chromium.googlesource.com/chromium/src/+/4e533644399b42111d85ab607219... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4e533644399b42111d85ab607219... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
