Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728803002:
Handles users rotating screen too early (Closed)
DescriptionHandles users rotating screen too early
If the user rotates screen too early while the previous animation is not
finished yet, it will create another |old_layer_tree| with additional
layers of previous |old_layer_tree|. The solution is that if there is a
new rotation request during rotation animation, it should stop the
animation immediately and start the new rotation request.
BUG=678763
TEST=manual, unit_tests
R=oshima@chromium.org, bruthig@chromium.org
Review-Url: https://codereview.chromium.org/2728803002
Cr-Commit-Position: refs/heads/master@{#457946}
Committed: https://chromium.googlesource.com/chromium/src/+/6ddafa38e3e8bfc0b856149a6a9366c7b0da80f4
Patch Set 1 #
Total comments: 30
Patch Set 2 : Rebased, reset is_rotating in layerCleanupObserver etc. #
Total comments: 38
Patch Set 3 : Make ScreenRotationAnimator handle its own internal states. #
Total comments: 43
Patch Set 4 : Add MultiLayerAnimatorTestController to controll test animation. #
Total comments: 27
Patch Set 5 : Fixes some nit. #
Total comments: 66
Patch Set 6 : Fixes some more nits. #
Total comments: 20
Patch Set 7 : Patch for commit. #Patch Set 8 : Rebased to commit. #Messages
Total messages: 65 (43 generated)
wutao@chromium.org changed reviewers: + afakhry@chromium.org, bruthig@chromium.org, oshima@chromium.org
Hi Oshima and Ben, Could you please look at this cl. Thanks, Tao
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
I took a quick look and have left some actionable comments. I will have to circle back when I have some more time. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:197: if (!screen_rotation_animator) Why do we need to support WeakPtrs here? If the DisplayConfigurationController manages the lifetime of the ScreenRotationAnimators then can't we always guarantee |screen_rotation_animator| to be non-null? https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:80: bool CanAnimate(int64_t display_id) const; "CanAnimate" is ambiguous, might I suggest "CanAnimateScreenRotation()" https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:81: display::Display::Rotation GetCurrentRotation(int64_t display_id) const; nit 1: newlines between function declarations here nit 2: Can you make the declaration order in the .h match the definition order in the .cc? super-nit: GetRotation() is somewhat ambiguous might I suggest GetCurrentScreenRotation()? And it would probably be helpful to document what the return value is going to be if an animation is in progress. e.g. the target rotation. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:82: // Polulate the ash::ScreenRotationAnimator nit: Polulate -> populate https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:83: ScreenRotationAnimator* GetOrMakeAnimatorByDisplayId(int64_t display_id); This class also appears to use a DisplayAnimator => from the function name it is ambiguous what animator is being created. Personally I don't think it's necessary for the callers of this function to know that a new instance may or may not be created so might I suggest just "GetRotationAnimatorForDisplay()" or "GetScreenRotationAnimatorForDisplay()" https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:104: // more pending rotation request. Very helpful comment!! https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:37: // struct ScreenRotationAnimator::RotationRequest { Did you mean to remove this? https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:51: int64_t display_id() const; You should inline the definition of simple getters in the .h file. https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:54: void set_is_animating(bool rotating); I think it is very odd that clients of the animator can change the |is_animating_| value. I would think that should all be handled inside of this. https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator_unittest.cc:101: animator->Rotate(display::Display::ROTATE_90, FYI AshTestBase via AshTestHelper::SetUp() uses the ZERO_DURATION ui::ScopedAnimationDurationScaleMode to control animations. I'm pretty sure it is in effect here which means that the animation triggered on line 101 will execute immediately and the second animation might not actually be getting queued up like you expect. I think you should be able to create a local ScopedAnimationDurationScaleMode instance with a SLOW_DURATION scale mode to have a higher chance of the second animation actually being queued up. https://codereview.chromium.org/2728803002/diff/1/ash/test/shell_test_api.h File ash/test/shell_test_api.h (right): https://codereview.chromium.org/2728803002/diff/1/ash/test/shell_test_api.h#n... ash/test/shell_test_api.h:37: ScreenRotationAnimator* GetOrMakeAnimatorByDisplayId(int64_t display_id); I think it makes more sense to put these in a DisplayConfigurationControllerTestApi instead. The ShellTestApi will become a huge monolithic class if it has accessors to small details within each service that it owns.
Looked only at display_configuration_controller.cc Will look at more when you do the rebase. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:110: screen_rotation_animator->Rotate(rotation, source); Do you need to check screen_rotation_animator->CanAnimate() before you call Rotate()? https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:192: return display_animator_map_[display_id].get(); This function has 4 map searches! This is very inefficient. Let's try to minimize that, something like this may work better: auto iter = display_animator_map_.find(display_id); if (iter != display_animator_map_.end()) return iter->second.get(); auto animator = base::MakeUnique<ScreenRotationAnimator>(display_id); animator->SetObserver(this); ScreenRotationAnimator* result = animator.get(); display_animator_map_.insert(std::make_pair(display_id, std::move(animator))); return result; https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:201: display_animator_map_.end()); You can replace that with the simpler: DCHECK(display_animator_map_.count(screen_rotation_animator->display_id())); https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:215: display_animator_map_.erase(screen_rotation_animator->display_id()); Can you minimize the verbosity of this code a bit by maybe const caching the display_id and/or new_rotation?
Hi Ben and Ahmed, Could you please look the new patch again. Thanks, Tao https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:110: screen_rotation_animator->Rotate(rotation, source); On 2017/03/02 19:47:55, afakhry wrote: > Do you need to check screen_rotation_animator->CanAnimate() before you call > Rotate()? The consideration here is that, if we do not check CanAnimate here, and creates a ScreenRotationAnimator* object. Later when we enter Rotate(), we find we need to return immediately, the object will not be clean. We can call the observer to clean the object but it is an extra work to create the object and delete it immediately. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:192: return display_animator_map_[display_id].get(); On 2017/03/02 19:47:55, afakhry wrote: > This function has 4 map searches! This is very inefficient. Let's try to > minimize that, something like this may work better: > > auto iter = display_animator_map_.find(display_id); > if (iter != display_animator_map_.end()) > return iter->second.get(); > > auto animator = base::MakeUnique<ScreenRotationAnimator>(display_id); > animator->SetObserver(this); > ScreenRotationAnimator* result = animator.get(); > display_animator_map_.insert(std::make_pair(display_id, > std::move(animator))); > return result; > Nice catch! Changed accordingly. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:197: if (!screen_rotation_animator) On 2017/03/02 18:25:45, bruthig wrote: > Why do we need to support WeakPtrs here? If the DisplayConfigurationController > manages the lifetime of the ScreenRotationAnimators then can't we always > guarantee |screen_rotation_animator| to be non-null? Right. I will make it a real ptr. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:201: display_animator_map_.end()); On 2017/03/02 19:47:55, afakhry wrote: > You can replace that with the simpler: > > DCHECK(display_animator_map_.count(screen_rotation_animator->display_id())); Cool! Thanks. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.cc:215: display_animator_map_.erase(screen_rotation_animator->display_id()); On 2017/03/02 19:47:55, afakhry wrote: > Can you minimize the verbosity of this code a bit by maybe const caching the > display_id and/or new_rotation? Done. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:80: bool CanAnimate(int64_t display_id) const; On 2017/03/02 18:25:45, bruthig wrote: > "CanAnimate" is ambiguous, might I suggest "CanAnimateScreenRotation()" Done. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:81: display::Display::Rotation GetCurrentRotation(int64_t display_id) const; On 2017/03/02 18:25:45, bruthig wrote: > nit 1: newlines between function declarations here > nit 2: Can you make the declaration order in the .h match the definition order > in the .cc? > super-nit: GetRotation() is somewhat ambiguous might I suggest > GetCurrentScreenRotation()? And it would probably be helpful to document what > the return value is going to be if an animation is in progress. e.g. the target > rotation. 1. Done. 2. Done. 3. Done. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:82: // Polulate the ash::ScreenRotationAnimator On 2017/03/02 18:25:45, bruthig wrote: > nit: Polulate -> populate Removed the comment since I changed the function name to GetScreenRotationAnimatorForDisplay, which is clear itself. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:83: ScreenRotationAnimator* GetOrMakeAnimatorByDisplayId(int64_t display_id); On 2017/03/02 18:25:45, bruthig wrote: > This class also appears to use a DisplayAnimator => from the function name it is > ambiguous what animator is being created. Personally I don't think it's > necessary for the callers of this function to know that a new instance may or > may not be created so might I suggest just "GetRotationAnimatorForDisplay()" or > "GetScreenRotationAnimatorForDisplay()" Changed to GetScreenRotationAnimatorForDisplay. https://codereview.chromium.org/2728803002/diff/1/ash/display/display_configu... ash/display/display_configuration_controller.h:104: // more pending rotation request. On 2017/03/02 18:25:45, bruthig wrote: > Very helpful comment!! Acknowledged. https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:37: // struct ScreenRotationAnimator::RotationRequest { On 2017/03/02 18:25:45, bruthig wrote: > Did you mean to remove this? Done. https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:51: int64_t display_id() const; On 2017/03/02 18:25:45, bruthig wrote: > You should inline the definition of simple getters in the .h file. I move the simple getters and setters in the .h. https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.h:54: void set_is_animating(bool rotating); On 2017/03/02 18:25:45, bruthig wrote: > I think it is very odd that clients of the animator can change the > |is_animating_| value. I would think that should all be handled inside of this. I was debating myself where is the best to reset is_animating. Maybe the layerCleanupObserver is good place when we delete the old layer tree owner? Moved to there. https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator_unittest.cc:101: animator->Rotate(display::Display::ROTATE_90, On 2017/03/02 18:25:45, bruthig wrote: > FYI AshTestBase via AshTestHelper::SetUp() uses the ZERO_DURATION > ui::ScopedAnimationDurationScaleMode to control animations. I'm pretty sure it > is in effect here which means that the animation triggered on line 101 will > execute immediately and the second animation might not actually be getting > queued up like you expect. > > I think you should be able to create a local ScopedAnimationDurationScaleMode > instance with a SLOW_DURATION scale mode to have a higher chance of the second > animation actually being queued up. Done. You are right. The trick here is that in the layer clean up observer, I do not reset the is_animating state so that the test will pass. I will modify the test to reset the state on-animation-ended or aborted and add the SLOW_DURATION. https://codereview.chromium.org/2728803002/diff/1/ash/test/shell_test_api.h File ash/test/shell_test_api.h (right): https://codereview.chromium.org/2728803002/diff/1/ash/test/shell_test_api.h#n... ash/test/shell_test_api.h:37: ScreenRotationAnimator* GetOrMakeAnimatorByDisplayId(int64_t display_id); On 2017/03/02 18:25:45, bruthig wrote: > I think it makes more sense to put these in a > DisplayConfigurationControllerTestApi instead. The ShellTestApi will become a > huge monolithic class if it has accessors to small details within each service > that it owns. Done. Also revised the old test when to access DisplayConfigurationController::ResetAnimatorForTest().
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Tao, I've left some high level comments and I think there is some opportunities for some re-work here. Feel free to IM me or set up a GVC as some discussions might be more efficient in real time. Cheers, Ben https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:107: if (CanAnimateScreenRotation(display_id) && Would it make sense to return early if GetCurrentScreenRotation(display_id) == |rotation| or is there some reason we still want to be calling display_manager_->SetDisplayRotation() ? Just a consideration in case it's easy to determine whether the change is safe, otherwise you don't need to do that in this CL. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:113: display_manager_->SetDisplayRotation(display_id, rotation, source); It might be worth having a DCHECK in this else branch to ensure a ScreenRotationAnimator doesn't exist. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:139: if (!screen_rotation_animator) When is it a valid use case that |screen_rotation_animator| is null? I suspect it's not and this early return would be masking a bug. If it can be null then it should be documented along with the declaration in ScreenRotationAnimatorObserver. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:146: if (screen_rotation_animator->last_pending_request()) { I think the it would be better to move the queue of requests into the ScreenRotationAnimator and only notify this observer when all of the queued animations have completed. This would reduce the coupling between the two classes and should help to encapsulate more of the implementation details in ScreenRotationAnimator. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.h:109: display_animator_map_; |display_animator_map_| is confusing because there is also a |display_animator_| variable and the two types are completely different. Might I suggest using |rotation_animator_map_| instead. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:20: DISALLOW_COPY_AND_ASSIGN(DisplayConfigurationControllerTest); Missing "#include base/macros.h" for DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:39: ScreenRotationAnimator::ScreenRotationAnimatorObserver:: nit: newline between c'tor and d'tor. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:86: explicit LayerCleanupObserver( You should drop 'explicit' now that you have more than one parameter. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:147: if (animator_) { Double check but I believe |this| may outlive the |animator_| instance and the |animator_| isn't detaching itself as an observer when being destroyed. e.g. If shutting down while an animtion is in progress, which is more likely to happen than you would think due to not-so accurate accelerometer data. i.e. ~Shell()->~DisplayConfigurationController()->...->~ScreenRotationAnimator() destruction https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:214: old_layer_tree->root()->set_name("old_layer_tree"); "old_layer_tree" will be pretty ambiguous to someone reading this in the logs or inspecting the Layer tree. I suggest "ScreenRotationAnimator:old_layer_tree". https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:221: new LayerCleanupObserver(std::move(old_layer_tree), Is there any reason that the ScreenRotationAnimator is passing ownership of the LayerTreeOwner to the LayerCleanupObserver to have the observer only pass the ownership right back to the animator? https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:316: last_pending_request_.reset(rotation_request.release()); nit: For readability, consider wrapping this code in an AbortAnimation() function (assuming that's what it is doing) https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:26: ~ScreenRotationAnimatorObserver(); This should be virtual because you expect classes to extend it. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:50: void set_is_rotating(bool rotating) { is_rotating_ = rotating; }; |is_rotating_| seems to me that it should be internal state of ScreenRotationAnimator and as such shouldn't have public visibility. I assume you are doing this so that the LayerCleanupObserver can access it. I see 2 (or 3) options here: 1) Move these functions to private and make the LayerCleanupObserver a friend class. 2) Simply have the LayerCleanupObserver notify the ScreenRotationAnimator with a general event like AnimationCompleted() and allow the animator to manage all of it's internal state. 3) Similar to 2 but replace the LayerCleanupObserver with something like the CallbackLayerAnimationObserver and have the animator own the LayerTreeOwner. The callback would handle deleting the layer tree and updating the animator's internal state. (This would probably require the animator to maintain a reference to the observer in order to safely complete destruction) I have a strong preference for option 2 or 3 and I think either of them will need the use of a WeakPtr. https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.cc:26: DisplayConfigurationControllerTestApi::GetScreenRotationAnimatorForDisplay( This function should probably be a raw accessor to the controller_->display_animator_map_ and shouldn't have the side effect of creating a ScreenRotationAnimator if one doesn't exist. i.e. this should possibly return nullptr. https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.h:26: ScreenRotationAnimator* GetScreenRotationAnimatorForDisplay( nit: newline between declarations https://codereview.chromium.org/2728803002/diff/20001/ash/test/shell_test_api.h File ash/test/shell_test_api.h (left): https://codereview.chromium.org/2728803002/diff/20001/ash/test/shell_test_api... ash/test/shell_test_api.h:33: void DisableDisplayAnimator(); Thx for moving this into DisplayConfigurationControllerTestApi :)
Hi Ben, Please have another look of the cl. I feel like we still need to use CallbackLayerAnimationObserver in a later cl to handle two screenshot animation callback. But currently LayerCleanupObserver works good for now and use one screenshot for animation. We can discuss more on this in the future. Thanks, Tao https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:107: if (CanAnimateScreenRotation(display_id) && On 2017/03/08 22:35:59, bruthig wrote: > Would it make sense to return early if GetCurrentScreenRotation(display_id) == > |rotation| or is there some reason we still want to be calling > display_manager_->SetDisplayRotation() ? Just a consideration in case it's easy > to determine whether the change is safe, otherwise you don't need to do that in > this CL. I cannot think of a reason why we still need to call display_manager_->SetDisplayRotation() if the rotation is the same. Will change the code. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:113: display_manager_->SetDisplayRotation(display_id, rotation, source); On 2017/03/08 22:35:59, bruthig wrote: > It might be worth having a DCHECK in this else branch to ensure a > ScreenRotationAnimator doesn't exist. Done. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:139: if (!screen_rotation_animator) On 2017/03/08 22:35:59, bruthig wrote: > When is it a valid use case that |screen_rotation_animator| is null? I suspect > it's not and this early return would be masking a bug. > > If it can be null then it should be documented along with the declaration in > ScreenRotationAnimatorObserver. DisplayConfigurationController should maintain the life cycle of the screen_rotation_animator and when this function called, the animator must exist. I will remove the check. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:146: if (screen_rotation_animator->last_pending_request()) { On 2017/03/08 22:35:59, bruthig wrote: > I think the it would be better to move the queue of requests into the > ScreenRotationAnimator and only notify this observer when all of the queued > animations have completed. This would reduce the coupling between the two > classes and should help to encapsulate more of the implementation details in > ScreenRotationAnimator. Good suggestion. I will work on it. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.h:109: display_animator_map_; On 2017/03/08 22:35:59, bruthig wrote: > |display_animator_map_| is confusing because there is also a |display_animator_| > variable and the two types are completely different. Might I suggest using > |rotation_animator_map_| instead. Done. Did not notice that. Thanks. https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:20: DISALLOW_COPY_AND_ASSIGN(DisplayConfigurationControllerTest); On 2017/03/08 22:35:59, bruthig wrote: > Missing "#include base/macros.h" for DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:39: ScreenRotationAnimator::ScreenRotationAnimatorObserver:: On 2017/03/08 22:35:59, bruthig wrote: > nit: newline between c'tor and d'tor. Done. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:86: explicit LayerCleanupObserver( On 2017/03/08 22:35:59, bruthig wrote: > You should drop 'explicit' now that you have more than one parameter. Done. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:147: if (animator_) { On 2017/03/08 22:35:59, bruthig wrote: > Double check but I believe |this| may outlive the |animator_| instance and the > |animator_| isn't detaching itself as an observer when being destroyed. > > e.g. If shutting down while an animtion is in progress, which is more likely to > happen than you would think due to not-so accurate accelerometer data. i.e. > ~Shell()->~DisplayConfigurationController()->...->~ScreenRotationAnimator() > destruction At second thought, when ~ScreenRotationAnimator() called, then delete std::unique_ptr<ui::LayerTreeOwner> old_layer_tree_owner_, this will trigger OnLayerAnimationAborted and delete |this| before |animator_| deleted. But still change this back to weak_ptr. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:214: old_layer_tree->root()->set_name("old_layer_tree"); On 2017/03/08 22:35:59, bruthig wrote: > "old_layer_tree" will be pretty ambiguous to someone reading this in the logs or > inspecting the Layer tree. I suggest "ScreenRotationAnimator:old_layer_tree". Done. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:221: new LayerCleanupObserver(std::move(old_layer_tree), On 2017/03/08 22:35:59, bruthig wrote: > Is there any reason that the ScreenRotationAnimator is passing ownership of the > LayerTreeOwner to the LayerCleanupObserver to have the observer only pass the > ownership right back to the animator? No. Only reason is that I plan to remove LayerCleanupObserver in the near future CLs, so I try to make the change minimum here. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:316: last_pending_request_.reset(rotation_request.release()); On 2017/03/08 22:35:59, bruthig wrote: > nit: For readability, consider wrapping this code in an AbortAnimation() > function (assuming that's what it is doing) Done. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:26: ~ScreenRotationAnimatorObserver(); On 2017/03/08 22:35:59, bruthig wrote: > This should be virtual because you expect classes to extend it. Done. https://codereview.chromium.org/2728803002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:50: void set_is_rotating(bool rotating) { is_rotating_ = rotating; }; On 2017/03/08 22:35:59, bruthig wrote: > |is_rotating_| seems to me that it should be internal state of > ScreenRotationAnimator and as such shouldn't have public visibility. I assume > you are doing this so that the LayerCleanupObserver can access it. > > I see 2 (or 3) options here: > > 1) Move these functions to private and make the LayerCleanupObserver a friend > class. > > 2) Simply have the LayerCleanupObserver notify the ScreenRotationAnimator with a > general event like AnimationCompleted() and allow the animator to manage all of > it's internal state. > > 3) Similar to 2 but replace the LayerCleanupObserver with something like the > CallbackLayerAnimationObserver and have the animator own the LayerTreeOwner. > The callback would handle deleting the layer tree and updating the animator's > internal state. (This would probably require the animator to maintain a > reference to the observer in order to safely complete destruction) > > I have a strong preference for option 2 or 3 and I think either of them will > need the use of a WeakPtr. (3) is almost close to what I implemented for the two screenshots version and I planed to submit for review in the next next CLs. The reason why I did not use CallbackLayerAnimationObserver to replace the LayerCleanupObserver here is try to make this Cl smaller. After talked offline with bruthig@, we plan to use option 2 here for now. But we need to think about later how to handle two screenshots, need to wait two animations to finish. I will update the code soon. https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.cc:26: DisplayConfigurationControllerTestApi::GetScreenRotationAnimatorForDisplay( On 2017/03/08 22:35:59, bruthig wrote: > This function should probably be a raw accessor to the > controller_->display_animator_map_ and shouldn't have the side effect of > creating a ScreenRotationAnimator if one doesn't exist. i.e. this should > possibly return nullptr. But in the test of ash/display/display_configuration_controller_unittest.cc, I try to call this function to create a screen_rotation_animator, and test the screen_rotation_animator can be deleted after rotation. I cannot return nullptr here. https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.h:26: ScreenRotationAnimator* GetScreenRotationAnimatorForDisplay( On 2017/03/08 22:35:59, bruthig wrote: > nit: newline between declarations Done. When do we group functions together? For example accessor/mutator? https://codereview.chromium.org/2728803002/diff/20001/ash/test/shell_test_api.h File ash/test/shell_test_api.h (left): https://codereview.chromium.org/2728803002/diff/20001/ash/test/shell_test_api... ash/test/shell_test_api.h:33: void DisableDisplayAnimator(); On 2017/03/08 22:35:59, bruthig wrote: > Thx for moving this into DisplayConfigurationControllerTestApi :) Acknowledged.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi Tao, Thanks for addressing all my comments. I've left a few more but I think we're getting there. Again feel free to ping my in real time to discuss anything if you wish. Cheers, Ben https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/display/display_con... ash/display/display_configuration_controller.cc:146: if (screen_rotation_animator->last_pending_request()) { On 2017/03/09 22:09:19, wutao wrote: > On 2017/03/08 22:35:59, bruthig wrote: > > I think the it would be better to move the queue of requests into the > > ScreenRotationAnimator and only notify this observer when all of the queued > > animations have completed. This would reduce the coupling between the two > > classes and should help to encapsulate more of the implementation details in > > ScreenRotationAnimator. > > Good suggestion. I will work on it. Thanks, I think this re-work is a big improvement :) https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.cc (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.cc:26: DisplayConfigurationControllerTestApi::GetScreenRotationAnimatorForDisplay( On 2017/03/09 22:09:19, wutao wrote: > On 2017/03/08 22:35:59, bruthig wrote: > > This function should probably be a raw accessor to the > > controller_->display_animator_map_ and shouldn't have the side effect of > > creating a ScreenRotationAnimator if one doesn't exist. i.e. this should > > possibly return nullptr. > > But in the test of ash/display/display_configuration_controller_unittest.cc, I > try to call this function to create a screen_rotation_animator, and test the > screen_rotation_animator can be deleted after rotation. I cannot return nullptr > here. Ack. I wish there was an easier, less white-box way to perform that test but I can't think of any good suggestions. https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.h:26: ScreenRotationAnimator* GetScreenRotationAnimatorForDisplay( On 2017/03/09 22:09:19, wutao wrote: > On 2017/03/08 22:35:59, bruthig wrote: > > nit: newline between declarations > > Done. > When do we group functions together? For example accessor/mutator? I'm not sure if there's a concrete rule, but I would group functions together in the following cases: - They are all overrides from a specific parent class - Sometimes getter/setters - It's easier to document the functions as a group - Possibly if they are all simple pass throughs from a TestAPI to a production class (which may be the case here pending the debate on GetScreenRotationAnimatorForDisplay()'s behavior). But I would add a comment above the grouping along the lines of '// Wrapper functions for DisplayConfigurationController'. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller.h:12: #include "ash/rotator/screen_rotation_animator.h" super-nit: If ScreenRotationAnimatorObserver was in it's own .h/.cc then I think you'd be able to forward declare the ScreenRotationAnimator, which would potentially speed up build times. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller.h:96: ScreenRotationAnimator* GetScreenRotationAnimatorForDisplay( nit: Can you doc this method that it creates one if it doesn't exist. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:16: class DisplayConfigurationControllerTest : public AshTestBase { Thanks for adding this missing test fixture!! https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:26: // TODO(wutao): needs display_configuration_controller. nit: Any chance you can include a bug number here? Leaving that as a breadcrumb might less likely to get missed. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:28: return; FYI I've proposed a change to the mus+ash team about this pattern for disabling tests (https://groups.google.com/a/google.com/forum/#!topic/mustash-team/TBmk6vWoXLw). I don't expect much push back on the proposal so it might be a good idea to use it in this CL as a concrete first use. It applies here and elsewhere. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:127: if (animator_) { nit: No need for the '{}' if the 'if' and 'body' only span 2 lines. Applies here and below. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:302: } else { nit: The 'else' is not really necessary since the 'if' block above will always return; https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:321: const ScreenRotationAnimator::ScreenRotationRequest pending_request = nit: Instead of creating a copy of the |last_pending_request_| I think it would be ok if you just re-used |last_pending_reqest_| in place of |pending_request| below. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:325: if (GetCurrentScreenRotation(display_id_) != new_rotation) { nit: I don't think duplicating this early return is adding much value. It's already done in Rotate() on line 277. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:333: this); If the body wraps onto more than one line the '{}' should be included. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:336: void ScreenRotationAnimator::AbortAnimations(ui::Layer* layer) { nit: It might make sense to make this a non-member, helper function in the anonymous namespace. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:345: if (old_layer_tree_owner_ && child_layer == old_layer_tree_owner_->root()) Adding a quick return if (!old_layer_tree_owner_) might make this more readable/efficient. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:13: #include "ui/compositor/layer_tree_owner.h" nit: Can you forward declare this instead? https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:32: struct ScreenRotationRequest { If you make Rotate() a member function, I think you will be able to prune this struct down to just |new_rotation| and |source|. And it would probably make sense to move this struct into the ScreenRotationAnimator class instead of the Observer at that point. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:47: void Rotate(display::Display::Rotation new_rotation, Can you document when the |observer_| will and will not be notified. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:61: void set_old_layer_tree_owner(ui::LayerTreeOwner* layer_tree_owner) { Instead of making set_old_layer_tree_owner() and GetOldLayerTreeRootLayer() public functions it might make more sense to make the ::RotateScreen() function in the .cc file a member function. RotateScreen() might be better named as AnimateRotation() when it's a member function. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:86: void ContinueOrCleanUp(); Some docs here would be helpful or a rename of ProcessAnimationQueue() might be a little more self-documenting. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:52: class ScreenRotationAnimatorTest : public AshTestBase { Thanks again for adding this missing test fixture!! https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:84: TEST_F(ScreenRotationAnimatorTest, DeletesAnimator) { Isn't this more verifying that the observer is called. Verifying that the animator_ is actually deleted is mixing in the implementation details of the DisplayConfigurationController? https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:138: I think it would be VERY valuable to have a test that queues up a second animation while one is in progress and ensures the animations don't continue forever. The processing of the |ScreenRotationAnimator::last_pending_request_| is not easy to follow. Check out the MultiLayerAnimatorTestController and MultiLayerAnimatorTestControllerDelegate classes on how you can control the rotation animations from tests and not have to use insufficient DurationScaleModes.
https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:26: // TODO(wutao): needs display_configuration_controller. On 2017/03/10 22:34:07, bruthig wrote: > nit: Any chance you can include a bug number here? Leaving that as a breadcrumb > might less likely to get missed. You can include crbug.com/686839 here. We need display animations working in mash before this test can be updated.
The CQ bit was checked by wutao@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...
Hi Ben, Could you please look this cl again. Thanks. Tao https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... File ash/test/display_configuration_controller_test_api.h (right): https://codereview.chromium.org/2728803002/diff/20001/ash/test/display_config... ash/test/display_configuration_controller_test_api.h:26: ScreenRotationAnimator* GetScreenRotationAnimatorForDisplay( On 2017/03/10 22:34:07, bruthig wrote: > On 2017/03/09 22:09:19, wutao wrote: > > On 2017/03/08 22:35:59, bruthig wrote: > > > nit: newline between declarations > > > > Done. > > When do we group functions together? For example accessor/mutator? > > I'm not sure if there's a concrete rule, but I would group functions together in > the following cases: > - They are all overrides from a specific parent class > - Sometimes getter/setters > - It's easier to document the functions as a group > - Possibly if they are all simple pass throughs from a TestAPI to a production > class (which may be the case here pending the debate on > GetScreenRotationAnimatorForDisplay()'s behavior). But I would add a comment > above the grouping along the lines of '// Wrapper functions for > DisplayConfigurationController'. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller.h:12: #include "ash/rotator/screen_rotation_animator.h" On 2017/03/10 22:34:07, bruthig wrote: > super-nit: If ScreenRotationAnimatorObserver was in it's own .h/.cc then I think > you'd be able to forward declare the ScreenRotationAnimator, which would > potentially speed up build times. Moved ScreenRotationAnimatorObserver to it's own .h. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller.h:96: ScreenRotationAnimator* GetScreenRotationAnimatorForDisplay( On 2017/03/10 22:34:07, bruthig wrote: > nit: Can you doc this method that it creates one if it doesn't exist. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:16: class DisplayConfigurationControllerTest : public AshTestBase { On 2017/03/10 22:34:07, bruthig wrote: > Thanks for adding this missing test fixture!! Acknowledged. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:26: // TODO(wutao): needs display_configuration_controller. On 2017/03/10 22:34:07, bruthig wrote: > nit: Any chance you can include a bug number here? Leaving that as a breadcrumb > might less likely to get missed. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:28: return; On 2017/03/10 22:34:07, bruthig wrote: > FYI I've proposed a change to the mus+ash team about this pattern for disabling > tests > (https://groups.google.com/a/google.com/forum/#!topic/mustash-team/TBmk6vWoXLw). > > I don't expect much push back on the proposal so it might be a good idea to use > it in this CL as a concrete first use. It applies here and elsewhere. As discussed offline and in the proposal thread, I will leave as it here but use the proposed pattern in the screen_rotation_animator_unittest file. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:127: if (animator_) { On 2017/03/10 22:34:08, bruthig wrote: > nit: No need for the '{}' if the 'if' and 'body' only span 2 lines. Applies > here and below. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:302: } else { On 2017/03/10 22:34:08, bruthig wrote: > nit: The 'else' is not really necessary since the 'if' block above will always > return; What if old_layer_tree_owner_ is not assigned yet? Here we just want to make sure we will not call GetOldLayerTreeRootLayer() before old_layer_tree_owner_ has value. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:321: const ScreenRotationAnimator::ScreenRotationRequest pending_request = On 2017/03/10 22:34:08, bruthig wrote: > nit: Instead of creating a copy of the |last_pending_request_| I think it would > be ok if you just re-used |last_pending_reqest_| in place of |pending_request| > below. lol. Right. Forget this when I moved the code to here from display_configuration_controller. With the change below, this if block is much clean now. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:325: if (GetCurrentScreenRotation(display_id_) != new_rotation) { On 2017/03/10 22:34:08, bruthig wrote: > nit: I don't think duplicating this early return is adding much value. It's > already done in Rotate() on line 277. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:333: this); On 2017/03/10 22:34:07, bruthig wrote: > If the body wraps onto more than one line the '{}' should be included. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:336: void ScreenRotationAnimator::AbortAnimations(ui::Layer* layer) { On 2017/03/10 22:34:08, bruthig wrote: > nit: It might make sense to make this a non-member, helper function in the > anonymous namespace. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:345: if (old_layer_tree_owner_ && child_layer == old_layer_tree_owner_->root()) On 2017/03/10 22:34:08, bruthig wrote: > Adding a quick return if (!old_layer_tree_owner_) might make this more > readable/efficient. Do we assume if (!old_layer_tree_owner_), then there is no animation? Looks like is this situation. Then it should not enter StopAnimating() any way. As you mentioned in patch 2, maybe this kind of early return would be masking a bug. I plan to remove all the if (old_layer_tree_owner_) check. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:13: #include "ui/compositor/layer_tree_owner.h" On 2017/03/10 22:34:08, bruthig wrote: > nit: Can you forward declare this instead? Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:32: struct ScreenRotationRequest { On 2017/03/10 22:34:08, bruthig wrote: > If you make Rotate() a member function, I think you will be able to prune this > struct down to just |new_rotation| and |source|. And it would probably make > sense to move this struct into the ScreenRotationAnimator class instead of the > Observer at that point. It is already in ScreenRotationAnimator class :) I will make RotateScreen() a member function. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:47: void Rotate(display::Display::Rotation new_rotation, On 2017/03/10 22:34:08, bruthig wrote: > Can you document when the |observer_| will and will not be notified. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:61: void set_old_layer_tree_owner(ui::LayerTreeOwner* layer_tree_owner) { On 2017/03/10 22:34:08, bruthig wrote: > Instead of making set_old_layer_tree_owner() and GetOldLayerTreeRootLayer() > public functions it might make more sense to make the ::RotateScreen() function > in the .cc file a member function. > > RotateScreen() might be better named as AnimateRotation() when it's a member > function. Done. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:86: void ContinueOrCleanUp(); On 2017/03/10 22:34:08, bruthig wrote: > Some docs here would be helpful or a rename of ProcessAnimationQueue() might be > a little more self-documenting. Renamed. A doc will be a little redundant when I add the doc for when the |observer_| will be notified. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:52: class ScreenRotationAnimatorTest : public AshTestBase { On 2017/03/10 22:34:08, bruthig wrote: > Thanks again for adding this missing test fixture!! Acknowledged. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:84: TEST_F(ScreenRotationAnimatorTest, DeletesAnimator) { On 2017/03/10 22:34:08, bruthig wrote: > Isn't this more verifying that the observer is called. Verifying that the > animator_ is actually deleted is mixing in the implementation details of the > DisplayConfigurationController? You are right. Deleting animator is tested in the DisplayConfigurationCOntroller. I will modify the test. https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:138: On 2017/03/10 22:34:08, bruthig wrote: > I think it would be VERY valuable to have a test that queues up a second > animation while one is in progress and ensures the animations don't continue > forever. The processing of the |ScreenRotationAnimator::last_pending_request_| > is not easy to follow. > > Check out the MultiLayerAnimatorTestController and > MultiLayerAnimatorTestControllerDelegate classes on how you can control the > rotation animations from tests and not have to use insufficient > DurationScaleModes. I will add a new test with a ScreenRotationAnimatorTestApi and MultiLayerAnimatorTestController etc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey Mitsuru, Tao, I took a quick look and left some comments. I will circle back for a more thorough review but this is probably ready for an OWNER's review too. Cheers, Ben https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:302: } else { On 2017/03/14 16:46:37, wutao wrote: > On 2017/03/10 22:34:08, bruthig wrote: > > nit: The 'else' is not really necessary since the 'if' block above will always > > return; > What if old_layer_tree_owner_ is not assigned yet? Here we just want to make > sure we will not call GetOldLayerTreeRootLayer() before old_layer_tree_owner_ > has value. I'm not suggesting to remove the body of the 'else' block, just the else block. i.e. ... if (old_layer_tree_owner_) return old_layer_tree_owner_->root(); NOTREACHED(); return nullptr; } https://codereview.chromium.org/2728803002/diff/60001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/display/display_con... ash/display/display_configuration_controller.h:30: class ScreenRotationAnimatorObserver; nit: remove? https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:94: explicit LayerCleanupObserver(base::WeakPtr<ScreenRotationAnimator> animator); Docs for accepts an |animator| and why it is a WeakPtr. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:175: // Set the screen orientation to |new_rotation| and animate the change. The Should these docs be moved to the .h file? https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:270: // controll the animation. nit: 'controll' -> 'control' https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:288: StopAnimating(); nit: A comment here explaining that the queue will be processed because the OnLayerAnimation(Ended|Aborted) methods should be called would be helpful. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:323: Rotate(last_pending_request_->new_rotation, last_pending_request_->source); Can we pop the request off the queue here or does it have to be done in ::Rotate() ? https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:60: base::WeakPtr<ScreenRotationAnimator> WeakPtr(); Is this required or can you use weak_factory_.GetWeakPtr() instead? At the very least this doesn't appear to require public visibility. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:68: bool disable_animation_timers_for_test_; This could use a doc. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:74: void AnimateRotation(const ScreenRotationRequest& rotation_request); Function declarations should go before member variables. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:79: void ProcessAnimationQueue(); Docs? https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_observer.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:12: ScreenRotationAnimatorObserver(){}; nit: I think the style guide wants a space between '()' and '{}' https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/test/screen... File ash/rotator/test/screen_rotation_animator_test_api.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/test/screen... ash/rotator/test/screen_rotation_animator_test_api.h:26: class ScreenRotationAnimatorTestApi This looks GREAT, I'm so happy it was possible to use this!!
Hi Oshima, Could you please have a look of the new patch as the OWNER. Hi Ben, I fixed the nits in the previous patch. Thanks, Tao https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:302: } else { On 2017/03/15 23:12:37, bruthig wrote: > On 2017/03/14 16:46:37, wutao wrote: > > On 2017/03/10 22:34:08, bruthig wrote: > > > nit: The 'else' is not really necessary since the 'if' block above will > always > > > return; > > What if old_layer_tree_owner_ is not assigned yet? Here we just want to make > > sure we will not call GetOldLayerTreeRootLayer() before old_layer_tree_owner_ > > has value. > > I'm not suggesting to remove the body of the 'else' block, just the else block. > i.e. > > ... > if (old_layer_tree_owner_) > return old_layer_tree_owner_->root(); > NOTREACHED(); > return nullptr; > } Done. https://codereview.chromium.org/2728803002/diff/60001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/display/display_con... ash/display/display_configuration_controller.h:30: class ScreenRotationAnimatorObserver; On 2017/03/15 23:12:37, bruthig wrote: > nit: remove? Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:94: explicit LayerCleanupObserver(base::WeakPtr<ScreenRotationAnimator> animator); On 2017/03/15 23:12:37, bruthig wrote: > Docs for accepts an |animator| and why it is a WeakPtr. Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:175: // Set the screen orientation to |new_rotation| and animate the change. The On 2017/03/15 23:12:37, bruthig wrote: > Should these docs be moved to the .h file? Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:270: // controll the animation. On 2017/03/15 23:12:37, bruthig wrote: > nit: 'controll' -> 'control' Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:288: StopAnimating(); On 2017/03/15 23:12:37, bruthig wrote: > nit: A comment here explaining that the queue will be processed because the > OnLayerAnimation(Ended|Aborted) methods should be called would be helpful. Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:323: Rotate(last_pending_request_->new_rotation, last_pending_request_->source); On 2017/03/15 23:12:37, bruthig wrote: > Can we pop the request off the queue here or does it have to be done in > ::Rotate() ? We might. We can make a copy and reset the last_pending_request_ before Rotate(). We also can pop after Rotate(). But the concern is that, when animation aborted/ended, if the last_pending_request_ is not reset, then Rotate() will be called again. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:60: base::WeakPtr<ScreenRotationAnimator> WeakPtr(); On 2017/03/15 23:12:38, bruthig wrote: > Is this required or can you use weak_factory_.GetWeakPtr() instead? > > At the very least this doesn't appear to require public visibility. I can use weak_factory_.GetWeakPtr(). https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:68: bool disable_animation_timers_for_test_; On 2017/03/15 23:12:37, bruthig wrote: > This could use a doc. Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:74: void AnimateRotation(const ScreenRotationRequest& rotation_request); On 2017/03/15 23:12:37, bruthig wrote: > Function declarations should go before member variables. Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:79: void ProcessAnimationQueue(); On 2017/03/15 23:12:37, bruthig wrote: > Docs? Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_observer.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:12: ScreenRotationAnimatorObserver(){}; On 2017/03/15 23:12:38, bruthig wrote: > nit: I think the style guide wants a space between '()' and '{}' > Done. https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/test/screen... File ash/rotator/test/screen_rotation_animator_test_api.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/test/screen... ash/rotator/test/screen_rotation_animator_test_api.h:26: class ScreenRotationAnimatorTestApi On 2017/03/15 23:12:38, bruthig wrote: > This looks GREAT, I'm so happy it was possible to use this!! Acknowledged.
The CQ bit was checked by wutao@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...
wutao@chromium.org changed reviewers: - afakhry@chromium.org, bruthig@chromium.org, oshima@chromium.org
https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_observer.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:12: ScreenRotationAnimatorObserver(){}; On 2017/03/15 23:12:38, bruthig wrote: > nit: I think the style guide wants a space between '()' and '{}' > I run cl format. It removes the space again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Handles users rotating screen too fast Maintain a state |display_animator_map_| in display_configuration_controller. If there is a new rotation request, it should populate the map with display_id and create a new screen_rotation_animator. If there is a new rotation request during rotation animation, it should stop the animation immediately and add the new rotation request to the |last_pending_request_|. If there is no new request, in the animation observer, it should delete the screen_rotation_animator in the map. BUG=678763 TEST=manual, unit_tests ========== to ========== Handles users rotating screen too fast Maintain a state |display_animator_map_| in display_configuration_controller. If there is a new rotation request, it should populate the map with display_id and create a new screen_rotation_animator. If there is a new rotation request during rotation animation, it should stop the animation immediately and add the new rotation request to the |last_pending_request_|. If there is no new request, in the animation observer, it should delete the screen_rotation_animator in the map. BUG=678763 TEST=manual, unit_tests ==========
wutao@chromium.org changed reviewers: + afakhry@chromium.org, bruthig@chromium.org, oshima@chromium.org
https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller.cc:184: bool DisplayConfigurationController::CanAnimateScreenRotation( instead of defining a method here, can you add IsActive(display_id) in DisplayManager and simply use it in caller side? https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller.cc:192: return display_manager_->GetDisplayInfo(display_id).GetActiveRotation(); inline this https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller.h:66: void OnEndedOrAbortedScreenRotationAnimation( OnRotationAnimationFinished. then document that it'll be called when ended or aborted. https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:14: namespace test { test namespace is for test utilities. test body should simply be ash. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:47: const int kCounterClockWiseRotation = 1; xxxRotationFactor (and update the comment) https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:66: display::Display::Rotation new_rotation) { mention thta 180 degree rotation is considered clockwise https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:224: if (child_layer == GetOldLayerTreeRootLayer()) get this once and reused in the method. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:311: return old_layer_tree_owner_->root(); can you document why only this place is checking null, while others don't? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:88: ScreenRotationAnimatorObserver* screen_rotation_animator_observer_; us ObserverList and follow the same pattern as other observers. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_observer.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:16: ScreenRotationAnimatorObserver(){}; space between () and {} https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:18: virtual void OnEndedOrAbortedScreenRotationAnimation( document this. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:19: ScreenRotationAnimator* animator){}; since this is the only one, make it pure virtual. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:22: virtual ~ScreenRotationAnimatorObserver(){}; ditto. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:37: namespace test { ditto https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:49: bool notified_; notified_ = false, and inline the ctor. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:50: }; move this to anonymous namespace https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:68: int64_t display_id() { return display_.id(); } const https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:91: base::MakeUnique<ScreenRotationAnimator>(display_.id()).release()); just = base::makeUnique<> should work. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:95: test_api()->SetDisableAnimationTimers(true); Either DisableAnimationTimers() (assuming you don't have to re-enable it) or SetAnimationTimersDisabled(true) https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:98: ui::ScopedAnimationDurationScaleMode::SLOW_DURATION)); makeunique https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:105: display_id()); do you need this? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:120: EXPECT_FALSE(test_api()->HasActiveAnimations()); don't you have to unobserve it? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:157: TEST_F(ScreenRotationAnimatorTest, AddsToPendingRequest) { RotateDuringRotation ? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:169: display::Display::RotationSource::ROTATION_SOURCE_USER); Isn't animation disabled this case? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:177: TEST_F(ScreenRotationAnimatorTest, ShouldCompleteAnimations) { can you document what this is testing? (compares to other tests) https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... File ash/rotator/test/screen_rotation_animator_test_api.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... ash/rotator/test/screen_rotation_animator_test_api.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. remove "(c)" same for the rest https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... File ash/rotator/test/screen_rotation_animator_test_api.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... ash/rotator/test/screen_rotation_animator_test_api.h:37: protected: why protected?
https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:323: Rotate(last_pending_request_->new_rotation, last_pending_request_->source); On 2017/03/16 07:37:59, wutao wrote: > On 2017/03/15 23:12:37, bruthig wrote: > > Can we pop the request off the queue here or does it have to be done in > > ::Rotate() ? > > We might. > We can make a copy and reset the last_pending_request_ before Rotate(). > We also can pop after Rotate(). But the concern is that, when animation > aborted/ended, if the last_pending_request_ is not reset, then Rotate() will be > called again. I think it would be easier to follow if you pop it off the queue here, making a copy, prior to calling Rotate(). https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_observer.h (right): https://codereview.chromium.org/2728803002/diff/60001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:12: ScreenRotationAnimatorObserver(){}; On 2017/03/16 07:40:50, wutao wrote: > On 2017/03/15 23:12:38, bruthig wrote: > > nit: I think the style guide wants a space between '()' and '{}' > > > > I run cl format. It removes the space again. Acknowledged. https://codereview.chromium.org/2728803002/diff/80001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2728803002/diff/80001/ash/BUILD.gn#newcode111 ash/BUILD.gn:111: "common/keyboard/keyboard_observer_register.cc", I'm assuming this delta came from a merge with master. For future reference you should upload a review patch set for these merges without any additional changes you are making to address review comments, etc. In many cases this makes reviewers jobs easier. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:269: // In unit test, we can use ui::test::MultiLayerAnimatorTestController to nit: Replace 'ui::test::MultiLayerAnimatorTestController' with 'ash::test::ScreenRotationAnimatorTestApi'. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:282: base::MakeUnique<ScreenRotationRequest>(); nit: Consider adding a constructor to the ScreenRotationRequest struct inline the assignment of |new_rotation| and |source| there. Less chance of a client forgetting to set them. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:323: screen_rotation_animator_observer_->OnEndedOrAbortedScreenRotationAnimation( Do we really want to be notifying the |observer_| here if we just kicked off a new animation? If possible can you add a test to confirm this is not happening? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:37: // display::Display that is not currently active. See www.crbug.com/479503. nit: 'Clients should only call |Rotate()| when can animate. For example an animation is not possible if |display_id_| specifies a display::Display that is not currently active. See www.crbug.com/479503.' is some what confusing and leaves the reader with more questions than it answers. I think the following would be better. 'Should only be called when |display_id_| references a valid display::Display.' This makes me wonder if there is a race condition. i.e. If a display is removed, like an external monitor, while there is a request pending, will it cause a crash when processing the queue? https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:40: // ongoging animation will be stoped and progressed to the target position, nit: 'ongoging' -> 'ongoing'
Hi Oshima and Ben, I uploaded another patch. Thanks for the review. Best, Tao https://codereview.chromium.org/2728803002/diff/80001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2728803002/diff/80001/ash/BUILD.gn#newcode111 ash/BUILD.gn:111: "common/keyboard/keyboard_observer_register.cc", On 2017/03/16 20:41:57, bruthig wrote: > I'm assuming this delta came from a merge with master. For future reference you > should upload a review patch set for these merges without any additional changes > you are making to address review comments, etc. In many cases this makes > reviewers jobs easier. Acknowledged. https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... File ash/display/display_configuration_controller.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller.cc:184: bool DisplayConfigurationController::CanAnimateScreenRotation( On 2017/03/16 20:13:09, oshima wrote: > instead of defining a method here, can you add IsActive(display_id) > in DisplayManager and simply use it in caller side? Done, name it IsDisplayValid(display_id) Is there any difference for the valid and active for display? https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller.cc:192: return display_manager_->GetDisplayInfo(display_id).GetActiveRotation(); On 2017/03/16 20:13:09, oshima wrote: > inline this Done. https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller.h:66: void OnEndedOrAbortedScreenRotationAnimation( On 2017/03/16 20:13:09, oshima wrote: > OnRotationAnimationFinished. then document that it'll be called when ended or > aborted. Changed to OnScreenRotationAnimationFinished https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... File ash/display/display_configuration_controller_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/display/display_con... ash/display/display_configuration_controller_unittest.cc:14: namespace test { On 2017/03/16 20:13:09, oshima wrote: > test namespace is for test utilities. test body should simply be ash. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:47: const int kCounterClockWiseRotation = 1; On 2017/03/16 20:13:09, oshima wrote: > xxxRotationFactor (and update the comment) Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:66: display::Display::Rotation new_rotation) { On 2017/03/16 20:13:09, oshima wrote: > mention thta 180 degree rotation is considered clockwise Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:224: if (child_layer == GetOldLayerTreeRootLayer()) On 2017/03/16 20:13:09, oshima wrote: > get this once and reused in the method. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:269: // In unit test, we can use ui::test::MultiLayerAnimatorTestController to On 2017/03/16 20:41:57, bruthig wrote: > nit: Replace 'ui::test::MultiLayerAnimatorTestController' with > 'ash::test::ScreenRotationAnimatorTestApi'. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:282: base::MakeUnique<ScreenRotationRequest>(); On 2017/03/16 20:41:57, bruthig wrote: > nit: Consider adding a constructor to the ScreenRotationRequest struct inline > the assignment of |new_rotation| and |source| there. Less chance of a client > forgetting to set them. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:311: return old_layer_tree_owner_->root(); On 2017/03/16 20:13:09, oshima wrote: > can you document why only this place is checking null, while others don't? I do not need to check. Removed it. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:323: screen_rotation_animator_observer_->OnEndedOrAbortedScreenRotationAnimation( On 2017/03/16 20:41:57, bruthig wrote: > Do we really want to be notifying the |observer_| here if we just kicked off a > new animation? If possible can you add a test to confirm this is not happening? A testing showing that I need to return after call |Rotate()|. Thanks! https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:37: // display::Display that is not currently active. See www.crbug.com/479503. On 2017/03/16 20:41:58, bruthig wrote: > nit: 'Clients should only call |Rotate()| when can animate. For example an > animation is not possible if |display_id_| specifies a display::Display that is > not currently active. See http://www.crbug.com/479503. is some what confusing and > leaves the reader with more questions than it answers. I think the following > would be better. > > 'Should only be called when |display_id_| references a valid display::Display.' > > This makes me wonder if there is a race condition. i.e. If a display is removed, > like an external monitor, while there is a request pending, will it cause a > crash when processing the queue? Right, in the ProcessAnimationQueue, I will add a check if (last_pending_request_ && IsDisplayValid(display_id_)) Rotate(); https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:40: // ongoging animation will be stoped and progressed to the target position, On 2017/03/16 20:41:57, bruthig wrote: > nit: 'ongoging' -> 'ongoing' Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.h:88: ScreenRotationAnimatorObserver* screen_rotation_animator_observer_; On 2017/03/16 20:13:09, oshima wrote: > us ObserverList and follow the same pattern as other observers. Currently only one observer. I will change to ObserverList though. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_observer.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:16: ScreenRotationAnimatorObserver(){}; On 2017/03/16 20:13:10, oshima wrote: > space between () and {} Added several times, but cl format removed it. I will make sure add it back. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:18: virtual void OnEndedOrAbortedScreenRotationAnimation( On 2017/03/16 20:13:09, oshima wrote: > document this. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:19: ScreenRotationAnimator* animator){}; On 2017/03/16 20:13:09, oshima wrote: > since this is the only one, make it pure virtual. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_observer.h:22: virtual ~ScreenRotationAnimatorObserver(){}; On 2017/03/16 20:13:09, oshima wrote: > ditto. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:37: namespace test { On 2017/03/16 20:13:10, oshima wrote: > ditto Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:49: bool notified_; On 2017/03/16 20:13:10, oshima wrote: > notified_ = false, and inline the ctor. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:50: }; On 2017/03/16 20:13:10, oshima wrote: > move this to anonymous namespace Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:68: int64_t display_id() { return display_.id(); } On 2017/03/16 20:13:10, oshima wrote: > const Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:91: base::MakeUnique<ScreenRotationAnimator>(display_.id()).release()); On 2017/03/16 20:13:10, oshima wrote: > just > = base::makeUnique<> > should work. Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:95: test_api()->SetDisableAnimationTimers(true); On 2017/03/16 20:13:10, oshima wrote: > Either > > DisableAnimationTimers() (assuming you don't have to re-enable it) > > or > > SetAnimationTimersDisabled(true) Changed to DisableAnimationTimers(). https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:98: ui::ScopedAnimationDurationScaleMode::SLOW_DURATION)); On 2017/03/16 20:13:10, oshima wrote: > makeunique Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:105: display_id()); On 2017/03/16 20:13:10, oshima wrote: > do you need this? This is new pattern proposed by Ben. I had discussion offline with him about what should check here. The proposal can be tracked by the link below: bruthig 2017/03/10 22:34:07 FYI I've proposed a change to the mus+ash team about this pattern for disabling tests (https://groups.google.com/a/google.com/forum/#!topic/mustash-team/TBmk6vWoXLw). I don't expect much push back on the proposal so it might be a good idea to use it in this CL as a concrete first use. It applies here and elsewhere. me 2017/03/14 16:46:37 Show quoted text As discussed offline and in the proposal thread, I will leave as it here but use the proposed pattern in the screen_rotation_animator_unittest file. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:120: EXPECT_FALSE(test_api()->HasActiveAnimations()); On 2017/03/16 20:13:10, oshima wrote: > don't you have to unobserve it? Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:157: TEST_F(ScreenRotationAnimatorTest, AddsToPendingRequest) { On 2017/03/16 20:13:10, oshima wrote: > RotateDuringRotation ? Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:169: display::Display::RotationSource::ROTATION_SOURCE_USER); On 2017/03/16 20:13:10, oshima wrote: > Isn't animation disabled this case? The animation is not disabled, just be held and cannot step. Must call |CompleteAnimations()| to finish the animation. The first animation will stop to target rotation (90) and start the second rotation to 180 degree. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator_unittest.cc:177: TEST_F(ScreenRotationAnimatorTest, ShouldCompleteAnimations) { On 2017/03/16 20:13:10, oshima wrote: > can you document what this is testing? (compares to other tests) This test is proposed by Ben to use MultiLayerAnimatorTestController to control the animation and will finish if we queued multiple requests. I added the doc. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... File ash/rotator/test/screen_rotation_animator_test_api.cc (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... ash/rotator/test/screen_rotation_animator_test_api.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/16 20:13:10, oshima wrote: > remove "(c)" > > same for the rest Done. https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... File ash/rotator/test/screen_rotation_animator_test_api.h (right): https://codereview.chromium.org/2728803002/diff/80001/ash/rotator/test/screen... ash/rotator/test/screen_rotation_animator_test_api.h:37: protected: On 2017/03/16 20:13:10, oshima wrote: > why protected? Moved to private.
The CQ bit was checked by wutao@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...
One request otherwise lgtm https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:329: Rotate(last_pending_request_->new_rotation, last_pending_request_->source); Ping on 'I think it would be easier to follow if you pop it (the last request) off the queue here, making a copy, prior to calling Rotate().' https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:39: // Otherwise, any ongoing animation will be stoped and progressed to the nit: stoped -> stopped
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can you also update the CL description? It should state what kind of issue this will address and the solution, instead of how implementation works. https://codereview.chromium.org/2728803002/diff/100001/ash/display/display_co... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/100001/ash/display/display_co... ash/display/display_configuration_controller.h:91: return display_manager_->GetDisplayInfo(display_id).GetActiveRotation(); this looks simple enough to do this in .cc file? https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:189: const ScreenRotationRequest& rotation_request) { probably better to take unique_ptr and caller pass the ownership? https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:213: old_layer_tree_owner_.reset(old_layer_tree.release()); nit: std::move() https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:297: StopAnimating(); don't have to be in this CL, but isn't it possible to continue the animation from the current animating position? https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:39: bool notified() { return notified_; } const method https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:46: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:51: } optional: you may inline this. (bc this is in .cc) https://codereview.chromium.org/2728803002/diff/100001/ash/test/display_confi... File ash/test/display_configuration_controller_test_api.h (right): https://codereview.chromium.org/2728803002/diff/100001/ash/test/display_confi... ash/test/display_configuration_controller_test_api.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. remove (c). please check other new files.
Description was changed from ========== Handles users rotating screen too fast Maintain a state |display_animator_map_| in display_configuration_controller. If there is a new rotation request, it should populate the map with display_id and create a new screen_rotation_animator. If there is a new rotation request during rotation animation, it should stop the animation immediately and add the new rotation request to the |last_pending_request_|. If there is no new request, in the animation observer, it should delete the screen_rotation_animator in the map. BUG=678763 TEST=manual, unit_tests ========== to ========== Handles users rotating screen too fast If the user rotates screen too fast while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests ==========
Hi Oshima, Could you please look it again. I also updated the description of the cl. Thanks, Tao https://codereview.chromium.org/2728803002/diff/100001/ash/display/display_co... File ash/display/display_configuration_controller.h (right): https://codereview.chromium.org/2728803002/diff/100001/ash/display/display_co... ash/display/display_configuration_controller.h:91: return display_manager_->GetDisplayInfo(display_id).GetActiveRotation(); On 2017/03/17 19:41:59, oshima wrote: > this looks simple enough to do this in .cc file? Removed. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:189: const ScreenRotationRequest& rotation_request) { On 2017/03/17 19:42:00, oshima wrote: > probably better to take unique_ptr and caller pass the ownership? Done. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:213: old_layer_tree_owner_.reset(old_layer_tree.release()); On 2017/03/17 19:41:59, oshima wrote: > nit: std::move() Done. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:297: StopAnimating(); On 2017/03/17 19:42:00, oshima wrote: > don't have to be in this CL, but isn't it possible to continue the animation > from the current animating position? Mark it. I will look into how to make it happen. Thanks. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.cc:329: Rotate(last_pending_request_->new_rotation, last_pending_request_->source); On 2017/03/17 00:38:44, bruthig wrote: > Ping on 'I think it would be easier to follow if you pop it (the last request) > off the queue here, making a > copy, prior to calling Rotate().' Did I miss this comment? Will make the change. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator.h (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator.h:39: // Otherwise, any ongoing animation will be stoped and progressed to the On 2017/03/17 00:38:44, bruthig wrote: > nit: stoped -> stopped Done. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... File ash/rotator/screen_rotation_animator_unittest.cc (right): https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:39: bool notified() { return notified_; } On 2017/03/17 19:42:00, oshima wrote: > const method Done. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:46: }; On 2017/03/17 19:42:00, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2728803002/diff/100001/ash/rotator/screen_rot... ash/rotator/screen_rotation_animator_unittest.cc:51: } On 2017/03/17 19:42:00, oshima wrote: > optional: you may inline this. (bc this is in .cc) Inlined. https://codereview.chromium.org/2728803002/diff/100001/ash/test/display_confi... File ash/test/display_configuration_controller_test_api.h (right): https://codereview.chromium.org/2728803002/diff/100001/ash/test/display_confi... ash/test/display_configuration_controller_test_api.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/03/17 19:42:00, oshima wrote: > remove (c). please check other new files. Missed this one. Checked again.
The CQ bit was checked by wutao@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...
nit: s/too fast/too early/ lgtm
Description was changed from ========== Handles users rotating screen too fast If the user rotates screen too fast while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests ========== to ========== Handles users rotating screen too early If the user rotates screen too early while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests ==========
Description was changed from ========== Handles users rotating screen too early If the user rotates screen too early while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests ========== to ========== Handles users rotating screen too early If the user rotates screen too early while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests R=oshima@chromium.org, bruthig@chromium.org ==========
On 2017/03/17 20:53:44, oshima wrote: > nit: s/too fast/too early/ > > lgtm Updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2728803002/#ps140001 (title: "Rebased to commit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489806192049780,
"parent_rev": "7fed384c1a1d7a6c89531dddbb8b539061a9e455", "commit_rev":
"6ddafa38e3e8bfc0b856149a6a9366c7b0da80f4"}
Message was sent while issue was closed.
Description was changed from ========== Handles users rotating screen too early If the user rotates screen too early while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests R=oshima@chromium.org, bruthig@chromium.org ========== to ========== Handles users rotating screen too early If the user rotates screen too early while the previous animation is not finished yet, it will create another |old_layer_tree| with additional layers of previous |old_layer_tree|. The solution is that if there is a new rotation request during rotation animation, it should stop the animation immediately and start the new rotation request. BUG=678763 TEST=manual, unit_tests R=oshima@chromium.org, bruthig@chromium.org Review-Url: https://codereview.chromium.org/2728803002 Cr-Commit-Position: refs/heads/master@{#457946} Committed: https://chromium.googlesource.com/chromium/src/+/6ddafa38e3e8bfc0b856149a6a93... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6ddafa38e3e8bfc0b856149a6a93... |
