[webkit-reviews] review granted: [Bug 217148] [macCatalyst] Crash when attempting to display a select dropdown in a UIWebView : [Attachment 410160] 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 217148: [macCatalyst] Crash when attempting to display a select dropdown in
a UIWebView
https://bugs.webkit.org/show_bug.cgi?id=217148

Attachment 410160: Patch

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




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

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

Seems OK, but could be even better.

> Source/WebKitLegacy/mac/ChangeLog:33
> +	   This change is only made for Catalyst, in order to avoid
compatibility
> +	   issues.

Could you elaborate on this? I understand there may be compatibility risk, but
aside from the risk, could this be helpful on Mac outside Catalyst?

> Source/WebKitLegacy/mac/ChangeLog:35
> +	   No new tests as the functionality is dependent on a UIKit change. 

I am concerned about something so subtle not being covered by a test.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4655
> +#if PLATFORM(MACCATALYST)

In a case like this where there is no obvious reason for a platform difference,
I think we need a comment explaining the reason we chose a different behavior
between platforms. This is not just leaving out some code that deals with a
platform-specific concept (like the secure input state and completion
controller on Mac and dictation markers on iOS above).

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:4657
> +	       if ([[self _webView] _isPerformingProgrammaticFocus])
> +		   return resign;

It seems a slightly unsafe pattern to use an early return here; does not seem
like the "null frame" and "null page" cases above. Could we instead just "skip
the setFocused call" even if that makes the patch bigger? Like a boolean
"shouldClearFocus" that is a constant outside PLATFORM(MACCATALYST).