Summary: | Handling autoscroll in EventHandler should be re-factor | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||||
Component: | UI Events | Assignee: | yosin | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | dglazkov, eric, gyuyoung.kim, morrita, ojan.autocc, rakuco, roger_fong, webkit-ews, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 104991 | ||||||||||||||||||
Bug Blocks: | 39725 | ||||||||||||||||||
Attachments: |
|
Description
yosin
2012-12-11 21:55:36 PST
Created attachment 179003 [details]
Patch 1
Comment on attachment 179003 [details]
Patch 1
Could you review this patch?
Thanks in advance.
Comment on attachment 179003 [details] Patch 1 Attachment 179003 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15280432 Comment on attachment 179003 [details] Patch 1 Attachment 179003 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15280431 Created attachment 179010 [details]
Patch 2
Comment on attachment 179010 [details]
Patch 2
Could you review this patch?
Thanks in advance.
= Changes from the last patch =
* Fix compilation error on Linux
** Windows compilation was succeeded on Patch 1...
Comment on attachment 179010 [details] Patch 2 Attachment 179010 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15275617 Comment on attachment 179010 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=179010&action=review > Source/WebCore/page/EventHandler.cpp:1057 > void EventHandler::stopAutoscrollTimer(bool rendererIsBeingDestroyed) This is bad. We should make this just a delegation to AutoScrollController then AutoScrollController can notify back to event handler to do cleanup. Hopefully more code could be move to the controller. > Source/WebCore/page/EventHandler.cpp:3940 > + , m_eventHandler(eventHandler) Can we pass this as a function parameter instead of holding this as a member? Cyclic reference is bad design and should be avoided as much as possible. > Source/WebCore/page/EventHandler.cpp:4078 > + bool east = m_panScrollStartPos.x() < (m_eventHandler->m_currentMousePosition.x() - ScrollView::noPanScrollRadius); If we can change to pass EventHandler as a parameter. We even no longer need to pass it: We only need to pass m_currentMousePosition and FrameView. > Source/WebCore/page/EventHandler.h:247 > + enum AutoscrollType { I think this is safe to be WebCore global. name looks unique enough. > Source/WebCore/page/EventHandler.h:255 > + class AutoscrollController { Let's move this out from EventHandler definition. nested class often results maintenance burden. Created attachment 179210 [details]
Patch 3
Created attachment 179211 [details]
Patch 4
Created attachment 179212 [details]
Patch 5
Comment on attachment 179212 [details]
Patch 5
Could you review this patch?
Thanks in advance.
= Changes since the last review =
* Move AutoscrollController class to its own files.
* Remove m_eventHandler from AutoscrollController
** Using m_autoscrollRenderer->frame()->eventHandler() if we needed.
* EventHandler::stopAutoscrollTimer() is just wrapper of AutoscrollController::stopAutoscrollTimer().
Comment on attachment 179212 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=179212&action=review This looks much better! > Source/WebCore/page/EventHandler.h:118 > + AutoscrollController& autoscrollController() { return m_autoscrollController; } Let's kill this. We can just provide didPanScrollStop() and didPanScrollStart() or something to limit the usage. > Source/WebCore/page/EventHandler.h:404 > + AutoscrollController m_autoscrollController; And make this OwnPtr. Then we can hide AutoscrollController.h in EventHandler.cpp. Created attachment 179224 [details]
Patch 6
Comment on attachment 179224 [details]
Patch 6
Could you review this patch?
Thanks in advance.
= Changes since the last review =
* In EventHandler, use OwnPtr for AutoscrollController instead of containment.
* Introduce didPanScrollStart/didPanScrollTop not to exposure AutoscrollControll.
Comment on attachment 179224 [details]
Patch 6
Please wait until bots become green.
Comment on attachment 179224 [details] Patch 6 Clearing flags on attachment: 179224 Committed r137691: <http://trac.webkit.org/changeset/137691> All reviewed patches have been landed. Closing bug. Regression caused: http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r137691%20(30897)/results.html platform/win/fast/events/panScroll-click-hyperlink.html platform/win/fast/events/panScroll-event-fired.html platform/win/fast/events/panScroll-nested-divs.html It says the tests fail because pan scrolling may not be supported on the platform. However, seeing as this is just a refactor, that does not seem to be the case. Reopening bug. (In reply to comment #19) > Regression caused: > > http://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r137691%20(30897)/results.html > > platform/win/fast/events/panScroll-click-hyperlink.html > platform/win/fast/events/panScroll-event-fired.html > platform/win/fast/events/panScroll-nested-divs.html > > It says the tests fail because pan scrolling may not be supported on the platform. > However, seeing as this is just a refactor, that does not seem to be the case. > > Reopening bug. Thanks for the taking care of this. Yes, this patch shouldn't change the behavior. Sorry, rolling out now. Reverted r137691 for reason: panscroll test on AppleWin failed Committed r137703: <http://trac.webkit.org/changeset/137703> Created attachment 179418 [details]
Patch 7
Comment on attachment 179418 [details]
Patch 7
Could you review this patch?
Thanks in advance.
= Changes since the last patch =
* In EventHandler::handleMouseReleaseEvent, calling stopAutoscrollTimer() if autoscroll in progress. Previous patch does't check it.
** So, during pan-scrolling mouse up stops pan-scrolling, then some tests failed, e.g. panscroll-clicking-hyperlink.html
On bug 104991, Yoshin is adding a test for Chromium port to cover this. Comment on attachment 179418 [details] Patch 7 Clearing flags on attachment: 179418 Committed r137726: <http://trac.webkit.org/changeset/137726> All reviewed patches have been landed. Closing bug. |