[webkit-reviews] review denied: [Bug 22119] [Transforms] clicks in scrollbars of transformed element don't work (e.g. listbox or overflow) : [Attachment 28676] New patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 22119: [Transforms] clicks in scrollbars of transformed element don't work
(e.g. listbox or overflow)
https://bugs.webkit.org/show_bug.cgi?id=22119

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
My high level commentary here is that new methods have been added that
essentially duplicate functionality of Widget and ScrollView.  The reasoning
being that the Widget/ScrollView methods aren't transform-aware.  I think a
better approach would be to make them transform-aware.	Take for example:

IntPoint Widget::convertToContainingWindow(const IntPoint& point) const
{
    IntPoint windowPoint = point;
    const Widget* childWidget = this;
    for (const ScrollView* parentScrollView = parent();
	 parentScrollView;
	 childWidget = parentScrollView, parentScrollView =
parentScrollView->parent())
	windowPoint = parentScrollView->convertChildToSelf(childWidget,
windowPoint);
    return windowPoint;
}

IntPoint Widget::convertFromContainingWindow(const IntPoint& point) const
{
    IntPoint widgetPoint = point;
    const Widget* childWidget = this;
    for (const ScrollView* parentScrollView = parent();
	 parentScrollView;
	 childWidget = parentScrollView, parentScrollView =
parentScrollView->parent())
	widgetPoint = parentScrollView->convertSelfToChild(childWidget,
widgetPoint);
    return widgetPoint;
}

There is nothing wrong with those methods if you imagine that
convertChildToSelf and convertSelfToChild are transform-aware.	In other words,
try to find the "leaf" functions used by all of these Widget/ScrollView methods
and make them virtual.	Subclass them on FrameView to do the right thing.

It is likely that you may need a "usesTransforms" boolean on FrameView, kind of
like we have slow repaints on ScrollView.  If that flag isn't set then you know
you could fall through to the much faster ScrollView functions and not have to
crawl up your render tree.  That is an optimization that could be done in a
separate patch though I think.

Now there are still some good things here though.  For example, it is good to
just send local points to Scrollbars always.  There's no reason to be sending
them window coordinates when you already hit tested all the way to the
scrollbar and so should know the local coordinate anyway.  So eliminating
window to/from conversions from scrollbar code is good.