[webkit-reviews] review granted: [Bug 219398] [iOS] Support image extraction interactions for accessibility : [Attachment 415168] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 219398: [iOS] Support image extraction interactions for accessibility
https://bugs.webkit.org/show_bug.cgi?id=219398

Attachment 415168: Patch

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 415168
  --> https://bugs.webkit.org/attachment.cgi?id=415168
Patch

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

r=me

> Source/WebKit/Shared/ios/InteractionInformationRequest.cpp:87
> +    if (other.includeImageData && !includeImageData && !includeSnapshot)

I'd add a comment (like what's in the ChangeLog) here explaining why the
`!includeSnapshot` is here too.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1066
> +    [self _tearDownImageExtraction];

Should this go above `_hasSetUpInteractions = NO;`?

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.h:40
> +- (instancetype)initWithDelegate:(UIView
<WKImageExtractionGestureRecognizerDelegate> *)delegate;

Should we be more specific like `initWithImageExtractionGestureDelegate`?  That
appears to be what some other `WK*GestureRecognizer` do.

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:32
> +    __weak UIView <WKImageExtractionGestureRecognizerDelegate>
*_imageExtractionDelegate;

`WeakObjCPtr`?

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:43
> +    self.allowableMovement = 1;

Why not `0`?  Or is this to allow some minuscule amount of movement that's
possibly expected because of how large a finger is vs a point unit?

> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:51
> +    [super setState:state];

Style: can you `self.state = state;` instead (like how you use the "get" on
:53)?