<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add kinetic scrolling"
href="https://bugs.webkit.org/show_bug.cgi?id=155750#c11">Comment # 11</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add kinetic scrolling"
href="https://bugs.webkit.org/show_bug.cgi?id=155750">bug 155750</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=277468&action=diff" name="attach_277468" title="WIP patch">attachment 277468</a> <a href="attachment.cgi?id=277468&action=edit" title="WIP patch">[details]</a></span>
WIP patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=277468&action=review">https://bugs.webkit.org/attachment.cgi?id=277468&action=review</a>
Some more general comments
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:32
> +#if PLATFORM(GTK)
> +#include "FloatPoint.h"
> +#endif</span >
Don't use platform ifdefs to include cross-platform headers, just include the header if it's really needed.
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:73
> + enum PlatformWheelEventPhase {
> + PlatformWheelEventPhaseNone = 0,
> + PlatformWheelEventPhaseStop = 1 << 0,
> + PlatformWheelEventPhaseSwipe = 1 << 1,</span >
Do not line up this enum with the previous one. I wonder if we could reuse the cocoa enum even if we don't use all the flags, or are they too different?
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:95
> + , m_phase(PlatformWheelEventPhaseNone)</span >
Initialize this below when the member is delcared and you don't need to do this in every constructor.
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:96
> + , m_swipeVelocity(FloatPoint())</span >
This is not needed
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:120
> + , m_phase(PlatformWheelEventPhaseNone)
> + , m_swipeVelocity(FloatPoint())</span >
Ditto.
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:182
> + FloatPoint swipeVelocity() const { return m_swipeVelocity; }</span >
This should return a const reference instead of a copy.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:112
> + , m_horizontalData(0, 0, 0, 0)
> + , m_verticalData(0, 0, 0, 0)</span >
These are not needed.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:123
> + return true;</span >
Why is this empty now?
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:142
> + if (velocity == FloatPoint())</span >
I would do if (!velocity.x() && !velocity.y()) instead of creating a zero FloatPoint just to compare it.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:53
> + // FIXME Should this be moved to the initializer list even if it's a bit complex?</span >
I think it's fine here in the body.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:55
> + updatePosition(FloatPoint(position));</span >
You should move the position, updatePosition(WTFMove(position));
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:59
> +#if ENABLE(SMOOTH_SCROLLING)
> + if (m_smoothAnimation)
> + m_smoothAnimation->setCurrentPosition(position);
> +#endif</span >
Maybe updatePosition() should set the current position on both animations to keep everything in sync whenever the position is updated?
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:111
> +const FloatPoint ScrollAnimatorGtk::computeVelocity()</span >
Since we are returning a new FloatPoint, why making it const?
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:131
> +bool ScrollAnimatorGtk::handleWheelEvent(const PlatformWheelEvent& e)</span >
e -> event
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:136
> + return e.timestamp() - event.timestamp() > 150;</span >
Use a global static const for this 150 instead of magic number.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:58
> +
> +#if ENABLE(SMOOTH_SCROLLING)
> + void ensureSmoothScrollingAnimation();
> #endif
> + const FloatPoint computeVelocity();
> +
> + void updatePosition(FloatPoint&&);</span >
Move this below after the virtual methods
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:93
> + std::function<void(FloatPoint&&)> m_positionChangedCallback;</span >
What is this?
<span class="quote">> Source/WebKit2/Shared/WebEvent.h:223
> + WebCore::FloatPoint swipeVelocity() const { return m_swipeVelocity; }</span >
This should return a const reference.
<span class="quote">> Source/WebKit2/Shared/WebEventConversion.cpp:162
> + m_phase = static_cast<WebCore::PlatformWheelEventPhase>(webEvent.phase());</span >
We normally try to avoid casts to convert from/to enums, because it's very fragile in case any of the enum changes. Better use to/from conversion functions.
<span class="quote">> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:195
> +#if GTK_CHECK_VERSION(3, 19, 7)</span >
Since 3.20 was already released, let's check for 3, 20, 0 better instead of using an unstable release number.
<span class="quote">> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:199
> +#elif GTK_CHECK_VERSION(3, 3, 18)</span >
I think 3.6 is the minimum GTK+ version we support, so we don't need to check for 3.3 version.
<span class="quote">> Source/WebKit2/UIProcess/WebPageProxy.cpp:1835
> +#if PLATFORM(GTK)
> +#if GTK_CHECK_VERSION(3, 19, 7)
> + bool isScrollStopEvent = gdk_event_is_scroll_stop_event(event.nativeEvent());
> +#else
> + const GdkEventScroll& scroll = event.nativeEvent()->scroll;
> + bool isScrollStopEvent = scroll.direction == GDK_SCROLL_SMOOTH && scroll.delta_x && scroll.delta_y;
> +#endif
> + if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && !isScrollStopEvent)
> + return;
> +#else
> if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold)
> return;
> +#endif</span >
We need a better way to handle this than this ifdef mess here :-) We could have a method in the page client, for example, to check if a given wheel event should never be discarded, with this implementation in the GTK+ page client and returning false for all other ports.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>