[webkit-reviews] review requested: [Bug 88807] Input elements with type=range do not have default touch handlers. : [Attachment 147321] Patch

[bugzilla-daemon at webkit.org]

Kevin Ellis <kevers at chromium.org> has asked  for review:
Bug 88807: Input elements with type=range do not have default touch handlers.
https://bugs.webkit.org/show_bug.cgi?id=88807

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

------- Additional Comments from Kevin Ellis <kevers at chromium.org>
(In reply to comment #5)
> (From update of attachment 146912 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=146912&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +	      No new tests. (OOPS!)
> > 
> > You should remove the 'No new tests' and either add and list tests, or
explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> You need to update this or it will get bounced from the commit queue.
> 
> Also, a longer more detailed ChangeLog is necessary.
> 
Added touch slider test, and more detail in the Change Log.

> > Source/WebCore/dom/Touch.cpp:76
> > +{
> 
> you will need to test your slider inside a transform. Like rotated.
> 
> > Source/WebCore/dom/Touch.h:69
> > +	 RefPtr<Frame> m_frame;
> 
> this seems wrong to me. How do other events deal with this?
> 
Removed keeping a reference to the frame.  The thumb scroll element relies on
absolute page position and not adjusted page position.	The logic for
correcting the page scale and zoom is consistent with MouseRelatedEvent.

> > Source/WebCore/dom/TouchEvent.h:72
> > +	 virtual bool isTouchEvent() const OVERRIDE;
> 
> afaik WebCore doesn't use the OVERRIDE macro
> 
> > Source/WebCore/html/RangeInputType.cpp:37
> > +#include "Frame.h"
> 
> i expect that you should avoid this.
> 
> > Source/WebCore/html/RangeInputType.cpp:155
> > +	     event->preventDefault();
> 
> you are the default handler? do you need to call this?
> 
Unless preventDefault is called, the gesture recognizer can still scroll the
page while trying to drag the slider.

> > Source/WebCore/html/RangeInputType.h:54
> > +	 virtual void handleTouchEvent(TouchEvent*) OVERRIDE;
> 
> ok... maybe I was wrong about the OVERRIDE. :-)