Bug 219952 – Suppress the image extraction interaction while editing text

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

<rdar://problem/72390053>

[Wenson Hsieh]

Created attachment 416347 [details]
Patch

[Devin Rousso]

Comment on attachment 416347 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:211
> +        if (interaction)

NIT: IMO it's a bit weird that we'd have an object that's supposed to suppress an interaction also function if no interaction is given.  Should we `ASSERT(interaction);`?  There's no harm in doing it as written, but it does seem a little odd to me :P

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1297
> +    if (self._imageExtractionEnabled && (!_isBlurringFocusedElement || !_isChangingFocus))

Are there any other flags we should also be checking (e.g. `_textInteractionIsHappening`)?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1298
> +        _suppressImageExtractionToken = isEditable ? makeUnique<WebKit::SuppressInteractionToken>(self, _imageExtractionInteraction.get()) : nullptr;

NIT: Is the `<WebKit::SuppressInteractionToken>` necessary?  I think this can be implicit.

[Wenson Hsieh]

Comment on attachment 416347 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:211
>> +        if (interaction)
> 
> NIT: IMO it's a bit weird that we'd have an object that's supposed to suppress an interaction also function if no interaction is given.  Should we `ASSERT(interaction);`?  There's no harm in doing it as written, but it does seem a little odd to me :P

Good point! Since the only place that creates one of these is behind `_imageExtractionEnabled`, the interaction should always be nonnull here. I'll add an `ASSERT(interaction);` to clarify this.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1297
>> +    if (self._imageExtractionEnabled && (!_isBlurringFocusedElement || !_isChangingFocus))
> 
> Are there any other flags we should also be checking (e.g. `_textInteractionIsHappening`)?

I /think/ this should be it, since the intent was just to avoid introducing the interaction just to remove it again when changing focus between editable fields.

IIUC, `_textInteractionIsHappening` is just about pencil, which (I think) shouldn't conflict with the new image extraction interaction.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1298
>> +        _suppressImageExtractionToken = isEditable ? makeUnique<WebKit::SuppressInteractionToken>(self, _imageExtractionInteraction.get()) : nullptr;
> 
> NIT: Is the `<WebKit::SuppressInteractionToken>` necessary?  I think this can be implicit.

Yes, it's necessary since this is outside of any `using namespace WebCore;` (since we're in an ObjC @implementation block, this code isn't wrapped inside a `namespace WebKit`).

[Wenson Hsieh]

Created attachment 416354 [details]
Patch for landing

[EWS]

Committed r270901: <https://trac.webkit.org/changeset/270901>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416354 [details].