[webkit-reviews] review granted: [Bug 221782] [iOS][FCR] Add new look for input type=range with datalist : [Attachment 420046] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Aditya Keerthi <akeerthi at apple.com>'s
request for review:
Bug 221782: [iOS][FCR] Add new look for input type=range with datalist
https://bugs.webkit.org/show_bug.cgi?id=221782

Attachment 420046: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 420046
  --> https://bugs.webkit.org/attachment.cgi?id=420046
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2305
> +    if (min == max)

Maybe >=?

> Source/WebCore/rendering/RenderThemeIOS.mm:2326
> +    auto& dataList = downcast<HTMLDataListElement>(*input.list());

Why aren’t we using the HTMLInputElement::dataList() function? This downcast is
literally just undoing what the the list() function does.

Also seems like we should only do this once, not separately for the null check
and then again for the local variable. A null check of a local variable would
be nice, although it’s probably less elegant to have a pointer instead of a
reference for the local.

> Source/WebCore/rendering/RenderThemeIOS.mm:2337
> +	   auto optionValue = optionElement.value();
> +	   if (!input.isValidValue(optionValue))
> +	       continue;
> +
> +	   auto parsedOptionValue =
parseToDoubleForNumberType(input.sanitizeValue(optionValue));

It’s not great to have this code here in the theme. I’d suggest we create a
function that returns an Optional<double> so this code doesn’t have to know
things like to call isValidValue and sanitizeValue and which parse function to
call.

The function could be a member of HTMLInputElement, or HTMLDataListElement, or
HTMLOptionElement. Not sure where the cleanest responsibility would be. Could
take a String or could take an HTMLOptionElement. Maybe the cleanest design
would be an overload of the valueAsNumber function in HTMLInputElement that
takes an HTMLOptionElement argument.

> Source/WebCore/rendering/RenderThemeIOS.mm:2346
> +	   context.fillRoundedRect(roundedTickRect, (value >=
parsedOptionValue) ? controlColor : controlBackgroundColor);

Does this need snapping to device pixel boundaries so it looks sharp? In the
past that’s been required for some form control rendering.