[Webkit-unassigned] [Bug 226986] Refactor MacOS keyboard scrolling and use KeyboardScroll struct

[bugzilla-daemon at webkit.org]

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

--- Comment #24 from Tim Horton <thorton at apple.com> ---
(In reply to Sam Weinig from comment #22)
> Comment on attachment 431997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431997&action=review
> 
> Seems like there is a bunch of code duplication going on. Instead, it would
> be better to make the code useable by both WKKeyboardScrollingAnimator and
> EventHandler, and have those classes called shared functionality.

Ideally, yes. In practice this is a bit annoying, because the input to this function is an event, and I don't think there is any event type that is *commonly* used both in WebCore in the Web Content process and WebKit in the UI process. Ideally we could make the code deal in terms of WebCore events (WebCore::KeyboardEvent), but it's a bit weird to use the DOMmy KeyboardEvent in the UI process, no?

I think it might be better if Dana doesn't try to solve this all the way in the first patch, since it will involve lots of thoughtful refactoring of the iOS code. But there is a good bit of sharing that she /can/ do; e.g. `unitVector` like you said, also the code to fill out a KeyboardScroll instance once you have a ScrollGranularity+ScrollDirection.

> > Source/WebCore/page/KeyboardScroll.h:47
> > +    CGFloat springMass { 1 };
> > +    CGFloat springStiffness { 109 };
> > +    CGFloat springDamping { 20 };
> > +
> > +    CGFloat maximumVelocityMultiplier { 25 };
> > +    CGFloat timeToMaximumVelocity { 1 };
> > +
> > +    CGFloat rubberBandForce { 5000 };
> 
> Do these ever change? Seems weird to have these constants just sitting here
> as default parameters. If we think they might vary, perhaps they should be
> on Settings? If not, perhaps just some constexprs would suffice?

This was my doing; back in the day they used to be adjustable with some UI, and I never constexpr'd them. Dana, you're welcome to do so, though I don't think it's vital for this patch.

> > Source/WebKit/UIProcess/ios/WKKeyboardScrollingAnimator.mm:127
> > +    case ScrollDirection::ScrollUp:
> >          return { 0, -1 };
> > -    case WebKit::ScrollingDirection::Down:
> > +    case ScrollDirection::ScrollDown:
> >          return { 0, 1 };
> > -    case WebKit::ScrollingDirection::Left:
> > +    case ScrollDirection::ScrollLeft:
> >          return { -1, 0 };
> > -    case WebKit::ScrollingDirection::Right:
> > +    case ScrollDirection::ScrollRight:
> 
> This seems like it is a duplicate of the code in WebCore. Why not just use
> that?

Agreed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210623/1793365b/attachment.htm>