[webkit-reviews] review granted: [Bug 193130] Share ink choice and ruler between all editable images : [Attachment 358297] Patch

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 193130: Share ink choice and ruler between all editable images
https://bugs.webkit.org/show_bug.cgi?id=193130

Attachment 358297: Patch

https://bugs.webkit.org/attachment.cgi?id=358297&action=review




--- Comment #4 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 358297
  --> https://bugs.webkit.org/attachment.cgi?id=358297
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
> + at 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)