[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
bugzilla-daemon at webkit.org
Wed Jul 30 19:59:31 PDT 2014
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
More information about the webkit-reviews
mailing list