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

Issue 958523002: ServiceWorker: Use scheduler's default task queue for posting tasks on main thread (Closed)

Created:
5 years, 10 months ago by Kunihiko Sakamoto
Modified:
5 years, 10 months ago
Reviewers:
kinuko, Sami
CC:
chromium-reviews, michaeln, mlamouri+watch-content_chromium.org, tzik, serviceworker-reviews, jam, jsbell+serviceworker_chromium.org, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, kinuko+serviceworker, kinuko+watch
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

ServiceWorker: Use scheduler's default task queue for posting tasks on main thread This fixes a task ordering bug between WebMessagePortChannelImpl and ServiceWorkerScriptContext. The former posts tasks via the Blink scheduler's default task queue, and the latter was using the MessageLoop directly. This patch makes tasks from service worker to the main thread go through the scheduler. BUG=460833 TEST=http/tests/serviceworker/postmessage-msgport-to-client.html Committed: https://crrev.com/a7fe515e3ca07e260b58ebf460f842601266ed87 Cr-Commit-Position: refs/heads/master@{#318022}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -15 lines) Patch
M content/renderer/service_worker/embedded_worker_context_client.h View 3 chunks +4 lines, -4 lines 0 comments Download
M content/renderer/service_worker/embedded_worker_context_client.cc View 5 chunks +5 lines, -5 lines 0 comments Download
M content/renderer/service_worker/service_worker_script_context.cc View 4 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 11 (2 generated)
Kunihiko Sakamoto
5 years, 10 months ago (2015-02-25 06:26:10 UTC) #2
kinuko
lgtm
5 years, 10 months ago (2015-02-25 07:00:54 UTC) #3
Sami
lgtm, thanks!
5 years, 10 months ago (2015-02-25 10:54:48 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/958523002/1
5 years, 10 months ago (2015-02-25 10:56:33 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 10 months ago (2015-02-25 10:59:20 UTC) #7
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/a7fe515e3ca07e260b58ebf460f842601266ed87 Cr-Commit-Position: refs/heads/master@{#318022}
5 years, 10 months ago (2015-02-25 11:00:02 UTC) #8
phoglund_chromium
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/954993002/ by phoglund@chromium.org. ...
5 years, 10 months ago (2015-02-25 14:00:33 UTC) #9
phoglund_chromium
Win7 appears to green up, so I think this revert may have been correct. I ...
5 years, 10 months ago (2015-02-25 14:32:19 UTC) #10
phoglund_chromium
5 years, 10 months ago (2015-02-25 16:25:55 UTC) #11
Message was sent while issue was closed.
On 2015/02/25 14:32:19, phoglund wrote:
> Win7 appears to green up, so I think this revert may have been correct. I can
> also see the same test break on win xp; let's see if those bots cycle green as
> well after they pick up the revert.

Hmm, no, I just saw another flake of the same test on the Vista bot, so I'll
reinstate this patch.

Powered by Google App Engine
This is Rietveld 408576698