[webkit-reviews] review denied: [Bug 180789] Support Autoscrolling in contenteditable for WK2 : [Attachment 329527] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 180789: Support Autoscrolling in contenteditable for WK2
https://bugs.webkit.org/show_bug.cgi?id=180789

Attachment 329527: Patch

https://bugs.webkit.org/attachment.cgi?id=329527&action=review




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 329527
  --> https://bugs.webkit.org/attachment.cgi?id=329527
Patch

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

Looks good but r- for cleanup and test improvement.

> Source/WebCore/page/EventHandler.h:341
> +    IntPoint m_targetAutoscrollPosition;

In what coordinate system relative to which element?

> Source/WebCore/page/EventHandler.h:342
> +    bool m_isAutoscrollActive { false };

"active" is ambiguous; it could be "is able to autoscroll" or "is autoscrolling
now". I would call this m_isAutoscrolling.

Or it seems to correspond to the state of the autoscroll timer. Maybe you don't
need this.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:695
> +- (void)cancelAutoscroll

blank line between methods please.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1389
> +	  
m_page->mainFrame().eventHandler().startTextAutoscroll(m_assistedNode->renderer
(), position);

What if m_assistedNode->renderer() is null?

>
LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.ht
ml:23
> +	       var tapPointX = targetRect.x+targetRect.width / 2;
> +	       var tapPointY = targetRect.y+targetRect.height / 2;

Spaces around +

>
LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.ht
ml:30
> +		       setTimeout(function(){ // wait a spell while the
keyboard comes up

We have functions to detect when the keyboard animation is finished.