Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
759073002:
Add canScroll bit to WebMouseWheelEvent (Closed)
DescriptionAdd canScroll bit to WebMouseWheelEvent
In the Blink, we add a flag to decide if Ctrl-wheel-scroll should scroll
or zoom. For all cases we will be doing a zoom with the wheel,
except touchpad scrolling for ChromeOS and all scrolling on MacOS.
There are some changes made in chromium and an unit test file
which depends on chromium change in the below patchs.
follow-up patch #2: https://codereview.chromium.org/835523006
follow-up patch #3: https://codereview.chromium.org/768443002
BUG=397027, 378755
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186780
Patch Set 1 : #Patch Set 2 : #
Total comments: 25
Patch Set 3 : Made changes based on comments #
Total comments: 4
Patch Set 4 : Modify unit test #
Total comments: 2
Patch Set 5 : rebase #
Messages
Total messages: 37 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:1) has been deleted
lanwei@chromium.org changed reviewers: + rbyers@chromium.org, tdresser@chromium.org
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... File Source/core/events/WheelEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... Source/core/events/WheelEvent.h:86: bool suppressScroll() const { return m_suppressScroll; } This naming is a bit confusing. suppressScroll() sounds like a method that would cause scrolling to be suppressed. Perhaps the variable should be named something like m_scrollingSuppressed, and this method should be scrollingSuppressed(). https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... File Source/platform/PlatformWheelEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... Source/platform/PlatformWheelEvent.h:109: bool suppressScroll() const { return m_suppressScroll; } I think the naming should be changed here as well. https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:716: TEST(WebInputEventConversionTest, CtrlWheelScroll) You should either modify an existing test, or add a new one, which looks at the case where control is not held. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:366: // scrolling on MACOS, we set suppressScroll to be false, which means always For consistency with other code, use "Mac OS" https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:367: // scrolling, for all other scrolling, we set it true, disabling all scrolling I find this comment hard to read. Can you try rewording it?
Thanks for fixing my hack to be more general Lan! This looks good, but I have a few suggestions. https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... File Source/core/events/WheelEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... Source/core/events/WheelEvent.h:86: bool suppressScroll() const { return m_suppressScroll; } On 2014/11/27 15:06:06, tdresser wrote: > This naming is a bit confusing. > suppressScroll() sounds like a method that would cause scrolling to be > suppressed. > > Perhaps the variable should be named something like m_scrollingSuppressed, and > this method should be scrollingSuppressed(). If it's clearer, we could also invert the meaning everywhere except the WebWheelEvent (where we really want the '0' value to be the common case). I.e. make this 'shouldScroll' or 'scrollingEnabled'. WDYT? https://codereview.chromium.org/759073002/diff/100001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:918: direction, curBox->isHorizontalWritingMode(), curBox->style()->isFlippedBlocksWritingMode()); What is this change? https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... File Source/platform/PlatformWheelEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... Source/platform/PlatformWheelEvent.h:130: bool m_suppressScroll; m_canScroll would be a name consistent with the rubber-band ones below... https://codereview.chromium.org/759073002/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:643: wheelTicksY = event.ticksY(); Since you ended up having to propagate your new bit now to WheelEvent, I think you need to update WebMouseWheelEventBuilder too (and add a test for it too - following the pattern of WebInputConversionTest::WebMouseEventBuilder). This could be important, for example, in browser tag scenarios where a WheelEvent ends up going out to a plugin and then across to another renderer process. https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:716: TEST(WebInputEventConversionTest, CtrlWheelScroll) On 2014/11/27 15:06:06, tdresser wrote: > You should either modify an existing test, Lan and I took a quick look together last night and I was surprised not to find an existing test for wheel events (other than a pinch-specific one), so I asked her to add a new one. > or add a new one, which looks at the > case where control is not held. Right, I'd just make this a simple generic test of PlatformWheelEventBuilder. The code in PlatformWheelEventBuilder is trivial enough that I don't think we need to test a variety of possible states, just verify that each field is getting propagated. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:364: // See comment at the top of the file for why an int is used here. Majid is just about to land his CL using bools (https://codereview.chromium.org/743883005/), please use a bool here as well and remove this comment. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:367: // scrolling, for all other scrolling, we set it true, disabling all scrolling On 2014/11/27 15:06:06, tdresser wrote: > I find this comment hard to read. > Can you try rewording it? How about something like this (remember blink and chromium are separate - blink shouldn't try to dictate exactly when chromium will set this)? When true, this wheel event should not trigger scrolling (or any other default action) if the event goes unhandled by JavaScript. This is used, for example, when the browser decides the default behavior for Ctrl+Wheel should be to zoom instead of scroll. This raises an interesting question I hadn't thought of before: should this really be "suppressScroll" or "preventDefault"? I'm not aware of any other default action for wheel events in blink (other than scrolling), but if there was one we'd probably want this bit to suppress it too. I kind of like the symmetry we get with Event.preventDefault by calling this 'preventDefault'. What do you guys think?
Also, please add bug 378755 to the list of bugs this CL applies to (i.e. BUG=397027,378755) - it's necessary for fixing both issues. And I'd update the CL summary and issue description to describe what you're changing in this specific CL rather than one of the bugs you're working towards fixing. I.e. "Add preventDefault bit to WebMouseWheelEvent".
On 2014/11/27 15:55:15, Rick Byers wrote: > Thanks for fixing my hack to be more general Lan! This looks good, but I have a > few suggestions. > > https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... > File Source/core/events/WheelEvent.h (right): > > https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... > Source/core/events/WheelEvent.h:86: bool suppressScroll() const { return > m_suppressScroll; } > On 2014/11/27 15:06:06, tdresser wrote: > > This naming is a bit confusing. > > suppressScroll() sounds like a method that would cause scrolling to be > > suppressed. > > > > Perhaps the variable should be named something like m_scrollingSuppressed, and > > this method should be scrollingSuppressed(). > > If it's clearer, we could also invert the meaning everywhere except the > WebWheelEvent (where we really want the '0' value to be the common case). I.e. > make this 'shouldScroll' or 'scrollingEnabled'. WDYT? > > https://codereview.chromium.org/759073002/diff/100001/Source/core/page/EventH... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/759073002/diff/100001/Source/core/page/EventH... > Source/core/page/EventHandler.cpp:918: direction, > curBox->isHorizontalWritingMode(), > curBox->style()->isFlippedBlocksWritingMode()); > What is this change? > > https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... > File Source/platform/PlatformWheelEvent.h (right): > > https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... > Source/platform/PlatformWheelEvent.h:130: bool m_suppressScroll; > m_canScroll would be a name consistent with the rubber-band ones below... > > https://codereview.chromium.org/759073002/diff/100001/Source/web/WebInputEven... > File Source/web/WebInputEventConversion.cpp (right): > > https://codereview.chromium.org/759073002/diff/100001/Source/web/WebInputEven... > Source/web/WebInputEventConversion.cpp:643: wheelTicksY = event.ticksY(); > Since you ended up having to propagate your new bit now to WheelEvent, I think > you need to update WebMouseWheelEventBuilder too (and add a test for it too - > following the pattern of WebInputConversionTest::WebMouseEventBuilder). This > could be important, for example, in browser tag scenarios where a WheelEvent > ends up going out to a plugin and then across to another renderer process. > > https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... > File Source/web/tests/WebInputEventConversionTest.cpp (right): > > https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... > Source/web/tests/WebInputEventConversionTest.cpp:716: > TEST(WebInputEventConversionTest, CtrlWheelScroll) > On 2014/11/27 15:06:06, tdresser wrote: > > You should either modify an existing test, > > Lan and I took a quick look together last night and I was surprised not to find > an existing test for wheel events (other than a pinch-specific one), so I asked > her to add a new one. > > > or add a new one, which looks at the > > case where control is not held. > > Right, I'd just make this a simple generic test of PlatformWheelEventBuilder. > The code in PlatformWheelEventBuilder is trivial enough that I don't think we > need to test a variety of possible states, just verify that each field is > getting propagated. > > https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEvent.h > File public/web/WebInputEvent.h (right): > > https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... > public/web/WebInputEvent.h:364: // See comment at the top of the file for why an > int is used here. > Majid is just about to land his CL using bools > (https://codereview.chromium.org/743883005/), please use a bool here as well and > remove this comment. > > https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... > public/web/WebInputEvent.h:367: // scrolling, for all other scrolling, we set it > true, disabling all scrolling > On 2014/11/27 15:06:06, tdresser wrote: > > I find this comment hard to read. > > Can you try rewording it? > > How about something like this (remember blink and chromium are separate - blink > shouldn't try to dictate exactly when chromium will set this)? > > When true, this wheel event should not trigger scrolling (or any other default > action) if the event goes unhandled by JavaScript. This is used, for example, > when the browser decides the default behavior for Ctrl+Wheel should be to zoom > instead of scroll. > > > This raises an interesting question I hadn't thought of before: should this > really be "suppressScroll" or "preventDefault"? I'm not aware of any other > default action for wheel events in blink (other than scrolling), but if there > was one we'd probably want this bit to suppress it too. I kind of like the > symmetry we get with Event.preventDefault by calling this 'preventDefault'. > What do you guys think? We talked about this in person and after considering a bunch of options decided that 'canScroll' is the best name to use everywhere. The WebMouseWheelEvent ctor should cause it to default to true, just like canRubberBandLeft/Right. preventDefault isn't a good name because then WheelEvent would have both m_preventDefault (the browser asking blink not to do it's default) and m_defaultPrevented (JS asking blink/browser not to do it's default) and that would be confusing.
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... File Source/core/events/WheelEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... Source/core/events/WheelEvent.h:86: bool suppressScroll() const { return m_suppressScroll; } On 2014/11/27 15:06:06, tdresser wrote: > This naming is a bit confusing. > suppressScroll() sounds like a method that would cause scrolling to be > suppressed. > > Perhaps the variable should be named something like m_scrollingSuppressed, and > this method should be scrollingSuppressed(). Done. https://codereview.chromium.org/759073002/diff/100001/Source/core/events/Whee... Source/core/events/WheelEvent.h:86: bool suppressScroll() const { return m_suppressScroll; } On 2014/11/27 15:55:14, Rick Byers wrote: > On 2014/11/27 15:06:06, tdresser wrote: > > This naming is a bit confusing. > > suppressScroll() sounds like a method that would cause scrolling to be > > suppressed. > > > > Perhaps the variable should be named something like m_scrollingSuppressed, and > > this method should be scrollingSuppressed(). > > If it's clearer, we could also invert the meaning everywhere except the > WebWheelEvent (where we really want the '0' value to be the common case). I.e. > make this 'shouldScroll' or 'scrollingEnabled'. WDYT? Done. https://codereview.chromium.org/759073002/diff/100001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:918: direction, curBox->isHorizontalWritingMode(), curBox->style()->isFlippedBlocksWritingMode()); On 2014/11/27 15:55:14, Rick Byers wrote: > What is this change? I do not know, I did not make this change. I guess when I pulled the recent change from master, this one showed up. https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... File Source/platform/PlatformWheelEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... Source/platform/PlatformWheelEvent.h:109: bool suppressScroll() const { return m_suppressScroll; } On 2014/11/27 15:06:06, tdresser wrote: > I think the naming should be changed here as well. Done. https://codereview.chromium.org/759073002/diff/100001/Source/platform/Platfor... Source/platform/PlatformWheelEvent.h:130: bool m_suppressScroll; On 2014/11/27 15:55:14, Rick Byers wrote: > m_canScroll would be a name consistent with the rubber-band ones below... Done. https://codereview.chromium.org/759073002/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:643: wheelTicksY = event.ticksY(); On 2014/11/27 15:55:14, Rick Byers wrote: > Since you ended up having to propagate your new bit now to WheelEvent, I think > you need to update WebMouseWheelEventBuilder too (and add a test for it too - > following the pattern of WebInputConversionTest::WebMouseEventBuilder). This > could be important, for example, in browser tag scenarios where a WheelEvent > ends up going out to a plugin and then across to another renderer process. Done. https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:716: TEST(WebInputEventConversionTest, CtrlWheelScroll) On 2014/11/27 15:06:06, tdresser wrote: > You should either modify an existing test, or add a new one, which looks at the > case where control is not held. Done. https://codereview.chromium.org/759073002/diff/100001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:716: TEST(WebInputEventConversionTest, CtrlWheelScroll) On 2014/11/27 15:55:15, Rick Byers wrote: > On 2014/11/27 15:06:06, tdresser wrote: > > You should either modify an existing test, > > Lan and I took a quick look together last night and I was surprised not to find > an existing test for wheel events (other than a pinch-specific one), so I asked > her to add a new one. > > > or add a new one, which looks at the > > case where control is not held. > > Right, I'd just make this a simple generic test of PlatformWheelEventBuilder. > The code in PlatformWheelEventBuilder is trivial enough that I don't think we > need to test a variety of possible states, just verify that each field is > getting propagated. Done. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:364: // See comment at the top of the file for why an int is used here. On 2014/11/27 15:55:15, Rick Byers wrote: > Majid is just about to land his CL using bools > (https://codereview.chromium.org/743883005/), please use a bool here as well and > remove this comment. I will update when I can pull the new changes from master. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:366: // scrolling on MACOS, we set suppressScroll to be false, which means always On 2014/11/27 15:06:06, tdresser wrote: > For consistency with other code, use "Mac OS" Done. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:367: // scrolling, for all other scrolling, we set it true, disabling all scrolling On 2014/11/27 15:55:15, Rick Byers wrote: > On 2014/11/27 15:06:06, tdresser wrote: > > I find this comment hard to read. > > Can you try rewording it? > > How about something like this (remember blink and chromium are separate - blink > shouldn't try to dictate exactly when chromium will set this)? > > When true, this wheel event should not trigger scrolling (or any other default > action) if the event goes unhandled by JavaScript. This is used, for example, > when the browser decides the default behavior for Ctrl+Wheel should be to zoom > instead of scroll. > > > This raises an interesting question I hadn't thought of before: should this > really be "suppressScroll" or "preventDefault"? I'm not aware of any other > default action for wheel events in blink (other than scrolling), but if there > was one we'd probably want this bit to suppress it too. I kind of like the > symmetry we get with Event.preventDefault by calling this 'preventDefault'. > What do you guys think? Done. https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:367: // scrolling, for all other scrolling, we set it true, disabling all scrolling On 2014/11/27 15:06:06, tdresser wrote: > I find this comment hard to read. > Can you try rewording it? Done.
Almost ready! https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEvent.h File public/web/WebInputEvent.h (right): https://codereview.chromium.org/759073002/diff/100001/public/web/WebInputEven... public/web/WebInputEvent.h:364: // See comment at the top of the file for why an int is used here. On 2014/12/02 06:28:22, lanwei wrote: > On 2014/11/27 15:55:15, Rick Byers wrote: > > Majid is just about to land his CL using bools > > (https://codereview.chromium.org/743883005/), please use a bool here as well > and > > remove this comment. > > I will update when I can pull the new changes from master. I talked with Majid. His CL might still be a couple days. We discovered that there is already at least one bool (WebKeyboardEvent::m_isSystemKey) and that seems to be fine. So you can safely use 'bool' for your new property without fear. Majid will clean up the others. https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:734: EXPECT_TRUE(webMouseWheel.canScroll); You should verify the other fields too while you're here (filling in the trivial missing test coverage instead of looking only at 'canScroll'). That probably means using different values for the co-ordinates instead of '1' everywhere. https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:756: webMouseWheelEvent.y = 10; nit: don't repeat values. That way your test would catch a simple mix-up where x and y got swapped.
https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:734: EXPECT_TRUE(webMouseWheel.canScroll); On 2014/12/02 15:48:21, Rick Byers wrote: > You should verify the other fields too while you're here (filling in the trivial > missing test coverage instead of looking only at 'canScroll'). That probably > means using different values for the co-ordinates instead of '1' everywhere. Done. https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:756: webMouseWheelEvent.y = 10; On 2014/12/02 15:48:21, Rick Byers wrote: > nit: don't repeat values. That way your test would catch a simple mix-up where > x and y got swapped. Done.
On 2014/12/02 22:27:54, lanwei wrote: > https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... > File Source/web/tests/WebInputEventConversionTest.cpp (right): > > https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... > Source/web/tests/WebInputEventConversionTest.cpp:734: > EXPECT_TRUE(webMouseWheel.canScroll); > On 2014/12/02 15:48:21, Rick Byers wrote: > > You should verify the other fields too while you're here (filling in the > trivial > > missing test coverage instead of looking only at 'canScroll'). That probably > > means using different values for the co-ordinates instead of '1' everywhere. > > Done. > > https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... > Source/web/tests/WebInputEventConversionTest.cpp:756: webMouseWheelEvent.y = 10; > On 2014/12/02 15:48:21, Rick Byers wrote: > > nit: don't repeat values. That way your test would catch a simple mix-up > where > > x and y got swapped. > > Done. I'll take another look once we've got a rebase that passes the tests.
lgtm with nit https://codereview.chromium.org/759073002/diff/180001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/180001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:747: EXPECT_NE(WebInputEvent::MetaKey, webMouseWheel.modifiers); nit: these three EXPECT_NE are unnecessary. modifiers is a set of flags and we're checking equality of the whole value, so checking modifiers==ControlKey means that modifiers can't possibly have any other value (eg. ControlKey | AltKey wouldn't pass the EQ test).
Patchset #6 (id:220001) has been deleted
rbyers@chromium.org changed reviewers: + haraken@chromium.org
+haraken for OWNERS in Source/web and Source/platform
+haraken for OWNERS in Source/web and Source/platform
Patchset #6 (id:240001) has been deleted
web/ and platform/ LGTM
Patchset #6 (id:260001) has been deleted
Patchset #7 (id:300001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:320001) has been deleted
Patchset #5 (id:340001) has been deleted
Patchset #5 (id:360001) has been deleted
On 2014/12/03 13:55:10, tdresser wrote: > On 2014/12/02 22:27:54, lanwei wrote: > > > https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... > > File Source/web/tests/WebInputEventConversionTest.cpp (right): > > > > > https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... > > Source/web/tests/WebInputEventConversionTest.cpp:734: > > EXPECT_TRUE(webMouseWheel.canScroll); > > On 2014/12/02 15:48:21, Rick Byers wrote: > > > You should verify the other fields too while you're here (filling in the > > trivial > > > missing test coverage instead of looking only at 'canScroll'). That > probably > > > means using different values for the co-ordinates instead of '1' everywhere. > > > > Done. > > > > > https://codereview.chromium.org/759073002/diff/160001/Source/web/tests/WebInp... > > Source/web/tests/WebInputEventConversionTest.cpp:756: webMouseWheelEvent.y = > 10; > > On 2014/12/02 15:48:21, Rick Byers wrote: > > > nit: don't repeat values. That way your test would catch a simple mix-up > > where > > > x and y got swapped. > > > > Done. > > I'll take another look once we've got a rebase that passes the tests. Tim, it has been updated to the latest version, can you please take a look at this? Thank you.
https://codereview.chromium.org/759073002/diff/180001/Source/web/tests/WebInp... File Source/web/tests/WebInputEventConversionTest.cpp (right): https://codereview.chromium.org/759073002/diff/180001/Source/web/tests/WebInp... Source/web/tests/WebInputEventConversionTest.cpp:747: EXPECT_NE(WebInputEvent::MetaKey, webMouseWheel.modifiers); On 2014/12/03 14:03:04, Rick Byers wrote: > nit: these three EXPECT_NE are unnecessary. modifiers is a set of flags and > we're checking equality of the whole value, so checking modifiers==ControlKey > means that modifiers can't possibly have any other value (eg. ControlKey | > AltKey wouldn't pass the EQ test). Done.
On 2014/12/09 05:16:50, lanwei wrote: > https://codereview.chromium.org/759073002/diff/180001/Source/web/tests/WebInp... > File Source/web/tests/WebInputEventConversionTest.cpp (right): > > https://codereview.chromium.org/759073002/diff/180001/Source/web/tests/WebInp... > Source/web/tests/WebInputEventConversionTest.cpp:747: > EXPECT_NE(WebInputEvent::MetaKey, webMouseWheel.modifiers); > On 2014/12/03 14:03:04, Rick Byers wrote: > > nit: these three EXPECT_NE are unnecessary. modifiers is a set of flags and > > we're checking equality of the whole value, so checking modifiers==ControlKey > > means that modifiers can't possibly have any other value (eg. ControlKey | > > AltKey wouldn't pass the EQ test). > > Done. LGTM!
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/759073002/380001
Message was sent while issue was closed.
Committed patchset #5 (id:380001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186780 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
