Bug 180789

Created attachment 329517 [details] Patch

<rdar://problem/19005092>

Created attachment 329527 [details] Patch

Comment on attachment 329527 [details] 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.html: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.html:30 > + setTimeout(function(){ // wait a spell while the keyboard comes up We have functions to detect when the keyboard animation is finished.

Created attachment 329542 [details] Patch

Comment on attachment 329542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329542&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:690 > +// @protocol UITextAutoscrolling Nit - we should usually indicate this using `#pragma mark - UITextAutoscrolling` > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:703 > + // used to scroll selection on keyboard up; we already scroll to visible Nit - inline comments like these should begin with a capital letter and end with a period. > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:22 > + var tapPointX = targetRect.x+targetRect.width / 2; Nit - spaces around the + > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:23 > + var tapPointY = targetRect.y+targetRect.height / 2; Ditto. > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:34 > + testRunner.runUIScript(continueTouchAndDragFromPointToPoint(dragX, tapPointY, dragX+5, tapPointY), function() { Ditto (spaces around +) > LayoutTests/fast/events/touch/ios/drag-to-autoscroll-in-single-line-editable.html:35 > + if (scrollBox.scrollLeft > 0 ) Nit - extra space after the 0. > LayoutTests/fast/events/touch/ios/resources/basic-gestures.js:173 > + uiController.uiScriptComplete(); Nit - looks like there's an extra indent here?

Comment on attachment 329542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329542&action=review > Source/WebCore/page/EventHandler.h:339 > + WEBCORE_EXPORT void startTextAutoscroll(RenderObject* renderer, const FloatPoint& position); position -> positionInWindow > Source/WebCore/page/EventHandler.h:342 > + bool m_isAutoscrolling { false }; My comment about not needing this and using the timer state still stands. > Source/WebCore/page/ios/EventHandlerIOS.mm:563 > +void EventHandler::startTextAutoscroll(RenderObject* renderer, const FloatPoint& position) position -> positionInWindow > Source/WebKit/UIProcess/WebPageProxy.h:573 > + void startAutoscrollAtPosition(const WebCore::FloatPoint&); Name the parameter positionInWindow > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:721 > +void WebPageProxy::startAutoscrollAtPosition(const WebCore::FloatPoint& position) Name the parameter positionInWindow > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:102 > + StartAutoscrollAtPosition(WebCore::FloatPoint position) Name the parameter positionInWindow > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1386 > +void WebPage::startAutoscrollAtPosition(const WebCore::FloatPoint& position) Name the parameter positionInWindow

The m_isAutoscrolling is used in a call that the timer uses to determine if it should still be updating, so using the timer state instead would be problematic.

<rdar://problem/36112755>

*** Bug 148061 has been marked as a duplicate of this bug. ***

Comment on attachment 329542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329542&action=review >> Source/WebCore/page/EventHandler.h:342 >> + bool m_isAutoscrolling { false }; > > My comment about not needing this and using the timer state still stands. ... also, these are in the public section :) they should be down with the other members