[webkit-reviews] review granted: [Bug 220454] Double tap to select does not work if the page clears selections on tap, like grammarly.com does : [Attachment 417394] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 220454: Double tap to select does not work if the page clears selections on
tap, like grammarly.com does
https://bugs.webkit.org/show_bug.cgi?id=220454

Attachment 417394: Patch

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




--- Comment #7 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 417394
  --> https://bugs.webkit.org/attachment.cgi?id=417394
Patch

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

> Source/WebKit/ChangeLog:9
> +	   Double tap to select content did not work on grammerly.com on iPad
because this is a desktop website run on a touch-based device,

Nit - grammerly.com => grammarly.com

> Source/WebKit/ChangeLog:14
> +	   these two events. This makes for a more expected change of events,
and does not let grammerly.com clear a valid selection based off of synthetic 

Ditto.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:856
> +    if (auto selectionChangedHandler =
std::exchange(m_selectionChangedHandler, nil))

Nit - nil => { }.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1172
> +    if (auto selectionChangedHandler =
std::exchange(m_selectionChangedHandler, nil))

Nit - nil => { }.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1191
> +    if (auto selectionChangedHandler =
std::exchange(m_selectionChangedHandler, nil))

Nit - nil => { }.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2141
> +

We should assert that `m_selectionChangedHandler` is not set here, and/or
invoke the previous handler if it is set.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2142
> +    m_selectionChangedHandler = [point, granularity,
isInteractingWithFocusedElement, completionHandler =
WTFMove(completionHandler), webPage = makeWeakPtr(*this), this]()mutable {

Nit - space before the `mutable`.