Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335343006:
Worker: Refine WorkerThreadTest (Closed)
|
Created:
4 years, 3 months ago by nhiroki Modified:
4 years, 3 months ago CC:
chromium-reviews, shimazu+worker_chromium.org, falken, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Refine WorkerThreadTest
This CL...
- shortens variable names for improving readability,
- renames tests with more concise naming convention:
<termination_mode>_<termination_timing>
- doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning
(StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and
- makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead
of simulating it to remove a test-only lifecycle observer implementation.
BUG=640843
Committed: https://crrev.com/8def110d7293b51fcdf0424a212b45d6ed33d24e
Cr-Commit-Position: refs/heads/master@{#419698}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address review comments #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Worker: Refine WorkerThreadTest BUG= ========== to ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames test names with more concise naming conventions: <termination_mode>_<termination_timing> - doesn't change test contents other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating the debugger task is running to remove the test-only util (WorkerThreadLifecycleObserverForTest). BUG=640843 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames test names with more concise naming conventions: <termination_mode>_<termination_timing> - doesn't change test contents other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating the debugger task is running to remove the test-only util (WorkerThreadLifecycleObserverForTest). BUG=640843 ========== to ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test contents other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating the debugger task is running to remove the test-only util (WorkerThreadLifecycleObserverForTest). BUG=640843 ==========
Description was changed from ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test contents other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating the debugger task is running to remove the test-only util (WorkerThreadLifecycleObserverForTest). BUG=640843 ========== to ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating the debugger task is running to remove the test-only util (WorkerThreadLifecycleObserverForTest). BUG=640843 ==========
Description was changed from ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating the debugger task is running to remove the test-only util (WorkerThreadLifecycleObserverForTest). BUG=640843 ========== to ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating it to remove a test-only lifecycle observer implementation. BUG=640843 ==========
The CQ bit was checked by nhiroki@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...
nhiroki@chromium.org changed reviewers: + falken@chromium.org, yhirano@chromium.org
Hi, can you review this? This CL is a preparation for subsequent CLs to make these tests run more deterministically for https://codereview.chromium.org/2280523002/ Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping :)
lgtm. It's a bit of a pain, but it'd be easier to review if function reordering is done in a separate patch or patchset (so you can view the deltas). I trust the description in the CL description. https://codereview.chromium.org/2335343006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2335343006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:24: // Used as a debugger task. Wait for a signal from the main thread. nit: Wait -> Waits. https://codereview.chromium.org/2335343006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:172: TEST_F(WorkerThreadTest, AsyncTerminate_ImeediatelyAfterStart) Immediately
On 2016/09/20 06:47:22, falken wrote: > lgtm. It's a bit of a pain, but it'd be easier to review if function reordering > is done in a separate patch or patchset (so you can view the deltas). I trust > the description in the CL description. Thanks for your comments. I'll do so in the next time.
https://codereview.chromium.org/2335343006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2335343006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:24: // Used as a debugger task. Wait for a signal from the main thread. On 2016/09/20 06:47:22, falken wrote: > nit: Wait -> Waits. Done. https://codereview.chromium.org/2335343006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:172: TEST_F(WorkerThreadTest, AsyncTerminate_ImeediatelyAfterStart) On 2016/09/20 06:47:22, falken wrote: > Immediately Done.
yhirano@, you look busy and this shouldn't be a harmful change, so let me land this without your review. If you have any concern etc, feel free to stop the CQ. Thank you!
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2335343006/#ps60001 (title: "address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Sorry for the late response.
On 2016/09/20 07:18:45, yhirano (slow) wrote: > lgtm > > Sorry for the late response. No problem! Thank you for reviewing :)
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating it to remove a test-only lifecycle observer implementation. BUG=640843 ========== to ========== Worker: Refine WorkerThreadTest This CL... - shortens variable names for improving readability, - renames tests with more concise naming convention: <termination_mode>_<termination_timing> - doesn't change test bodies other than Terminate_WhileDebuggerTaskIsRunning (StartAndTerminateOnScriptLoaded_TerminateWhileDebuggerTaskIsRunning), and - makes Terminate_WhileDebuggerTaskIsRunning to run a real debugger task instead of simulating it to remove a test-only lifecycle observer implementation. BUG=640843 Committed: https://crrev.com/8def110d7293b51fcdf0424a212b45d6ed33d24e Cr-Commit-Position: refs/heads/master@{#419698} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8def110d7293b51fcdf0424a212b45d6ed33d24e Cr-Commit-Position: refs/heads/master@{#419698} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
