Bug 209230

Created attachment 393862 [details]
Patch

I think the 2 style errors are in keeping with the rest of the file and the immediate surrounds of the two lines too, but let me know if there's something I can do to fix that.

Comment on attachment 393862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393862&action=review

> Source/WebCore/ChangeLog:38
> +

I miss an explanation of changes made to several of these files. I don't have time for detailed review, sorry, but I can add a few comments.

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:189
> +void ScrollingTreeScrollingNode::stopScrollAnimations()
> +{
> +}

This empty impl can be moved to the header.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:39
> +#if ENABLE(KINETIC_SCROLLING)
> +#include "ScrollAnimationKinetic.h"
> +#endif

For ifdefed headers we don't follow the alphabetic order, move this after the unconditional includes block,

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:127
> -        scrollBy({ wheelEvent.deltaX(), -wheelEvent.deltaY() });
> +        scrollBy({ -wheelEvent.deltaX(), -wheelEvent.deltaY() });

This is bug #208638, but Zan told me that breaks touch scrolling, I don't know why.

> Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.h:44
> +#if ENABLE(KINETIC_SCROLLING)
> +class ScrollAnimationKinetic;
> +#endif

I don't think we need ifdefs for forward declarations.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:109
> +    : m_scrollExtentsFunction(scrollExtentsFunction)

WTFMove(scrollExtentsFunction)

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:113
>  {
>  }

Now that animation timer is a RunLoop::Timer we should give it a priority.

> Source/WebCore/platform/ScrollAnimationKinetic.h:68
> +using ScrollExtentsCallback = std::function<ScrollExtents(void)>;
> +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;

Is this correctly indented? I think we can use Function instead of std::function to make sure they aren't copyable.

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> -            m_axisEvent.x_axis = m_offset.x - touchPoint->x;
> +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);

This might explain why touch scrolling was broken with bug #208638

Thanks for the mini-review Carlos :)

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 393862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393862&action=review
> 
> > Source/WebCore/ChangeLog:38
> > +
> 
> I miss an explanation of changes made to several of these files. I don't
> have time for detailed review, sorry, but I can add a few comments.

My bad, I should have put more detail here - will do so.

> > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:189
> > +void ScrollingTreeScrollingNode::stopScrollAnimations()
> > +{
> > +}
> 
> This empty impl can be moved to the header.

If I move it to the header, I get a style warning (although I already get a style warning for the other two functions added and the file already seems to ignore this particular style requirement, so maybe I should here too)

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:39
> > +#if ENABLE(KINETIC_SCROLLING)
> > +#include "ScrollAnimationKinetic.h"
> > +#endif
> 
> For ifdefed headers we don't follow the alphabetic order, move this after
> the unconditional includes block,

Okidokes.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:127
> > -        scrollBy({ wheelEvent.deltaX(), -wheelEvent.deltaY() });
> > +        scrollBy({ -wheelEvent.deltaX(), -wheelEvent.deltaY() });
> 
> This is bug #208638, but Zan told me that breaks touch scrolling, I don't
> know why.

Indeed, the change you mention below is why.

> > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.h:44
> > +#if ENABLE(KINETIC_SCROLLING)
> > +class ScrollAnimationKinetic;
> > +#endif
> 
> I don't think we need ifdefs for forward declarations.

Okidokes

> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:109
> > +    : m_scrollExtentsFunction(scrollExtentsFunction)
> 
> WTFMove(scrollExtentsFunction)

Good spot.

> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:113
> >  {
> >  }
> 
> Now that animation timer is a RunLoop::Timer we should give it a priority.

Any suggestions about this? Every other similar use I could find via grep in the codebase doesn't set any kind of priority.

> > Source/WebCore/platform/ScrollAnimationKinetic.h:68
> > +using ScrollExtentsCallback = std::function<ScrollExtents(void)>;
> > +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
> 
> Is this correctly indented? I think we can use Function instead of
> std::function to make sure they aren't copyable.

I believe so. At least, I don't get any warnings and it matches formatting in several other files. I was following what was there before regarding use of std::function, but can change both to WTF::Function.

> > Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> > -            m_axisEvent.x_axis = m_offset.x - touchPoint->x;
> > +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
> 
> This might explain why touch scrolling was broken with bug #208638

Yes, this is the cause after applying the above fix - and judging on other similar event handling code, I believe this is the right fix for it.

(In reply to Chris Lord from comment #4)
> Thanks for the mini-review Carlos :)
> 
> (In reply to Carlos Garcia Campos from comment #3)
> > Comment on attachment 393862 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393862&action=review
> > 
> > > Source/WebCore/ChangeLog:38
> > > +
> > 
> > I miss an explanation of changes made to several of these files. I don't
> > have time for detailed review, sorry, but I can add a few comments.
> 
> My bad, I should have put more detail here - will do so.

Thanks. I see now that ScrollAnimator is not available in the case of async scrolling. That needs to be explained in the changelog, please.

> > > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:189
> > > +void ScrollingTreeScrollingNode::stopScrollAnimations()
> > > +{
> > > +}
> > 
> > This empty impl can be moved to the header.
> 
> If I move it to the header, I get a style warning (although I already get a
> style warning for the other two functions added and the file already seems
> to ignore this particular style requirement, so maybe I should here too)

What warning exactly?

> > > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:39
> > > +#if ENABLE(KINETIC_SCROLLING)
> > > +#include "ScrollAnimationKinetic.h"
> > > +#endif
> > 
> > For ifdefed headers we don't follow the alphabetic order, move this after
> > the unconditional includes block,
> 
> Okidokes.
> 
> > > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.cpp:127
> > > -        scrollBy({ wheelEvent.deltaX(), -wheelEvent.deltaY() });
> > > +        scrollBy({ -wheelEvent.deltaX(), -wheelEvent.deltaY() });
> > 
> > This is bug #208638, but Zan told me that breaks touch scrolling, I don't
> > know why.
> 
> Indeed, the change you mention below is why.
> 
> > > Source/WebCore/page/scrolling/nicosia/ScrollingTreeFrameScrollingNodeNicosia.h:44
> > > +#if ENABLE(KINETIC_SCROLLING)
> > > +class ScrollAnimationKinetic;
> > > +#endif
> > 
> > I don't think we need ifdefs for forward declarations.
> 
> Okidokes
> 
> > > Source/WebCore/platform/ScrollAnimationKinetic.cpp:109
> > > +    : m_scrollExtentsFunction(scrollExtentsFunction)
> > 
> > WTFMove(scrollExtentsFunction)
> 
> Good spot.
> 
> > > Source/WebCore/platform/ScrollAnimationKinetic.cpp:113
> > >  {
> > >  }
> > 
> > Now that animation timer is a RunLoop::Timer we should give it a priority.
> 
> Any suggestions about this? Every other similar use I could find via grep in
> the codebase doesn't set any kind of priority.

Include <wtf/glib/RunLoopSourcePriority.h> and then simply m_animationTimer.setPriority(). I think we can use LayerFlushTimer or add a new priority for this (even if it has the same value).

> > > Source/WebCore/platform/ScrollAnimationKinetic.h:68
> > > +using ScrollExtentsCallback = std::function<ScrollExtents(void)>;
> > > +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
> > 
> > Is this correctly indented? I think we can use Function instead of
> > std::function to make sure they aren't copyable.
> 
> I believe so. At least, I don't get any warnings and it matches formatting
> in several other files. I was following what was there before regarding use
> of std::function, but can change both to WTF::Function.

This code was added before WTF::Function existed.

> > > Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> > > -            m_axisEvent.x_axis = m_offset.x - touchPoint->x;
> > > +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
> > 
> > This might explain why touch scrolling was broken with bug #208638
> 
> Yes, this is the cause after applying the above fix - and judging on other
> similar event handling code, I believe this is the right fix for it.

Let's move this to the other patch then and land it before this one.

Comment on attachment 393862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393862&action=review

>>> Source/WebCore/platform/ScrollAnimationKinetic.h:68
>>> +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
>> 
>> Is this correctly indented? I think we can use Function instead of std::function to make sure they aren't copyable.
> 
> I believe so. At least, I don't get any warnings and it matches formatting in several other files. I was following what was there before regarding use of std::function, but can change both to WTF::Function.

These type aliases have to be indented like the rest of the declarations inside this class.

> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:182
> +    if (event->type == wpe_input_touch_event_type_null) {
> +        return WebWheelEvent(WebEvent::Wheel, position, position,
> +            delta, wheelTicks, phase, momentumPhase,
> +            WebWheelEvent::ScrollByPixelWheelEvent,
> +            OptionSet<WebEvent::Modifier> { }, wallTimeForEventTime(event->time));
> +    }

From where are touch events coming into `createWebWheelEvent()`? Why are null events expected to be handled here?

>>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
>>> +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
>> 
>> This might explain why touch scrolling was broken with bug #208638
> 
> Yes, this is the cause after applying the above fix - and judging on other similar event handling code, I believe this is the right fix for it.

This breaks horizontal touch scrolling.

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:178
> -                    struct wpe_input_axis_event* axisEvent = scrollGestureController.axisEvent();
> -                    if (axisEvent->type != wpe_input_axis_event_type_null)
> -                        page.handleWheelEvent(WebKit::NativeWebWheelEvent(axisEvent, page.deviceScaleFactor()));
> +                    page.handleWheelEvent(WebKit::NativeWebWheelEvent(scrollGestureController.axisEvent(), page.deviceScaleFactor(), scrollGestureController.phase(), WebWheelEvent::Phase::PhaseNone));

Why are null events being handled?

(In reply to Zan Dobersek from comment #6)
> Comment on attachment 393862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393862&action=review
> 
> >>> Source/WebCore/platform/ScrollAnimationKinetic.h:68
> >>> +using NotifyPositionChangedCallback = std::function<void(FloatPoint&&)>;
> >> 
> >> Is this correctly indented? I think we can use Function instead of std::function to make sure they aren't copyable.
> > 
> > I believe so. At least, I don't get any warnings and it matches formatting in several other files. I was following what was there before regarding use of std::function, but can change both to WTF::Function.
> 
> These type aliases have to be indented like the rest of the declarations
> inside this class.

Will change.

> > Source/WebKit/Shared/libwpe/WebEventFactory.cpp:182
> > +    if (event->type == wpe_input_touch_event_type_null) {
> > +        return WebWheelEvent(WebEvent::Wheel, position, position,
> > +            delta, wheelTicks, phase, momentumPhase,
> > +            WebWheelEvent::ScrollByPixelWheelEvent,
> > +            OptionSet<WebEvent::Modifier> { }, wallTimeForEventTime(event->time));
> > +    }
> 
> From where are touch events coming into `createWebWheelEvent()`? Why are
> null events expected to be handled here?

The scroll gesture controller synthesises a null axis event on release - I guess we could give it any type we liked, but null made sense to me.

> >>> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:61
> >>> +            m_axisEvent.x_axis = -(m_offset.x - touchPoint->x);
> >> 
> >> This might explain why touch scrolling was broken with bug #208638
> > 
> > Yes, this is the cause after applying the above fix - and judging on other similar event handling code, I believe this is the right fix for it.
> 
> This breaks horizontal touch scrolling.

Seems fine with the corresponding changes in handleWheelEvent to the scrollBy calls. I certainly don't notice anything wrong when testing it.

> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:178
> > -                    struct wpe_input_axis_event* axisEvent = scrollGestureController.axisEvent();
> > -                    if (axisEvent->type != wpe_input_axis_event_type_null)
> > -                        page.handleWheelEvent(WebKit::NativeWebWheelEvent(axisEvent, page.deviceScaleFactor()));
> > +                    page.handleWheelEvent(WebKit::NativeWebWheelEvent(scrollGestureController.axisEvent(), page.deviceScaleFactor(), scrollGestureController.phase(), WebWheelEvent::Phase::PhaseNone));
> 
> Why are null events being handled?

(see above) - in short, we need to handle release events. I can change this to be axis events instead of null.

Comment on attachment 393862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393862&action=review

>>> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:182
>>> +    }
>> 
>> From where are touch events coming into `createWebWheelEvent()`? Why are null events expected to be handled here?
> 
> The scroll gesture controller synthesises a null axis event on release - I guess we could give it any type we liked, but null made sense to me.

If needed, null is fine, but the test should be using wpe_input_axis_event_type_null, not the touch equivalent (even if it has the same 0 value).

Created attachment 394478 [details]
Patch

Comment on attachment 394478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394478&action=review

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:146
> +    auto first = m_scrollHistory[0].timestamp();
> +    auto last = m_scrollHistory.rbegin()->timestamp();

These Vector lookups should use first() and last().

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:149
> +    if (last == first)
> +        return { };

Furthermore, the `last - first` difference can be computed early, tested here to be above-zero, and then used in the return statement.

> Source/WebCore/platform/ScrollAnimationKinetic.cpp:157
> +    return FloatPoint(accumDelta.x() * -1 / (last - first).value(), accumDelta.y() * -1 / (last - first).value());

The -1 can be incorporated as a sign into the accumDelta value lookup.

Given this is all existing code being moved around, you could take this polishing up in a different patch.

> Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:92
>  #if WPE_CHECK_VERSION(1, 5, 0)
>              m_axisEvent.base = {
> -                wpe_input_axis_event_type_null,
> -                0, 0, 0, 0, 0, 0
> +                m_axisEvent.base.type,
> +                touchPoint->time, m_start.x, m_start.y,
> +                0, 0, 0
>              };
>              m_axisEvent.x_axis = m_axisEvent.y_axis = 0;
>  #else
>              m_axisEvent = {
> -                wpe_input_axis_event_type_null,
> -                0, 0, 0, 0, 0, 0
> +                wpe_input_axis_event_type_motion,
> +                touchPoint->time, m_start.x, m_start.y,
> +                0, 0, 0
>              };
>  #endif

These two changes don't seem aligned. The first one is maintaining the event type, the second one is overriding it.

(In reply to Zan Dobersek from comment #10)
> Comment on attachment 394478 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394478&action=review
> 
> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:146
> > +    auto first = m_scrollHistory[0].timestamp();
> > +    auto last = m_scrollHistory.rbegin()->timestamp();
> 
> These Vector lookups should use first() and last().
> 
> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:149
> > +    if (last == first)
> > +        return { };
> 
> Furthermore, the `last - first` difference can be computed early, tested
> here to be above-zero, and then used in the return statement.
> 
> > Source/WebCore/platform/ScrollAnimationKinetic.cpp:157
> > +    return FloatPoint(accumDelta.x() * -1 / (last - first).value(), accumDelta.y() * -1 / (last - first).value());
> 
> The -1 can be incorporated as a sign into the accumDelta value lookup.
> 
> Given this is all existing code being moved around, you could take this
> polishing up in a different patch.

Yes, a separate patch I think - I need to look at Gtk, if this is essentially copy-pasted from there, I'd like to avoid changing it in any way that makes it read differently (I expect the first/last bits are fine though).

> > Source/WebKit/UIProcess/API/wpe/ScrollGestureController.cpp:92
> >  #if WPE_CHECK_VERSION(1, 5, 0)
> >              m_axisEvent.base = {
> > -                wpe_input_axis_event_type_null,
> > -                0, 0, 0, 0, 0, 0
> > +                m_axisEvent.base.type,
> > +                touchPoint->time, m_start.x, m_start.y,
> > +                0, 0, 0
> >              };
> >              m_axisEvent.x_axis = m_axisEvent.y_axis = 0;
> >  #else
> >              m_axisEvent = {
> > -                wpe_input_axis_event_type_null,
> > -                0, 0, 0, 0, 0, 0
> > +                wpe_input_axis_event_type_motion,
> > +                touchPoint->time, m_start.x, m_start.y,
> > +                0, 0, 0
> >              };
> >  #endif
> 
> These two changes don't seem aligned. The first one is maintaining the event
> type, the second one is overriding it.

In the second case, the type can only ever be type_motion, where as there are two types in >=1.5 - you're right that this reads confusingly though, so I'll just do the same in both blocks.

Created attachment 394712 [details]
Patch

Comment on attachment 394712 [details]
Patch

Already reviewed.

Committed r259112: <https://trac.webkit.org/changeset/259112>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394712 [details].

<rdar://problem/60964765>