Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2494943005:
[ash-md] Added a delay between system menu default/detailed view transitions. (Closed)
Description[ash-md] Added a delay between system menu default/detailed view transitions.
This change adds a delay before initiating the transtions to/from
the default/detailed views in the system menu in an effort to make the
ink drop animation on the clicked target more noticable.
BUG=664823
TEST=ash_unittests
Committed: https://crrev.com/eb01167a5b2d97d18446ca7b502761d658a7a995
Cr-Commit-Position: refs/heads/master@{#433366}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Merge branch 'master' into system_menu_transition_animation #Patch Set 3 : Addressed review comments. #
Total comments: 12
Patch Set 4 : Actually removed ScopedMockTaskRunnerWrapper. #Patch Set 5 : Addressed review comments from patch set 3. #
Total comments: 4
Patch Set 6 : Merge branch 'master' into system_menu_transition_animation #Patch Set 7 : Merge branch 'master' into system_menu_transition_animation #
Total comments: 4
Patch Set 8 : Addressed gab@'s comments. #
Total comments: 40
Patch Set 9 : Addressed gab@ review comments. #Patch Set 10 : Merge branch 'master' into system_menu_transition_animation #
Total comments: 7
Patch Set 11 : Addressed gab@ comments. #
Total comments: 10
Patch Set 12 : Addressed danakj@ comments. #Messages
Total messages: 60 (32 generated)
The CQ bit was checked by bruthig@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...
bruthig@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you PTAL? Also, I do have some concerns about this approach. Specifically what if the user clicks another surface during the delay period. Let's discuss in-person.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for ash/common/system https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_item.h (right): https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_item.h:2: // Use of this source code is governed by a BSD-style license that can be don't forget to have something in the BUG= line. https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/tray... ash/common/system/tray/tray_constants.cc:117: const int kTrayPopupTransitionToDefaultViewDelayMs[] = {0, 80, 100}; I'm OK with trying this approach, given that we have a really easy way to revert back to a zero delay in beta if needed (i.e., changing these values to 0).
Description was changed from ========== [ash-md] Added a delay between system menu default/detailed view transitions. This change adds a delay before initiating the transtions to/from the default/detailed views in the system menu in an effort to make the ink drop animation on the clicked target more noticable. BUG= TEST=ash_unittests ========== to ========== [ash-md] Added a delay between system menu default/detailed view transitions. This change adds a delay before initiating the transtions to/from the default/detailed views in the system menu in an effort to make the ink drop animation on the clicked target more noticable. BUG=664823 TEST=ash_unittests ==========
bruthig@chromium.org changed reviewers: + danakj@chromium.org
danakj@, can you take a look at: - base/test/*? https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/syst... File ash/common/system/tray/system_tray_item.h (right): https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/syst... ash/common/system/tray/system_tray_item.h:2: // Use of this source code is governed by a BSD-style license that can be On 2016/11/13 20:09:02, tdanderson wrote: > don't forget to have something in the BUG= line. Done. https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/tray... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2494943005/diff/1/ash/common/system/tray/tray... ash/common/system/tray/tray_constants.cc:117: const int kTrayPopupTransitionToDefaultViewDelayMs[] = {0, 80, 100}; On 2016/11/13 20:09:02, tdanderson wrote: > I'm OK with trying this approach, given that we have a really easy way to revert > back to a zero delay in beta if needed (i.e., changing these values to 0). Acknowledged.
danakj@chromium.org changed reviewers: + gab@chromium.org
This introduces a dependency on MessageLoop though which folks have been tirelessly working to remove. Maybe this one makes sense tho? +gab
On 2016/11/14 20:03:19, danakj wrote: > This introduces a dependency on MessageLoop though which folks have been > tirelessly working to remove. Maybe this one makes sense tho? +gab First I frowned because I'm trying to make https://codereview.chromium.org/2491613004/ sequence-friendly (i.e. beyond MessageLoop) and just found https://codereview.chromium.org/2484023002 which conflicts with it and uses TestMockTimeTaskRunner... so I want to make TestMockTimeTaskRunner sequence-friendly. But then I realized it's okay because ScopedMockTaskRunnerWrapper merely introduces a dependency that its user be on a MessageLoop, but TestMockTimeTaskRunner can still be sequence-friendly (MessageLoop is a subset of allowed sequences). tl;dr; lg % comments below on the API itself. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:273: ScopedMockTaskRunnerWrapper::ScopedMockTaskRunnerWrapper() { DCHECK(base::MessageLoop::current()); https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:273: ScopedMockTaskRunnerWrapper::ScopedMockTaskRunnerWrapper() { : task_runner_(new base::TestMockTimeTaskRunner), previous_task_runner_(base::ThreadTaskRunnerHandle::Get()) { https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:279: ScopedMockTaskRunnerWrapper::~ScopedMockTaskRunnerWrapper() { DCHECK(previous_task_runner_->RunsTasksOnCurrentThread()); (to confirm destructor is called on thread it originally overwrote) https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:281: task_runner_->ClearPendingTasks(); This feels wrong. Remaining tasks should be transferred to |previous_task_runner_| for continuation. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:282: base::MessageLoop::current()->SetTaskRunner(previous_task_runner_); ->SetTaskRunner(std::move(previous_task_runner_)); (and #include <utility> for std::move) https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:168: // A scoped wrapper around TestMockTimeTaskRunner that replaces message loop's s/message loop's task runner/MessageLoop::current()'s task runner (and consequently ThreadTaskRunnerHandle)/ https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:171: class ScopedMockTaskRunnerWrapper { Make name ML specific, i.e. ScopedMockMessageLoopTaskRunnerWrapper https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:174: virtual ~ScopedMockTaskRunnerWrapper(); non-virtual destructor for class with no virtual methods. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:176: base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); } const https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:179: scoped_refptr<base::TestMockTimeTaskRunner> task_runner_; const (will also require to move initialization to constructor's initializer list as suggested in .cc) https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:180: scoped_refptr<base::SingleThreadTaskRunner> previous_task_runner_; nit: no base:: prefix above nor in .cc file (this is in base:: already)
Thanks gab, your comments are great. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:171: class ScopedMockTaskRunnerWrapper { On 2016/11/15 19:52:01, gab wrote: > Make name ML specific, i.e. ScopedMockMessageLoopTaskRunnerWrapper I think this class could go in its own .h and .cc files also. 1 per file is the one true way.
The CQ bit was checked by bruthig@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...
Thanks for the comments danakj@ & gab@. I think I've addressed all concerns/requests. Can you please take another look? https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:273: ScopedMockTaskRunnerWrapper::ScopedMockTaskRunnerWrapper() { On 2016/11/15 19:52:01, gab wrote: > DCHECK(base::MessageLoop::current()); Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:273: ScopedMockTaskRunnerWrapper::ScopedMockTaskRunnerWrapper() { On 2016/11/15 19:52:01, gab wrote: > : task_runner_(new base::TestMockTimeTaskRunner), > previous_task_runner_(base::ThreadTaskRunnerHandle::Get()) { Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:279: ScopedMockTaskRunnerWrapper::~ScopedMockTaskRunnerWrapper() { On 2016/11/15 19:52:01, gab wrote: > DCHECK(previous_task_runner_->RunsTasksOnCurrentThread()); > > (to confirm destructor is called on thread it originally overwrote) Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:281: task_runner_->ClearPendingTasks(); On 2016/11/15 19:52:01, gab wrote: > This feels wrong. Remaining tasks should be transferred to > |previous_task_runner_| for continuation. Not exactly sure if I implemented what you were asking for here. Can you double check? Also let me know if there is a more efficient way of implementing TestMockTimeTaskRunner::TakePendingTasks() with std:swap() or something. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.cc:282: base::MessageLoop::current()->SetTaskRunner(previous_task_runner_); On 2016/11/15 19:52:01, gab wrote: > ->SetTaskRunner(std::move(previous_task_runner_)); > > (and #include <utility> for std::move) Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:168: // A scoped wrapper around TestMockTimeTaskRunner that replaces message loop's On 2016/11/15 19:52:01, gab wrote: > s/message loop's task runner/MessageLoop::current()'s task runner (and > consequently ThreadTaskRunnerHandle)/ Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:171: class ScopedMockTaskRunnerWrapper { On 2016/11/15 19:58:13, danakj wrote: > On 2016/11/15 19:52:01, gab wrote: > > Make name ML specific, i.e. ScopedMockMessageLoopTaskRunnerWrapper > > I think this class could go in its own .h and .cc files also. 1 per file is the > one true way. Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:171: class ScopedMockTaskRunnerWrapper { On 2016/11/15 19:52:01, gab wrote: > Make name ML specific, i.e. ScopedMockMessageLoopTaskRunnerWrapper Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:174: virtual ~ScopedMockTaskRunnerWrapper(); On 2016/11/15 19:52:01, gab wrote: > non-virtual destructor for class with no virtual methods. Done. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:176: base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); } On 2016/11/15 19:52:01, gab wrote: > const If we return a "const TestMockTimeTaskRunner*" then TrayDetailsViewTest won't be able to invoke FastForwardBy() on it. Or am I misunderstanding? https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:179: scoped_refptr<base::TestMockTimeTaskRunner> task_runner_; On 2016/11/15 19:52:01, gab wrote: > const (will also require to move initialization to constructor's initializer > list as suggested in .cc) Done, but not sure if you meant const scoped_refptr<TestMockTimeTaskRunner> OR scoped_refptr<const TestMockTimeTaskRunner>. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:180: scoped_refptr<base::SingleThreadTaskRunner> previous_task_runner_; On 2016/11/15 19:52:01, gab wrote: > nit: no base:: prefix above nor in .cc file (this is in base:: already) Done.
https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:173: class ScopedMockTaskRunnerWrapper { This moved to another file right? Why's it here too?
The CQ bit was checked by bruthig@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...
https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.h:173: class ScopedMockTaskRunnerWrapper { On 2016/11/15 21:56:37, danakj wrote: > This moved to another file right? Why's it here too? Just to make sure my reviewers are on the ball. Kudos, you passed! :P
https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:176: base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); } On 2016/11/15 21:52:45, bruthig wrote: > On 2016/11/15 19:52:01, gab wrote: > > const > > If we return a "const TestMockTimeTaskRunner*" then TrayDetailsViewTest won't be > able to invoke FastForwardBy() on it. Or am I misunderstanding? Sorry, I meant to make the method const, i.e. base::TestMockTimeTaskRunner* task_runner() const { return task_runner_.get(); } ^^^^^ https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:179: scoped_refptr<base::TestMockTimeTaskRunner> task_runner_; On 2016/11/15 21:52:45, bruthig wrote: > On 2016/11/15 19:52:01, gab wrote: > > const (will also require to move initialization to constructor's initializer > > list as suggested in .cc) > > Done, but not sure if you meant > const scoped_refptr<TestMockTimeTaskRunner> OR > scoped_refptr<const TestMockTimeTaskRunner>. const scoped_refptr<TestMockTimeTaskRunner> (i.e. pointer is not changed outside of initializer list -- whereas scoped_refptr<const TestMockTimeTaskRunner> makes for an unusable TaskRunner per PostTask() not being const) https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... File base/test/scoped_mock_message_loop_task_runner_wrapper.cc (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:19: #include "base/time/tick_clock.h" rm unused includes https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:23: // ScopedTestMockTimeTaskRunner ----------------------------------------------- not needed https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... File base/test/scoped_mock_message_loop_task_runner_wrapper.h (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.h:21: namespace base { empty line after namespace https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:14: #include "base/threading/thread_task_runner_handle.h" rm includes added for impl which moved (as well as impl itself) https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:185: } Use std::move like TestSimpleTaskRunner does? https://cs.chromium.org/chromium/src/base/test/test_simple_task_runner.cc?rcl...
The CQ bit was checked by bruthig@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...
Sorry for the haste in the previous 2 patch sets. Hopefully all is well now. Can you please take another look? Thanks a million for your quick turn around!! https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:176: base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); } On 2016/11/15 22:02:05, gab wrote: > On 2016/11/15 21:52:45, bruthig wrote: > > On 2016/11/15 19:52:01, gab wrote: > > > const > > > > If we return a "const TestMockTimeTaskRunner*" then TrayDetailsViewTest won't > be > > able to invoke FastForwardBy() on it. Or am I misunderstanding? > > Sorry, I meant to make the method const, i.e. > > base::TestMockTimeTaskRunner* task_runner() const { return task_runner_.get(); } > ^^^^^ Are you sure? The style guide advises not to make functions const if they are returning a non-const pointer to a member variable. https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:179: scoped_refptr<base::TestMockTimeTaskRunner> task_runner_; On 2016/11/15 22:02:05, gab wrote: > On 2016/11/15 21:52:45, bruthig wrote: > > On 2016/11/15 19:52:01, gab wrote: > > > const (will also require to move initialization to constructor's initializer > > > list as suggested in .cc) > > > > Done, but not sure if you meant > > const scoped_refptr<TestMockTimeTaskRunner> OR > > scoped_refptr<const TestMockTimeTaskRunner>. > > const scoped_refptr<TestMockTimeTaskRunner> > > (i.e. pointer is not changed outside of initializer list -- whereas > scoped_refptr<const TestMockTimeTaskRunner> makes for an unusable TaskRunner per > PostTask() not being const) Thx https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... File base/test/scoped_mock_message_loop_task_runner_wrapper.cc (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:19: #include "base/time/tick_clock.h" On 2016/11/15 22:02:05, gab wrote: > rm unused includes Done. https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:23: // ScopedTestMockTimeTaskRunner ----------------------------------------------- On 2016/11/15 22:02:05, gab wrote: > not needed Done. https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... File base/test/scoped_mock_message_loop_task_runner_wrapper.h (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.h:21: namespace base { On 2016/11/15 22:02:06, gab wrote: > empty line after namespace Done. https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:14: #include "base/threading/thread_task_runner_handle.h" On 2016/11/15 22:02:06, gab wrote: > rm includes added for impl which moved (as well as impl itself) Done. https://codereview.chromium.org/2494943005/diff/40001/base/test/test_mock_tim... base/test/test_mock_time_task_runner.cc:185: } On 2016/11/15 22:02:06, gab wrote: > Use std::move like TestSimpleTaskRunner does? > https://cs.chromium.org/chromium/src/base/test/test_simple_task_runner.cc?rcl... Unless I'm missing something that won't work because TestMockTimeTaskRunner::tasks_ is of type TaskPriorityQueue whereas TestSimpleTaskRunner::pending_tasks_ is of type std::deque<TestPendingTask>.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Can we add a test (given the non-trivial logic highlighted below to pass remaining tasks)? https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... File base/test/test_mock_time_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/1/base/test/test_mock_time_ta... base/test/test_mock_time_task_runner.h:176: base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); } On 2016/11/15 22:17:57, bruthig wrote: > On 2016/11/15 22:02:05, gab wrote: > > On 2016/11/15 21:52:45, bruthig wrote: > > > On 2016/11/15 19:52:01, gab wrote: > > > > const > > > > > > If we return a "const TestMockTimeTaskRunner*" then TrayDetailsViewTest > won't > > be > > > able to invoke FastForwardBy() on it. Or am I misunderstanding? > > > > Sorry, I meant to make the method const, i.e. > > > > base::TestMockTimeTaskRunner* task_runner() const { return task_runner_.get(); > } > > ^^^^^ > > Are you sure? The style guide advises not to make functions const if they are > returning a non-const pointer to a member variable. Ah interesting, had missed that part of it, please ignore me :) https://codereview.chromium.org/2494943005/diff/80001/base/test/scoped_mock_m... File base/test/scoped_mock_message_loop_task_runner_wrapper.cc (right): https://codereview.chromium.org/2494943005/diff/80001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:29: for (auto it = tasks.begin(); it != tasks.end(); ++it) 28-29 => for (const auto& pending_task : task_runner_->TakePendingTasks()) { ... } https://codereview.chromium.org/2494943005/diff/80001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:30: previous_task_runner_->PostDelayedTask(it->location, it->task, it->delay); Last parameter consider the time that has passed according to the mock, i.e.: pending_task.GetTimeToRun() - task_runner_->Now()
Actually, it just occurred to me while reading timer_unittest.cc (which also use a TickClock [1]), why do we even need to unset the TestMockTimeTaskRunner from the MessageLoop at the end of the test? There's a new MessageLoop for every test and as such cleaning up at the end of the test sounds futile? Since the MessageLoop will pump remaining ripe tasks (non-delayed or with expired delay) in its destructor, the only thing re-instantiating a TaskRunner with a real clock does is add non determinism as to whether scheduled delayed tasks will fire... [1] https://cs.chromium.org/chromium/src/base/timer/timer_unittest.cc?q=message_l...
On 2016/11/16 16:08:11, gab wrote: > Actually, it just occurred to me while reading timer_unittest.cc (which also use > a TickClock [1]), why do we even need to unset the TestMockTimeTaskRunner from > the MessageLoop at the end of the test? There's a new MessageLoop for every test > and as such cleaning up at the end of the test sounds futile? > > Since the MessageLoop will pump remaining ripe tasks (non-delayed or with > expired delay) in its destructor, the only thing re-instantiating a TaskRunner > with a real clock does is add non determinism as to whether scheduled delayed > tasks will fire... > > [1] > https://cs.chromium.org/chromium/src/base/timer/timer_unittest.cc?q=message_l... For those following at home: we tried this and it failed. I'm once again in favor of adding ScopedMockMessageLoopTaskRunnerWrapper. The failure is caused by some things grabbing ThreadTaskRunnerHandle early in AshTestBase (e.g. SequencedWorkerPool) and not being happy when posting to that doesn't work on shutdown.
https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... File base/test/scoped_mock_message_loop_task_runner_wrapper.cc (right): https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:21: MessageLoop::current()->SetTaskRunner(task_runner_); One more thing, add this above this line: RunLoop().RunUntilIdle(); to ensure that we process any initialization tasks posted to the MessageLoop by the fixture before replacing its TaskRunner. (and #include run_loop.h)
https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... File base/test/scoped_mock_message_loop_task_runner_wrapper.h (right): https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... base/test/scoped_mock_message_loop_task_runner_wrapper.h:30: class ScopedMockMessageLoopTaskRunnerWrapper { Also, just noticed this name (I'm thinking of using this in an unrelated CL of mine now :)). Let's just call it ScopedMockTimeMessageLoopTaskRunner? "Scoped" and "Wrapper" feel redundant and it's important to mention "Time" in there IMO.
The CQ bit was checked by bruthig@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...
gab@, can you take another look? https://codereview.chromium.org/2494943005/diff/80001/base/test/scoped_mock_m... File base/test/scoped_mock_message_loop_task_runner_wrapper.cc (right): https://codereview.chromium.org/2494943005/diff/80001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:29: for (auto it = tasks.begin(); it != tasks.end(); ++it) On 2016/11/15 23:16:59, gab wrote: > 28-29 => > > for (const auto& pending_task : task_runner_->TakePendingTasks()) { > ... > } Done. https://codereview.chromium.org/2494943005/diff/80001/base/test/scoped_mock_m... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:30: previous_task_runner_->PostDelayedTask(it->location, it->task, it->delay); On 2016/11/15 23:16:59, gab wrote: > Last parameter consider the time that has passed according to the mock, i.e.: > > pending_task.GetTimeToRun() - task_runner_->Now() Done. https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... File base/test/scoped_mock_message_loop_task_runner_wrapper.cc (right): https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... base/test/scoped_mock_message_loop_task_runner_wrapper.cc:21: MessageLoop::current()->SetTaskRunner(task_runner_); On 2016/11/17 19:35:54, gab wrote: > One more thing, add this above this line: > > RunLoop().RunUntilIdle(); > > to ensure that we process any initialization tasks posted to the MessageLoop by > the fixture before replacing its TaskRunner. > > (and #include run_loop.h) Done. https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... File base/test/scoped_mock_message_loop_task_runner_wrapper.h (right): https://codereview.chromium.org/2494943005/diff/120001/base/test/scoped_mock_... base/test/scoped_mock_message_loop_task_runner_wrapper.h:30: class ScopedMockMessageLoopTaskRunnerWrapper { On 2016/11/17 20:04:37, gab wrote: > Also, just noticed this name (I'm thinking of using this in an unrelated CL of > mine now :)). > > Let's just call it ScopedMockTimeMessageLoopTaskRunner? "Scoped" and "Wrapper" > feel redundant and it's important to mention "Time" in there IMO. Done.
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_...)
Very nice tests :), lvg thanks! https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.cc:12: #include "base/test/test_mock_time_task_runner.h" rm since it will be in header now https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:8: #include <stddef.h> rm https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:10: #include <memory> For scoped_refptr you don't need this one, you need "base/memory/ref_counted.h" https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:11: #include <queue> rm https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:12: #include <vector> rm https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:15: #include "base/single_thread_task_runner.h" nit: include only required in .cc (fwd-decl below is sufficient as type is only used in .cc) https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:16: #include "base/synchronization/lock.h" rm https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:17: #include "base/test/test_pending_task.h" rm https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:18: #include "base/threading/thread_checker.h" rm https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:19: #include "base/time/time.h" to .cc https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:23: class TestMockTimeTaskRunner; Since I can't imagine anyone using this class without also requiring TestMockTimeTaskRunner, how about we #include "base/test/test_mock_time_task_runner.h" in this file instead of .cc and alleviate the need for users to always include both headers. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:23: class TestMockTimeTaskRunner; Along similar lines of thoughts as the above how about we replace task_runner() with: TestMockTimeTaskRunner* operator->() { return task_runner_.get(); } (to allow scoped_task_runner_->FastForwardBy(...);) Then users don't need two members (your use case is simple but I'm just starting to use it for another unit test issue I'm trying to solve and I need the TestMockTimeTaskRunner* as a member to avoid verbose calls which is kind of redundant...). (can also keep the task_runner() method, e.g. for tests and for users that might care about handing off the TaskRunner to a sub-component) https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:29: // the end of the scope. Another thing I noticed when using this for my own use case is that it breaks RunLoop() (which still pumps messages from the MessageLoop's original queue). This should at least be noted in a comment here: // Note: RunLoop() will not work in the scope of a ScopedMockTimeMessageLoopTaskRunner, the underlying TestMockTimeTaskRunner's methods must be used instead to pump tasks. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:36: void DummyClosure() {} Use base::DoNothing() for this (without the base:: prefix since this test is in base:: :)) https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:60: : original_task_runner_(new TestMockTimeTaskRunner()), message_loop_() { No need for "message_loop_()", the same thing happens implicitly when omitted. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:62: } I generally tend to prefer inlining definitions in test fixture (eases readability and preventing inlining of calls -- the reason we define things out of line in general -- isn't a concern for classes in .cc files). https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:65: ~ScopedMockTimeMessageLoopTaskRunnerTest() {} Don't think you need an explicit destructor since it doesn't do anything more and the base class' is already virtual. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:94: const int task_11_delay = 11; Use constexpr TimeDelta task_1_delay = TimeDelta::FromSeconds(1); syntax for these constants and omit TimeDelta::FromSeconds() in computations inline below. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:115: EXPECT_EQ(size_t(2), pending_tasks.size()); s/size_t(2)/2U/ https://codereview.chromium.org/2494943005/diff/140001/base/test/test_mock_ti... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/test_mock_ti... base/test/test_mock_time_task_runner.cc:7: #include <algorithm> why?
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
gab@, thanks for the helpful comments. Can you take another look? BTW what does 'lvg' stand for? https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.cc:12: #include "base/test/test_mock_time_task_runner.h" On 2016/11/18 15:13:54, gab wrote: > rm since it will be in header now Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:8: #include <stddef.h> On 2016/11/18 15:13:54, gab wrote: > rm Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:10: #include <memory> On 2016/11/18 15:13:54, gab wrote: > For scoped_refptr you don't need this one, you need "base/memory/ref_counted.h" Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:11: #include <queue> On 2016/11/18 15:13:54, gab wrote: > rm Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:12: #include <vector> On 2016/11/18 15:13:54, gab wrote: > rm Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:15: #include "base/single_thread_task_runner.h" On 2016/11/18 15:13:54, gab wrote: > nit: include only required in .cc (fwd-decl below is sufficient as type is only > used in .cc) Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:16: #include "base/synchronization/lock.h" On 2016/11/18 15:13:54, gab wrote: > rm Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:17: #include "base/test/test_pending_task.h" On 2016/11/18 15:13:54, gab wrote: > rm Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:18: #include "base/threading/thread_checker.h" On 2016/11/18 15:13:54, gab wrote: > rm Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:19: #include "base/time/time.h" On 2016/11/18 15:13:54, gab wrote: > to .cc Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:23: class TestMockTimeTaskRunner; On 2016/11/18 15:13:54, gab wrote: > Since I can't imagine anyone using this class without also requiring > TestMockTimeTaskRunner, how about we > > #include "base/test/test_mock_time_task_runner.h" > > in this file instead of .cc and alleviate the need for users to always include > both headers. Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:23: class TestMockTimeTaskRunner; On 2016/11/18 15:13:54, gab wrote: > Along similar lines of thoughts as the above how about we replace task_runner() > with: > > TestMockTimeTaskRunner* operator->() { return task_runner_.get(); } > > (to allow scoped_task_runner_->FastForwardBy(...);) > > Then users don't need two members (your use case is simple but I'm just starting > to use it for another unit test issue I'm trying to solve and I need the > TestMockTimeTaskRunner* as a member to avoid verbose calls which is kind of > redundant...). > > (can also keep the task_runner() method, e.g. for tests and for users that might > care about handing off the TaskRunner to a sub-component) Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:29: // the end of the scope. On 2016/11/18 15:13:54, gab wrote: > Another thing I noticed when using this for my own use case is that it breaks > RunLoop() (which still pumps messages from the MessageLoop's original queue). > > This should at least be noted in a comment here: > > // Note: RunLoop() will not work in the scope of a > ScopedMockTimeMessageLoopTaskRunner, the underlying TestMockTimeTaskRunner's > methods must be used instead to pump tasks. Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:36: void DummyClosure() {} On 2016/11/18 15:13:55, gab wrote: > Use base::DoNothing() for this (without the base:: prefix since this test is in > base:: :)) Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:60: : original_task_runner_(new TestMockTimeTaskRunner()), message_loop_() { On 2016/11/18 15:13:55, gab wrote: > No need for "message_loop_()", the same thing happens implicitly when omitted. Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:62: } On 2016/11/18 15:13:54, gab wrote: > I generally tend to prefer inlining definitions in test fixture (eases > readability and preventing inlining of calls -- the reason we define things out > of line in general -- isn't a concern for classes in .cc files). Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:65: ~ScopedMockTimeMessageLoopTaskRunnerTest() {} On 2016/11/18 15:13:54, gab wrote: > Don't think you need an explicit destructor since it doesn't do anything more > and the base class' is already virtual. Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:94: const int task_11_delay = 11; On 2016/11/18 15:13:55, gab wrote: > Use > > constexpr TimeDelta task_1_delay = TimeDelta::FromSeconds(1); > > syntax for these constants and omit TimeDelta::FromSeconds() in computations > inline below. Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:115: EXPECT_EQ(size_t(2), pending_tasks.size()); On 2016/11/18 15:13:54, gab wrote: > s/size_t(2)/2U/ Done. https://codereview.chromium.org/2494943005/diff/140001/base/test/test_mock_ti... File base/test/test_mock_time_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/140001/base/test/test_mock_ti... base/test/test_mock_time_task_runner.cc:7: #include <algorithm> On 2016/11/18 15:13:55, gab wrote: > why? Not sure.... removed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Awesome work, base/test lgtm w/ nits PS: lvg == looks very good (I usually use it to say: "barring proper resolution of comments this lgtm" when there are too many comments to lgtm w/ nits) https://codereview.chromium.org/2494943005/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_details_view_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_details_view_unittest.cc:142: scoped_task_runner_.task_runner()->FastForwardBy( scoped_task_runner_->FastForwardBy() should work now :) https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:19: // the end of the scope. s/the scope/its scope/ https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:95: scoped_task_runner_->task_runner()->FastForwardBy(step_time_by); scoped_task_runner_->FastForwardBy()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2494943005/diff/180001/ash/common/system/tray... File ash/common/system/tray/tray_details_view_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/180001/ash/common/system/tray... ash/common/system/tray/tray_details_view_unittest.cc:142: scoped_task_runner_.task_runner()->FastForwardBy( On 2016/11/18 18:18:54, gab wrote: > scoped_task_runner_->FastForwardBy() > > should work now :) Done. https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.h (right): https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.h:19: // the end of the scope. On 2016/11/18 18:18:54, gab wrote: > s/the scope/its scope/ Done. https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:95: scoped_task_runner_->task_runner()->FastForwardBy(step_time_by); On 2016/11/18 18:18:54, gab wrote: > scoped_task_runner_->FastForwardBy() It doesn't work here because |scoped_task_runner_| is a unique_ptr and the '->' operator is returning the raw pointer. I'm not sure how to change it so that the desired syntax works.
danakj@, can you provide an OWNER's review?
The CQ bit was checked by bruthig@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...
lgtm++ https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/180001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:95: scoped_task_runner_->task_runner()->FastForwardBy(step_time_by); On 2016/11/18 18:47:59, bruthig wrote: > On 2016/11/18 18:18:54, gab wrote: > > scoped_task_runner_->FastForwardBy() > > It doesn't work here because |scoped_task_runner_| is a unique_ptr and the '->' > operator is returning the raw pointer. I'm not sure how to change it so that > the desired syntax works. Ah ok, you could do scoped_task_runner_.get()-> I guess but that's weird and given it's specific to this test it's fine to ignore this (no user of this mock would do this IMO).
LGTM https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.cc:22: RunLoop().RunUntilIdle(); Can you leave a comment explaining this? https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:37: } // namespace nit: wrapping the entire unit test file in the anon namespace is nice https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:63: std::unique_ptr<ScopedMockTimeMessageLoopTaskRunner> scoped_task_runner_ = can use auto if you want https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:65: nit: every second line is whitespace seems excessive just remove it all? https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:75: std::unique_ptr<ScopedMockTimeMessageLoopTaskRunner> scoped_task_runner_ = can use auto if you want
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bruthig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2494943005/#ps210001 (title: "Addressed danakj@ comments.")
https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner.cc (right): https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner.cc:22: RunLoop().RunUntilIdle(); On 2016/11/18 21:27:21, danakj wrote: > Can you leave a comment explaining this? Done. https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... File base/test/scoped_mock_time_message_loop_task_runner_unittest.cc (right): https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:37: } // namespace On 2016/11/18 21:27:21, danakj wrote: > nit: wrapping the entire unit test file in the anon namespace is nice Done, but why? https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:63: std::unique_ptr<ScopedMockTimeMessageLoopTaskRunner> scoped_task_runner_ = On 2016/11/18 21:27:21, danakj wrote: > can use auto if you want Done. https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:65: On 2016/11/18 21:27:21, danakj wrote: > nit: every second line is whitespace seems excessive just remove it all? Done. https://codereview.chromium.org/2494943005/diff/190001/base/test/scoped_mock_... base/test/scoped_mock_time_message_loop_task_runner_unittest.cc:75: std::unique_ptr<ScopedMockTimeMessageLoopTaskRunner> scoped_task_runner_ = On 2016/11/18 21:27:21, danakj wrote: > can use auto if you want Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Added a delay between system menu default/detailed view transitions. This change adds a delay before initiating the transtions to/from the default/detailed views in the system menu in an effort to make the ink drop animation on the clicked target more noticable. BUG=664823 TEST=ash_unittests ========== to ========== [ash-md] Added a delay between system menu default/detailed view transitions. This change adds a delay before initiating the transtions to/from the default/detailed views in the system menu in an effort to make the ink drop animation on the clicked target more noticable. BUG=664823 TEST=ash_unittests ==========
Message was sent while issue was closed.
Committed patchset #12 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Added a delay between system menu default/detailed view transitions. This change adds a delay before initiating the transtions to/from the default/detailed views in the system menu in an effort to make the ink drop animation on the clicked target more noticable. BUG=664823 TEST=ash_unittests ========== to ========== [ash-md] Added a delay between system menu default/detailed view transitions. This change adds a delay before initiating the transtions to/from the default/detailed views in the system menu in an effort to make the ink drop animation on the clicked target more noticable. BUG=664823 TEST=ash_unittests Committed: https://crrev.com/eb01167a5b2d97d18446ca7b502761d658a7a995 Cr-Commit-Position: refs/heads/master@{#433366} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/eb01167a5b2d97d18446ca7b502761d658a7a995 Cr-Commit-Position: refs/heads/master@{#433366} |
