[Webkit-unassigned] [Bug 49345] [Chromium] Add a basic GestureManager to Chromium

Fri Dec 10 16:18:09 PST 2010

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

--- Comment #10 from Robert Kroeger <rjkroege at chromium.org>  2010-12-10 16:18:09 PST ---
(In reply to comment #5)
> (From update of attachment 75402 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75402&action=review
> 
> > LayoutTests/fast/events/touch/touch-gesture-scroll.html:46
> > +<body style="margin:0" onload="setTimeout('runTest();', 100)">
> 
> Why wait 100ms?

because EventSender::sendCurrentTouchEvent didn't call webview()->layout(). fixed.

> 
> > LayoutTests/fast/events/touch/touch-gesture-scroll.html:84
> > +        successfullyParsed = true;
> > +        isSuccessfullyParsed();
> 
> Seems like we should do these even if there's no layoutTestController.

done.

> 
> > LayoutTests/fast/events/touch/touch-gesture-scroll.html:113
> > +    // Wait for layout.
> > +    setTimeout(checkScrollOffset, 50);
> 
> Boo.  setTimeout makes tests slow and flaky.

setTimeout removed throughout. I had forgotten to clean up after https://bugs.webkit.org/show_bug.cgi?id=49285

done.

> 
> > LayoutTests/fast/events/touch/touch-gesture-scroll.html:139
> > +    layoutTestController.notifyDone();
> 
> This should be protected by if (window.layoutTestController)

done, fourth patch.

> 
> > LayoutTests/fast/events/touch/touch-gesture-scroll.html:155
> > +            setTimeout(exitIfNecessary, 200);
> 
> boo

fixed.

> 
> > WebCore/page/EventHandler.cpp:3001
> > -}
> > +} // namespace WebCore
> 
> This change isn't really necessary.  We're inconsistent about whether to include these comments.

done

> 
> > WebCore/page/GestureManager.cpp:34
> > +// #include <wtf/Assertions.h>
> 
> No commented out code, please.  :)

done.

> 
> > WebCore/page/GestureManager.cpp:53
> > +static const double maxClickDownTime = .8;
> 
> 0.8
> 
> > WebCore/page/GestureManager.cpp:56
> > +static const double minClickDownTime = .01;
> 
> ditto

done

> 
> > WebCore/page/GestureManager.cpp:78
> > +void GestureManager::addEdgeFunction(State state,
> > +                                     unsigned finger,
> > +                                     PlatformTouchPoint::State touchType,
> > +                                     bool handled,
> > +                                     GestureTransitionFunction f)
> 
> All on one line please.

done.

> 
> > WebCore/page/GestureManager.cpp:108
> > +    PlatformMouseEvent fakeMouseDown(p.pos(),
> > +                                     p.screenPos(),
> > +                                     LeftButton,
> > +                                     MouseEventPressed,
> > +                                     1,
> > +                                     false,
> > +                                     false,
> > +                                     false,
> > +                                     false,
> > +                                     WTF::currentTime());
> 
> WebKit doesn't have an 80 col limit.  Please feel free to use more horizontal space.  :)

google3 habits. :-) my bad. fixed.

> 
> > WebCore/page/GestureManager.cpp:153
> > +        GestureTransitionFunction ef = m_edgeFunctions.get(hash(m_state, p.id(), p.state(), handled));
> > +        if (ef)
> 
> We usually combine these lines.

done.

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