<rdar://problem/70744914>
Created attachment 415142 [details] Patch
Comment on attachment 415142 [details] Patch The previous mac-debug build failure appears to due to build artifacts from #219371. Removing cq-
Created attachment 415168 [details] Patch
Comment on attachment 415168 [details] 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)?
Comment on attachment 415168 [details] Patch 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. Sounds good — added! >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1066 >> + [self _tearDownImageExtraction]; > > Should this go above `_hasSetUpInteractions = NO;`? Good catch — it probably should, along with some of the other stuff here (but we can clean that up separately). >> 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. Sounds good — changed to -initWithImageExtractionGestureDelegate: >> Source/WebKit/UIProcess/ios/WKImageExtractionGestureRecognizer.mm:32 >> + __weak UIView <WKImageExtractionGestureRecognizerDelegate> *_imageExtractionDelegate; > > `WeakObjCPtr`? Hm..I think we actually prefer __weak for members (perhaps thorton can comment). >> 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? That's true — 0 should have essentially the same effect. I'll change it to 0. >> 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)? Err…I think this would result in ∞. It does, however, look like `super.state = state;` is a thing (which I hadn't known before)! I'll change it to that.
Comment on attachment 415168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415168&action=review >>> 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)? > > Err…I think this would result in ∞. > > It does, however, look like `super.state = state;` is a thing (which I hadn't known before)! I'll change it to that. OH 🤦♂️ right yes this is inside the `setState` function sorry ignore me
Created attachment 415176 [details] Address comments
Committed r270332: <https://trac.webkit.org/changeset/270332> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415176 [details].