Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2852373004: Use ScopedTaskEnvironment instead of MessageLoop in tests that use v8. (Closed)

Created:
1 year, 10 months ago by fdoray
Modified:
1 year, 10 months ago
CC:
Aaron Boodman, abarth-chromium, chromium-apps-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, extensions-reviews_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use ScopedTaskEnvironment instead of MessageLoop in tests that use v8. This is a prerequisite to migrate v8 from base::WorkerPool to base::TaskScheduler. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in everytest that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3Pkk/edit BUG=708584 Review-Url: https://codereview.chromium.org/2852373004 Cr-Commit-Position: refs/heads/master@{#470007} Committed: https://chromium.googlesource.com/chromium/src/+/09a43efb68e4412a8e091272df60eef6f50ac355

Patch Set 1 #

Patch Set 2 : self-review #

Patch Set 3 : fix-hang #

Total comments: 5

Patch Set 4 : CR-gab-12 #

Total comments: 3

Patch Set 5 : Reset-RenderViewTest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -76 lines) Patch
M chrome/test/base/v8_unit_test.h View 3 chunks +3 lines, -6 lines 0 comments Download
M chrome/test/base/v8_unit_test.cc View 2 chunks +0 lines, -3 lines 0 comments Download
M content/test/blink_test_environment.cc View 1 2 3 3 chunks +13 lines, -21 lines 0 comments Download
M content/test/ppapi_unittest.h View 2 chunks +5 lines, -9 lines 0 comments Download
M content/test/ppapi_unittest.cc View 3 chunks +0 lines, -4 lines 0 comments Download
M extensions/renderer/api_binding_test.h View 2 chunks +3 lines, -2 lines 0 comments Download
M extensions/renderer/gc_callback_unittest.cc View 1 3 chunks +5 lines, -5 lines 0 comments Download
M gin/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M gin/modules/timer_unittest.cc View 1 2 5 chunks +12 lines, -18 lines 0 comments Download
M gin/shell_runner_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M gin/test/file_runner.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M gin/test/v8_test.h View 2 chunks +2 lines, -2 lines 0 comments Download
M mojo/edk/js/tests/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M mojo/edk/js/tests/js_to_cpp_tests.cc View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 31 (22 generated)
fdoray
PTAL
1 year, 10 months ago (2017-05-02 21:19:43 UTC) #3
gab
lgtm but make sure OWNERS take a look at this one as they are some ...
1 year, 10 months ago (2017-05-03 17:38:36 UTC) #12
fdoray
sky@chromium.org: Please review changes in chrome/test/base/, content/public/test/, content/test/, mojo/ jochen@chromium.org: Please review changes in extensions/, ...
1 year, 10 months ago (2017-05-03 20:48:44 UTC) #18
sky
https://codereview.chromium.org/2852373004/diff/60001/content/public/test/render_view_test.cc File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/2852373004/diff/60001/content/public/test/render_view_test.cc#newcode159 content/public/test/render_view_test.cc:159: void RenderViewTest::ProcessPendingMessages() { How about removing this function entirely ...
1 year, 10 months ago (2017-05-04 02:58:55 UTC) #19
jochen (gone - plz use gerrit)
gin and extensions lgtm
1 year, 10 months ago (2017-05-05 14:26:52 UTC) #20
fdoray
sky@: Please take another look. https://codereview.chromium.org/2852373004/diff/60001/content/public/test/render_view_test.cc File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/2852373004/diff/60001/content/public/test/render_view_test.cc#newcode159 content/public/test/render_view_test.cc:159: void RenderViewTest::ProcessPendingMessages() { On ...
1 year, 10 months ago (2017-05-08 15:22:08 UTC) #23
sky
LGTM
1 year, 10 months ago (2017-05-08 16:12:22 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2852373004/80001
1 year, 10 months ago (2017-05-08 16:15:56 UTC) #28
commit-bot: I haz the power
1 year, 10 months ago (2017-05-08 16:27:11 UTC) #31
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/09a43efb68e4412a8e091272df60...

Powered by Google App Engine
This is Rietveld 408576698