[webkit-reviews] review granted: [Bug 75904] WebKit 1: Scrollbar uiStateTransitionProgress requires tracking the mouse all the time : [Attachment 122104] New patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 75904: WebKit 1: Scrollbar uiStateTransitionProgress requires tracking the
mouse all the time
https://bugs.webkit.org/show_bug.cgi?id=75904

Attachment 122104: New patch
https://bugs.webkit.org/attachment.cgi?id=122104&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=122104&action=review


r=me, but I think there are some things that need to be improved after
check-in, if not before

> Source/WebCore/page/EventHandler.cpp:1585
> +    HitTestResult hoveredNode = HitTestResult(LayoutPoint());

There’s no reason to include the part after the equal sign. The default
constructor does the same thing.

> Source/WebCore/page/EventHandler.cpp:1586
> +    return handleMouseMoveEvent(event, &hoveredNode, true);

A boolean is not good when you are passing a constant value. This kind of
situation leads us to use enums or refactor so function can be separate.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1211
> +	       if (performFullHitTest)
> +		   return
frame->eventHandler()->mouseMoved(platformMouseEvent);
> +	       else
> +		   return
frame->eventHandler()->passMouseMovedEventToScrollbars(platformMouseEvent);

We normally don’t do else after return.

The name “perform full hit test” doesn’t seem to express the entire difference
here between “pass event to everything” and “pass event to scrollbars”.

> Source/WebKit/mac/WebView/WebHTMLView.mm:1553
> +static bool mouseButtonIsPressedForEvent(NSEvent *event)

I think this is not quite the right name. After all, the mouse button is not
pressed for a mouse up event. It’s more that this event is “part of a click or
drag”. Sorry, I don’t have a great name to suggest.

> Source/WebKit/mac/WebView/WebHTMLView.mm:1617
> +	       // If it is one of those cases where the page is not active and
the mouse is not pressed, then we can
> +	       // fire a more efficient scrollbars-only version of the event.

The issue is not just one of efficiency, so I think the comment is misleading
in this regard.

There are things we do not want to do when the mouse passes over the window and
it’s not frontmost. For example, we don’t want to dispatch DOM events.

> Source/WebKit/mac/WebView/WebHTMLView.mm:3415
> +	   // Legacy style scrollbars have design details that rely on tracking
the mouse all the time.
> +	   // It's easiest to do this with a tracking area, which we will
remove when the window is key
> +	   // again.
> +	   _private->trackingAreaForNonKeyWindow = [[NSTrackingArea alloc]
initWithRect:[self frame]
> +								      
options:NSTrackingMouseMoved | NSTrackingMouseEnteredAndExited |
NSTrackingInVisibleRect | NSTrackingActiveAlways
> +									
owner:self
> +								     
userInfo:nil];

I believe [self frame] is wrong for the rectangle. I think we want [self
bounds], based on the documentation in NSTrackingArea. The code says that the
rectangle is in the view’s coordinate system, and bounds is the rectangle of a
view in its own coordinate system.

Better not to try to line the colons up. I am guessing that Xcode tried to do
this automatically.

Why add the tracking area only when non-key, and keep creating and destroying
it each time? Does the tracking area do any harm when the window is key?