Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy-pasted attachments in mail show up as generic file icons #735

Merged
merged 0 commits into from May 19, 2022

Conversation

kcheney1
Copy link
Contributor

@kcheney1 kcheney1 commented May 18, 2022

bf65cfe

Copy-pasted attachments in mail show up as generic file icons
https://bugs.webkit.org/show_bug.cgi?id=240586
rdar://91067102

Reviewed by Wenson Hsieh.

Generate QuickLook previews for attachments that are copy-pasted as
webarchives, such as pages, keynote and numbers docs.

* Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
(testiWorkAttachmentFileURL):
(testiWorkAttachmentData):
(TestWebKitAPI::TEST):
(TestWebKitAPI::_generateBestRepresentationForRequest):
Swizzle QLThumbnailGenerator code to make sure we hit image generation.
This is not an ideal test but ensures we at least hit the right code
path since we are unable to directly compare icons.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/test.pages: Added.
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::registerAttachmentsFromSerializedData):

Canonical link: https://commits.webkit.org/250719@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294451 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@kcheney1 kcheney1 self-assigned this May 18, 2022
@@ -1487,6 +1502,7 @@ @implementation FileWrapper
auto attachment = retainPtr([webView _attachmentForIdentifier:attachmentIdentifier]);
EXPECT_WK_STREQ(attachmentIdentifier, [attachment uniqueIdentifier]);
EXPECT_WK_STREQ(attachmentIdentifier, [webView stringByEvaluatingJavaScript:@"document.querySelector('img').attachmentIdentifier"]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, will remove this extra space

@kcheney1 kcheney1 added WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). WebKit Nightly Build labels May 18, 2022
Comment on lines 1654 to 1663
InstanceMethodSwizzler quickLookSwizzler {
getQLThumbnailGeneratorClass(),
@selector(generateBestRepresentationForRequest:completionHandler:),
reinterpret_cast<IMP>(_generateBestRepresentationForRequest)
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to put this swizzler below the

[webView _synchronouslyExecuteEditCommand:@"Cut" argument:nil];

..line below. If we eventually fix -_insertAttachmentWithFileWrapper: to automatically request QL thumbnails for the inserted attachment, then I think this test might become a false positive (since we'll get two calls to the swizzled _generateBestRepresentationForRequest).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - I reset the bool variable to false below to avoid that but probably safe to move the swizzler as well.

Thanks for the review!

@kcheney1 kcheney1 added the merge-queue Applied to send a pull request to merge-queue label May 18, 2022
@webkit-early-warning-system webkit-early-warning-system added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels May 18, 2022
@kcheney1 kcheney1 added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels May 18, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit bf65cfe into WebKit:main May 19, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r294451 (250719@main): https://commits.webkit.org/250719@main

Reviewed commits have been landed. Closing PR #735 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
3 participants