[webkit-reviews] review denied: [Bug 24733] Video controller doesn't respond to clicks after full-page-zoom : [Attachment 28818] Patch, changelog

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 24733: Video controller doesn't respond to clicks after full-page-zoom
https://bugs.webkit.org/show_bug.cgi?id=24733

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

------- Additional Comments from Darin Adler <darin at apple.com>
Great solution!

I’m not a big fan of the word “actual” here in the name of the new function,
but I don’t have a better suggestion. I also don’t think “page” was great
naming either, but that came straight from the DOM specification. The idea here
is that it’s the point in the document’s view’s coordinate system, I think.
Maybe that could lead to a better name?

I think Node::dispatchWheelEvent needs the code to handle this properly for
wheel events. I don’t see anything in there dealing with zoom factor though, so
I’m concerned it might already be wrong.

I think the code in MouseEvent::computePageLocation should be in
MouseRelatedEvent, not MouseEvent.

> +    Frame* frame = view()->frame();
> +    if (frame) {
> +	   float zoomFactor = frame->pageZoomFactor();
> +	   setActualPageLocation(roundedIntPoint(FloatPoint(pageX() *
zoomFactor, pageY() * zoomFactor)));
> +    }

Is it really OK to leave the page location as 0,0 if there’s no frame? Maybe it
should instead act as if the zoom factor is 1 in that case instead of leaving
it 0,0. I also think initCoordinates should set it to the same value it sets
m_pageX and m_pageY to.

> +	   // Page point in pixels, independent of zoom factor. c.f. pageX,
pageY.

I don’t think “in pixels” is helpful in this comment.

> +    int adjustedPageX = pageX, adjustedPageY = pageY;

We normally don’t define two variables on a single line, even in a simple case
like this one.

> +	   if (evt->isMouseEvent() && evt->type() ==
eventNames().mousedownEvent && static_cast<MouseEvent*>(evt)->button() ==
LeftButton)
> +	       slider->handleLeftMouseDownEvent(static_cast<MouseEvent*>(evt));


When I was doing some cleanup of slider in my own patch, one I probably won’t
land at this time, I moved this code into RenderSlider’s forwardEvent function,
because I think this qualifies as forwarding just as much as the mechanical
forwarding. You should consider doing that rather than adding a new function.

> -    IntPoint point = IntPoint(mouseEvent->pageX(), mouseEvent->pageY());
> -    HitTestResult result(point);
> +    HitTestResult result(mouseEvent->actualPageLocation());
>  
> -    if (Frame* frame = event->target()->toNode()->document()->frame()) {
> -	   float zoomFactor = frame->pageZoomFactor();
> -	   point.setX(static_cast<int>(point.x() * zoomFactor));
> -	   point.setY(static_cast<int>(point.y() * zoomFactor));
> -	   result = frame->eventHandler()->hitTestResultAtPoint(point, false);
> -    }
> +    if (Frame* frame = event->target()->toNode()->document()->frame())
> +	   result =
frame->eventHandler()->hitTestResultAtPoint(mouseEvent->actualPageLocation(),
false);

I think this is going to have an unwanted behavior change in the frame == 0
case so you should consider my suggestion above.

There are enough things that could be improved here that I’ll say review- for
now.