Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1214673006:
Reset unusedDelta on Onverscroll for residual values. (Closed)
DescriptionReset unusedDelta on Onverscroll for residual values.
Reset unusedDelta on Onverscroll for residual values. This avoid's
unnecessary didOverscroll calls for small residual values(as did in
compositor).
BUG=508079
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198641
Patch Set 1 #
Total comments: 18
Patch Set 2 : addressed review comments #Patch Set 3 : addressed review comments #
Total comments: 6
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : rebased to the latest #Messages
Total messages: 20 (8 generated)
sataya.m@samsung.com changed reviewers: + majidvp@chromium.org
sataya.m@samsung.com changed reviewers: + jdduke@chromium.org
Hi majid, PTAL. Though yesterday jdduke@ was mentioning the same https://codereview.chromium.org/1203693003/#msg49, i missed checking the same for android. sorry jddduke@. Thanks, MuVen.
First line in description should be < 80. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:100: const float kEpsilon = 0.1; Please add a comment explaining why 0.1. I suppose it is to be consistent with CC. Also make it static and choose a more descriptive name e.g., minimumOverscrollDelta and move it next to other constants. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:102: bool IsApproxZero(float value) This function is superfluous as it is only used once. Please get rid of it and inline the calculation. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:107: FloatSize ZeroSmallComponents(FloatSize unUsedDelta) I think the name should reflect that it is meant to be used for overscroll and not as a generic utility. I prefer using the same naming as CC for consistency which is |adjustOverscoll|. Also, I think a utility function like this should be made static inline. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:109: if (IsApproxZero(unUsedDelta.width())) s/unUsedDelta/unusedDelta/ for consistency https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:2275: unnecessary blank line. https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7926: TEST_F(WebFrameOverscrollTest, NoOverscrollForResidualvalues) s/Residualvalues/SmallValues/ https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7938: // ForResidual values overscrollDelta should be nullified and didOverscroll shouldn't be called. s/ForResidual/For residual/ s/nullified/reset/ https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7939: EXPECT_CALL(client, didOverscroll(WebFloatSize(), WebFloatSize(), WebFloatPoint(100, 100), WebFloatSize())).Times(0); I think these call expectations are busted. Essentially you are testing here that didOverscroll was not called with empty arguments which can be true even if it is called with 0.001. You should use _ as arguments matcher. https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7948: ScrollUpdate(&webViewHelper, 0.001, 0); Please use the threshold value (0.1) and a negative value in the tests too.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
PTAL. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:100: const float kEpsilon = 0.1; On 2015/07/09 15:02:46, majidvp wrote: > Please add a comment explaining why 0.1. I suppose it is to be consistent with > CC. > > Also make it static and choose a more descriptive name e.g., > minimumOverscrollDelta and move it next to other constants. Done. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:102: bool IsApproxZero(float value) On 2015/07/09 15:02:46, majidvp wrote: > This function is superfluous as it is only used once. > Please get rid of it and inline the calculation. Done. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:107: FloatSize ZeroSmallComponents(FloatSize unUsedDelta) On 2015/07/09 15:02:46, majidvp wrote: > I think the name should reflect that it is meant to be used for overscroll and > not as a generic utility. I prefer using the same naming as CC for consistency > which is |adjustOverscoll|. > > Also, I think a utility function like this should be made static inline. Done. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:109: if (IsApproxZero(unUsedDelta.width())) On 2015/07/09 15:02:46, majidvp wrote: > s/unUsedDelta/unusedDelta/ for consistency Done. https://codereview.chromium.org/1214673006/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:2275: On 2015/07/09 15:02:46, majidvp wrote: > unnecessary blank line. Done. https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7926: TEST_F(WebFrameOverscrollTest, NoOverscrollForResidualvalues) On 2015/07/09 15:02:46, majidvp wrote: > s/Residualvalues/SmallValues/ Done. https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7938: // ForResidual values overscrollDelta should be nullified and didOverscroll shouldn't be called. On 2015/07/09 15:02:46, majidvp wrote: > s/ForResidual/For residual/ > s/nullified/reset/ Done. https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7939: EXPECT_CALL(client, didOverscroll(WebFloatSize(), WebFloatSize(), WebFloatPoint(100, 100), WebFloatSize())).Times(0); On 2015/07/09 15:02:46, majidvp wrote: > I think these call expectations are busted. Essentially you are testing here > that didOverscroll was not called with empty arguments which can be true even if > it is called with 0.001. > > You should use _ as arguments matcher. Done. https://codereview.chromium.org/1214673006/diff/1/Source/web/tests/WebFrameTe... Source/web/tests/WebFrameTest.cpp:7948: ScrollUpdate(&webViewHelper, 0.001, 0); On 2015/07/09 15:02:46, majidvp wrote: > Please use the threshold value (0.1) and a negative value in the tests too. used threshold value(0.9) as if (std::abs(unusedDelta.width()) < minimumOverscrollDelta) wouldnt be true.
some nits: First line in description should be < 80. https://codereview.chromium.org/1214673006/diff/100001/Source/core/input/Even... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1214673006/diff/100001/Source/core/input/Even... Source/core/input/EventHandler.cpp:102: static const float minimumOverscrollDelta = 0.1; Please move this next to other constants. https://codereview.chromium.org/1214673006/diff/100001/Source/core/input/Even... Source/core/input/EventHandler.cpp:104: static inline FloatSize adjustOverscoll(FloatSize unusedDelta) Given that there is a single member func that uses this I think it help with readability if you move this just above where it is being used. In any case it should not be before using statements or constant definitions. https://codereview.chromium.org/1214673006/diff/100001/Source/web/tests/WebFr... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1214673006/diff/100001/Source/web/tests/WebFr... Source/web/tests/WebFrameTest.cpp:7937: Mock::VerifyAndClearExpectations(&client); I suggest adding a test to verify that the threshold value 0.1 is not reset. This ensures that tests document the expected behaviour.
Thanks for your patience. PTAL. https://codereview.chromium.org/1214673006/diff/100001/Source/core/input/Even... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1214673006/diff/100001/Source/core/input/Even... Source/core/input/EventHandler.cpp:102: static const float minimumOverscrollDelta = 0.1; On 2015/07/09 18:01:18, majidvp wrote: > Please move this next to other constants. Done. https://codereview.chromium.org/1214673006/diff/100001/Source/core/input/Even... Source/core/input/EventHandler.cpp:104: static inline FloatSize adjustOverscoll(FloatSize unusedDelta) On 2015/07/09 18:01:18, majidvp wrote: > Given that there is a single member func that uses this I think it help with > readability if you move this just above where it is being used. > > In any case it should not be before using statements or constant definitions. Done.
https://codereview.chromium.org/1214673006/diff/100001/Source/web/tests/WebFr... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1214673006/diff/100001/Source/web/tests/WebFr... Source/web/tests/WebFrameTest.cpp:7937: Mock::VerifyAndClearExpectations(&client); On 2015/07/09 18:01:18, majidvp wrote: > I suggest adding a test to verify that the threshold value 0.1 is not reset. > This ensures that tests document the expected behaviour. What about this?
Updated.
lgtm https://codereview.chromium.org/1214673006/diff/140001/Source/web/tests/WebFr... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1214673006/diff/140001/Source/web/tests/WebFr... Source/web/tests/WebFrameTest.cpp:7972: // Overscroll is not reported. nit: this comment does not add any value.
majidvp@chromium.org changed reviewers: + rbyers@chromium.org
+ rbyers@: for OWNERS review.
core/ LGTM
The CQ bit was checked by sataya.m@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from majidvp@chromium.org Link to the patchset: https://codereview.chromium.org/1214673006/#ps160001 (title: "rebased to the latest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1214673006/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198641 |
