Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1063823007:
Stop Using MessageLoopProxy in Test Since It's Deprecated (Closed)
|
Created:
5 years, 8 months ago by weiliangc Modified:
5 years, 8 months ago Reviewers:
danakj CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, sievers+watch_chromium.org, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop Using MessageLoopProxy in Test Since It's Deprecated
Fix a recent use of MessageLoopProxy in test.
BUG=
Committed: https://crrev.com/24688c68a803ad34c4779b485f7ab8675e14b000
Cr-Commit-Position: refs/heads/master@{#326365}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 12 (2 generated)
weiliangc@chromium.org changed reviewers: + danakj@chromium.org
LGTM
https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... ui/compositor/compositor_unittest.cc:33: task_runner_ = base::ThreadTaskRunnerHandle::Get(); This might DCHECK if you didn't make a MessageLoop while this class is alive. Bots will show.
On 2015/04/22 at 19:34:04, danakj wrote: > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > File ui/compositor/compositor_unittest.cc (right): > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > ui/compositor/compositor_unittest.cc:33: task_runner_ = base::ThreadTaskRunnerHandle::Get(); > This might DCHECK if you didn't make a MessageLoop while this class is alive. Bots will show. Is there way to guarantee safely get? When is MessageLoop set up in unittest?
On Wed, Apr 22, 2015 at 12:37 PM, <weiliangc@chromium.org> wrote: > On 2015/04/22 at 19:34:04, danakj wrote: > > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > >> File ui/compositor/compositor_unittest.cc (right): >> > > > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > >> ui/compositor/compositor_unittest.cc:33: task_runner_ = >> > base::ThreadTaskRunnerHandle::Get(); > >> This might DCHECK if you didn't make a MessageLoop while this class is >> alive. >> > Bots will show. > > Is there way to guarantee safely get? When is MessageLoop set up in > unittest? > In tests I /think/ you have to just make a MessageLoop if you want one to exist. If you run this test locally does it dcheck? To guarantee it you need to have a MessageLoop in existence. An example: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co... > > https://codereview.chromium.org/1063823007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/22 at 19:39:22, danakj wrote: > On Wed, Apr 22, 2015 at 12:37 PM, <weiliangc@chromium.org> wrote: > > > On 2015/04/22 at 19:34:04, danakj wrote: > > > > > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > > > >> File ui/compositor/compositor_unittest.cc (right): > >> > > > > > > > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > > > >> ui/compositor/compositor_unittest.cc:33: task_runner_ = > >> > > base::ThreadTaskRunnerHandle::Get(); > > > >> This might DCHECK if you didn't make a MessageLoop while this class is > >> alive. > >> > > Bots will show. > > > > Is there way to guarantee safely get? When is MessageLoop set up in > > unittest? > > > > In tests I /think/ you have to just make a MessageLoop if you want one to > exist. If you run this test locally does it dcheck? To guarantee it you > need to have a MessageLoop in existence. > > An example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co... > No it doesn't DCHECK locally. Before this MessageLoopProxy::current() doesn't set up MessageLoop either, so this shouldn't be behavior difference? I thought maybe .reset throws away MessageLoop and set up a new one? > > > > > https://codereview.chromium.org/1063823007/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On 2015/04/22 19:42:35, weiliangc wrote: > On 2015/04/22 at 19:39:22, danakj wrote: > > On Wed, Apr 22, 2015 at 12:37 PM, <mailto:weiliangc@chromium.org> wrote: > > > > > On 2015/04/22 at 19:34:04, danakj wrote: > > > > > > > > > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > > > > > >> File ui/compositor/compositor_unittest.cc (right): > > >> > > > > > > > > > > > > > https://codereview.chromium.org/1063823007/diff/1/ui/compositor/compositor_un... > > > > > >> ui/compositor/compositor_unittest.cc:33: task_runner_ = > > >> > > > base::ThreadTaskRunnerHandle::Get(); > > > > > >> This might DCHECK if you didn't make a MessageLoop while this class is > > >> alive. > > >> > > > Bots will show. > > > > > > Is there way to guarantee safely get? When is MessageLoop set up in > > > unittest? > > > > > > > In tests I /think/ you have to just make a MessageLoop if you want one to > > exist. If you run this test locally does it dcheck? To guarantee it you > > need to have a MessageLoop in existence. > > > > An example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co... > > > > No it doesn't DCHECK locally. OK if it doesn't then you're good. The test suite probably sets up a loop.. oh yep it does https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/test... > > Before this MessageLoopProxy::current() doesn't set up MessageLoop either, so > this shouldn't be behavior difference? > > I thought maybe .reset throws away MessageLoop and set up a new one? > > > > > > > > > https://codereview.chromium.org/1063823007/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email to mailto:chromium-reviews+unsubscribe@chromium.org. > >
The CQ bit was checked by weiliangc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1063823007/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/24688c68a803ad34c4779b485f7ab8675e14b000 Cr-Commit-Position: refs/heads/master@{#326365} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
