Bug 143383 - Minor cleanups to various scrolling classes
Summary: Minor cleanups to various scrolling classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-03 12:17 PDT by Brent Fulgham
Modified: 2015-04-03 14:33 PDT (History)
6 users (show)

See Also:


Attachments
Patch (28.04 KB, patch)
2015-04-03 12:25 PDT, Brent Fulgham
darin: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-04-03 12:17:48 PDT
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.
Comment 1 Brent Fulgham 2015-04-03 12:25:54 PDT
Created attachment 250092 [details]
Patch
Comment 2 Darin Adler 2015-04-03 13:30:36 PDT
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 3 Brent Fulgham 2015-04-03 13:51:39 PDT
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 4 Brent Fulgham 2015-04-03 13:57:40 PDT
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.
Comment 5 Brent Fulgham 2015-04-03 14:33:47 PDT
Committed r182334: <http://trac.webkit.org/changeset/182334>