RESOLVED FIXED 193130
Share ink choice and ruler between all editable images
https://bugs.webkit.org/show_bug.cgi?id=193130
Summary Share ink choice and ruler between all editable images
Tim Horton
Reported 2019-01-03 17:25:54 PST
Share ink choice and ruler between all editable images
Attachments
Patch (26.49 KB, patch)
2019-01-03 17:27 PST, Tim Horton
no flags
Patch (26.64 KB, patch)
2019-01-03 17:45 PST, Tim Horton
no flags
Patch (26.94 KB, patch)
2019-01-04 11:25 PST, Tim Horton
no flags
Tim Horton
Comment 1 2019-01-03 17:27:15 PST
Tim Horton
Comment 2 2019-01-03 17:27:17 PST
Tim Horton
Comment 3 2019-01-03 17:45:59 PST
Wenson Hsieh
Comment 4 2019-01-03 19:42:07 PST
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)
Tim Horton
Comment 5 2019-01-04 11:05:21 PST
(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!
Tim Horton
Comment 6 2019-01-04 11:25:54 PST
Wenson Hsieh
Comment 7 2019-01-04 12:02:01 PST
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)!
WebKit Commit Bot
Comment 8 2019-01-04 12:24:36 PST
Comment on attachment 358343 [details] Patch Clearing flags on attachment: 358343 Committed r239628: <https://trac.webkit.org/changeset/239628>
WebKit Commit Bot
Comment 9 2019-01-04 12:24:37 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.