Bug 143383 – Minor cleanups to various scrolling classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Brent Fulgham]

This is an initial set of changes that clean up a few things I noticed while extending testing support for scroll animations and wheel event gestures. (Bug 143286)
    1. Reduce the amount of #ifdef code in EventHandler{Mac}.
    2. Consolidate the idea of an "End Gesture" in the PlatformWheelEvent class.
    3. Remove a number of unneeded null checks in EventHandler.
    4. ScrollController must always have a client, so hold a reference instead of using a pointer.

[Brent Fulgham]

Created attachment 250092 [details]
Patch

[Darin Adler]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Committed r182334: <http://trac.webkit.org/changeset/182334>