[webkit-reviews] review granted: [Bug 27941] HTMLInputElement is not controllable by assistive technologies : [Attachment 33976] proposed patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 27941: HTMLInputElement is not controllable by assistive technologies
https://bugs.webkit.org/show_bug.cgi?id=27941

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
Seems this should be in some sort of method on HTMLInputElement instead:
8     // Fire change event manually, as RenderSlider::setValueForPosition does.

 109	 input->dispatchFormControlChangeEvent();

Null check needed?
ntRect AccessibilitySliderThumb::elementRect() const
 139 {
 140	 IntRect intRect =
static_cast<RenderSlider*>(m_parentSlider->renderer())->thumbRect();
 141	 FloatQuad floatQuad =
m_parentSlider->renderer()->localToAbsoluteQuad(FloatRect(intRect));
 142 

Eventually we should write a helper to do this:
 296	
thumbRect.setWidth(thumb->style()->width().calcMinValue(contentWidth()));
 297	
thumbRect.setHeight(thumb->style()->height().calcMinValue(contentHeight()));
 I feel like we do that often all over the code. :)

Same is true with the centering algorithms you use in the next line.  :)  (Not
for this patch, just noting that eventually we'll need more Rect/Point math
code.)

I find AccessibilityUIElement.h is a confusing name for this DRT-only class.

It's sad that everyone replicates the js-testing harness in their tests. :( 
You know about fast/js/resources/, TEMPLATE.html, and make-js-tests right?

In general this looks fine though.