Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2851593002:
Use ScopedTaskEnvironment instead of MessageLoopForUI in chrome tests. (Closed)
|
Created:
3 years, 7 months ago by fdoray Modified:
3 years, 7 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, vabr+watchlistpasswordmanager_chromium.org, jdonnelly+watch_chromium.org, tfarina, dcheng, oshima+watch_chromium.org, mac-reviews_chromium.org, chromium-apps-reviews_chromium.org, gcasto+watchlist_chromium.org, miu+watch_chromium.org, davemoore+watch_chromium.org, Matt Giuca Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse ScopedTaskEnvironment instead of MessageLoopForUI in chrome tests.
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/2851593002
Cr-Commit-Position: refs/heads/master@{#468608}
Committed: https://chromium.googlesource.com/chromium/src/+/72a220118ee319f86c84d6062ceae9200cb9e710
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : self-review #Patch Set 4 : self-review #
Total comments: 10
Patch Set 5 : CR #Patch Set 6 : fix-build-errors #Patch Set 7 : revert-image_writer_private_apitest #Patch Set 8 : revert_extension_user_script_loader_unittest #Patch Set 9 : fix-build-error #Messages
Total messages: 49 (39 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
gab@: PTAL The change will then be TBRed to the owner.
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 checked by fdoray@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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader_unittest.cc (left): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader_unittest.cc:58: // ExtensionUserScriptLoader posts tasks to the file thread so make the Is this the same with ScopedTaskEvironment? https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/alert_indicator_button_cocoa_unittest.mm (right): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/alert_indicator_button_cocoa_unittest.mm:88: scoped_task_environment_; // Needed for gfx::Animation. Move the comment above the member. https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell_unittest.mm (right): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell_unittest.mm:36: scoped_task_environment_; // Needed for gfx::Animation. Move the comment above the member. https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/clipboard_utils_unittest.cc (right): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/clipboard_utils_unittest.cc:5: #include "chrome/browser/ui/omnibox/clipboard_utils.h" newline between 5/6.
Description was changed from ========== Use ScopedTaskEnvironment instead of MessageLoopForUI in chrome tests. 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-6VLykYXyl3P... BUG=708584 R=gab@chromium.org TBR=sky@chromium.org ========== to ========== Use ScopedTaskEnvironment/TestBrowserThreadBundle instead of MessageLoopForUI in chrome tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in every test that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). TestBrowserThreadBundle is a ScopedTaskEnvironment with MainThreadType::UI that also provides browser threads. Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 R=gab@chromium.org TBR=sky@chromium.org ==========
The CQ bit was checked by fdoray@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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...
Please take another look https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader_unittest.cc (left): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader_unittest.cc:58: // ExtensionUserScriptLoader posts tasks to the file thread so make the On 2017/04/27 22:17:46, sky wrote: > Is this the same with ScopedTaskEvironment? I added a TestBrowserThreadBundle instead of a ScopedTaskEnvironment to this test. It runs all the existing types of BrowserThreads on the main thread (equivalent to instantiating multiple content::TestBrowserThread). https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tabs/alert_indicator_button_cocoa_unittest.mm (right): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tabs/alert_indicator_button_cocoa_unittest.mm:88: scoped_task_environment_; // Needed for gfx::Animation. On 2017/04/27 22:17:46, sky wrote: > Move the comment above the member. Done. https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell_unittest.mm (right): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell_unittest.mm:36: scoped_task_environment_; // Needed for gfx::Animation. On 2017/04/27 22:17:46, sky wrote: > Move the comment above the member. Done. https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/omnib... File chrome/browser/ui/omnibox/clipboard_utils_unittest.cc (right): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/ui/omnib... chrome/browser/ui/omnibox/clipboard_utils_unittest.cc:5: #include "chrome/browser/ui/omnibox/clipboard_utils.h" On 2017/04/27 22:17:46, sky wrote: > newline between 5/6. Done.
LGTM - but get a local owner for the extension change https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader_unittest.cc (left): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader_unittest.cc:58: // ExtensionUserScriptLoader posts tasks to the file thread so make the On 2017/05/01 17:44:59, fdoray wrote: > On 2017/04/27 22:17:46, sky wrote: > > Is this the same with ScopedTaskEvironment? > > I added a TestBrowserThreadBundle instead of a ScopedTaskEnvironment to this > test. It runs all the existing types of BrowserThreads on the main thread > (equivalent to instantiating multiple content::TestBrowserThread). I'm not sure that is the same thing. Could you get a local owner to make sure that is appropriate.
The CQ bit was checked by fdoray@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...
Description was changed from ========== Use ScopedTaskEnvironment/TestBrowserThreadBundle instead of MessageLoopForUI in chrome tests. ScopedTaskEnvironment allows usage of ThreadTaskRunnerHandle and base/task_scheduler/post_task.h within its scope. It should be instantiated in every test that uses either of these APIs (i.e. no test should instantiate a MessageLoop directly). TestBrowserThreadBundle is a ScopedTaskEnvironment with MainThreadType::UI that also provides browser threads. Motivation for ScopedTaskEnvironment can be found in: https://docs.google.com/document/d/1QabRo8c7D9LsYY3cEcaPQbOCLo8Tu-6VLykYXyl3P... BUG=708584 R=gab@chromium.org TBR=sky@chromium.org ========== to ========== Use ScopedTaskEnvironment instead of MessageLoopForUI in chrome tests. 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-6VLykYXyl3P... BUG=708584 ==========
The CQ bit was unchecked by fdoray@chromium.org
https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_user_script_loader_unittest.cc (left): https://codereview.chromium.org/2851593002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_user_script_loader_unittest.cc:58: // ExtensionUserScriptLoader posts tasks to the file thread so make the On 2017/05/01 18:07:07, sky wrote: > On 2017/05/01 17:44:59, fdoray wrote: > > On 2017/04/27 22:17:46, sky wrote: > > > Is this the same with ScopedTaskEvironment? > > > > I added a TestBrowserThreadBundle instead of a ScopedTaskEnvironment to this > > test. It runs all the existing types of BrowserThreads on the main thread > > (equivalent to instantiating multiple content::TestBrowserThread). > > I'm not sure that is the same thing. Could you get a local owner to make sure > that is appropriate. I'll do this part in a separate CL.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2851593002/#ps140001 (title: "revert_extension_user_script_loader_unittest")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@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 fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2851593002/#ps160001 (title: "fix-build-error")
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": 160001, "attempt_start_ts": 1493726409518320,
"parent_rev": "0eb55d6c752c6046493edcaf4fbcf222712211e9", "commit_rev":
"72a220118ee319f86c84d6062ceae9200cb9e710"}
Message was sent while issue was closed.
Description was changed from ========== Use ScopedTaskEnvironment instead of MessageLoopForUI in chrome tests. 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-6VLykYXyl3P... BUG=708584 ========== to ========== Use ScopedTaskEnvironment instead of MessageLoopForUI in chrome tests. 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-6VLykYXyl3P... BUG=708584 Review-Url: https://codereview.chromium.org/2851593002 Cr-Commit-Position: refs/heads/master@{#468608} Committed: https://chromium.googlesource.com/chromium/src/+/72a220118ee319f86c84d6062cea... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/72a220118ee319f86c84d6062cea... |
