WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Tim Horton
Comment 1
2019-01-03 17:27:15 PST
Created
attachment 358295
[details]
Patch
Tim Horton
Comment 2
2019-01-03 17:27:17 PST
<
rdar://problem/46826491
>
Tim Horton
Comment 3
2019-01-03 17:45:59 PST
Created
attachment 358297
[details]
Patch
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
Created
attachment 358343
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug