Summary: | Serialize and deserialize editable image strokes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||
Component: | New Bugs | Assignee: | 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
Tim Horton
2018-11-27 02:37:32 PST
Created attachment 355718 [details]
Patch
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.
(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 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 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? (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. (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. Created attachment 355754 [details]
Patch
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 on attachment 355754 [details] Patch Clearing flags on attachment: 355754 Committed r238566: <https://trac.webkit.org/changeset/238566> All reviewed patches have been landed. Closing bug. |