| Summary: | [chromium] synthesize wheel events for fling on main thread | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Robert Kroeger <rjkroege> | ||||||||||||||
| Component: | UI Events | Assignee: | Robert Kroeger <rjkroege> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | davemoore, jamesr, tkent, webkit.review.bot, wjmaclean | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 81457 | ||||||||||||||||
| Bug Blocks: | 81479 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Robert Kroeger
2012-03-17 14:43:23 PDT
Created attachment 132472 [details]
Patch
Comment on attachment 132472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132472&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:370 > + , m_flingmodifier(0) nit: m_flingModifier > Source/WebKit/chromium/src/WebViewImpl.cpp:617 > + const float tickDivisor = (float)WebCore::WheelEvent::tickMultiplier; nit: webkit doesn't typically use const for locals like this nit: webkit requires static_cast<float>() for stuff like this > Source/WebKit/chromium/src/WebViewImpl.cpp:633 > + m_lastWheelGlobalPosition.x += delta.x(); > + m_lastWheelGlobalPosition.y += delta.y(); is it valid to update these regardless of what the mouseWheel handler did? what if it didn't move the page as far as you thought, or if the page didn't move at all? i think we might have to be a bit more careful about how this value is calculated > Source/WebKit/chromium/src/WebViewImpl.cpp:646 > + FloatPoint fp(event.deltaX, event.deltaY); nit: need a better name for this, or just do the conversion inside the ::create() call > Source/WebKit/chromium/src/WebViewImpl.h:729 > + WebPoint m_lastWheelGlobalPosition; nit: spacing went odd here Created attachment 132496 [details]
Patch
Attachment 132496 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/src/WebViewImpl.cpp..." exit_code: 1
Source/WebKit/chromium/src/WebViewImpl.h:729: Extra space between WebPoint and m_lastWheelGlobalPosition [whitespace/declaration] [3]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 132472 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132472&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:370 >> + , m_flingmodifier(0) > > nit: m_flingModifier done, p3 >> Source/WebKit/chromium/src/WebViewImpl.cpp:617 >> + const float tickDivisor = (float)WebCore::WheelEvent::tickMultiplier; > > nit: webkit doesn't typically use const for locals like this > nit: webkit requires static_cast<float>() for stuff like this nit1: I thought it made the lines below easier to read to do it this way. Please let me know your preference. nit2: done in p3 >> Source/WebKit/chromium/src/WebViewImpl.cpp:633 >> + m_lastWheelGlobalPosition.y += delta.y(); > > is it valid to update these regardless of what the mouseWheel handler did? what if it didn't move the page as far as you thought, or if the page didn't move at all? > > i think we might have to be a bit more careful about how this value is calculated per your observation in the design note, this position can be fixed with respect to the container -- every wheel event has the same position wrt to the window. fixed in p3 >> Source/WebKit/chromium/src/WebViewImpl.cpp:646 >> + FloatPoint fp(event.deltaX, event.deltaY); > > nit: need a better name for this, or just do the conversion inside the ::create() call done, patch3 >> Source/WebKit/chromium/src/WebViewImpl.h:729 >> + WebPoint m_lastWheelGlobalPosition; > > nit: spacing went odd here fixed, p3 Comment on attachment 132496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132496&action=review I don't see any patchset 3 here > Source/WebKit/chromium/src/WebViewImpl.cpp:643 > + m_gestureAnimation = ActivePlatformGestureAnimation::create(TouchFlingPlatformGestureCurve::create(fp), this); shouldn't this be a WheelFlingPlatformGestureCurve? Comment on attachment 132496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132496&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:1330 > + // Create synthetic wheel events as necessary for fling. > + if (m_gestureAnimation) { you need to move this block into WebViewImpl::updateAnimations() or it'll be broken in the threaded compositing path Created attachment 132513 [details]
Patch
(In reply to comment #6) > (From update of attachment 132496 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=132496&action=review > > I don't see any patchset 3 here sorry for the delay in uploading it. I went off and added layout tests. > > > Source/WebKit/chromium/src/WebViewImpl.cpp:643 > > + m_gestureAnimation = ActivePlatformGestureAnimation::create(TouchFlingPlatformGestureCurve::create(fp), this); > > shouldn't this be a WheelFlingPlatformGestureCurve? No. The shape of that curve is even worse. Comment on attachment 132496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132496&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:1330 >> + if (m_gestureAnimation) { > > you need to move this block into WebViewImpl::updateAnimations() or it'll be broken in the threaded compositing path done, p4 Comment on attachment 132513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132513&action=review Please remove the "[not ready for review]" from the bug title. Otherwise, very close but not quite. > Source/WebKit/chromium/src/WebViewImpl.cpp:370 > + , m_flingModifier(0) this member is always 0 as far as I can tell. why even have it? > Source/WebKit/chromium/src/WebViewImpl.cpp:417 > + m_lastWheelPosition = WebPoint(-1, -1); > + m_lastWheelGlobalPosition = WebPoint(-1, -1); WebPoint members will be initialized to (0, 0) - I don't see any value in initializing them again to a different arbitrary value > Source/WebKit/chromium/src/WebViewImpl.cpp:616 > + const float tickDivisor = static_cast<float>(WebCore::WheelEvent::tickMultiplier); do you need an explicit cast here? i don't think we have > Source/WebKit/chromium/src/WebViewImpl.cpp:645 > + // FIXME: put this into a sub-function? you don't need a sub-function for m_gestureAnimation.clear(), i don't think > Source/WebKit/chromium/src/WebViewImpl.cpp:649 > + m_flingModifier = 0; > + m_lastWheelPosition = WebPoint(-1, -1); > + m_lastWheelGlobalPosition = WebPoint(-1, -1); these all useless useless - these values won't be touched if m_gestureAnimation is NULL > Source/WebKit/chromium/src/WebViewImpl.cpp:1326 > + // Create synthetic wheel events as necessary for fling. this has to be in updateAnimations(). this function is not called at all when using the threaded compositor > Source/WebKit/chromium/src/WebViewImpl.cpp:1331 > + scheduleAnimation(); > + else > + m_gestureAnimation.clear(); indentation here is off Created attachment 132516 [details]
Patch
Comment on attachment 132516 [details]
Patch
Most of my comments from the previous patch still apply. Not sure why this is up for review.
Comment on attachment 132513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132513&action=review >> Source/WebKit/chromium/src/WebViewImpl.cpp:370 >> + , m_flingModifier(0) > > this member is always 0 as far as I can tell. why even have it? i forgot to initialize it. fixed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:417 >> + m_lastWheelGlobalPosition = WebPoint(-1, -1); > > WebPoint members will be initialized to (0, 0) - I don't see any value in initializing them again to a different arbitrary value fixed. >> Source/WebKit/chromium/src/WebViewImpl.cpp:645 >> + // FIXME: put this into a sub-function? > > you don't need a sub-function for m_gestureAnimation.clear(), i don't think great. >> Source/WebKit/chromium/src/WebViewImpl.cpp:649 >> + m_lastWheelGlobalPosition = WebPoint(-1, -1); > > these all useless useless - these values won't be touched if m_gestureAnimation is NULL done >> Source/WebKit/chromium/src/WebViewImpl.cpp:1326 >> + // Create synthetic wheel events as necessary for fling. > > this has to be in updateAnimations(). this function is not called at all when using the threaded compositor done. >> Source/WebKit/chromium/src/WebViewImpl.cpp:1331 >> + m_gestureAnimation.clear(); > > indentation here is off done Created attachment 132519 [details]
Patch
Comment on attachment 132519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132519&action=review OK > Source/WebKit/chromium/src/WebViewImpl.cpp:1348 > + else > + m_gestureAnimation.clear(); indentation is still broken here. the "else" should be lined up with the "if" it is the other branch for, not some other if statement. as is, this is very confusing to read Created attachment 132571 [details]
Patch
Comment on attachment 132571 [details] Patch Clearing flags on attachment: 132571 Committed r111182: <http://trac.webkit.org/changeset/111182> All reviewed patches have been landed. Closing bug. |