[Webkit-unassigned] [Bug 155750] [GTK] Add kinetic scrolling
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 21 03:15:36 PDT 2016
https://bugs.webkit.org/show_bug.cgi?id=155750
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #281354|review?, commit-queue? |review-, commit-queue-
Flags| |
--- Comment #21 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 281354
--> https://bugs.webkit.org/attachment.cgi?id=281354
[GTK] Add kinetic scrolling
View in context: https://bugs.webkit.org/attachment.cgi?id=281354&action=review
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.
> Source/WebCore/platform/PlatformWheelEvent.h:168
> + bool useLatchedEventElement() const { return false; }
> + bool isEndOfNonMomentumScroll() const;
> + bool isTransitioningToMomentumScroll() const;
We should reorder what we share with cocoa instead of duplicating it
> Source/WebCore/platform/PlatformWheelEvent.h:198
> +#elif PLATFORM(GTK)
> + PlatformWheelEventPhase m_phase { PlatformWheelEventPhaseNone };
> + PlatformWheelEventPhase m_momentumPhase { PlatformWheelEventPhaseNone };
> #endif
Ditto.
> 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;
> +}
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.
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:136
> + m_position = position;
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()
> 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());
We could use initialPosition.x(), initialPosition.y() here
> Source/WebCore/platform/ScrollAnimationKinetic.cpp:173
> + m_position = FloatPoint(m_horizontalData.position(), m_verticalData.position());
> +
> + m_notifyPositionChangedFunction(FloatPoint(m_position));
And pass the position directly to the callback
> 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 };
We don't use m_ prefix for struct members.
> 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;
I don't think these should be public.
> Source/WebCore/platform/ScrollAnimationKinetic.h:77
> + PerAxisData m_horizontalData { PerAxisData(0, 0, 0, 0) };
> + PerAxisData m_verticalData { PerAxisData(0, 0, 0, 0) };
You don't need to initialize these, all PerAxisData members are initialized.
> Source/WebCore/platform/ScrollAnimationKinetic.h:81
> +
Extra line here.
> Source/WebCore/platform/graphics/FloatPoint.h:125
> + bool isOrigin() const { return m_x == 0.0 && m_y == 0.0; }
> +
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.
> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:91
> + m_phase = event->is_stop ?
> + PlatformWheelEventPhaseEnded :
> + PlatformWheelEventPhaseChanged;
I don't understand this. When is it None then? Why is initially changed?
> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:93
> + m_phase = event->direction == GDK_SCROLL_SMOOTH && m_deltaX == 0.0 && m_deltaY = 0.0 ?
We don't compare to 0 in WebKit, see https://webkit.org/code-style-guidelines/#null-false-and-zero
> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:98
> + m_phase = PlatformWheelEventPhaseChanged;
Do we really need this? m_pahse is initialized in the header and will not be used in GTK+2 for sure.
> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:101
> + m_momentumPhase = PlatformWheelEventPhaseNone;
This is already None as initialized in the header.
> Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:116
> + return isTransitioningToMomentumScroll() ? FloatPoint(m_wheelTicksX, m_wheelTicksY) : FloatPoint();
FloatPoint() -> { }
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:79
> + updatePosition(FloatPoint(position));
updatePosition(WTFMove(position));
> 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
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.
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:108
> + updatePosition(FloatPoint(position));
updatePosition(WTFMove(position));
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:114
> + return FloatPoint();
return { };
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:120
> + return FloatPoint();
Ditto.
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:128
> + return FloatPoint(accumDelta.x() * -1000 / (last - first), accumDelta.y() * -1000 / (last - first));
And here you could also use { }
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:135
> + m_scrollHistory.removeAllMatching([&] (PlatformWheelEvent& otherEvent) -> bool {
Capture explicitly event, instead of using &
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:140
> + m_kineticAnimation->start(m_currentPosition, computeVelocity());
Don't we need to add the event to the history in this case too?
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:141
> +
Extra line here.
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.cpp:147
> +
Ditto.
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:34
> +#include "ScrollAnimationKinetic.h"
Why do you need to include this in the header?
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:48
> -#if ENABLE(SMOOTH_SCROLLING)
> bool scroll(ScrollbarOrientation, ScrollGranularity, float step, float multiplier) override;
scroll is still specific to smooth scrolling
> Source/WebCore/platform/gtk/ScrollAnimatorGtk.h:81
> + std::unique_ptr<ScrollAnimationKinetic> m_kineticAnimation;
This should be ScrollAnimation not ScrollAnimationKinetic
> Source/WebKit2/Shared/NativeWebWheelEvent.h:55
> + NativeWebWheelEvent(GdkEvent*, WebWheelEvent::Phase, WebWheelEvent::Phase momentumPhase);
Maybe instead of adding a new constructor we could add the phase parameters with default values
> 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
We should reorder things to share with coca, instead of duplicating.
> Source/WebKit2/Shared/WebEvent.h:241
> +#elif PLATFORM(GTK)
> + uint32_t m_phase { Phase::PhaseNone }; // Phase
> + uint32_t m_momentumPhase { Phase::PhaseNone }; // Phase
> #endif
Ditto. Comments here // Phase don't really add anything, so I would remove them.
> 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
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.
> Source/WebKit2/Shared/WebEventConversion.cpp:191
> +#elif PLATFORM(GTK)
> + m_phase = toPlatformWheeleventPhase(webEvent.phase());
> + m_momentumPhase = toPlatformWheeleventPhase(webEvent.momentumPhase());
> #endif
And then we could reuse this code too, instead of duplicate.
> Source/WebKit2/Shared/WebWheelEvent.cpp:105
> +#elif PLATFORM(GTK)
> + encoder << m_phase;
> + encoder << m_momentumPhase;
> #endif
Reorder things here too share the code with cocoa
> 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;
Ditto.
> Source/WebKit2/Shared/gtk/WebEventFactory.cpp:165
> +WebWheelEvent WebEventFactory::createWebWheelEvent(const GdkEvent* event, WebWheelEvent::Phase phase, WebWheelEvent::Phase momentumPhase)
Here we could probably also keep a single method just with default values for the phases.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:1819
> - if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold)
> + if (m_wheelEventQueue.size() < wheelEventQueueSizeThreshold && canSkipWheelEvent(event))
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.
> Source/WebKit2/UIProcess/WebPageProxy.cpp:1865
> +#if !PLATFORM(GTK)
> +bool WebPageProxy::canSkipWheelEvent(const WebWheelEvent&)
> +{
> + return true;
> +}
> +#endif
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.
> 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)
use create instead of new
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160621/ed88d69d/attachment-0001.html>
More information about the webkit-unassigned
mailing list