Bug 193130 – Share ink choice and ruler between all editable images

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Share ink choice and ruler between all editable images

[Tim Horton]

Created attachment 358295 [details]
Patch

[Tim Horton]

<rdar://problem/46826491>

[Tim Horton]

Created attachment 358297 [details]
Patch

[Wenson Hsieh]

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]

(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]

Created attachment 358343 [details]
Patch

[Wenson Hsieh]

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 on attachment 358343 [details]
Patch

Clearing flags on attachment: 358343

Committed r239628: <https://trac.webkit.org/changeset/239628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.