[webkit-reviews] review granted: [Bug 215780] Move PDF heads-up display to UI process on macOS : [Attachment 407490] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 215780: Move PDF heads-up display to UI process on macOS
https://bugs.webkit.org/show_bug.cgi?id=215780

Attachment 407490: Patch

https://bugs.webkit.org/attachment.cgi?id=407490&action=review




--- Comment #26 from Tim Horton <thorton at apple.com> ---
Comment on attachment 407490
  --> https://bugs.webkit.org/attachment.cgi?id=407490
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407490&action=review

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1727
> +    auto hud = adoptNS([[WKPDFHUDView alloc] initWithFrame:rect
pluginIdentifier:identifier page:m_page.get()]);

My (maybe) last and most annoying piece of feedback: Maybe we should call it
something other than HUD :)

> Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:165
> +    if (CGRectContainsPoint([self convertRect:CGRectInset([_layer frame],
-16.0, -16.0) toView:nil], NSPointToCGPoint(event.locationInWindow))) {

Where do the 16s come from? Can they be a constant?

> Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:170
> +}

Technically you're missing the classic macOS "button de-highlights if you move
out of it while the mouse is down" behavior, but I think the old implementation
was too?

> Source/WebKit/UIProcess/PDF/WKPDFHUDView.mm:329
> +    // This was introduced in rdar://problem/59118412

The comment /still/ doesn't explain why this isn't using the normal symbol API.