Bug 192002 – Serialize and deserialize editable image strokes

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Serialize and deserialize editable image strokes

[Tim Horton]

Created attachment 355718 [details]
Patch

[Tim Horton]

<rdar://problem/30900149>

[EWS Watchlist]

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.

[Tim Horton]

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

[Wenson Hsieh]

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.

[Dean Jackson]

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?

[Tim Horton]

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

[Tim Horton]

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

[Tim Horton]

Created attachment 355754 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 355754 [details]
Patch

Clearing flags on attachment: 355754

Committed r238566: <https://trac.webkit.org/changeset/238566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.