[Chromium] Dragging outside the frame immediately causes page to scroll

[Hajime Morrita]

Originated from:
 * http://code.google.com/p/chromium/issues/detail?id=45900

[Hajime Morrita]

Created attachment 58446 [details]
patch v0

[Tony Chang]

Comment on attachment 58446 [details]
patch v0

Looks OK to me, but I am not a reviewer.  I guess it's hard to write a layout test for this since it has a timer.
---------------------------------
http://wkrietveld.appspot.com/40461/diff/1/3
File WebKit/chromium/src/WebViewImpl.cpp (right):

http://wkrietveld.appspot.com/40461/diff/1/3#newcode236
WebKit/chromium/src/WebViewImpl.cpp:236: if (shouldScroll() && !m_timer.isActive())
Nit: Just make this an else if?

[Hajime Morrita]

Created attachment 58871 [details]
patch v1

[Hajime Morrita]

Hi Tony, thank you for reviewing!
> Looks OK to me, but I am not a reviewer.  I guess it's hard to write a layout test for this since it has a timer.
Ya, timer and drag-n-drop make automated tests harder...

> http://wkrietveld.appspot.com/40461/diff/1/3#newcode236
> WebKit/chromium/src/WebViewImpl.cpp:236: if (shouldScroll() && !m_timer.isActive())
> Nit: Just make this an else if?
Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 58871 [details]
patch v1

WebKit/chromium/src/WebViewImpl.cpp:167
 +  class WebDragScrollTimer {
this should get its own .h,.cpp files.  it should not be prefixed
with Web since it is not part of the WebKit API.  DragScrollTimer
should be fine.

[Hajime Morrita]

Created attachment 58965 [details]
patch v2

[Hajime Morrita]

Hi Darin, thank you for quick response!

(In reply to comment #5)
> (From update of attachment 58871 [details])
> WebKit/chromium/src/WebViewImpl.cpp:167
>  +  class WebDragScrollTimer {
> this should get its own .h,.cpp files.  it should not be prefixed
> with Web since it is not part of the WebKit API.  DragScrollTimer
> should be fine.
Fixed to move to DragScrollTimer to separate source files.

[WebKit Review Bot]

Attachment 58965 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/DragScrollTimer.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 58965 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3314273

[Hajime Morrita]

Created attachment 59056 [details]
patch v3

[WebKit Review Bot]

Attachment 59056 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3330281

[Hajime Morrita]

(In reply to comment #11)
> Attachment 59056 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3330281

Hmm. buildbot cannot detect files addition...

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> (In reply to comment #11)
> > Attachment 59056 [details] [details] did not build on chromium:
> > Build output: http://webkit-commit-queue.appspot.com/results/3330281
> 
> Hmm. buildbot cannot detect files addition...

Nope. We should fix that :)

[Darin Fisher (:fishd, Google)]

Comment on attachment 59056 [details]
patch v3

WebKit/chromium/src/DragScrollTimer.h:67
 +  
nit: extra new line

[Hajime Morrita]

Committed r61598: <http://trac.webkit.org/changeset/61598>