[webkit-reviews] review granted: [Bug 135434] Hit-testing broken in WebKit 1 views with AppKit's contentInsets : [Attachment 235784] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 135434: Hit-testing broken in WebKit 1 views with AppKit's contentInsets
https://bugs.webkit.org/show_bug.cgi?id=135434

Attachment 235784: Patch
https://bugs.webkit.org/attachment.cgi?id=235784&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235784&action=review


This makes sense.

> Source/WebCore/ChangeLog:10
> +	   AppKit’s contentInsets are factored into scroll positions and
mouse positions, but 

Some encoding issues in the changelog...could be bugzilla.

> Source/WebCore/page/FrameView.cpp:3943
> +	   rect.moveBy(-scrollPosition() + IntPoint(0, headerHeight() +
topContentInset(ManualOrPlatformContentInset)));

The name convertFromRenderer is quite confusing with the topContentInset.
Shouldn't it be renamed convertFromRendererToContainingView()?

> Source/WebCore/page/FrameView.cpp:3968
> -	   point.moveBy(-scrollPosition() + IntPoint(0, headerHeight() +
topContentInset()));
> +	   point.moveBy(-scrollPosition() + IntPoint(0, headerHeight() +
topContentInset(ManualOrPlatformContentInset)));

ditto.

> Source/WebCore/platform/ScrollView.h:164
> +    // will be returned instead of the value set on Page. In the future, we
could obviously expand this enum to include
> +    // a value for only PlatformContentInset, but that value is not needed
at this time.

You can drop the sentence: "In the future, we could obviously expand this enum
to include"...

> Source/WebCore/platform/ScrollView.h:165
> +    enum TopContentInsetType { ManualContentInset,
ManualOrPlatformContentInset };

"Manual" is a bit generic. What about WebKitContentInset or
InternalContentInset?

I like typed enum better (e.g. TopContentInsetType::Platform), but that's
personal preference.

> Source/WebCore/platform/mac/ScrollViewMac.mm:176
> +    // AppKit has the inset factored into all of its scroll positions. In
WebCore, we use positons that ignore

typo: positons