[webkit-reviews] review granted: [Bug 52464] Remove event forwarding logic from input[type=range], simplify event flow and thumb positioning logic. : [Attachment 79287] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 52464: Remove event forwarding logic from input[type=range], simplify event
flow and thumb positioning logic.
https://bugs.webkit.org/show_bug.cgi?id=52464

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79287&action=review

> Source/WebCore/html/shadow/SliderThumbElement.cpp:111
> +    ASSERT(input);
> +    ASSERT(input->renderer());
> +    ASSERT(renderer());
> +
> +    if (!input->renderer() || !renderer())
> +	   return;

A couple minor thoughts on this. It’s a little strange to assert three things
but only check two at runtime.

Checking for a 0 renderer often indicates that a function is suitable for
calling only when layout and style calculation is up to date. That makes such
functions dangerous. They can’t be called, for example, from JavaScript, which
can’t guarantee layout and style is up to date. I think the event handlers that
call this function may need to trigger layout and style calculation explicitly
unless there’s some guarantee these have already occurred. Otherwise there can
be bugs where a script changes the state of a document and the dispatches an
event immediately when the renderer is in a bad state.

Also, this function later assumes both renderers are of class RenderBox, but it
does not assert that nor check it at runtime outside an assertion. Is there a
guarantee that we can’t get a different renderer class? If we did, renderBox()
would return 0.

I think it would be better to put the two renderers into local variables and
use those rather than repeatedly refetching them. In fact, each call to
renderBox() redoes the runtime type check!

It’s possible none of this applies because the element is hidden from script
inside a shadow tree. Not sure.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:122
> +	   position = offset.y() - renderBox()->height() / 2;

I think the rounding here rounds in the vertical down direction and we normally
round in the vertical up direction.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:126
> +	   position = offset.x() - renderBox()->width() / 2;

I think the rounding here rounds to the right and we normally round to the
left.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:140
> +    // FIXME: This is no longer being set from renderer. Consider updating
the
> +    // method name.

I’d do this comment on a single line.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:151
> +    if (Frame* frame = document()->frame()) {
> +	   frame->eventHandler()->setCapturingMouseEventsNode(this);
> +	   m_inDragMode = true;
> +    }

What if some code moves the thumb element between documents while it’s being
dragged? Will everything turn out OK?

> Source/WebCore/html/shadow/SliderThumbElement.cpp:163
> +    if (renderer())
> +	   renderer()->setNeedsLayout(true);

Why? This code needs a why comment. What change causes the need for a layout?
The m_inDragMode change? If so, it’s surprising that there is no need to do
this in startDragging.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:185
> +	   // FIXME: Ideally, we should setDefaultHandled here, but we need to
fall
> +	   // through to keep MediaInputControlElement bits working. Add it
once
> +	   // MediaElement is refactored to use shadow DOM.

But only if we are dragging. I don’t think we’d need that if the element got a
mouse up that did not trigger dragging. Or maybe there is no such thing.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:189
> +	   dragTo(mouseEvent->absoluteLocation());
> +	   event->setDefaultHandled();

It seems wrong to set default handled on the mouse move event when we are just
getting events about the mouse passing over the element and not during
dragging.

Separately, I’m also not sure that we ever want to set default handled for
mouse move, since a move doesn’t really need to be “consumed” the way other
events are.