[Webkit-unassigned] [Bug 135434] Hit-testing broken in WebKit 1 views with AppKit's contentInsets

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135434


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #235784|review?                     |review+
               Flag|                            |




--- Comment #5 from Benjamin Poulain <benjamin at webkit.org>  2014-07-30 19:59:44 PST ---
(From update of attachment 235784)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.