Bug 192002

Summary: Serialize and deserialize editable image strokes
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, commit-queue, dino, ews-watchlist, mitz, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Tim Horton 2018-11-27 02:37:32 PST
Serialize and deserialize editable image strokes
Comment 1 Tim Horton 2018-11-27 02:37:45 PST
Created attachment 355718 [details]
Patch
Comment 2 Tim Horton 2018-11-27 02:37:47 PST
<rdar://problem/30900149>
Comment 3 EWS Watchlist 2018-11-27 02:39:29 PST
Attachment 355718 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/PencilKitSPI.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Tim Horton 2018-11-27 02:41:10 PST
(In reply to Build Bot from comment #3)
> Attachment 355718 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/Platform/spi/ios/PencilKitSPI.h:31:  Alphabetical
> sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 16 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Silly stylebot.
Comment 5 Wenson Hsieh 2018-11-27 09:29:27 PST
Comment on attachment 355718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355718&action=review

> Source/WebKit/UIProcess/ios/EditableImageController.mm:144
> +            attachmentBacksEditableImage = true;

Can we just return `WebPageProxy::ShouldUpdateAttachmentAttributes::No` here, or do we anticipate an API::Attachment identifier mapping to more than one editable image?

> Source/WebKit/UIProcess/ios/WKDrawingView.mm:126
> +            (__bridge NSString *)kCGImagePropertyExifUserComment : base64Drawing

From what I can tell, base64Drawing should always exist because [drawing serialize] is never nil. But it might still be a good idea to bail if !base64Drawing, since this might not be the case in the future.
Comment 6 Dean Jackson 2018-11-27 10:40:28 PST
Comment on attachment 355718 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=355718&action=review

> Source/WebKit/ChangeLog:41
> +        Serialize strokes into the EXIF User Comment field.

Wow.

> Source/WebKit/UIProcess/WebPageProxy.cpp:8102
> +WebPageProxy::ShouldUpdateAttachmentAttributes WebPageProxy::willUpdateAttachmentAttributes(const API::Attachment& attachment)

Do you need the WebPageProxy:: namespace on ShouldUpdateAttachmentAttributes here?
Comment 7 Tim Horton 2018-11-27 11:39:07 PST
(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 355718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355718&action=review
> 
> > Source/WebKit/UIProcess/ios/EditableImageController.mm:144
> > +            attachmentBacksEditableImage = true;
> 
> Can we just return `WebPageProxy::ShouldUpdateAttachmentAttributes::No`
> here, or do we anticipate an API::Attachment identifier mapping to more than
> one editable image?

I think that’s fine, you’re right. Of course the thing I was worried about /was/ multiple editable images with the same API::Attachment, but your deduplicate-on-insert code should handle that.

> > Source/WebKit/UIProcess/ios/WKDrawingView.mm:126
> > +            (__bridge NSString *)kCGImagePropertyExifUserComment : base64Drawing
> 
> From what I can tell, base64Drawing should always exist because [drawing
> serialize] is never nil. But it might still be a good idea to bail if
> !base64Drawing, since this might not be the case in the future.

Yeah, I think so.


(In reply to Dean Jackson from comment #6)
> Comment on attachment 355718 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355718&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:8102
> > +WebPageProxy::ShouldUpdateAttachmentAttributes WebPageProxy::willUpdateAttachmentAttributes(const API::Attachment& attachment)
> 
> Do you need the WebPageProxy:: namespace on ShouldUpdateAttachmentAttributes
> here?

Shouldn’t, I think that’s just a copy-paste-o.
Comment 8 Tim Horton 2018-11-27 11:43:01 PST
(In reply to Tim Horton from comment #7)
> (In reply to Dean Jackson from comment #6)
> > Comment on attachment 355718 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355718&action=review
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.cpp:8102
> > > +WebPageProxy::ShouldUpdateAttachmentAttributes WebPageProxy::willUpdateAttachmentAttributes(const API::Attachment& attachment)
> > 
> > Do you need the WebPageProxy:: namespace on ShouldUpdateAttachmentAttributes
> > here?
> 
> Shouldn’t, I think that’s just a copy-paste-o.

Nope, definitely need that.
Comment 9 Tim Horton 2018-11-27 11:46:36 PST
Created attachment 355754 [details]
Patch
Comment 10 EWS Watchlist 2018-11-27 11:50:37 PST
Attachment 355754 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/PencilKitSPI.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Commit Bot 2018-11-27 12:20:27 PST
Comment on attachment 355754 [details]
Patch

Clearing flags on attachment: 355754

Committed r238566: <https://trac.webkit.org/changeset/238566>
Comment 12 WebKit Commit Bot 2018-11-27 12:20:29 PST
All reviewed patches have been landed.  Closing bug.