RESOLVED FIXED 40461
[Chromium] Dragging outside the frame immediately causes page to scroll
https://bugs.webkit.org/show_bug.cgi?id=40461
Summary [Chromium] Dragging outside the frame immediately causes page to scroll
Hajime Morrita
Reported 2010-06-10 22:09:27 PDT
Attachments
patch v0 (9.64 KB, patch)
2010-06-10 22:21 PDT, Hajime Morrita
no flags
patch v1 (9.62 KB, patch)
2010-06-16 04:06 PDT, Hajime Morrita
no flags
patch v2 (14.13 KB, patch)
2010-06-17 00:02 PDT, Hajime Morrita
no flags
patch v3 (14.13 KB, patch)
2010-06-17 18:15 PDT, Hajime Morrita
fishd: review+
fishd: commit-queue-
Hajime Morrita
Comment 1 2010-06-10 22:21:07 PDT
Created attachment 58446 [details] patch v0
Tony Chang
Comment 2 2010-06-11 00:10:54 PDT
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
Comment 3 2010-06-16 04:06:14 PDT
Created attachment 58871 [details] patch v1
Hajime Morrita
Comment 4 2010-06-16 04:07:47 PDT
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 5 2010-06-16 11:25:40 PDT
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
Comment 6 2010-06-17 00:02:00 PDT
Created attachment 58965 [details] patch v2
Hajime Morrita
Comment 7 2010-06-17 00:05:13 PDT
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
Comment 8 2010-06-17 00:05:34 PDT
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
Comment 9 2010-06-17 00:22:41 PDT
Hajime Morrita
Comment 10 2010-06-17 18:15:57 PDT
Created attachment 59056 [details] patch v3
WebKit Review Bot
Comment 11 2010-06-17 19:28:58 PDT
Hajime Morrita
Comment 12 2010-06-17 22:09:03 PDT
(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)
Comment 13 2010-06-17 22:14:26 PDT
(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 14 2010-06-18 13:38:05 PDT
Comment on attachment 59056 [details] patch v3 WebKit/chromium/src/DragScrollTimer.h:67 + nit: extra new line
Hajime Morrita
Comment 15 2010-06-21 19:29:25 PDT
Note You need to log in before you can comment on or make changes to this bug.