Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11453012:
Fix black background when locking with fullscreen window: (Closed)
|
Created:
8 years ago by Denis Kuznetsov (DE-MUC) Modified:
8 years ago CC:
jar (doing other things), chromium-reviews, piman+watch_chromium.org, jonathan.backer, sadrul, ben+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix black background when locking with fullscreen window:
Store background container visibility before lock animation, animate it's visibility on lock/unlock.
Change launcher to animate opacity-only (no size transformations).
Update tests so that handle new cases, and become more readable.
Also contains two lock animation fixes:
1) Shutdown animation now have correct cancel timing (it was tarting actual shutdown too early)
2) Flickering on starting and cancelling lock was removed, and tests were updated.
BUG=162646, 162645
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173461
Patch Set 1 #Patch Set 2 : Self-review #Patch Set 3 : Merge with ToT #
Total comments: 4
Patch Set 4 : Explicit float constants #Patch Set 5 : Add tests for no flickering on undoing animations + fix code #Patch Set 6 : Add fix for shutdown cut-off timing #
Total comments: 8
Patch Set 7 : set preemtion strategy for lock animation #
Total comments: 2
Patch Set 8 : Mock TimeTicks::Now() #
Total comments: 13
Patch Set 9 : Fix review comments, move test-related thigs to test_support targets #Patch Set 10 : Tune animation timings #
Total comments: 6
Patch Set 11 : Remove virtual from destructor #Patch Set 12 : Merge with ToT #Patch Set 13 : Add OVERRIDE #Patch Set 14 : Fix another win compilation warning #Patch Set 15 : Move AnimationFinishedObserver impl to SessionStateController #Patch Set 16 : Fix test (release power button) #Patch Set 17 : Use ease-in for shutdown animation #Patch Set 18 : Another attempt to fix win build #
Total comments: 3
Patch Set 19 : Remove TimeTicks:HighResNow mocking, more comments and checks #Patch Set 20 : And another one change for build on Win #
Total comments: 23
Patch Set 21 : And another one change for build on Win #
Total comments: 5
Patch Set 22 : Merge with ToT #Patch Set 23 : Fix review comments #Patch Set 24 : Comment for TransformWindowToBaseState #
Total comments: 2
Patch Set 25 : Merge with ToT again #Patch Set 26 : Add comment about lock/unlock workflow #
Total comments: 2
Patch Set 27 : Rename undo/stop to cancel #
Total comments: 21
Patch Set 28 : Final fixes #Patch Set 29 : Remove stuff for testing and disable some tests" #
Messages
Total messages: 52 (0 generated)
Ian for ui/compositor/ changes (new observers, TestHelper creation in LayerAnimator) Scott for ui/base/ (AnimationContainerTestHelper) and ash/wm/session_state_controller_impl2 changes (store information about background layer while locking). Nikita for detailed look on ash/wm/session_state_controller_impl2_unittest.cc
https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... File ui/compositor/layer_animation_observer.h (right): https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... ui/compositor/layer_animation_observer.h:137: // layer or the mixture of both. I worry about the addition of so many special purpose observers here. It would be nice if we could generalize the LayerAnimationObserver or ImplicitAnimationObserver to meet your needs. Perhaps, OnLayerAnimationFoo could have empty definitions rather than being pure virtual, and we could add SetStartedCallback(...), SetAbortedCallback(...), etc, to the LayerAnimationObserver interface? Pause/Unpause could go there as well. The creation of your observer would take two lines rather than one (to install the callback), but should pretty similar otherwise, right? https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... File ui/compositor/layer_animator.h (right): https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... ui/compositor/layer_animator.h:201: CreateAnimatorHelperForTest(); If these interfere with CreateAnimatorHelperForTest, perhaps we could we deprecate them and find a way to accomplish the same things with the animation container test helper? Leaving around multiple ways to control animations in tests seems like a bad idea. sky@, I should mention that I'd proposed the following alternate approach for controlling animations in tests: https://codereview.chromium.org/11316135/.
https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... File ui/compositor/layer_animation_observer.h (right): https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... ui/compositor/layer_animation_observer.h:137: // layer or the mixture of both. On 2012/12/05 17:36:20, vollick wrote: > I worry about the addition of so many special purpose observers here. It would > be nice if we could generalize the LayerAnimationObserver or > ImplicitAnimationObserver to meet your needs. Perhaps, OnLayerAnimationFoo > could have empty definitions rather than being pure virtual, and we could add > SetStartedCallback(...), SetAbortedCallback(...), etc, to the > LayerAnimationObserver interface? Pause/Unpause could go there as well. The > creation of your observer would take two lines rather than one (to install the > callback), but should pretty similar otherwise, right? Current usage of this observer is a lock animation, when several layers are animated, some layers having its own animated properties and duration. So in general case it is not clear when Started/Aborted callback should be called. These two callbacks specify a solution for this case : Finished one is called when all animations are finished and none was aborted, and Aborted callback is called when all animations are finished, and at least one was aborted. These callbacks also manage their own lifetime, and that aspect is also not clear in a general case. https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... File ui/compositor/layer_animator.h (right): https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... ui/compositor/layer_animator.h:201: CreateAnimatorHelperForTest(); On 2012/12/05 17:36:20, vollick wrote: > If these interfere with CreateAnimatorHelperForTest, perhaps we could we > deprecate them a find a way to accomplish the same things with the animation > container test helper? Leaving around multiple ways to control animations in > tests seems like a bad idea. I can remove this comment and explicitly call set_disable...for_tests(false) in implementation - that would work in an expected way. Flags above provide internal/automatic animation control, and TestHelper provides external/manual control, most tests need former for simplicity and latter for fine testing.
Scott, Ian please review this CL as it blocks enabling this feature on trunk for M25.
https://codereview.chromium.org/11453012/diff/30001/ui/base/animation/animati... File ui/base/animation/animation_container.cc (right): https://codereview.chromium.org/11453012/diff/30001/ui/base/animation/animati... ui/base/animation/animation_container.cc:58: void AnimationContainer::Run() { Could you accomplish what you need by splitting Run into the following pieces: void Run() { Run(TimeTicks::Now()); } void Run(base::TimeTicks time) { what is in Run now. } Test code would then invoke Run(some timeticks in the future). That way you don't need test_timing_enabled_ and test_time_.
https://codereview.chromium.org/11453012/diff/30001/ui/base/animation/animati... File ui/base/animation/animation_container.cc (right): https://codereview.chromium.org/11453012/diff/30001/ui/base/animation/animati... ui/base/animation/animation_container.cc:58: void AnimationContainer::Run() { On 2012/12/07 17:01:13, sky wrote: > Could you accomplish what you need by splitting Run into the following pieces: > > void Run() { > Run(TimeTicks::Now()); > } > > void Run(base::TimeTicks time) { > what is in Run now. > } > > Test code would then invoke Run(some timeticks in the future). > > That way you don't need test_timing_enabled_ and test_time_. I still need test_time_ (and, thus, test_timing_enabled_) to avoid TimeTicks::Now() usage in LayerAnimator, to make tests fully deterministic. In other case test may fail if TimeTicks::Now() changes between creation of animation and call to TestHelper->Ping()/Forward(). It did happen several times when I was running tests locally.
Seems like it would be better to provide a place to mock Time/TimeTicks rather than doing in a bunch of random places. I'added Brett to the review list for his thoughts.
https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... File ash/wm/session_state_controller_impl2_unittest.cc (right): https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:43: aura::Window* GetContainer(int container ) { nit: extra space https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:49: return !GetContainer(internal::kShellWindowId_DesktopBackgroundContainer)-> nit: Such alignment looks better: return !GetContainer( internal::kShellWindowId_DesktopBackgroundContainer)->IsVisible(); https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:182: } Nit: Check that lock is not seen? https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:303: for (aura::Window::Windows::const_iterator it = containers.begin(); Export lines 303-310 as a separate function. Lines 324-331 are the same. https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:319: As discussed, add check that _lock_ background is visible + opacity/transform are correct. https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:398: Initialize(true, user::LOGGED_IN_USER); nit: /* legacy button */ https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:407: nit: empty line https://codereview.chromium.org/11453012/diff/28001/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:598: // TODO(antrim): Check that background is correctly placed. As discussed that could be done when login/lock code is moved to Ash.
On 2012/12/07 17:50:35, sky wrote: > Seems like it would be better to provide a place to mock Time/TimeTicks rather > than doing in a bunch of random places. I'added Brett to the review list for his > thoughts. Do you mean we need some system-wide TimerHelper/TestTimer that would mock all Timer behavior?
On 2012/12/05 18:32:17, Denis Kuznetsov wrote: > https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... > File ui/compositor/layer_animation_observer.h (right): > > https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... > ui/compositor/layer_animation_observer.h:137: // layer or the mixture of both. > On 2012/12/05 17:36:20, vollick wrote: > > I worry about the addition of so many special purpose observers here. It would > > be nice if we could generalize the LayerAnimationObserver or > > ImplicitAnimationObserver to meet your needs. Perhaps, OnLayerAnimationFoo > > could have empty definitions rather than being pure virtual, and we could add > > SetStartedCallback(...), SetAbortedCallback(...), etc, to the > > LayerAnimationObserver interface? Pause/Unpause could go there as well. The > > creation of your observer would take two lines rather than one (to install the > > callback), but should pretty similar otherwise, right? > Current usage of this observer is a lock animation, when several layers are > animated, some layers having its own animated properties and duration. > So in general case it is not clear when Started/Aborted callback should be > called. These two callbacks specify a solution for this case : Finished one is > called when all animations are finished and none was aborted, and Aborted > callback is called when all animations are finished, and at least one was > aborted. > These callbacks also manage their own lifetime, and that aspect is also not > clear in a general case. How would you feel about creating a CallbackAnimationObserver that has the ability for firing callbacks for all animation events? One of these events could be that all the observed animations have either finished or aborted. CallbackAnimationObserver::SetAnimationsCompletedCallback(...), could be used for this, for example. I imagine that the signature for the callback functions could all be 'bool callbackFunctionName(CallbackAnimationObserver*)'. The return value for the callback could indicate whether or not the observer should be destructed, and passing the observer as the parameter lets the callback access any observer methods that may be required. For your purposes, I think you'd need the observer to have the following accessor, 'bool CallbackAnimationObserver::ObservedAbortedAnimation() const'. > > https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... > File ui/compositor/layer_animator.h (right): > > https://chromiumcodereview.appspot.com/11453012/diff/4001/ui/compositor/layer... > ui/compositor/layer_animator.h:201: CreateAnimatorHelperForTest(); > On 2012/12/05 17:36:20, vollick wrote: > > If these interfere with CreateAnimatorHelperForTest, perhaps we could we > > deprecate them a find a way to accomplish the same things with the animation > > container test helper? Leaving around multiple ways to control animations in > > tests seems like a bad idea. > I can remove this comment and explicitly call set_disable...for_tests(false) in > implementation - that would work in an expected way. > > Flags above provide internal/automatic animation control, and TestHelper > provides external/manual control, most tests need former for simplicity and > latter for fine testing. I agree with you for set_disable_animations_for_tests (and I think your solution is a very good idea), but not for set_disable_timer_for_test. This function (along with AnimationContainerElement::Step and LayerAnimator::last_step_time) are used in the unit tests to manually advance the animations, and unless I'm mistaken, this is something that your test container also does. If this is true, I would like to get rid of this duplicated functionality. That said, I think that converting the unit tests is a rather big job and I don't want to add to your m25 work. I would be more than happy if the unit tests were converted in a separate cl. Perhaps you could add a TODO to remove these functions in the code and create a matching crbug?
On Fri, Dec 7, 2012 at 9:58 AM, <antrim@chromium.org> wrote: > On 2012/12/07 17:50:35, sky wrote: >> >> Seems like it would be better to provide a place to mock Time/TimeTicks >> rather >> than doing in a bunch of random places. I'added Brett to the review list >> for > > his >> >> thoughts. > > Do you mean we need some system-wide TimerHelper/TestTimer that would mock > all > Timer behavior? Yes, exactly. -Scott > > > > https://codereview.chromium.org/11453012/
I'm OK with adding a global mock time thingy. Random musings (feel free to ignore): put a nice class in base/test that registers itself when you create it, and exposes some nice functions for advancing time in different ways.
On 2012/12/07 19:01:21, sky wrote: > On Fri, Dec 7, 2012 at 9:58 AM, <mailto:antrim@chromium.org> wrote: > > On 2012/12/07 17:50:35, sky wrote: > >> > >> Seems like it would be better to provide a place to mock Time/TimeTicks > >> rather > >> than doing in a bunch of random places. I'added Brett to the review list > >> for > > > > his > >> > >> thoughts. > > > > Do you mean we need some system-wide TimerHelper/TestTimer that would mock > > all > > Timer behavior? > > Yes, exactly. I've thought about that at some point, but I wanted to keep changes as simple as possible, mocking Timer can have very subtle effects all over the system. And I am not sure that I can mock it in a safe enough way with my current knowledge of project. However, I feel fine about replacing/dropping AnimationContainerTestHelper once there is a way to mock timer.
> I've thought about that at some point, but I wanted to keep changes as > simple as possible, mocking Timer can have very subtle effects all over the > system. > And I am not sure that I can mock it in a safe enough way with my current > knowledge of project. I mean - correct mocking of Timers will require mocking both Time (for all systems), as well as message_pump.
On Fri, Dec 7, 2012 at 11:20 AM, <antrim@chromium.org> wrote: > On 2012/12/07 19:01:21, sky wrote: > >> On Fri, Dec 7, 2012 at 9:58 AM, <mailto:antrim@chromium.org> wrote: >> > On 2012/12/07 17:50:35, sky wrote: >> >> >> >> Seems like it would be better to provide a place to mock Time/TimeTicks >> >> rather >> >> than doing in a bunch of random places. I'added Brett to the review >> >> list >> >> for >> > >> > his >> >> >> >> thoughts. >> > >> > Do you mean we need some system-wide TimerHelper/TestTimer that would >> > mock >> > all >> > Timer behavior? > > >> Yes, exactly. > > > I've thought about that at some point, but I wanted to keep changes as > simple as possible, mocking Timer can have very subtle effects all over the > system. > And I am not sure that I can mock it in a safe enough way with my current > knowledge of project. > However, I feel fine about replacing/dropping AnimationContainerTestHelper > once > there is a way to mock timer. I thought you only needed to mock time, not timers? -Scott
Brett (and Jim as cc:), look at mocked Time/TimeTicks in base/. Scott, check if everything is fine now in ui/base/.
Yes, this is what I was thinking for AnimationContainer. Thanks! https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... File ui/base/animation/animation_container_test_helper.h (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:23: virtual ~AnimationContainerTestHelper() {}; Why the virtual? And don't inline and remove ; https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:34: void AdvanceWhileRunning(); How about AdvanceUntilDone. https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:39: void Ping(); Is there a reason this is needed instead of Advance(0)? https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:44: scoped_ptr<base::test::ScopedTimeController> time_controller_; Declare this by value (eg not in a scoped_ptr).
https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/ui.gyp File ui/ui.gyp (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/ui.gyp#newcode64 ui/ui.gyp:64: 'base/animation/animation_container_test_helper.cc', Got a compile error while trying to build Chrome with this patch applied: In file included from ./base/callback_internal.h:15:0, from ./base/callback.h:14, from ./base/bind_helpers.h:151, from ./base/bind_internal.h:13, from ./base/bind.h:13, from ./base/timer.h:53, from ./ui/base/animation/animation_container.h:12, from ./ui/base/animation/animation_container_test_helper.h:10, from ui/base/animation/animation_container_test_helper.cc:5: ./base/memory/scoped_ptr.h: In instantiation of 'scoped_ptr<C>::~scoped_ptr() [with C = base::test::ScopedTimeController]': ./ui/base/animation/animation_container_test_helper.h:23:43: required from here ./base/memory/scoped_ptr.h:163:5: error: deleting object of polymorphic class type 'base::test::ScopedTimeController' which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor] This file should probably only be built for tests, right? If it's testing-specific, including it in the main Chrome build doesn't make sense to me.
https://chromiumcodereview.appspot.com/11453012/diff/37001/base/test/scoped_t... File base/test/scoped_time_controller.h (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/base/test/scoped_t... base/test/scoped_time_controller.h:18: ~ScopedTimeController(); nit: need 'virtual' here
Moved test-related things around .gyp files, tuned some animation timings. Scott, Brett, still waiting for your comments. Ian, I'm still playing with callback observer implementation. https://chromiumcodereview.appspot.com/11453012/diff/37001/base/test/scoped_t... File base/test/scoped_time_controller.h (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/base/test/scoped_t... base/test/scoped_time_controller.h:18: ~ScopedTimeController(); On 2012/12/10 23:56:52, Daniel Erat wrote: > nit: need 'virtual' here Done. https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... File ui/base/animation/animation_container_test_helper.h (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:23: virtual ~AnimationContainerTestHelper() {}; On 2012/12/10 22:20:16, sky wrote: > Why the virtual? And don't inline and remove ; So that there are no warnings if it is used with scoped_ptr. Done. https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:34: void AdvanceWhileRunning(); On 2012/12/10 22:20:16, sky wrote: > How about AdvanceUntilDone. Done. https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:39: void Ping(); On 2012/12/10 22:20:16, sky wrote: > Is there a reason this is needed instead of Advance(0)? Done. https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:44: scoped_ptr<base::test::ScopedTimeController> time_controller_; On 2012/12/10 22:20:16, sky wrote: > Declare this by value (eg not in a scoped_ptr). Done. https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/ui.gyp File ui/ui.gyp (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/ui.gyp#newcode64 ui/ui.gyp:64: 'base/animation/animation_container_test_helper.cc', On 2012/12/10 23:56:12, Daniel Erat wrote: > Got a compile error while trying to build Chrome with this patch applied: > > In file included from ./base/callback_internal.h:15:0, > from ./base/callback.h:14, > from ./base/bind_helpers.h:151, > from ./base/bind_internal.h:13, > from ./base/bind.h:13, > from ./base/timer.h:53, > from ./ui/base/animation/animation_container.h:12, > from ./ui/base/animation/animation_container_test_helper.h:10, > from ui/base/animation/animation_container_test_helper.cc:5: > ./base/memory/scoped_ptr.h: In instantiation of 'scoped_ptr<C>::~scoped_ptr() > [with C = base::test::ScopedTimeController]': > ./ui/base/animation/animation_container_test_helper.h:23:43: required from > here > ./base/memory/scoped_ptr.h:163:5: error: deleting object of polymorphic class > type 'base::test::ScopedTimeController' which has non-virtual destructor might > cause undefined behaviour [-Werror=delete-non-virtual-dtor] > > This file should probably only be built for tests, right? If it's > testing-specific, including it in the main Chrome build doesn't make sense to > me. moved to the test_support_base section (missed that first time)
https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... File ui/base/animation/animation_container_test_helper.h (right): https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... ui/base/animation/animation_container_test_helper.h:23: virtual ~AnimationContainerTestHelper() {}; On 2012/12/11 13:27:57, Denis Kuznetsov wrote: > On 2012/12/10 22:20:16, sky wrote: > > Why the virtual? And don't inline and remove ; > So that there are no warnings if it is used with scoped_ptr. > Done. scoped_ptr shouldn't require a virtual destructor. What error do you see when the destructor is not virtual?
On 2012/12/11 16:44:16, sky wrote: > https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... > File ui/base/animation/animation_container_test_helper.h (right): > > https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... > ui/base/animation/animation_container_test_helper.h:23: virtual > ~AnimationContainerTestHelper() {}; > On 2012/12/11 13:27:57, Denis Kuznetsov wrote: > > On 2012/12/10 22:20:16, sky wrote: > > > Why the virtual? And don't inline and remove ; > > So that there are no warnings if it is used with scoped_ptr. > > Done. > > scoped_ptr shouldn't require a virtual destructor. What error do you see when > the destructor is not virtual? Sorry, I've thought about error Daniel got in #18.
On Tue, Dec 11, 2012 at 2:09 PM, <antrim@chromium.org> wrote: > On 2012/12/11 16:44:16, sky wrote: > > https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... >> >> File ui/base/animation/animation_container_test_helper.h (right): > > > > https://chromiumcodereview.appspot.com/11453012/diff/37001/ui/base/animation/... >> >> ui/base/animation/animation_container_test_helper.h:23: virtual >> ~AnimationContainerTestHelper() {}; >> On 2012/12/11 13:27:57, Denis Kuznetsov wrote: >> > On 2012/12/10 22:20:16, sky wrote: >> > > Why the virtual? And don't inline and remove ; >> > So that there are no warnings if it is used with scoped_ptr. >> > Done. > > >> scoped_ptr shouldn't require a virtual destructor. What error do you see >> when >> the destructor is not virtual? > > > Sorry, I've thought about error Daniel got in #18. > > https://chromiumcodereview.appspot.com/11453012/ I don't think that's related.
https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.cc File base/time.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.cc#newco... base/time.cc:21: //static Can you add a space after // https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h File base/time.h (right): https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h#newcod... base/time.h:643: class BASE_EXPORT TimeFactory { Can you add a comment for this about what it's for? You should also explain the thing about the instance_ and how it gets installed. https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h#newcod... base/time.h:647: virtual TimeTicks TimeTicksHighResNow() = 0; I'd not bother with this, it makes it more difficult to write a test impl and I don't think there's any need for tests to do anything different for regular/high res ticks. So just have both the Now and HighResNow call through to the same factory function.
(Drive-by, since I was whining about needing a Clock interface, and brettw pointed me here.) https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h File base/time.h (right): https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h#newcod... base/time.h:652: static TimeFactory* instance_; This scares me. Overriding time globally seems like a blunt instrument and may have unintended consequences (since a lot of stuff assumes that TimeTicks increase monotonically, etc.). Is there a way to simply dep-inject TimeFactory into the class that needs it? Or at least separate out the singleton instance stuff from the interface itself? Also, I feel like adding a TimeFactory interface in base/ is a pretty big change -- I wonder if it would be better to do it in a separate CL, especially since it seems like there's some time pressure to get this CL in. Thoughts?
https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.cc File base/time.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.cc#newco... base/time.cc:21: //static On 2012/12/12 00:35:09, brettw wrote: > Can you add a space after // Done. https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h File base/time.h (right): https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h#newcod... base/time.h:643: class BASE_EXPORT TimeFactory { On 2012/12/12 00:35:09, brettw wrote: > Can you add a comment for this about what it's for? You should also explain the > thing about the instance_ and how it gets installed. Done. https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h#newcod... base/time.h:647: virtual TimeTicks TimeTicksHighResNow() = 0; On 2012/12/12 00:35:09, brettw wrote: > I'd not bother with this, it makes it more difficult to write a test impl and I > don't think there's any need for tests to do anything different for regular/high > res ticks. So just have both the Now and HighResNow call through to the same > factory function. Done. https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h File base/time.h (right): https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h#newcod... base/time.h:652: static TimeFactory* instance_; On 2012/12/12 00:58:12, akalin wrote: > This scares me. Overriding time globally seems like a blunt instrument and may > have unintended consequences (since a lot of stuff assumes that TimeTicks > increase monotonically, etc.). Is there a way to simply dep-inject TimeFactory > into the class that needs it? Or at least separate out the singleton instance > stuff from the interface itself? That was my initial approach, but Scott insisted that we should mock Time/TimeTicks (https://chromiumcodereview.appspot.com/11453012/#msg7 and https://chromiumcodereview.appspot.com/11453012/#msg11). I've added docs in implementation that would guide how to use mocked timer properly, made mock object to preempt current time on creation, and added DCHECK so that time can not be moved backwards - the only case when time can decrease now is removal of mock time. Also, mocking object should not persist across tests, so all changes in time are test-local. > Also, I feel like adding a TimeFactory interface in base/ is a pretty big change > -- I wonder if it would be better to do it in a separate CL, especially since it > seems like there's some time pressure to get this CL in. Thoughts? Core change of this CL changes the way some callbacks are called (animation finish VS timer trigger). So there is a need to advance animations in a some way. There is a way to make this testing without mocking TimeTicks::Now(), but it results in a flakiness caused by ocassional off-by-one changes in TimeTicks::Now() result between several places. So I see three options: disabling tests, introducing flakiness into tests, and mocking time.
ui/base changes LGTM I have not looked at any other changes, if you need me to review them please ask.
On 2012/12/12 19:20:12, Denis Kuznetsov wrote: > https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.cc > File base/time.cc (right): > > https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.cc#newco... > base/time.cc:21: //static > On 2012/12/12 00:35:09, brettw wrote: > > Can you add a space after // > > Done. > > https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h > File base/time.h (right): > > https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h#newcod... > base/time.h:643: class BASE_EXPORT TimeFactory { > On 2012/12/12 00:35:09, brettw wrote: > > Can you add a comment for this about what it's for? You should also explain > the > > thing about the instance_ and how it gets installed. > > Done. > > https://chromiumcodereview.appspot.com/11453012/diff/48002/base/time.h#newcod... > base/time.h:647: virtual TimeTicks TimeTicksHighResNow() = 0; > On 2012/12/12 00:35:09, brettw wrote: > > I'd not bother with this, it makes it more difficult to write a test impl and > I > > don't think there's any need for tests to do anything different for > regular/high > > res ticks. So just have both the Now and HighResNow call through to the same > > factory function. > > Done. > > https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h > File base/time.h (right): > > https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h#newcod... > base/time.h:652: static TimeFactory* instance_; > On 2012/12/12 00:58:12, akalin wrote: > > This scares me. Overriding time globally seems like a blunt instrument and > may > > have unintended consequences (since a lot of stuff assumes that TimeTicks > > increase monotonically, etc.). Is there a way to simply dep-inject > TimeFactory > > into the class that needs it? Or at least separate out the singleton instance > > stuff from the interface itself? > > That was my initial approach, but Scott insisted that we should mock > Time/TimeTicks (https://chromiumcodereview.appspot.com/11453012/#msg7 and > https://chromiumcodereview.appspot.com/11453012/#msg11). > > I've added docs in implementation that would guide how to use mocked timer > properly, made mock object to preempt current time on creation, and added DCHECK > so that time can not be moved backwards - the only case when time can decrease > now is removal of mock time. > Also, mocking object should not persist across tests, so all changes in time are > test-local. > > > Also, I feel like adding a TimeFactory interface in base/ is a pretty big > change > > -- I wonder if it would be better to do it in a separate CL, especially since > it > > seems like there's some time pressure to get this CL in. Thoughts? > > Core change of this CL changes the way some callbacks are called (animation > finish VS timer trigger). So there is a need to advance animations in a some > way. There is a way to make this testing without mocking TimeTicks::Now(), but > it results in a flakiness caused by ocassional off-by-one changes in > TimeTicks::Now() result between several places. > > So I see three options: disabling tests, introducing flakiness into tests, and > mocking time. ui/compositor lgtm.
Dan knows the session state code more than I, it would be great if he could take a look too. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_animator.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:194: void TransformWindowToBaseState(aura::Window* window, Add a comment for this. How about naming SetToInitialState https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:201: settings.SetPreemptionStrategy( Why do you need this (say question on 162)? https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:230: ui::ScopedLayerAnimationSettings settings(layer->GetAnimator()); Why do you need a ScopedLayerAnimationSettings here and resetting the preemption strategy on 233? If you need the values immediately set can't you set them without wrapping in a ScopedLayerAnimationSettings. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:239: TransformWindowToBaseState(window, duration, observer); This means you'll have two ScopedLayerAnimationSettings in effect, right? One on 230 and the other on 198. We should avoid doing that. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:529: void SessionStateAnimator::StartAnimationWithObserver( Is this used? https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_controller_impl2.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:71: } Can you make this private so it's clear AnimationFinishedObserver deletes itself. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:79: // Unpauses observer. It does a check ant calls callback if conditions are ant -> and https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:111: }; remove ; https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:118: // Callback to be called. nit: newline between 117/118. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_controller_impl2.h (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.h:86: struct UnlockedStateProperties { Is there a reason you aren't using a bool directly? https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.h:215: bool undoable_lock_animation_; can_stop_lock_animation_. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.h:217: scoped_ptr<UnlockedStateProperties> unlocked_properties_; Why the scoped_ptr instead of by value?
https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h File base/time.h (right): https://chromiumcodereview.appspot.com/11453012/diff/59001/base/time.h#newcod... base/time.h:652: static TimeFactory* instance_; On 2012/12/12 00:58:12, akalin wrote: > This scares me. Overriding time globally seems like a blunt instrument and may > have unintended consequences (since a lot of stuff assumes that TimeTicks > increase monotonically, etc.). Is there a way to simply dep-inject TimeFactory > into the class that needs it? Or at least separate out the singleton instance > stuff from the interface itself? I prefer mocking at a central place. It means supporting code doesn't need to change to take a Clock object. Additionally injecting a Clock like object in all places a particular code path calls into can be tedious and require exposing objects you might not normally expose. That said, I'm not an owner here and leave the decision to Brett (or other base/OWNERS). > Also, I feel like adding a TimeFactory interface in base/ is a pretty big change > -- I wonder if it would be better to do it in a separate CL, especially since it > seems like there's some time pressure to get this CL in. Thoughts? Agreed. Splitting patches into smaller chunks with fewer reviewers is always a good thing.
I still need to go through session_state_controller_impl2.cc; there's a lot of new code there. I think that some of my objections to terminology that earlier changes were trying to introduce (e.g. "undoable lock") still stands, though. I strongly prefer "pre-lock animation", "lock animation", "pre-shutdown animation", and "shutdown animation", assuming that those four distinct phases still exist. https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... File ash/wm/session_state_animator.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... ash/wm/session_state_animator.cc:123: // Fades |window| in to |opacity| over |duration|. nit: s/Fades |window| in to/Fades |window| to/ https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... ash/wm/session_state_animator.cc:513: // Apply animation |type| to all containers described by |container_mask|. nit: delete this comment; it should be in the header instead of here https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... ash/wm/session_state_animator.cc:529: void SessionStateAnimator::StartAnimationWithObserver( this is almost exactly the same as StartAnimationWithCallback(). can StartAnimationWithCallback() just construct an observer and pass it to this method? (i was thinking that maybe it couldn't because this SAWC() creates a separate observer for each container, but SAWO() passes the same observer to each container, so presumably it's okay to do that.)
> change > > -- I wonder if it would be better to do it in a separate CL, especially since > it > > seems like there's some time pressure to get this CL in. Thoughts? > > Agreed. Splitting patches into smaller chunks with fewer reviewers is always a > good thing. Yes, but when I asked if it is fine to mock clock in AnimationContainer and move it to Time/TimeTicks later (in separate CL) you said that it should be done in this CL. And without timers mocked, tests would get flaky.
https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_animator.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:201: settings.SetPreemptionStrategy( On 2012/12/12 20:35:56, sky wrote: > Why do you need this (say question on 162)? This method can be called directly (without ShowWindow call), e.g. when cancelling initial lock animation. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:239: TransformWindowToBaseState(window, duration, observer); On 2012/12/12 20:35:56, sky wrote: > This means you'll have two ScopedLayerAnimationSettings in effect, right? One on > 230 and the other on 198. We should avoid doing that. As TransformWindowToBaseState can be called outside from ShowWindow, it needs it's own ScopedLayerAnimationSettings. Some legacy code sets layer animation strategy explicitly, so I need to make sure correct strategy in this method. I am going to fix this (direct setting of animator's stragegy) in a separate cleanup CL. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:529: void SessionStateAnimator::StartAnimationWithObserver( On 2012/12/12 20:35:56, sky wrote: > Is this used? Yes, SessionStateControllerImpl2 uses it. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_controller_impl2.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:71: } On 2012/12/12 20:35:56, sky wrote: > Can you make this private so it's clear AnimationFinishedObserver deletes > itself. Done. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:79: // Unpauses observer. It does a check ant calls callback if conditions are On 2012/12/12 20:35:56, sky wrote: > ant -> and Done. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:111: }; On 2012/12/12 20:35:56, sky wrote: > remove ; Done. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.cc:118: // Callback to be called. On 2012/12/12 20:35:56, sky wrote: > nit: newline between 117/118. Done. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_controller_impl2.h (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.h:86: struct UnlockedStateProperties { On 2012/12/12 20:35:56, sky wrote: > Is there a reason you aren't using a bool directly? Yes, it seems that there would be similar parameters for launcher state later, and I want to have them groupped together. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.h:215: bool undoable_lock_animation_; On 2012/12/12 20:35:56, sky wrote: > can_stop_lock_animation_. Done. https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_controller_impl2.h:217: scoped_ptr<UnlockedStateProperties> unlocked_properties_; On 2012/12/12 20:35:56, sky wrote: > Why the scoped_ptr instead of by value? One reason is that I want to have just another layer of defence if things go wrong, like, make sure that we never would use value from 2 locks ago - properties are reset on every unlock. https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... File ash/wm/session_state_animator.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... ash/wm/session_state_animator.cc:123: // Fades |window| in to |opacity| over |duration|. On 2012/12/13 05:02:02, Daniel Erat wrote: > nit: s/Fades |window| in to/Fades |window| to/ Done. https://chromiumcodereview.appspot.com/11453012/diff/69001/ash/wm/session_sta... ash/wm/session_state_animator.cc:513: // Apply animation |type| to all containers described by |container_mask|. On 2012/12/13 05:02:02, Daniel Erat wrote: > nit: delete this comment; it should be in the header instead of here Done.
https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... File ash/wm/session_state_animator.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/2004/ash/wm/session_stat... ash/wm/session_state_animator.cc:194: void TransformWindowToBaseState(aura::Window* window, On 2012/12/12 20:35:56, sky wrote: > Add a comment for this. How about naming SetToInitialState Added comment. I don't like BaseState, but I think Initial is worse : 'Initial' state means that there is some single process and it has starting state. Here we have lots of animation, so it is not clear what the initial state is.
https://codereview.chromium.org/11453012/diff/81001/ash/wm/session_state_anim... File ash/wm/session_state_animator.cc (right): https://codereview.chromium.org/11453012/diff/81001/ash/wm/session_state_anim... ash/wm/session_state_animator.cc:533: ui::LayerAnimationObserver* observer) { repasting comment from last time: this is almost exactly the same as StartAnimationWithCallback(). can StartAnimationWithCallback() just construct an observer and pass it to this method? (i was thinking that maybe it couldn't because this SAWC() creates a separate observer for each container, but SAWO() passes the same observer to each container, so presumably it's okay to do that.) https://codereview.chromium.org/11453012/diff/75025/ash/wm/session_state_cont... File ash/wm/session_state_controller_impl2.cc (right): https://codereview.chromium.org/11453012/diff/75025/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:382: can_stop_lock_animation_ = false; please use consistent terminology within this class. for example: - this variable refers to "stopping the lock animation". - StartUndoableLockAnimationPhaseOne() refers to "undoing" it. - StartCancellableShutdownAnimation() refers to "canceling" the shutdown animation. - i believe that the phrases "phase one" and "phase two" refer to the parts of the lock animation that occur before and after locking can be canceled, but there's no indication from their names that that's the case. i still feel that the PreLock/PreShutdown vs. Lock/Shutdown naming distinction is clearest in terms of what's actually happening -- requesting that the screen be locked or that the system be shut down are the main things that this class does, so it's best to describe its methods by their relationship to those actions wherever possible. if the unlock animation is split between parts that occur before and after the lock UI has been destroyed, names like UnlockBeforeUIDestroyed and UnlockAfterUIDestroyed may be clearer. "phase one" and "phase two" don't convey any information apart from the relative order in which these methods will be called.
https://codereview.chromium.org/11453012/diff/81001/ash/wm/session_state_anim... File ash/wm/session_state_animator.cc (right): https://codereview.chromium.org/11453012/diff/81001/ash/wm/session_state_anim... ash/wm/session_state_animator.cc:533: ui::LayerAnimationObserver* observer) { On 2012/12/13 19:31:54, Daniel Erat wrote: > repasting comment from last time: > > this is almost exactly the same as StartAnimationWithCallback(). can > StartAnimationWithCallback() just construct an observer and pass it to this > method? (i was thinking that maybe it couldn't because this SAWC() creates a > separate observer for each container, but SAWO() passes the same observer to > each container, so presumably it's okay to do that.) I am going to replace all callbacks with observer in upcoming cleanup CL. https://codereview.chromium.org/11453012/diff/75025/ash/wm/session_state_cont... File ash/wm/session_state_controller_impl2.cc (right): https://codereview.chromium.org/11453012/diff/75025/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:382: can_stop_lock_animation_ = false; On 2012/12/13 19:31:54, Daniel Erat wrote: > please use consistent terminology within this class. for example: > - this variable refers to "stopping the lock animation". > - StartUndoableLockAnimationPhaseOne() refers to "undoing" it. > - StartCancellableShutdownAnimation() refers to "canceling" the shutdown > animation. > - i believe that the phrases "phase one" and "phase two" refer to the parts of > the lock animation that occur before and after locking can be canceled, but > there's no indication from their names that that's the case. > > i still feel that the PreLock/PreShutdown vs. Lock/Shutdown naming distinction > is clearest in terms of what's actually happening -- requesting that the screen > be locked or that the system be shut down are the main things that this class > does, so it's best to describe its methods by their relationship to those > actions wherever possible. > > if the unlock animation is split between parts that occur before and after the > lock UI has been destroyed, names like UnlockBeforeUIDestroyed and > UnlockAfterUIDestroyed may be clearer. "phase one" and "phase two" don't convey > any information apart from the relative order in which these methods will be > called. Done.
On Thu, Dec 13, 2012 at 9:59 AM, <antrim@chromium.org> wrote: >> change >> > -- I wonder if it would be better to do it in a separate CL, especially > > since >> >> it >> > seems like there's some time pressure to get this CL in. Thoughts? > > >> Agreed. Splitting patches into smaller chunks with fewer reviewers is >> always a >> good thing. > > Yes, but when I asked if it is fine to mock clock in AnimationContainer and > move > it to Time/TimeTicks later (in separate CL) you said that it should be done > in > this CL. > And without timers mocked, tests would get flaky. Sorry for not being clear. I wanted us to agree on the solution and then implement it. I'm totally fine if parts of the solution are implemented in separate cls. What I didn't want to have happen is implement one thing here and then replace it with something totally different. -Scott > > https://chromiumcodereview.appspot.com/11453012/
base lgtm https://chromiumcodereview.appspot.com/11453012/diff/73002/base/test/scoped_t... File base/test/scoped_time_controller.cc (right): https://chromiumcodereview.appspot.com/11453012/diff/73002/base/test/scoped_t... base/test/scoped_time_controller.cc:36: } // namespace base Flip the order of these two lines :)
ash LGTM after some more comments are addressed https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_anim... File ash/wm/session_state_animator.h (right): https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_anim... ash/wm/session_state_animator.h:163: // Apply animation |type| to all containers included in |container_mask| with nit: indent this comment two more spaces https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... File ash/wm/session_state_controller_impl2.cc (right): https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:52: // This observer is intended to use in cases when some action have to be taken nit: s/have/has/ https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:57: // there was no aborted animations. nit: s/was/were/ https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:71: // sequence have some immediate animations in the beginning. nit: s/sequence have/a sequence has/ https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:167: // This is also a case when user signs off. nit: "... also the case when the user ..." https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:329: GetDuration(internal::SessionStateAnimator::ANIMATION_SPEED_SHUTDOWN), nit: change this to: animator_->GetDuration( internal::..., this, ...); https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:344: void SessionStateControllerImpl2::StartRealShutdownTimer(bool animation_time) { nit: s/animation_time/with_animation_time/ to match the header https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:427: nit: delete extra blank lines between these function calls; i don't think they help readability (i.e. they're not being used to group related code together) https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:465: nit: delete extra blank line https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:477: nit: delete blank line https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:502: nit: delete blank lines https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:519: nit: delete blank lines https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.cc:544: nit: delete blank lines https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... File ash/wm/session_state_controller_impl2.h (right): https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.h:37: // * StartLockAnimation (bool shutdown after lock) - starts lock that can be nit: delete the "(bool shutdown after lock)" part https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.h:39: // * StartLockAnimationAndLockImmediately - starts uninterruptable lock nit: s/uninterruptable/uninterruptible/ https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.h:83: return controller_->can_cancel_lock_animation_; nit: call CanCancelLockAnimation() instead? https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.h:108: struct UnlockedStateProperties { if this isn't accessed by other classes, move it to the top of the private section https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.h:187: // Lock phase one : windows lift (can be undone in some cases) nit: delete these outdated comments https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2.h:206: // Stores properties of UI that have to be temporary modified while locking. nit: s/temporary/temporarily/ https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... File ash/wm/session_state_controller_impl2_unittest.cc (right): https://codereview.chromium.org/11453012/diff/73002/ash/wm/session_state_cont... ash/wm/session_state_controller_impl2_unittest.cc:158: void ExpectLockPartOneAnimationStarted() { please update the names of these methods to match the names in the implementation
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/11453012/83024
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/11453012/83024
On 2012/12/14 15:55:29, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/antrim%40chromium.org/11453012/83024 I'm unchecking the CQ box because I don't think we have agreement on Time mocking. Perhaps I'm the only one that feels strongly about this, but I think that mocking out time in this fashion is a pretty big change and one not to be checked in lightly. At the very least, it should be separate from this change, which as far as I can tell is UI-based. That said, I understand there's some time pressure to get this CL in. Can we compromise and come up with a time mocking scheme that doesn't involve touching base? Then once we agree on the way forward for mocking time, we can implement that and port any time-mocking code in this CL to use that. But we can do that without having to worry about the M25 deadline. Scott, you're the one who suggested doing the time mocking scheme in this CL. What do you think?
On 2012/12/14 19:54:03, akalin wrote: > On 2012/12/14 15:55:29, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/antrim%2540chromium.org/11453012/83024 > > I'm unchecking the CQ box because I don't think we have agreement on Time > mocking. Fred, if you've been interested in this CL why didn't you post any comments for the last 2 days? > > Perhaps I'm the only one that feels strongly about this, but I think that > mocking out time in this fashion is a pretty big change and one not to be > checked in lightly. At the very least, it should be separate from this change, > which as far as I can tell is UI-based. Do you have any specific comments about the implementation? Please provide them in this context. > > That said, I understand there's some time pressure to get this CL in. Can we > compromise and come up with a time mocking scheme that doesn't involve touching > base? That's not a change that could be done over the single day (Monday). Plus get reviewed over 2 locations with 12 hour time difference. Then once we agree on the way forward for mocking time, we can implement > that and port any time-mocking code in this CL to use that. But we can do that > without having to worry about the M25 deadline. > > Scott, you're the one who suggested doing the time mocking scheme in this CL. > What do you think? So you're proposing to disable some subset of tests because clearly this CL could not be committed with the tests if we don't include changes that support mocking time. Correct?
On 2012/12/14 20:54:42, Nikita Kostylev wrote: > On 2012/12/14 19:54:03, akalin wrote: > > On 2012/12/14 15:55:29, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > > https://chromium-status.appspot.com/cq/antrim%252540chromium.org/11453012/83024 > > > > I'm unchecking the CQ box because I don't think we have agreement on Time > > mocking. > > Fred, if you've been interested in this CL why didn't you post any comments for > the last 2 days? > > > > > Perhaps I'm the only one that feels strongly about this, but I think that > > mocking out time in this fashion is a pretty big change and one not to be > > checked in lightly. At the very least, it should be separate from this > change, > > which as far as I can tell is UI-based. > > Do you have any specific comments about the implementation? Please provide them > in this context. > > > > > That said, I understand there's some time pressure to get this CL in. Can we > > compromise and come up with a time mocking scheme that doesn't involve > touching > > base? > > That's not a change that could be done over the single day (Monday). Plus get > reviewed over 2 locations with 12 hour time difference. > > Then once we agree on the way forward for mocking time, we can implement > > that and port any time-mocking code in this CL to use that. But we can do that > > without having to worry about the M25 deadline. > > > > Scott, you're the one who suggested doing the time mocking scheme in this CL. > > What do you think? > > So you're proposing to disable some subset of tests because clearly this CL > could not be committed with the tests if we don't include changes that support > mocking time. Correct? That's the solution that I'm proposing to go with this CL on Monday (it's 1 am in Moscow now). Please propose any other actionable alternative proposal if you disagree. Another alternate solution that I see is just to move current implementation out of base. No other changes Fred, do you agree with that? Doing something different like some temporary mocking implementation just for this CL is basically doing something in the blind first then discuss and maybe change that or agree that we don't want such temporary solution at all.
Fred, I think it would be useful in the context of this CL if you would answer Scott reply to your initial comment.
On 2012/12/14 20:54:42, Nikita Kostylev wrote: > Fred, if you've been interested in this CL why didn't you post any comments for > the last 2 days? I was waiting for the base/ changes to be split out into another patch, as sky@ suggested. > Do you have any specific comments about the implementation? Please provide them > in this context. Yes. It introduces a singleton for Time, which is a glorified global. I took a look at this patch and couldn't even find what exactly was relying on Time and how its behavior changed with the overridden TimeFactory, but maybe I'm looking in the wrong place and not hard enough. That's bad. If I change something and the test breaks, how will I find out what's wrong? Ideally, I'd be able to search for "clock->Now()" or something. > > That said, I understand there's some time pressure to get this CL in. Can we > > compromise and come up with a time mocking scheme that doesn't involve > touching > > base? > > That's not a change that could be done over the single day (Monday). Plus get > reviewed over 2 locations with 12 hour time difference. I sympathize with that, but it has to be done it has to be done. Why is this blocked on M25? Is it a bug fix? If so, then it can be merged after the feature freeze deadline, right? > Then once we agree on the way forward for mocking time, we can implement > > that and port any time-mocking code in this CL to use that. But we can do that > > without having to worry about the M25 deadline. > > > > Scott, you're the one who suggested doing the time mocking scheme in this CL. > > What do you think? > > So you're proposing to disable some subset of tests because clearly this CL > could not be committed with the tests if we don't include changes that support > mocking time. Correct? Are you claiming that the tests cannot work unless base::Time is mocked out in this specific fashion (with a singleton)? That may be true if the code you're adding has a lot of subobjects that call base::Time, and it would be infeasible to dep-inject TimeFactory into all of those easily, and there were no existing tests that ran into the same problem. If so, and if committing for the M25 feature-freeze deadline is absolutely imperative, then checking in with disabled tests is an option, as long as you plan to fix the code to be more testable with respect to Time.
On 2012/12/14 21:02:36, Nikita Kostylev wrote: > Fred, I think it would be useful in the context of this CL if you would answer > Scott reply to your initial comment. Sorry, we were discussing Clock/TimeFactory objects on an e-mail thread. Here's what sky@ said: In thinking about this a bit more I now see the testing problem. In particular if a test changed the time during a browser_test there is no way to limit the scope and no doubt all sorts of assumptions would break. So, while I like the factory for unit tests it won't work for browser tests and I want only one solution. That implies the Clock approach. (The Clock approach is to dep-inject a Clock object into whatever needs it.) I'll add you to the thread.
On 2012/12/14 22:08:25, akalin wrote: > > So you're proposing to disable some subset of tests because clearly this CL > > could not be committed with the tests if we don't include changes that support > > mocking time. Correct? After thinking about it, I think reverting the base/ changes and marking the tests that rely on overridden Time as DISABLED_ is the most practical compromise so that this change can go in soon (for M25). Then we can hopefully come to an agreement on what to do re. mocking Time, implement that, and re-enable the tests before they get too stale. What do you think? Other reviewers, does this sound acceptable?
On 2012/12/14 22:08:25, akalin wrote: > On 2012/12/14 20:54:42, Nikita Kostylev wrote: > > Fred, if you've been interested in this CL why didn't you post any comments > for > > the last 2 days? > > I was waiting for the base/ changes to be split out into another patch, as sky@ > suggested. > > > Do you have any specific comments about the implementation? Please provide > them > > in this context. > > Yes. It introduces a singleton for Time, which is a glorified global. I took a > look at this patch and couldn't even find what exactly was relying on Time and > how its behavior changed with the overridden TimeFactory, but maybe I'm looking > in the wrong place and not hard enough. That's bad. If I change something and > the test breaks, how will I find out what's wrong? Ideally, I'd be able to > search for "clock->Now()" or something. > > > > That said, I understand there's some time pressure to get this CL in. Can > we > > > compromise and come up with a time mocking scheme that doesn't involve > > touching > > > base? > > > > That's not a change that could be done over the single day (Monday). Plus get > > reviewed over 2 locations with 12 hour time difference. > > I sympathize with that, but it has to be done it has to be done. Why is this > blocked on M25? Is it a bug fix? If so, then it can be merged after the > feature freeze deadline, right? It's not a bug fix but latest fixes for M25 functionality that is mostly done except for this CL. > > > Then once we agree on the way forward for mocking time, we can implement > > > that and port any time-mocking code in this CL to use that. But we can do > that > > > without having to worry about the M25 deadline. > > > > > > Scott, you're the one who suggested doing the time mocking scheme in this > CL. > > > What do you think? > > > > So you're proposing to disable some subset of tests because clearly this CL > > could not be committed with the tests if we don't include changes that support > > mocking time. Correct? > > Are you claiming that the tests cannot work unless base::Time is mocked out in > this specific fashion (with a singleton)? Correct. As far as I understand certain aspects could not be tested without base::Time being mocked out. >That may be true if the code you're > adding has a lot of subobjects that call base::Time, and it would be infeasible > to dep-inject TimeFactory into all of those easily, and there were no existing > tests that ran into the same problem. If so, and if committing for the M25 > feature-freeze deadline is absolutely imperative, then checking in with disabled > tests is an option, as long as you plan to fix the code to be more testable with > respect to Time. Good, it make sense to follow that direction then. Denis who is the original author of this CL could provide more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/11453012/88001
Message was sent while issue was closed.
Change committed as 173461 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
