[webkit-reviews] review granted: [Bug 77477] [chromium] Process scroll-gesture events from the MT compositor : [Attachment 125032] style

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted sadrul at chromium.org's request
for review:
Bug 77477: [chromium] Process scroll-gesture events from the MT compositor
https://bugs.webkit.org/show_bug.cgi?id=77477

Attachment 125032: style
https://bugs.webkit.org/attachment.cgi?id=125032&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125032&action=review


Looks great - have many nits, but nothing serious.

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.cpp:155
> +	   if (m_scrollStarted) {

i think we should ASSERT() on m_scrollStarted, not branch. if we get a
scrollupdate without a scrollbegin it means that the caller has broken the
contract with us - we should fail fast and loudly, not try to limp along IMO

> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:74
> +    bool m_expectScrollUpdateEnd;
> +    bool m_scrollStarted;

since these are just for assertions can you wrap them (and their use) in
#ifndef NDEBUG? I like to keep state that's just kept for debugging separate
from state that's used for actual logic

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:77
> +    MockWebCompositorInputHandlerClient() : m_handled(false),
m_sendToWidget(false) { }

expand the initialization out please - we generally don't like to have more
than one statement per line

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:80
> +    void Reset()

in WebKit, method names are lowecase

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:143
> +    OwnPtr<WebCompositorInputHandlerImpl> comp =
WebCompositorInputHandlerImpl::create(&mockInputHandler);

we don't typically like abbreviations in WebKit, even in tests. it's also not
super clear that 'comp' means 'WebCompositorInputHandlerImpl' can you name this
something like "inputHandler" ?

> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:149
> +    gesture.x = gesture.y = gesture.globalX = gesture.globalY =
gesture.deltaX = gesture.deltaY = 0;

WebGestureEvent's constructor zeros all these fields out, no need to do so
yourself