Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2518403005:
Correct EmbeddedTestServer usage in service worker browsertests. (Closed)
|
Created:
4 years ago by martijnc Modified:
4 years ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCorrect EmbeddedTestServer usage in service worker browsertests.
Adding EmbeddedTestServer handlers after the server has started isn't
thread safe.
BUG=546060
Committed: https://crrev.com/97c1bf6a19348cbac3b43a28e5572d6383fbe835
Cr-Commit-Position: refs/heads/master@{#435060}
Patch Set 1 #
Total comments: 2
Patch Set 2 : comment #
Total comments: 2
Patch Set 3 : comments falken #Messages
Total messages: 16 (7 generated)
martijn@martijnc.be changed reviewers: + michaeln@chromium.org
Can you review this patch? Because there are some tests that add their own request handlers the server cannot be started in SetupOnMainThread(). This makes that all tests now have to add any handlers before starting the test server themselves, resulting in a lot of changes.
shimazu@chromium.org changed reviewers: + shimazu@chromium.org
https://codereview.chromium.org/2518403005/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2518403005/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_browsertest.cc:409: void StartServerAndNavigateToSetup() { drive-by: Could you add a comment when StartServerAndNavigateToSetup should be called?
https://codereview.chromium.org/2518403005/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2518403005/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_browsertest.cc:409: void StartServerAndNavigateToSetup() { On 2016/11/24 at 05:47:35, shimazu wrote: > drive-by: > Could you add a comment when StartServerAndNavigateToSetup should be called? Done.
shimazu@chromium.org changed reviewers: + falken@chromium.org
non-owner lgtm +falken, who is now actively working on SW codes
lgtm, thanks for fixing this https://codereview.chromium.org/2518403005/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2518403005/diff/20001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:412: void StartServerAndNavigateToSetup() { Might it be good to DCHECK_CURRENTLY_ON(BrowserThread::UI) here or mention this is called on the main thread in the comments?
https://codereview.chromium.org/2518403005/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_browsertest.cc (right): https://codereview.chromium.org/2518403005/diff/20001/content/browser/service... content/browser/service_worker/service_worker_browsertest.cc:412: void StartServerAndNavigateToSetup() { On 2016/11/29 at 02:24:06, falken wrote: > Might it be good to DCHECK_CURRENTLY_ON(BrowserThread::UI) here or mention this is called on the main thread in the comments? Added.
The CQ bit was checked by martijn@martijnc.be
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, shimazu@chromium.org Link to the patchset: https://codereview.chromium.org/2518403005/#ps40001 (title: "comments falken")
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": 1480446363256690,
"parent_rev": "3d2a5f5b6e1fabe02136a0a6b8e08a01b57f5d4a", "commit_rev":
"2254c07522f2bd2db0903f5d50704f47ab45d030"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Correct EmbeddedTestServer usage in service worker browsertests. Adding EmbeddedTestServer handlers after the server has started isn't thread safe. BUG=546060 ========== to ========== Correct EmbeddedTestServer usage in service worker browsertests. Adding EmbeddedTestServer handlers after the server has started isn't thread safe. BUG=546060 Committed: https://crrev.com/97c1bf6a19348cbac3b43a28e5572d6383fbe835 Cr-Commit-Position: refs/heads/master@{#435060} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/97c1bf6a19348cbac3b43a28e5572d6383fbe835 Cr-Commit-Position: refs/heads/master@{#435060} |
