[Webkit-unassigned] [Bug 77477] [chromium] Process scroll-gesture events from the MT compositor

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77477





--- Comment #11 from sadrul at chromium.org  2012-02-02 20:21:21 PST ---
(From update of attachment 125032)
View in context: https://bugs.webkit.org/attachment.cgi?id=125032&action=review

>> 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

Indeed. I am using m_expectedScrollUpdateEnd for precisely this purpose (e.g see lines 139, 154, 162). m_scrollStarted is used to make sure we don't call scrollBy or scrollEnd if the call to scrollBegin returned something other than ScrollStarted.

>> Source/WebKit/chromium/src/WebCompositorInputHandlerImpl.h:74
>> +    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

m_expectedScrollUpdated is only used for assertions. So I have put it in #ifndef. Since m_scrollStarted is used to decide whether or not to call scrollBy/scrollEnd, I have left it unchanged.

>> 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

Done.

>> Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp:80
>> +    void Reset()
> 
> in WebKit, method names are lowecase

Done.

>> 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" ?

Done.

>> 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

Cool! Thanks. Removed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.