Summary: | Please add API to save a PDF from the contents of a WKWebView | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brad Andalman <bya> | ||||||||||||||||||||||||||||
Component: | WebKit API | Assignee: | Brady Eidson <beidson> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aestes, bdakin, beidson, commit-queue, esprehn+autocc, ews-watchlist, ggaren, glenn, kondapallykalyan, pdr, sabouhallawa, simon.fraser, thorton, webkit-bug-importer, webkit | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | Safari 11 | ||||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||||
OS: | macOS 10.14 | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Brad Andalman
2019-03-14 14:38:58 PDT
Seconding this; I stumbled upon being able to capture a WebView as PDF (with clickable links) years ago via -dataWithPDFInsideRect:, and having to remove this feature would be a bummer. (Tangentially, compiling an app with the 10.14 SDK seems to make -dataWithPDFInsideRect: on WebView produce PDFs that can't be opened properly with non-Apple apps.) *** Bug 201520 has been marked as a duplicate of this bug. *** Created attachment 378128 [details]
Patch
Comment on attachment 378128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378128&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5032 > + // FIXME: <rdar://problem/55081578> PDF exporting on iOS should respect WKSnapshotConfiguration.afterScreenUpdates I don't think there's anything for it to do? That's there because the image API can snapshot without bouncing to the Web Content process (and thus won't naturally include changes that haven't arrived in the latest transaction), but in the PDF case you always get everything because you have to go through the Web Content process. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:435 > +- (void)_takePDFSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSData * pdfSnapshotData, NSError * error))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); No spaces after the stars > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2412 > + auto dictionary = adoptCF(CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); > + CGRect mediaBox = CGRectMake(0, 0, bitmapSize.width(), bitmapSize.height()); > + auto mediaBoxData = adoptCF(CFDataCreate(NULL, (const UInt8 *)&mediaBox, sizeof(CGRect))); > + CFDictionarySetValue(dictionary.get(), kCGPDFContextMediaBox, mediaBoxData.get()); ObjC dictionary literal syntax is much nicer > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4786 > + auto originalLayoutViewportOverrideRect = frameView.layoutViewportOverrideRect(); > + frameView.setLayoutViewportOverrideRect(LayoutRect(snapshotRect)); Ooh you should add a test for this code (by having a fixed position thing and testing that it ends up in the right place). I didn't test because I'm not a good person like you, but now you can! > Tools/MiniBrowser/MiniBrowser.entitlements:4 > <dict> Unfortunate that the sorting and read-only -> read-write change are together > Tools/TestWebKitAPI/Configurations/TestWebKitAPILibrary.xcconfig:32 > + weird extra space here Comment on attachment 378128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378128&action=review > Tools/TestWebKitAPI/cocoa/TestPDFDocument.mm:121 > + auto context = adoptCF(CGBitmapContextCreate(NULL, boundsRect.size.width, boundsRect.size.height, 8, 0, colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big)); You do not have to draw the whole PDF to know the color at a point. You can create 1x1 bitmap context, transform the context to this point and then draw the PDF to the context. unsigned char pixel[4]; auto context = adoptCF(CGBitmapContextCreate(pixel, 1, 1, 8, sizeof(pixel), colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big)); context.translate(-x, -y); Created attachment 378139 [details]
Patch
(In reply to Said Abou-Hallawa from comment #6) > Comment on attachment 378128 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378128&action=review > > > Tools/TestWebKitAPI/cocoa/TestPDFDocument.mm:121 > > + auto context = adoptCF(CGBitmapContextCreate(NULL, boundsRect.size.width, boundsRect.size.height, 8, 0, colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big)); > > You do not have to draw the whole PDF to know the color at a point. You can > create 1x1 bitmap context, transform the context to this point and then draw > the PDF to the context. > > unsigned char pixel[4]; > auto context = adoptCF(CGBitmapContextCreate(pixel, 1, 1, 8, sizeof(pixel), > colorSpace.get(), kCGImageAlphaPremultipliedLast | kCGBitmapByteOrder32Big)); > context.translate(-x, -y); Oh super neat! Will try out this change the next upload. Created attachment 378218 [details]
Patch
Created attachment 378228 [details]
Patch
Created attachment 378300 [details]
Patch
Created attachment 378301 [details]
Patch
Created attachment 378303 [details]
Patch
Created attachment 378330 [details]
Patch
Created attachment 378331 [details]
Patch
The <win> bot build failure has nothing to do with this patch. Comment on attachment 378331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378331&action=review Plus the one offline comment > Source/WebCore/rendering/PaintPhase.h:70 > + AnnotateLinks = 1 << 12, This one is weird enough it probably deserves a comment > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-4216 > - 537CF84822EFD72000C6EBB3 /* Check .xcfilelists */ = { Where did this go?? > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5048 > + "$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Versions/A/Frameworks", Shouldn't this be in an xcconfig? (In reply to Tim Horton from comment #17) > Comment on attachment 378331 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378331&action=review > > Plus the one offline comment > > > Source/WebCore/rendering/PaintPhase.h:70 > > + AnnotateLinks = 1 << 12, > > This one is weird enough it probably deserves a comment > > > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:-4216 > > - 537CF84822EFD72000C6EBB3 /* Check .xcfilelists */ = { > > Where did this go?? No clue - I did not consciously remove it. I will restore it manually, though. > > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:5048 > > + "$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Versions/A/Frameworks", > > Shouldn't this be in an xcconfig? Per the patch, it actually *is* in an xcconfig... I'm not actually sure how it got into the xcodeproj. I'll cleanup the xcodeproj stuff before landing. Created attachment 378336 [details]
Patch
Created attachment 378337 [details]
Patch
Created attachment 378339 [details]
Patch
Created attachment 378476 [details]
Patch
Comment on attachment 378476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378476&action=review > Source/WebCore/rendering/RenderElement.cpp:2036 > + return element() && element()->isLink() && (document().printing() || (view().frameView().paintBehavior() & PaintBehavior::AnnotateLinks)); Would be nicer to set the PaintBehavior::AnnotateLinks bit when printing. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5040 > +- (void)_takePDFSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSData *, NSError *))completionHandler Should the format (PDF) be part of the WKSnapshotConfiguration? > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5057 > + handler(nil, createNSError(WKErrorUnknown).get()); Can we do better than "unknown error"? > Source/WebKit/UIProcess/WebPageProxy.cpp:7754 > +#if PLATFORM(COCOA) > +void WebPageProxy::drawToPDF(FrameIdentifier frameID, const Optional<FloatRect>& rect, DrawToPDFCallback::CallbackFunction&& callback) Why not put it in WebPageProxyCocoa.mm? > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:263 > + auto mediaBoxData = adoptCF(CFDataCreate(NULL, (const UInt8 *)&mediaBox, sizeof(CGRect))); That's pretty crazy but looks correct. > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:276 > + CGPDFContextBeginPage(pdfContext.get(), dictionary); > + > + GraphicsContext graphicsContext { pdfContext.get() }; > + graphicsContext.scale({ 1, -1 }); > + graphicsContext.translate(0, -bitmapSize.height()); > + > + paintSnapshotAtSize(rect, bitmapSize, options, *coreFrame, *frameView, graphicsContext); > + > + CGPDFContextEndPage(pdfContext.get()); Use some {} to scope the GraphicsContext between the begin page/end page > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4719 > #if PLATFORM(COCOA) > +void WebPage::drawToPDF(FrameIdentifier frameID, const Optional<FloatRect>& rect, CallbackID callbackID) WebPageCocoa.mm? > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4735 > + frameView.setLayoutViewportOverrideRect(LayoutRect(snapshotRect)); Do you really want position:fixed thing laid out relative to the snapshotRect if it's just a small portion of the document? Probably not? Comment on attachment 378476 [details] Patch Clearing flags on attachment: 378476 Committed r249739: <https://trac.webkit.org/changeset/249739> All reviewed patches have been landed. Closing bug. Sorry for missing these comments before CQ landed. (In reply to Simon Fraser (smfr) from comment #23) > Comment on attachment 378476 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378476&action=review > > > Source/WebCore/rendering/RenderElement.cpp:2036 > > + return element() && element()->isLink() && (document().printing() || (view().frameView().paintBehavior() & PaintBehavior::AnnotateLinks)); > > Would be nicer to set the PaintBehavior::AnnotateLinks bit when printing. I agree the printing codepath has room for cleanup, this included! But I also suspect our printing tests don't cover annotations in the generated PDF, and therefore the risk of regression is... non-trivial. > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5040 > > +- (void)_takePDFSnapshotWithConfiguration:(WKSnapshotConfiguration *)snapshotConfiguration completionHandler:(void (^)(NSData *, NSError *))completionHandler > > Should the format (PDF) be part of the WKSnapshotConfiguration? The current API in its current form literally cannot handle this new use-case, so we had to add PDF-specific API. The hope is that we can get support from other teams to make the current API work, at which point we would fold the "make PDF" bit onto snapshot configuration. > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:5057 > > + handler(nil, createNSError(WKErrorUnknown).get()); > > Can we do better than "unknown error"? Not with API, sadly. (We need a big WKError API revamp, not just related to this patch! > > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4735 > > + frameView.setLayoutViewportOverrideRect(LayoutRect(snapshotRect)); > > Do you really want position:fixed thing laid out relative to the > snapshotRect if it's just a small portion of the document? Probably not? Discussed this IRL - Great point, and I'm working on it now! |