[webkit-reviews] review denied: [Bug 237192] [InputElement] Add HTMLInputElement::showPicker() method : [Attachment 454691] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied zsun at igalia.com's request for review:
Bug 237192: [InputElement] Add HTMLInputElement::showPicker() method
https://bugs.webkit.org/show_bug.cgi?id=237192

Attachment 454691: Patch

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




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

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

Looks really good, but still some sticking points, sadly.

> Source/WebCore/html/HTMLInputElement.cpp:1279
> +    m_inputType->allowsShowPickerAcrossFrames();

This isn’t checking the result and returning it. Needs to be more like this:

    if (auto result = m_inputType->allowsShowPickerAcrossFrames();
result.hasException())
	return result;

And, we need to add test cases for this. The fact that no tests failed proves
we aren’t covering this with tests at all.

> Source/WebCore/html/InputType.cpp:520
> +ExceptionOr<void> InputType::allowsShowPickerAcrossFrames()

What I was referring to was a boolean-returning "allowsShowPickerAcrossFrames"
function overridden by file upload and color controls. So there is no
isFileUpload and isColorControl function call. Would you consider that?

I did *not* mean that all of this logic would be moved into the InputType base
class.