[webkit-reviews] review granted: [Bug 180651] Add more auto fill button types : [Attachment 329054] Patch and layout tests

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 180651: Add more auto fill button types
https://bugs.webkit.org/show_bug.cgi?id=180651

Attachment 329054: Patch and layout tests

https://bugs.webkit.org/attachment.cgi?id=329054&action=review




--- Comment #19 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 329054
  --> https://bugs.webkit.org/attachment.cgi?id=329054
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=329054&action=review

Looks good. I had a minor suggestion about RefPtr -> Ref, and I think you
should have two separate AX labels. But otherwise this is great.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3089
> +	       return @"strong password";

I think these should be separate roles. I think it would be confusing to get
the same role reported by AX -- how would a blind person know which of the
password fields they were on?

> Source/WebCore/html/HTMLInputElement.cpp:2046
> +    firstStop.m_position = CSSValuePool::singleton().createValue(3,
CSSPrimitiveValue::UnitType::CSS_EMS);

Not to be addressed in this patch: It sure seems like these should be
constructor arguments, with the CSSValuePool::singleton being used inside the
constructor! This is a lot of extra typing!

> Source/WebCore/html/HTMLInputElement.cpp:2056
> +    return gradient;

It seems weird to return a RefPtr<>, when it's only use case immediately
dereferences the pointer. Should we return a Ref<> here instead?

> Source/WebCore/html/HTMLInputElement.h:247
> +    AutoFillButtonType autoFillButtonType() const { return
static_cast<AutoFillButtonType>(m_autoFillButtonType); }

+1 :-)

> Source/WebCore/html/TextFieldInputType.cpp:411
> +	   return AXAutoFillStrongPasswordLabel();

I think these should be two separate AX labels.

> Source/WebCore/platform/LocalizedStrings.cpp:646
> +}

Please add a label for the confirmation password field.