<rdar://problem/72390053>
Created attachment 416347 [details] Patch
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.
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`).
Created attachment 416354 [details] Patch for landing
Committed r270901: <https://trac.webkit.org/changeset/270901> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416354 [details].