Summary: | Minor cleanups to various scrolling classes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||
Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cmarcelo, commit-queue, darin, jamesr, luiz, tonikitoo | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Brent Fulgham
2015-04-03 12:17:48 PDT
Created attachment 250092 [details]
Patch
Comment on attachment 250092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250092&action=review > Source/WebCore/page/mac/EventHandlerMac.mm:942 > FrameView* view = m_frame.view(); > + ASSERT(view); I would suggest instead writing: ASSERT(m_frame.view()); FrameView& = *m_frame.view(); > Source/WebCore/page/mac/EventHandlerMac.mm:1001 > + if (ScrollAnimator* scrollAnimator = scrollableArea ? scrollableArea->existingScrollAnimator() : nullptr) > scrollAnimator->processWheelEventForScrollSnap(wheelEvent); I’d be tempted to nest if two levels deep instead of using ? : here. > Source/WebCore/platform/PlatformWheelEvent.h:183 > + if (m_phase == PlatformWheelEventPhaseCancelled || m_phase == PlatformWheelEventPhaseMayBegin || isEndGesture()) > return true; > > return false; Instead of writing: if (x) return true; return false; we can just write return x. > Source/WebCore/platform/PlatformWheelEvent.h:189 > + bool isEndGesture() const > + { > + return m_phase == PlatformWheelEventPhaseNone && m_momentumPhase == PlatformWheelEventPhaseEnded; > + } I think it would be better style to move these function bodies outside the class definition. That way you can get an overview of what functions are supported by looking at the class rather than seeing all the code at once. > Source/WebCore/platform/cocoa/ScrollController.mm:398 > + if (!isRubberBandInProgress()) > + stopSnapRubberbandTimer(); Change log doesn’t mention this change. Is this a bug fix? > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1158 > + if ((wheelEvent.phase() != PlatformWheelEventPhaseEnded) && !wheelEvent.isEndGesture()) > return false; I think you should omit the parentheses around the first half of this expression. Comment on attachment 250092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250092&action=review >> Source/WebCore/page/mac/EventHandlerMac.mm:942 >> + ASSERT(view); > > I would suggest instead writing: > > ASSERT(m_frame.view()); > FrameView& = *m_frame.view(); Will do! >> Source/WebCore/page/mac/EventHandlerMac.mm:1001 >> scrollAnimator->processWheelEventForScrollSnap(wheelEvent); > > I’d be tempted to nest if two levels deep instead of using ? : here. Done. >> Source/WebCore/platform/PlatformWheelEvent.h:183 >> return false; > > Instead of writing: > > if (x) > return true; > return false; > > we can just write return x. That's much cleaner. Done! >> Source/WebCore/platform/PlatformWheelEvent.h:189 >> + } > > I think it would be better style to move these function bodies outside the class definition. That way you can get an overview of what functions are supported by looking at the class rather than seeing all the code at once. Done. >> Source/WebCore/platform/cocoa/ScrollController.mm:398 >> + stopSnapRubberbandTimer(); > > Change log doesn’t mention this change. Is this a bug fix? Yes -- it's a drive-by fix I found while working on the new wheel event/scrolling tests. I'll update the changeling. >> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1158 >> return false; > > I think you should omit the parentheses around the first half of this expression. Done. Comment on attachment 250092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250092&action=review >>> Source/WebCore/page/mac/EventHandlerMac.mm:942 >>> + ASSERT(view); >> >> I would suggest instead writing: >> >> ASSERT(m_frame.view()); >> FrameView& = *m_frame.view(); > > Will do! Oops -- I can't make it a reference, because we sometimes reassign the view variable to the latched view for the gesture. We can't reassign a reference, so that doesn't compile. Committed r182334: <http://trac.webkit.org/changeset/182334> |