Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2615613003:
Fix double ripple on activated flood fill ripple (Closed)
DescriptionFix double ripple on activated flood fill ripple
When switching from pending to activated state, there is no need to do
more animations as the final state of animations for these states are
the same. Otherwise, janks on slow devices might create some unwanted
effects like double ripples.
BUG=670831
TEST=manual
Review-Url: https://codereview.chromium.org/2615613003
Cr-Commit-Position: refs/heads/master@{#443366}
Committed: https://chromium.googlesource.com/chromium/src/+/30c4e21c362f23e2a40c5bba0483e59c3fc672ff
Patch Set 1 #
Total comments: 2
Patch Set 2 : Used pause animation to maintain observer order #
Total comments: 4
Patch Set 3 : Added tests #
Total comments: 11
Patch Set 4 : Addressed review comments #Patch Set 5 : Fixed InkDropHighlightTest crash #Patch Set 6 : Used AssertionResult #
Total comments: 4
Patch Set 7 : Addressed nits #Messages
Total messages: 45 (29 generated)
The CQ bit was checked by mohsen@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...
mohsen@chromium.org changed reviewers: + bruthig@chromium.org
Please take a look...
https://codereview.chromium.org/2615613003/diff/1/ui/views/animation/flood_fi... File ui/views/animation/flood_fill_ink_drop_ripple.cc (right): https://codereview.chromium.org/2615613003/diff/1/ui/views/animation/flood_fi... ui/views/animation/flood_fill_ink_drop_ripple.cc:263: if (old_ink_drop_state == InkDropState::ACTION_PENDING) { I believe this will cause the ACTIVATED |animation_observer| to be notified before the ACTION_PENDING |animation_observer| which is not desirable. The observers, namely InkDropRipple, expects the notifications to come in the same order that the animations were triggered in. You could try using a Pause LayerAnimationElement on the opacity and transform properties that you can enqueue and attach the |animation_observer| to.
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 mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2615613003/diff/1/ui/views/animation/flood_fi... File ui/views/animation/flood_fill_ink_drop_ripple.cc (right): https://codereview.chromium.org/2615613003/diff/1/ui/views/animation/flood_fi... ui/views/animation/flood_fill_ink_drop_ripple.cc:263: if (old_ink_drop_state == InkDropState::ACTION_PENDING) { On 2017/01/04 at 22:52:07, bruthig wrote: > I believe this will cause the ACTIVATED |animation_observer| to be notified before the ACTION_PENDING |animation_observer| which is not desirable. The observers, namely InkDropRipple, expects the notifications to come in the same order that the animations were triggered in. > > You could try using a Pause LayerAnimationElement on the opacity and transform properties that you can enqueue and attach the |animation_observer| to. Right. Done.
Looking good, just a request for tests. https://codereview.chromium.org/2615613003/diff/20001/ui/views/animation/floo... File ui/views/animation/flood_fill_ink_drop_ripple.cc (right): https://codereview.chromium.org/2615613003/diff/20001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple.cc:263: // The final state of pending animation is the same as the final state Can this be confirmed/enforced by a test? https://codereview.chromium.org/2615613003/diff/20001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple.cc:265: // so that animation observers are notified in order. Can this be confirmed/enforced by a test?
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 mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2615613003/diff/20001/ui/views/animation/floo... File ui/views/animation/flood_fill_ink_drop_ripple.cc (right): https://codereview.chromium.org/2615613003/diff/20001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple.cc:263: // The final state of pending animation is the same as the final state On 2017/01/05 at 20:20:39, bruthig wrote: > Can this be confirmed/enforced by a test? Done. https://codereview.chromium.org/2615613003/diff/20001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple.cc:265: // so that animation observers are notified in order. On 2017/01/05 at 20:20:39, bruthig wrote: > Can this be confirmed/enforced by a test? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry I forgot about this yesterday. Looking pretty good, just some nits and one question about some tests. https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/floo... File ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc:85: ripple.set_observer(&observer); nit: Can you add a TODO in InkDropRipple to handle null observers? Given that this change is targetting M56 I think it's better to keep this |observer| in the test but longer term we probably shouldn't require this overhead. https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc:91: float activated_opacity = test_api.GetCurrentOpacity(); nit: Might be a good idea to mark these as const. https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple_unittest.cc:382: "ACTION_PENDING ACTIVATED", How come you are building concatenated strings here when elsewhere you use the gmock ElementsAre matcher? My preference is to use the ElementsAre matcher and if we want better log output I think we can do this by adding a GTest printer for InkDropStates. See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop_ripple_observer.h (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop_ripple_observer.h:39: std::vector<InkDropState> GetAndResetAnimationStartedStates(); nit: Can these be pushed up into TestInkDropAnimationObserverHelper? nit: Can you add some docs too?
https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/floo... File ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc:85: ripple.set_observer(&observer); On 2017/01/10 at 17:08:03, bruthig wrote: > nit: Can you add a TODO in InkDropRipple to handle null observers? Given that this change is targetting M56 I think it's better to keep this |observer| in the test but longer term we probably shouldn't require this overhead. It's a very small change to add null check to InkDropRipple. Done. https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/floo... ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc:91: float activated_opacity = test_api.GetCurrentOpacity(); On 2017/01/10 at 17:08:03, bruthig wrote: > nit: Might be a good idea to mark these as const. Done. https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple_unittest.cc:382: "ACTION_PENDING ACTIVATED", On 2017/01/10 at 17:08:03, bruthig wrote: > How come you are building concatenated strings here when elsewhere you use the gmock ElementsAre matcher? > My preference is to use the ElementsAre matcher and if we want better log output I think we can do this by adding a GTest printer for InkDropStates. See https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... Yeah, I also prefer gmock matchers, but gmock is forbidden in views at the moment. https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/test... File ui/views/animation/test/test_ink_drop_ripple_observer.h (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/test... ui/views/animation/test/test_ink_drop_ripple_observer.h:39: std::vector<InkDropState> GetAndResetAnimationStartedStates(); On 2017/01/10 at 17:08:03, bruthig wrote: > nit: Can these be pushed up into TestInkDropAnimationObserverHelper? > > nit: Can you add some docs too? Done.
The CQ bit was checked by mohsen@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple_unittest.cc:382: "ACTION_PENDING ACTIVATED", On 2017/01/10 22:28:16, mohsen wrote: > On 2017/01/10 at 17:08:03, bruthig wrote: > > How come you are building concatenated strings here when elsewhere you use the > gmock ElementsAre matcher? > > My preference is to use the ElementsAre matcher and if we want better log > output I think we can do this by adding a GTest printer for InkDropStates. See > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > > Yeah, I also prefer gmock matchers, but gmock is forbidden in views at the > moment. Hmm that's unfortunate. Is that documented somewhere or were you just told by an OWNER? I wonder if bringing this CL to the deciders attention would lend weight to the argument at all...
https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple_unittest.cc:382: "ACTION_PENDING ACTIVATED", On 2017/01/11 at 00:41:07, bruthig wrote: > On 2017/01/10 22:28:16, mohsen wrote: > > On 2017/01/10 at 17:08:03, bruthig wrote: > > > How come you are building concatenated strings here when elsewhere you use the > > gmock ElementsAre matcher? > > > My preference is to use the ElementsAre matcher and if we want better log > > output I think we can do this by adding a GTest printer for InkDropStates. See > > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > > > > Yeah, I also prefer gmock matchers, but gmock is forbidden in views at the > > moment. > > Hmm that's unfortunate. Is that documented somewhere or were you just told by an OWNER? I wonder if bringing this CL to the deciders attention would lend weight to the argument at all... It is explicitly banned in ui/views/DEPS file (see https://crrev.com/107363004). I also talked with sadrul@ and he confirmed that's still the case.
lgtm https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... File ui/views/animation/ink_drop_ripple_unittest.cc (right): https://codereview.chromium.org/2615613003/diff/40001/ui/views/animation/ink_... ui/views/animation/ink_drop_ripple_unittest.cc:382: "ACTION_PENDING ACTIVATED", On 2017/01/11 15:59:44, mohsen wrote: > On 2017/01/11 at 00:41:07, bruthig wrote: > > On 2017/01/10 22:28:16, mohsen wrote: > > > On 2017/01/10 at 17:08:03, bruthig wrote: > > > > How come you are building concatenated strings here when elsewhere you use > the > > > gmock ElementsAre matcher? > > > > My preference is to use the ElementsAre matcher and if we want better log > > > output I think we can do this by adding a GTest printer for InkDropStates. > See > > > > https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... > > > > > > Yeah, I also prefer gmock matchers, but gmock is forbidden in views at the > > > moment. > > > > Hmm that's unfortunate. Is that documented somewhere or were you just told by > an OWNER? I wonder if bringing this CL to the deciders attention would lend > weight to the argument at all... > > It is explicitly banned in ui/views/DEPS file (see https://crrev.com/107363004). > I also talked with sadrul@ and he confirmed that's still the case. Thx
The CQ bit was checked by mohsen@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 mohsen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bruthig@, I've changed the code to use AssertionResult. Can you take another look, please?
slgtm Thanks for adding the AssertionResult! https://codereview.chromium.org/2615613003/diff/100001/ui/views/animation/tes... File ui/views/animation/test/test_ink_drop_animation_observer_helper.h (right): https://codereview.chromium.org/2615613003/diff/100001/ui/views/animation/tes... ui/views/animation/test/test_ink_drop_animation_observer_helper.h:117: testing::AssertionResult AnimationStartedContextsMatches( nit: "Match" reads better to me than "Matches", here and elsewhere https://codereview.chromium.org/2615613003/diff/100001/ui/views/animation/tes... ui/views/animation/test/test_ink_drop_animation_observer_helper.h:143: return testing::AssertionFailure() nit: Consider capturing log output for the entire |expected_contexts| and |actual_contexts| contents when a mis-match occurs, including size discrepancies. e.g. "Expected != Actual: {(ACTION_PENDING == ACTION_PENDING), (ACTIVATED != ACTION_TRIGGERED)}"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2615613003/diff/100001/ui/views/animation/tes... File ui/views/animation/test/test_ink_drop_animation_observer_helper.h (right): https://codereview.chromium.org/2615613003/diff/100001/ui/views/animation/tes... ui/views/animation/test/test_ink_drop_animation_observer_helper.h:117: testing::AssertionResult AnimationStartedContextsMatches( On 2017/01/11 at 23:18:04, bruthig wrote: > nit: "Match" reads better to me than "Matches", here and elsewhere Done. https://codereview.chromium.org/2615613003/diff/100001/ui/views/animation/tes... ui/views/animation/test/test_ink_drop_animation_observer_helper.h:143: return testing::AssertionFailure() On 2017/01/11 at 23:18:04, bruthig wrote: > nit: Consider capturing log output for the entire |expected_contexts| and |actual_contexts| contents when a mis-match occurs, including size discrepancies. > > e.g. > > "Expected != Actual: {(ACTION_PENDING == ACTION_PENDING), (ACTIVATED != ACTION_TRIGGERED)}" Done.
The CQ bit was checked by mohsen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2615613003/#ps120001 (title: "Addressed nits")
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": 120001, "attempt_start_ts": 1484249522248710,
"parent_rev": "ae7d8e56c8cc244b4ca0ae692b76e662c2563329", "commit_rev":
"30c4e21c362f23e2a40c5bba0483e59c3fc672ff"}
Message was sent while issue was closed.
Description was changed from ========== Fix double ripple on activated flood fill ripple When switching from pending to activated state, there is no need to do more animations as the final state of animations for these states are the same. Otherwise, janks on slow devices might create some unwanted effects like double ripples. BUG=670831 TEST=manual ========== to ========== Fix double ripple on activated flood fill ripple When switching from pending to activated state, there is no need to do more animations as the final state of animations for these states are the same. Otherwise, janks on slow devices might create some unwanted effects like double ripples. BUG=670831 TEST=manual Review-Url: https://codereview.chromium.org/2615613003 Cr-Commit-Position: refs/heads/master@{#443366} Committed: https://chromium.googlesource.com/chromium/src/+/30c4e21c362f23e2a40c5bba0483... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/30c4e21c362f23e2a40c5bba0483... |
