[webkit-reviews] review denied: [Bug 26229] Can't click outside the slider thumb and start dragging : [Attachment 32494] patch v3.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 8 18:15:53 PDT 2009


Simon Fraser (smfr) <simon.fraser at apple.com> has denied Pierre d'Herbemont
<pdherbemont at apple.com>'s request for review:
Bug 26229: Can't click outside the slider thumb and start dragging
https://bugs.webkit.org/show_bug.cgi?id=26229

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/rendering/RenderSlider.cpp
b/WebCore/rendering/RenderSlider.cpp
> index 610a060..9e00d68 100644
> --- a/WebCore/rendering/RenderSlider.cpp
> +++ b/WebCore/rendering/RenderSlider.cpp
> @@ -127,41 +127,52 @@ private:
>      virtual Node* shadowParentNode() { return m_shadowParent; }
>  
>      Node* m_shadowParent;
> -    FloatPoint m_initialClickPoint;	     // initial click point in
RenderSlider-local coordinates
> -    int m_initialPosition;
>      bool m_inDragMode;
> +    FloatPoint m_mouseDragVectorToThumb;

Please move this above the bool to optimize packing. I'm not sure
'mouseDragVector' is really tbe best way to describe this. Is it
"offsetFromThumb"? Maybe it should be a FloatSize? 

> +    , m_mouseDragVectorToThumb(0, 0)

No need to init a FloatPoint.

> +		   }
> +		   else {

Should be } else { on one line.

> +		       // We are outside the thumb, move the thumb to the point
were
> +		       // we clicked. We'll be exactly at the center of the
thumb.
> +		       m_mouseDragVectorToThumb.setX(0);
> +		       m_mouseDragVectorToThumb.setY(0);

Is it possible for the click to be off the end of the slider, so we'd end up
with a negative thumb position? i.e. does this code need to handle end
conditions?

> +FloatPoint RenderSlider::mouseEventVectorToThumb(MouseEvent* evt)
> +{
> +    ASSERT (m_thumb && m_thumb->renderer());
> +    FloatPoint localPoint =
m_thumb->renderBox()->absoluteToLocal(evt->absoluteLocation(), false, true);
> +    IntRect thumbBounds = m_thumb->renderBox()->borderBoxRect();
> +    FloatPoint distance;
> +    distance.setX(thumbBounds.x() + thumbBounds.height() / 2 -
localPoint.x());
> +    distance.setY(thumbBounds.y() + thumbBounds.width() / 2 -
localPoint.y());

I think you have height and width switched.

> +    return distance;

You call this distance, but the method says vector.

r- because I think this needs another revision, but it's certainly in the right
direction.


More information about the webkit-reviews mailing list