Bug 39725

Created attachment 57296 [details] patch v0

Attachment 57296 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKit/chromium/src/WebViewImpl.cpp:1670: Tab found; better to use spaces [whitespace/tab] [1] WebKit/chromium/src/WebViewImpl.cpp:1681: Missing spaces around / [whitespace/operators] [3] WebKit/chromium/src/WebViewImpl.cpp:1690: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 57298 [details] patch v1 - fix style violation

Comment on attachment 57298 [details] patch v1 - fix style violation WebCore/platform/graphics/IntRect.cpp:57 + IntSize IntRect::directionTo(const WebCore::IntPoint& target) const The name directionTo seems to be confusing. Will it be better to distanceTo? Also better to comment what distance to rectangle is meant. In addition, I am thinking if this routine is only needed in chromium platform, probably we do not want to add it to IntRect.*. WebCore/platform/graphics/IntRect.h:119 + IntSize directionTo(const WebCore::IntPoint& target) const; The argument name 'target' can be omitted. WebKit/chromium/public/WebView.h:172 + // Notified the WebView that a dragging is going on. Notified => Notifies WebKit/chromium/public/WebView.h:173 + virtual void dragSourceMovedTo( No need to fold into multiple lines. WebKit/chromium/src/WebViewImpl.cpp:1550 + void WebViewImpl::dragSourceMovedTo( ditto. WebKit/chromium/src/WebViewImpl.cpp:1678 + WebCore::FrameView* frameView = mainFrameImpl()->frameView(); Do we need to check if frameView is NULL? WebKit/chromium/src/WebViewImpl.cpp:1684 + bounds.setHeight(bounds.height() - margin*2); Should have one space before and after '*'. WebKit/chromium/src/WebViewImpl.cpp:1686 + bounds.setWidth(bounds.width() - margin*2); ditto. WebKit/chromium/src/WebViewImpl.h:136 + virtual void dragSourceMovedTo( ditto. WebKit/chromium/src/WebViewImpl.h:331 + static const int kScrollMarginRatio = 20; This const definition should be moved to WebViewImpl::scrollForDragging. Also please comment why choosing value 20? WebKit/chromium/src/WebViewImpl.h:353 + void scrollForDragging(const WebPoint& clientPoint); The argument name can be omitted.

Created attachment 57634 [details] patch v2

Hi Jian, thank you for reviewing! I updated the patch. (In reply to comment #4) > (From update of attachment 57298 [details]) > WebCore/platform/graphics/IntRect.cpp:57 > + IntSize IntRect::directionTo(const WebCore::IntPoint& target) const > The name directionTo seems to be confusing. Will it be better to distanceTo? Also better to comment what distance to rectangle is meant. > In addition, I am thinking if this routine is only needed in chromium platform, probably we do not want to add it to IntRect.*. Renamed, and moved it to WebViewImpl.cpp local > > WebCore/platform/graphics/IntRect.h:119 > + IntSize directionTo(const WebCore::IntPoint& target) const; > The argument name 'target' can be omitted. Done. > > WebKit/chromium/public/WebView.h:172 > + // Notified the WebView that a dragging is going on. > Notified => Notifies Done. > > WebKit/chromium/public/WebView.h:173 > + virtual void dragSourceMovedTo( > No need to fold into multiple lines. Done. > > WebKit/chromium/src/WebViewImpl.cpp:1550 > + void WebViewImpl::dragSourceMovedTo( > ditto. Done. > > WebKit/chromium/src/WebViewImpl.cpp:1678 > + WebCore::FrameView* frameView = mainFrameImpl()->frameView(); > Do we need to check if frameView is NULL? Guarded null case. > > WebKit/chromium/src/WebViewImpl.cpp:1684 > + bounds.setHeight(bounds.height() - margin*2); > Should have one space before and after '*'. Done. > > WebKit/chromium/src/WebViewImpl.cpp:1686 > + bounds.setWidth(bounds.width() - margin*2); > ditto. Done. > > WebKit/chromium/src/WebViewImpl.h:136 > + virtual void dragSourceMovedTo( > ditto. Done. > > WebKit/chromium/src/WebViewImpl.h:331 > + static const int kScrollMarginRatio = 20; > This const definition should be moved to WebViewImpl::scrollForDragging. Also please comment why choosing value 20? Moved to function static. The value has left under experience. It was bad thiing... I changed the value based on the observation on Safari behavior. > > WebKit/chromium/src/WebViewImpl.h:353 > + void scrollForDragging(const WebPoint& clientPoint); > The argument name can be omitted. Done.

Comment on attachment 57634 [details] patch v2 WebKit/chromium/public/WebView.h:173 + virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation) = 0; nit: please use the same formatting as dragSourceEndedAt WebKit/chromium/src/WebViewImpl.cpp:1550 + void WebViewImpl::dragSourceMovedTo( nit: this method impl should be placed after dragSourceEndedAt to match the declaration order. WebKit/chromium/src/WebViewImpl.cpp:1557 + nit: only one new line between method impls. WebKit/chromium/src/WebViewImpl.cpp:1676 + static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect) nit: no need for WebCore:: prefix since there is a using namespace WebCore at the top of this file. also, please move static helper functions up to the top of the file after the static data declarations. WebKit/chromium/src/WebViewImpl.cpp:1676 + static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect) nit: how about naming this distanceToRect? how about this as a comment: // Computes the distance from a point outside a rect to the nearest edge of the rect. (I would also assert that the point is not contained within the rect if that is indeed appropriate.) WebKit/chromium/src/WebViewImpl.cpp:1695 + WebCore::FrameView* view = mainFrameImpl()->frameView(); nit: no WebCore:: prefix needed WebKit/chromium/src/WebViewImpl.cpp:1693 + static const int kScrollMargin = 30; nit: constants should not have the k prefix. just do scrollMargin. WebKit/chromium/src/WebViewImpl.h:136 + virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation); please list this method in the same order that it is declared in WebView.h WebKit/chromium/src/WebViewImpl.cpp:1690 + void WebViewImpl::scrollForDragging(const WebPoint& clientPoint) this method should be defined just above hideSelectPopup() so that the order of methods in the .cpp file matches the order of the methods in the .h file.

Created attachment 57722 [details] patch v3

Comment on attachment 57722 [details] patch v3 Oops, I missed to take some feedbacks, canceling v3.

Created attachment 57729 [details] patch v4

Hi Darin, thank you for revewing! (In reply to comment #7) > (From update of attachment 57634 [details]) > WebKit/chromium/public/WebView.h:173 > + virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation) = 0; > nit: please use the same formatting as dragSourceEndedAt Done. > > WebKit/chromium/src/WebViewImpl.cpp:1550 > + void WebViewImpl::dragSourceMovedTo( > nit: this method impl should be placed after dragSourceEndedAt > to match the declaration order. Done. > > WebKit/chromium/src/WebViewImpl.cpp:1557 > + > nit: only one new line between method impls. Done. > > WebKit/chromium/src/WebViewImpl.cpp:1676 > + static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect) > nit: no need for WebCore:: prefix since there is a using namespace WebCore > at the top of this file. also, please move static helper functions up to > the top of the file after the static data declarations. Done. > > WebKit/chromium/src/WebViewImpl.cpp:1676 > + static IntSize distanceTo(const WebCore::IntPoint& point, const WebCore::IntRect& rect) > nit: how about naming this distanceToRect? Renamed. > > how about this as a comment: > // Computes the distance from a point outside a rect to the nearest edge of the rect. > > (I would also assert that the point is not contained within the rect if that > is indeed appropriate.) In some case it goes (0,0) because IntRect::contains() doesn't count its borders as its area. So I added check instead assertion. > > WebKit/chromium/src/WebViewImpl.cpp:1695 > + WebCore::FrameView* view = mainFrameImpl()->frameView(); > nit: no WebCore:: prefix needed Done. > > WebKit/chromium/src/WebViewImpl.cpp:1693 > + static const int kScrollMargin = 30; > nit: constants should not have the k prefix. just do scrollMargin. Done. > > WebKit/chromium/src/WebViewImpl.h:136 > + virtual void dragSourceMovedTo(const WebPoint& clientPoint, const WebPoint& screenPoint, WebDragOperation); > please list this method in the same order that it is declared > in WebView.h Done. > > WebKit/chromium/src/WebViewImpl.cpp:1690 > + void WebViewImpl::scrollForDragging(const WebPoint& clientPoint) > this method should be defined just above hideSelectPopup() so that > the order of methods in the .cpp file matches the order of the > methods in the .h file. Done.

Comment on attachment 57729 [details] patch v4 Looks good except some minor issues. Please land it with these issues addressed. WebKit/chromium/public/WebView.h:172 + // Notifies the WebView that a dragging is going on. dragging => drag WebKit/chromium/src/WebViewImpl.cpp:845 + // This margin approximates Safari behavior Please end the sentence with period. WebKit/chromium/src/WebViewImpl.cpp:828 + // Computes disatances from a point to the nearest point from a rect for both x and y coordinates. I think what Darin suggested for the comment is easy to understand. We have "the distance" represented by a pair of values, not "distances". WebKit/chromium/src/WebViewImpl.cpp:846 + static const int scrollMargin = 30; Please comment why you pick 30. Maybe you could say that this is based on the observation of ...

Committed r60651: <http://trac.webkit.org/changeset/60651>

Hi Jian, thank you for reviewing again! The patch landing with fix you suggested. > WebKit/chromium/public/WebView.h:172 > + // Notifies the WebView that a dragging is going on. > dragging => drag Done. > > WebKit/chromium/src/WebViewImpl.cpp:845 > + // This margin approximates Safari behavior > Please end the sentence with period. Done. > > WebKit/chromium/src/WebViewImpl.cpp:828 > + // Computes disatances from a point to the nearest point from a rect for both x and y coordinates. > I think what Darin suggested for the comment is easy to understand. We have "the distance" represented by a pair of values, not "distances". Done. I'm sorry that I missed this line.... > > WebKit/chromium/src/WebViewImpl.cpp:846 > + static const int scrollMargin = 30; > Please comment why you pick 30. Maybe you could say that this is based on the observation of ... Done.

This does not seem to work. I'm reopening and adding a test case.

Created attachment 59272 [details] Test case

Hi Erik, thank you for trying this! (In reply to comment #15) > This does not seem to work. I'm reopening and adding a test case. It does work for outermost frame(window), but does not work for iframe. Safari behaves similarly. So I filed this new suggestion to as a separate bug on Bug 40981 and close this.

This bug is about Elements with scrollbars. 40981 is about iframes.

> This bug is about Elements with scrollbars. 40981 is about iframes. Oops. So what did happen is I misunderstood the bug and fixed the non-filed bug! Thank you for your correction.

removing [Chromium] from summary line because this problem is not chromium-specific.

Created attachment 60710 [details] patch v0

Attachment 60710 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/graphics/IntRect.h:150: More than one command on the same line [whitespace/newline] [4] WebCore/rendering/RenderLayer.cpp:1458: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/rendering/RenderLayer.cpp:1471: Missing space after , [whitespace/comma] [3] WebCore/rendering/RenderLayer.cpp:1479: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/rendering/RenderLayer.cpp:1491: One line control clauses should not use braces. [whitespace/braces] [4] WebCore/rendering/RenderLayer.cpp:1489: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] WebCore/page/DragController.cpp:74: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 60711 [details] patch v1 fix style violation

Added Darin to CC because git annotate told me to do so.

I’d like to review this patch, but probably won’t get to it soon. Is this always a good idea? Do other browsers do this? Any cases where doing this would be a bad thing?

Hi Darin, thank you for your feedback! > Is this always a good idea? Do other browsers do this? No other browser( firefox, opera, ie) doesn't support autoscroll at all, even for outermost frames. Only safari (and latest chromium) does support it. Drag-drop support on other browsers seems immature compared to WebKit. I think they don't have autoscroll yet because they don't have enough drag-drop support yet. So we could pioneer in this area. Actually, some desktop applications including (non-internet) explorer also support autoscroll by dragging. > Any cases where doing this would be a bad thing? Users could trigger scrolling accidentally if the autoscroll criteria is too sensitive. So this patch uses a timer to delay triggering the autoscroll (as Cocoa's autoscroll does). With such delay, chances for accidental scrolling can be minimized. Just FYI, here is a viewpoint from chromium land: We have the bookmark manager implemented in HTML, that heavily uses drag-and-drop. This feature is originally requested for improving its experience. Regards.

Created attachment 66868 [details] sync to latest

Created attachment 71721 [details] Patch

Could anyone give me a review? This scroll-following-drag is vital for high-density UI with drag operation.

Comment on attachment 71721 [details] Patch Got obsolete, giving up at now.

Created attachment 176933 [details] WIP Patch 2.0

Comment on attachment 176933 [details] WIP Patch 2.0 This one looks much simpler than the last version that I wrote. This is great! One caveat to this patch is that we have too much rendering related logic in EventHandler, which violates layering. It would be better if we move them to somewhere in rendering/

Created attachment 177680 [details] Patch 2.0-1

Comment on attachment 177680 [details] Patch 2.0-1 Could you review this patch? Thanks in advance.

Comment on attachment 177680 [details] Patch 2.0-1 Attachment 177680 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15153327 New failing tests: fast/events/drag-and-drop-dataTransfer-types-nocrash.html fast/events/drag-and-drop-autoscroll.html

Comment on attachment 177680 [details] Patch 2.0-1 Oops, failed on Mac test. Investigating...

Created attachment 177950 [details] Patch 2.0-2

Comment on attachment 177950 [details] Patch 2.0-2 Could you review this patch? Thanks in advance.

Comment on attachment 177950 [details] Patch 2.0-2 View in context: https://bugs.webkit.org/attachment.cgi?id=177950&action=review The approach looks good. Let's take this as an opportunity to cleanup auto-scrolling code. > Source/WebCore/page/EventHandler.cpp:136 > +// Delay time in second for start autoscroll if pointer is in border edge of scrollable element. can be static since it's referred only from this file. > Source/WebCore/page/EventHandler.cpp:137 > +const double autoscrollDelay = 0.2; Ditto. > Source/WebCore/page/EventHandler.cpp:141 > +const int autoscrollBeltSize = 20; same.

Comment on attachment 177950 [details] Patch 2.0-2 View in context: https://bugs.webkit.org/attachment.cgi?id=177950&action=review > Source/WebCore/page/EventHandler.h:408 > + IntPoint m_autoscrollReferencePoint; It's sad to see EventHandler getting fat. It looks we already have bunch of autoscroll related state and we're adding more. Why not factor these out to a class which is responsible for auto scrolling? > Source/WebCore/rendering/RenderObject.h:275 > + virtual bool canAutoscroll() const { return false; } I don't think this need to be virtual. It can be a method of RenderBox. > Source/WebCore/rendering/RenderObject.h:276 > + RenderObject* findAutoscrollableRenderer(); For that, this should return RenderBox. I think this can be a static method of RenderBox so that autoscrolling related code can be stay in the same class.

Created attachment 179448 [details] Patch 3.0

Comment on attachment 179448 [details] Patch 3.0 Could you review this patch? Thanks in advance.

Comment on attachment 179448 [details] Patch 3.0 Could you clarify when an autoscrol of the specific type starts and ends? AutoscrollForDragAndDrop is hidden in ambiguous "update" function. That isn't good. We need start and hopefully end functions.

Created attachment 181638 [details] Patch-3.0.1

Comment on attachment 181638 [details] Patch-3.0.1 Could you review this patch? Thanks in advance. = Changes since the last review = Changes AutoscrollController::updateDragAndDrop to be clear, it does - If new drop target is not autoscrollable, stops autoscroll. - If autoscroll is in progress, update autoscroll renderer and update event start time if needed. - Otherwise, start autoscroll for DnD.

Comment on attachment 181638 [details] Patch-3.0.1 OK.

Comment on attachment 181638 [details] Patch-3.0.1 Clearing flags on attachment: 181638 Committed r139044: <http://trac.webkit.org/changeset/139044>

All reviewed patches have been landed. Closing bug.

The test added by this patch is failing on Mac: http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK2%20(Tests)/r139087%20(4563)/fast/events/drag-and-drop-autoscroll-pretty-diff.html --- /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/events/drag-and-drop-autoscroll-expected.txt +++ /Volumes/Data/slave/mountainlion-release-tests-wk2/build/layout-test-results/fast/events/drag-and-drop-autoscroll-actual.txt @@ -3,6 +3,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS scrollable.scrollTop > 0 +FAIL No autoscroll For manual testing, drag and drop "Drop Me" to "Scrollable" area.

The test is failing on all but chromium port: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fevents%2Fdrag-and-drop-autoscroll.html Is this supposed to work only in chromium? If so, why was the test added to fast/events instead of chromium/fast/events? If not, then I would like to know why you didn't fix non-chromium ports.

If I don't get any response from either one of you in a timely mannar, I'm going to revert this patch.

(In reply to comment #51) > If I don't get any response from either one of you in a timely mannar, I'm going to revert this patch. This doesn't look Chromium specific. Morrita is not in your time zone so please be patient.

This patch started from Chromium-specific approach but changed direction during updates. Anyway, It shoudln't break any tests. I'll rollout this.

Re-opened since this is blocked by bug 106395

(In reply to comment #54) > Re-opened since this is blocked by bug 106395 We can just mark it fail.

This patch is caused unwanted scroll behavior on Mac. There are several issues: 1. If I drag a URL over a page, it will autoscroll when near the top and bottom for no useful reason: there are no drop targets anywhere on the page, even out of view. 2. The page seems to get stuck in "autoscroll" mode even when the drag has finished; scrolling the page manually will cause it to drift in one direction, or fight the user scrolling. I'm going to revert this change.

Re-opened since this is blocked by bug 106702

Steps to reproduce the bug that caused the rollout: 1. Go to a website in a Mac WebKit2 view 2. Drag something into the window (e.g. text from a text editor) 3. Some glitches happens: the content moves up and down real fast 4. Scroll to the middle of the page and release 5. The content slowly scroll back to the top.

Was this rolled out? This causes autoscroll issues in Chromium: https://code.google.com/p/chromium/issues/detail?id=169630 Repro steps there, though it seems that you see them in other ports too.

(In reply to comment #59) > Was this rolled out? This causes autoscroll issues in Chromium: > > https://code.google.com/p/chromium/issues/detail?id=169630 > > Repro steps there, though it seems that you see them in other ports too. Yes, it has been rolled out.

(In reply to comment #56) > This patch is caused unwanted scroll behavior on Mac. There are several issues: > 1. If I drag a URL over a page, it will autoscroll when near the top and bottom for no useful reason: there are no drop targets anywhere on the page, even out of view. This particular bit seems fine to me. I prefer the consistency of always doing the scrolling over only doing so when there are drop targets. Perhaps this is a UI decision different ports will want to handle differently though. I suppose we should check with our UI folks to get an official word on what behavior we'd like. If we need to, we could put it behind a setting with a FIXME to enable it once there's a setting to only scroll when there are drop targets. I wonder if we should also have some heuristic to not scroll when you initially drag in, but only when you move back to the edge of the scrollable region to avoid scrolling if you are aiming for a currently visible drop target. There were clearly other bugs with this patch, so rolling out makes sense.

Oops. I'm sorry to be late here. Thanks for taking care of this. > 1. If I drag a URL over a page, it will autoscroll when near the top and bottom for no useful reason: there are no drop targets anywhere on the page, even out of view. > 2. The page seems to get stuck in "autoscroll" mode even when the drag has finished; scrolling the page manually will cause it to drift in one direction, or fight the user scrolling. At least 2 is obviously a bug and need to be fixed. For 1, it might need some heuristic as Ojan said. It can be platform/client-specific and always return no-scroll by default.

Created attachment 182711 [details] Patch-3.0.2

Comment on attachment 182711 [details] Patch-3.0.2 Could you review this patch? Thanks in advance. This patch fixes non-stopping autoscroll issue. The root cause is forgetting call of stop autscroll timer at cancellation of drag-and-drop, e.g. pressing ESCAPE key, drag off the window from another process's drag source. = Changes since the last review = * Call stopAutoscrollTimer() at EventHandler::clearDragState() which called after drop or cancel of drag-and-drop. * Introduce ChromeClient::shouldAutoscrollForDragAndDrop() for enabling drag-and-drop autoscroll by each port rater than all ports. Note: Chromium port enables. * Layout test drag-and-drop-autosccroll.html checks stopping of autoscroll after drag-and-drop cancellation.

Attachment 182711 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 182711 [details] Patch-3.0.2 Attachment 182711 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15871816

Created attachment 182718 [details] Patch-3.0.3

Comment on attachment 182718 [details] Patch-3.0.3 Could you review this patch? Thanks in advance. * Mac compilation failure was fixed, unused parameter in shouldAutoscrollForDragAndDrop() declaration. * Style failure for VKEY_ESCAPE is a tool issue, https://bugs.webkit.org/show_bug.cgi?id=106881 This patch fixes non-stopping autoscroll issue. The root cause is forgetting call of stop autscroll timer at cancellation of drag-and-drop, e.g. pressing ESCAPE key, drag off the window from another process's drag source. = Changes since the last review = * Call stopAutoscrollTimer() at EventHandler::clearDragState() which called after drop or cancel of drag-and-drop. * Introduce ChromeClient::shouldAutoscrollForDragAndDrop() for enabling drag-and-drop autoscroll by each port rater than all ports. Note: Chromium port enables. * Layout test drag-and-drop-autosccroll.html checks stopping of autoscroll after drag-and-drop cancellation.

Attachment 182718 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 183109 [details] Patch-3.0.4

Comment on attachment 183109 [details] Patch-3.0.4 Could you review this patch? Thanks in advance. = Changes since the last patch = * Add skip entry for drag-and-drop-autoscroll.html into TestExpectations other than Chromium ports. = Changes since the last review = * Mac compilation failure was fixed, unused parameter in shouldAutoscrollForDragAndDrop() declaration. * Style failure for VKEY_ESCAPE is a tool issue, https://bugs.webkit.org/show_bug.cgi?id=106881 This patch fixes non-stopping autoscroll issue. The root cause is forgetting call of stop autscroll timer at cancellation of drag-and-drop, e.g. pressing ESCAPE key, drag off the window from another process's drag source. = Changes since the last review = * Call stopAutoscrollTimer() at EventHandler::clearDragState() which called after drop or cancel of drag-and-drop. * Introduce ChromeClient::shouldAutoscrollForDragAndDrop() for enabling drag-and-drop autoscroll by each port rater than all ports. Note: Chromium port enables. * Layout test drag-and-drop-autosccroll.html checks stopping of autoscroll after drag-and-drop cancellation.

Attachment 183109 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 183400 [details] Patch 3.0.5

Comment on attachment 183400 [details] Patch 3.0.5 Could you review this patch? Thanks in advance. This is rebase version of previous patch.

Attachment 183400 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Tools/DumpRenderTree/chromium/TestRunner/src/KeyCodeMapping.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 183400 [details] Patch 3.0.5 Let's try this again.

Comment on attachment 183400 [details] Patch 3.0.5 Clearing flags on attachment: 183400 Committed r140286: <http://trac.webkit.org/changeset/140286>

All reviewed patches have been landed. Closing bug.