Share ink choice and ruler between all editable images
Created attachment 358295 [details] Patch
<rdar://problem/46826491>
Created attachment 358297 [details] Patch
Comment on attachment 358297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358297&action=review r=me! It seems like it *might* be possible to write a test for this if we added hooks to set or grab the current ink color of the focused editable image (i.e. the selected PKInk's -color). Then the test could do something like: - Focus an editable image (I hesitate to write "and wait for input session, because this no longer uses the platform keyboard IIRC"). - Set the selected ink color. - Focus a different editable image. - Check that the selected ink color is the same. > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6298 > + if (!_drawingCoordinator) Is this something we ought to throw out in -cleanUpInteraction, when the web process crashes? (Or maybe just reset _drawingCoordinator's _focusedEmbeddedViewID to 0?) > Source/WebKit/UIProcess/ios/WKDrawingCoordinator.mm:43 > + __weak WKContentView *_contentView; Just out of curiosity — why __weak over WeakObjCPtr! > Source/WebKit/UIProcess/ios/WKDrawingView.h:31 > OBJC_CLASS PKCanvasView; Nit - I think you can remove this forward declaration. > Source/WebKit/UIProcess/ios/WKDrawingView.mm:49 > + __weak WKContentView *_contentView; (Same question here) > Source/WebKit/UIProcess/ios/WKInkPickerView.h:41 > +@property (nonatomic, assign) BOOL rulerEnabled; Nit - I don't think you need to explicitly mark this property as assign. > Source/WebKit/UIProcess/ios/WKInkPickerView.mm:40 > + __weak WKContentView *_contentView; (Same question here)
(In reply to Wenson Hsieh from comment #4) > Comment on attachment 358297 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358297&action=review > > r=me! > > It seems like it *might* be possible to write a test for this if we added > hooks to set or grab the current ink color of the focused editable image > (i.e. the selected PKInk's -color). Then the test could do something like: > > - Focus an editable image (I hesitate to write "and wait for input session, > because this no longer uses the platform keyboard IIRC"). > - Set the selected ink color. > - Focus a different editable image. > - Check that the selected ink color is the same. In the end with all of the mocking/digging into and interacting with PK internals that would happen, the % of this patch that would actually be tested is pretty small. Might revisit and add more testing mechanisms for more interesting patches. > > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:6298 > > + if (!_drawingCoordinator) > > Is this something we ought to throw out in -cleanUpInteraction, when the web > process crashes? Seems reasonable. > (Or maybe just reset _drawingCoordinator's _focusedEmbeddedViewID to 0?) > > > Source/WebKit/UIProcess/ios/WKDrawingCoordinator.mm:43 > > + __weak WKContentView *_contentView; > > Just out of curiosity — why __weak over WeakObjCPtr! To avoid post-landing review comments :) > > Source/WebKit/UIProcess/ios/WKDrawingView.h:31 > > OBJC_CLASS PKCanvasView; > > Nit - I think you can remove this forward declaration. Indeed! > > Source/WebKit/UIProcess/ios/WKInkPickerView.h:41 > > +@property (nonatomic, assign) BOOL rulerEnabled; > > Nit - I don't think you need to explicitly mark this property as assign. Indeed!
Created attachment 358343 [details] Patch
Comment on attachment 358343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358343&action=review > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:852 > + _drawingCoordinator = nil; #if HAVE(PENCILKIT)!
Comment on attachment 358343 [details] Patch Clearing flags on attachment: 358343 Committed r239628: <https://trac.webkit.org/changeset/239628>
All reviewed patches have been landed. Closing bug.