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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 1 15:48:09 PST 2020

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

Attachment 415168: Patch


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

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


> 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>


> 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

More information about the webkit-reviews mailing list