Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257023004:
Coalesced mouse wheel event has the position of the most recent event. (Closed)
DescriptionCoalesced mouse wheel event has the position of the most recent event.
BUG=632010
TEST=EventWithLatencyInfoTest.WebMouseWheelEventCoalescing
Committed: https://crrev.com/0268c7fbb4919f712fca7021e9f0080b2b40885a
Cr-Commit-Position: refs/heads/master@{#413229}
Patch Set 1 #
Total comments: 6
Patch Set 2 : a more verbose unittest. #
Messages
Total messages: 21 (11 generated)
sahel@chromium.org changed reviewers: + dtapuska@chromium.org, tdresser@chromium.org
tdresser@chromium.org: Please review changes in dtapuska@chromium.org: Please review changes in
The CQ bit was checked by sahel@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.
lgtm
https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... File content/common/input/event_with_latency_info_unittest.cc (right): https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... content/common/input/event_with_latency_info_unittest.cc:325: Coalesce(mouse_wheel_0, &mouse_wheel_1); Ultimately this coalescing test could probably be a little more verbose checking the delta and the other items are added correctly.. And testing that all the conditionals of CanCoalese are tested.
https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... File content/common/input/event_with_latency_info.cc (right): https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... content/common/input/event_with_latency_info.cc:99: event_to_coalesce.accelerationRatioY); WebMouseWheelEvent is a WebMouseEvent. Could we call WebMouseEvent::Coalesce in here, and then only deal explicitly with the deltas and wheelTicks? That way if someone adds a new field to WebMouseEvent, we don't need to remember to write the coalescing logic in both places. https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... File content/common/input/event_with_latency_info_unittest.cc (right): https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... content/common/input/event_with_latency_info_unittest.cc:325: Coalesce(mouse_wheel_0, &mouse_wheel_1); The naming here is a bit confusing. We might want to rename the events something like mouse_wheel_old, mouse_wheel_new.
https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... File content/common/input/event_with_latency_info.cc (right): https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... content/common/input/event_with_latency_info.cc:99: event_to_coalesce.accelerationRatioY); On 2016/08/19 12:43:30, tdresser wrote: > WebMouseWheelEvent is a WebMouseEvent. > Could we call WebMouseEvent::Coalesce in here, and then only deal explicitly > with the deltas and wheelTicks? > > That way if someone adds a new field to WebMouseEvent, we don't need to remember > to write the coalescing logic in both places. I see your point here, but this way we guarantee that the coalesced event is the new one with merged delta. It will be true if some logic is added to either of the classes. https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... File content/common/input/event_with_latency_info_unittest.cc (right): https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... content/common/input/event_with_latency_info_unittest.cc:325: Coalesce(mouse_wheel_0, &mouse_wheel_1); On 2016/08/19 01:41:41, dtapuska wrote: > Ultimately this coalescing test could probably be a little more verbose checking > the delta and the other items are added correctly.. > > And testing that all the conditionals of CanCoalese are tested. Done. https://codereview.chromium.org/2257023004/diff/1/content/common/input/event_... content/common/input/event_with_latency_info_unittest.cc:325: Coalesce(mouse_wheel_0, &mouse_wheel_1); On 2016/08/19 12:43:30, tdresser wrote: > The naming here is a bit confusing. > > We might want to rename the events something like mouse_wheel_old, > mouse_wheel_new. > Then naming is consistent with the rest of the unittests. I'll change the names of the events for all unittests to be more meaningful in a separate patch.
LGTM
The CQ bit was checked by sahel@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...
On 2016/08/19 15:39:09, tdresser wrote: > LGTM still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sahel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Coalesced mouse wheel event has the position of the most recent event. BUG=632010 TEST=EventWithLatencyInfoTest.WebMouseWheelEventCoalescing ========== to ========== Coalesced mouse wheel event has the position of the most recent event. BUG=632010 TEST=EventWithLatencyInfoTest.WebMouseWheelEventCoalescing Committed: https://crrev.com/0268c7fbb4919f712fca7021e9f0080b2b40885a Cr-Commit-Position: refs/heads/master@{#413229} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0268c7fbb4919f712fca7021e9f0080b2b40885a Cr-Commit-Position: refs/heads/master@{#413229} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
