Bug 193130 - Share ink choice and ruler between all editable images
Summary: Share ink choice and ruler between all editable images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-03 17:25 PST by Tim Horton
Modified: 2019-01-04 12:24 PST (History)
5 users (show)

See Also:


Attachments
Patch (26.49 KB, patch)
2019-01-03 17:27 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (26.64 KB, patch)
2019-01-03 17:45 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (26.94 KB, patch)
2019-01-04 11:25 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2019-01-03 17:25:54 PST
Share ink choice and ruler between all editable images
Comment 1 Tim Horton 2019-01-03 17:27:15 PST
Created attachment 358295 [details]
Patch
Comment 2 Tim Horton 2019-01-03 17:27:17 PST
<rdar://problem/46826491>
Comment 3 Tim Horton 2019-01-03 17:45:59 PST
Created attachment 358297 [details]
Patch
Comment 4 Wenson Hsieh 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)
Comment 5 Tim Horton 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!
Comment 6 Tim Horton 2019-01-04 11:25:54 PST
Created attachment 358343 [details]
Patch
Comment 7 Wenson Hsieh 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)!
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2019-01-04 12:24:37 PST
All reviewed patches have been landed.  Closing bug.