<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><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> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #281354 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<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#c21">Comment # 21</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=281354&action=diff" name="attach_281354" title="[GTK] Add kinetic scrolling">attachment 281354</a> <a href="attachment.cgi?id=281354&action=edit" title="[GTK] Add kinetic scrolling">[details]</a></span>
[GTK] Add kinetic scrolling
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=281354&action=review">https://bugs.webkit.org/attachment.cgi?id=281354&action=review</a>
Sorry for the delay reviewing this. Looks quite good in general, but there are still several minor issues to solve. Also, please check the EWS after submitting a patch to make sure it builds.
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:168
> + bool useLatchedEventElement() const { return false; }
> + bool isEndOfNonMomentumScroll() const;
> + bool isTransitioningToMomentumScroll() const;</span >
We should reorder what we share with cocoa instead of duplicating it
<span class="quote">> Source/WebCore/platform/PlatformWheelEvent.h:198
> +#elif PLATFORM(GTK)
> + PlatformWheelEventPhase m_phase { PlatformWheelEventPhaseNone };
> + PlatformWheelEventPhase m_momentumPhase { PlatformWheelEventPhaseNone };
> #endif</span >
Ditto.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:126
> +bool ScrollAnimationKinetic::scroll(ScrollbarOrientation, ScrollGranularity, float, float)
> +{
> + // This method isn't needed and shouldn't do anything as 'scroll' isn't
> + // how this animation needs to be used, use 'start' instead.
> + return true;
> +}</span >
We could move this to the header then, with the comment before the function, or even consider making scroll not pure virtual, I guess I assumed all animations would have to implement this when I added it.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:136
> + m_position = position;</span >
I wonder if we really need to save the current position, since we are not actually scrolling, and the initial position is always passed to start()
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:152
> + m_position.x(), velocity.x());
> + m_verticalData = PerAxisData(m_scrollableArea.minimumScrollPosition().y(),
> + m_scrollableArea.maximumScrollPosition().y(),
> + m_position.y(), velocity.y());</span >
We could use initialPosition.x(), initialPosition.y() here
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.cpp:173
> + m_position = FloatPoint(m_horizontalData.position(), m_verticalData.position());
> +
> + m_notifyPositionChangedFunction(FloatPoint(m_position));</span >
And pass the position directly to the callback
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:57
> + double m_lower { 0 };
> + double m_upper { 0 };
> +
> + double m_coef1 { 0 };
> + double m_coef2 { 0 };
> +
> + double m_elapsedTime { 0 };
> + double m_position { 0 };
> + double m_velocity { 0 };</span >
We don't use m_ prefix for struct members.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:67
> + bool scroll(ScrollbarOrientation, ScrollGranularity, float step, float multiplier) override;
> + void stop() override;
> + void updateVisibleLengths() override { }
> + void setCurrentPosition(const FloatPoint&) override;</span >
I don't think these should be public.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:77
> + PerAxisData m_horizontalData { PerAxisData(0, 0, 0, 0) };
> + PerAxisData m_verticalData { PerAxisData(0, 0, 0, 0) };</span >
You don't need to initialize these, all PerAxisData members are initialized.
<span class="quote">> Source/WebCore/platform/ScrollAnimationKinetic.h:81
> +</span >
Extra line here.
<span class="quote">> Source/WebCore/platform/graphics/FloatPoint.h:125
> + bool isOrigin() const { return m_x == 0.0 && m_y == 0.0; }
> +</span >
We don't compare to 0 in WebKit. I would leave this change to FloatPoint to a separate patch, since the rest of the patch is GTK+ specific, and just check x, y in the code. Also note that IntPoint has this methods as isZero(), so we should be consistent here.
<span class="quote">> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:91
> + m_phase = event->is_stop ?
> + PlatformWheelEventPhaseEnded :
> + PlatformWheelEventPhaseChanged;</span >
I don't understand this. When is it None then? Why is initially changed?
<span class="quote">> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:93
> + m_phase = event->direction == GDK_SCROLL_SMOOTH && m_deltaX == 0.0 && m_deltaY = 0.0 ?</span >
We don't compare to 0 in WebKit, see <a href="https://webkit.org/code-style-guidelines/#null-false-and-zero">https://webkit.org/code-style-guidelines/#null-false-and-zero</a>
<span class="quote">> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:98
> + m_phase = PlatformWheelEventPhaseChanged;</span >
Do we really need this? m_pahse is initialized in the header and will not be used in GTK+2 for sure.
<span class="quote">> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:101
> + m_momentumPhase = PlatformWheelEventPhaseNone;</span >
This is already None as initialized in the header.
<span class="quote">> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:116
> + return isTransitioningToMomentumScroll() ? FloatPoint(m_wheelTicksX, m_wheelTicksY) : FloatPoint();</span >
FloatPoint() -> { }
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:79
> + updatePosition(FloatPoint(position));</span >
updatePosition(WTFMove(position));
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:94
> +#if ENABLE(SMOOTH_SCROLLING)
> if (!m_scrollableArea.scrollAnimatorEnabled() || granularity == ScrollByPrecisePixel)
> return ScrollAnimator::scroll(orientation, granularity, step, multiplier);
>
> ensureSmoothScrollingAnimation();
> - return m_animation->scroll(orientation, granularity, step, multiplier);
> + return m_smoothAnimation->scroll(orientation, granularity, step, multiplier);
> +#else
> + return ScrollAnimator::scroll(orientation, granularity, step, multiplier);
> +#endif</span >
I think we could just keep this inside the #if ENABLE(SMOOTH_SCROLLING) as it was before, since you are not adding anything for non smooth scrolling.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:108
> + updatePosition(FloatPoint(position));</span >
updatePosition(WTFMove(position));
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:114
> + return FloatPoint();</span >
return { };
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:120
> + return FloatPoint();</span >
Ditto.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:128
> + return FloatPoint(accumDelta.x() * -1000 / (last - first), accumDelta.y() * -1000 / (last - first));</span >
And here you could also use { }
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:135
> + m_scrollHistory.removeAllMatching([&] (PlatformWheelEvent& otherEvent) -> bool {</span >
Capture explicitly event, instead of using &
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:140
> + m_kineticAnimation->start(m_currentPosition, computeVelocity());</span >
Don't we need to add the event to the history in this case too?
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:141
> +</span >
Extra line here.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:147
> +</span >
Ditto.
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:34
> +#include "ScrollAnimationKinetic.h"</span >
Why do you need to include this in the header?
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:48
> -#if ENABLE(SMOOTH_SCROLLING)
> bool scroll(ScrollbarOrientation, ScrollGranularity, float step, float multiplier) override;</span >
scroll is still specific to smooth scrolling
<span class="quote">> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:81
> + std::unique_ptr<ScrollAnimationKinetic> m_kineticAnimation;</span >
This should be ScrollAnimation not ScrollAnimationKinetic
<span class="quote">> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
> + NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);</span >
Maybe instead of adding a new constructor we could add the phase parameters with default values
<span class="quote">> Source/WebKit2/Shared/WebEvent.h:218
> +#elif PLATFORM(GTK)
> + Phase phase() const { return static_cast<Phase>(m_phase); }
> + Phase momentumPhase() const { return static_cast<Phase>(m_momentumPhase); }
> #endif</span >
We should reorder things to share with coca, instead of duplicating.
<span class="quote">> Source/WebKit2/Shared/WebEvent.h:241
> +#elif PLATFORM(GTK)
> + uint32_t m_phase { Phase::PhaseNone }; // Phase
> + uint32_t m_momentumPhase { Phase::PhaseNone }; // Phase
> #endif</span >
Ditto. Comments here // Phase don't really add anything, so I would remove them.
<span class="quote">> Source/WebKit2/Shared/WebEventConversion.cpp:151
> +#if PLATFORM(GTK)
> +inline WebCore::PlatformWheelEventPhase& operator|=(WebCore::PlatformWheelEventPhase& self, WebCore::PlatformWheelEventPhase other)
> +{
> + return self = static_cast<WebCore::PlatformWheelEventPhase>(self | other);
> +}
> +
> +static WebCore::PlatformWheelEventPhase toPlatformWheeleventPhase(WebWheelEvent::Phase phase)
> +{
> + WebCore::PlatformWheelEventPhase result = WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseNone;
> +
> + if (phase & WebWheelEvent::Phase::PhaseBegan)
> + result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseBegan;
> + if (phase & WebWheelEvent::Phase::PhaseStationary)
> + result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseStationary;
> + if (phase & WebWheelEvent::Phase::PhaseChanged)
> + result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseChanged;
> + if (phase & WebWheelEvent::Phase::PhaseEnded)
> + result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseEnded;
> + if (phase & WebWheelEvent::Phase::PhaseCancelled)
> + result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseCancelled;
> + if (phase & WebWheelEvent::Phase::PhaseMayBegin)
> + result |= WebCore::PlatformWheelEventPhase::PlatformWheelEventPhaseMayBegin;
> +
> + return result;
> +}
> +#endif</span >
I agree we shouldn't cast directly between WebKit and WebCore types, but this code should be shared with mac. Since they are casting, I suggest to share their current code for now to make this patch simpoler, and propose a new patch to do conversions instead of casts.
<span class="quote">> Source/WebKit2/Shared/WebEventConversion.cpp:191
> +#elif PLATFORM(GTK)
> + m_phase = toPlatformWheeleventPhase(webEvent.phase());
> + m_momentumPhase = toPlatformWheeleventPhase(webEvent.momentumPhase());
> #endif</span >
And then we could reuse this code too, instead of duplicate.
<span class="quote">> Source/WebKit2/Shared/WebWheelEvent.cpp:105
> +#elif PLATFORM(GTK)
> + encoder << m_phase;
> + encoder << m_momentumPhase;
> #endif</span >
Reorder things here too share the code with cocoa
<span class="quote">> Source/WebKit2/Shared/WebWheelEvent.cpp:139
> +#elif PLATFORM(GTK)
> + if (!decoder.decode(t.m_phase))
> + return false;
> + if (!decoder.decode(t.m_momentumPhase))
> + return false;</span >
Ditto.
<span class="quote">> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> +WebWheelEvent WebEventFactory::createWebWheelEvent(const GdkEvent* event, WebWheelEvent::Phase phase, WebWheelEvent::Phase momentumPhase)</span >
Here we could probably also keep a single method just with default values for the phases.
<span class="quote">> Source/WebKit2/UIProcess/WebPageProxy.cpp:1819
> - if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold)
> + if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && canSkipWheelEvent(event))</span >
m_wheelEventQueue.size() < wheelEventQueueSizeThreshold is also a condition to skip wheel events. So, I would replace all this with just if (!shouldProcessWheelEventNow(event)) and move the condition to the function. Note that we are not actually skipping events, just queuing them to process them later when we have a batch. So, the main question is why we are doing this, and why this doesn't work for us in this case. Because maybe this is just a matter of using 0 as threshold for the GTK+ port, and we always process wheel events immediately.
<span class="quote">> Source/WebKit2/UIProcess/WebPageProxy.cpp:1865
> +#if !PLATFORM(GTK)
> +bool WebPageProxy::canSkipWheelEvent(const WebWheelEvent&)
> +{
> + return true;
> +}
> +#endif</span >
Since we are adding platform ifdefs in cross-platform code in any case here, I would keep the function common, checking the event queue size for all platforms, and then adding an #ifdef for GTK+, something like:
bool WebPageProxy::shouldProcessWheelEventNow(const WebWheelEvent& event)
{
if (m_wheelEventQueue.size() >= wheelEventQueueSizeThreshold)
return true;
#if PLATFORM(GTK)
return ....
#endif
}
Another way to avoid platform ifdefs here would be to add a method to WebWheelEvent, something like shouldBeProcessesImmediately or something like that, with a default implementation returning false, and a GTK+ implementation. I still don't understand the whole thing to propose any of the solution, though. In any case, we should explain this in the changelog and add a comment in the code as well.
<span class="quote">> Source/WebKit2/UIProcess/gtk/GestureController.cpp:80
> -void GestureController::DragGesture::handleDrag(const GdkEvent* event, double x, double y)
> +static GUniquePtr<GdkEvent> newScrollEvent(const GdkEvent* event, double x, double y, double deltaX, double deltaY, gboolean isStop)</span >
use create instead of new</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>