Make PDF range requests to the network
Created attachment 392909 [details] Patch
Comment on attachment 392909 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392909&action=review This patch looks good so far. I haven't finished reading it. If someone else wants to finish reviewing go for it do NOT wait for me. I have to run, but I will finish reading later. > Source/WebKit/ChangeLog:18 > + While it is now entirely mis-named, NetscapePlugInStreamLoader is a perfect fit for this. This is ok as-is. No change is needed. mis-named => misnamed > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:339 > + WebCore::NetscapePlugInStreamLoader* streamLoader() const { return m_streamLoader; } This is ok as-is. No change is needed. Optimal solution is to either return const NetscapePlugInStreamLoader* because: 1. Prevents m_streamLoader from being mutated out from under this object. OR make this function non-const because: 1. Acknowledge m_streamLoader can mutate out from under this object so long as caller holds non-const ref to this object.
Created attachment 392920 [details] Patch
Created attachment 392921 [details] Patch
Comment on attachment 392920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392920&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:326 > + // WebCore::NetscapePlugInStreamLoaderClient Ouch @ the name > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:336 > + ByteRangeRequest(off_t position, size_t count, CompletionHandler<void(const uint8_t*, size_t count)>&& completionHandler) off_t or uint64_t? why both? > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:770 > + TextStream stream; > + stream << "bytes=" << position << "-" << position + count - 1; Is this really how we compose headers? Also, shouldn't this use WTF::makeString? > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:817 > + m_completionHandler(data, count); Do you need to clear the completion handler? Is there any chance of a cycle here?
(In reply to Tim Horton from comment #5) > Comment on attachment 392920 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392920&action=review > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:326 > > + // WebCore::NetscapePlugInStreamLoaderClient > > Ouch @ the name > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:336 > > + ByteRangeRequest(off_t position, size_t count, CompletionHandler<void(const uint8_t*, size_t count)>&& completionHandler) > > off_t or uint64_t? why both? Comes in to CGDataProvider as off_t We know it's always positive and therefore go uint64_t. I can make that switch when creating the request I guess. > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:770 > > + TextStream stream; > > + stream << "bytes=" << position << "-" << position + count - 1; > > Is this really how we compose headers? When it's a rare header WebKit has never set before? Sure. > Also, shouldn't this use WTF::makeString? I can never keep track. > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:817 > > + m_completionHandler(data, count); > > Do you need to clear the completion handler? Is there any chance of a cycle > here? WTF::CompletionHandlers can only be called once.
Comment on attachment 392921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392921&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:770 > + TextStream stream; > + stream << "bytes=" << position << "-" << position + count - 1; makeString can be used for this if StringConcatenateNumbers.h is included.
Created attachment 392925 [details] Patch
Comment on attachment 392925 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392925&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:77 > +, private WebCore::NetscapePlugInStreamLoaderClient Indent > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:327 > + void willSendRequest(WebCore::NetscapePlugInStreamLoader*, WebCore::ResourceRequest&&, const WebCore::ResourceResponse& redirectResponse, CompletionHandler<void(WebCore::ResourceRequest&&)>&&) final; These won't override anything if HAVE(INCREMENTAL_PDF_APIS) is false. > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:335 > + ByteRangeRequest() = default; Do we need a default constructor?
(In reply to Alex Christensen from comment #9) > Comment on attachment 392925 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392925&action=review > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:77 > > +, private WebCore::NetscapePlugInStreamLoaderClient > > Indent > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:327 > > + void willSendRequest(WebCore::NetscapePlugInStreamLoader*, WebCore::ResourceRequest&&, const WebCore::ResourceResponse& redirectResponse, CompletionHandler<void(WebCore::ResourceRequest&&)>&&) final; > > These won't override anything if HAVE(INCREMENTAL_PDF_APIS) is false. The class only implements NetscapePlugInStreamLoaderClient if HAVE(INCREMENTAL_PDF_APIS) is true, and these overrides are in a HAVE(INCREMENTAL_PDF_APIS) block. > > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:335 > > + ByteRangeRequest() = default; > > Do we need a default constructor? Yes for the HashMap.
Created attachment 392928 [details] Patch (fixed the indent)
Comment on attachment 392928 [details] Patch (fixed the indent) Clearing flags on attachment: 392928 Committed r258096: <https://trac.webkit.org/changeset/258096>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60195730>
Comment on attachment 392928 [details] Patch (fixed the indent) View in context: https://bugs.webkit.org/attachment.cgi?id=392928&action=review > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:351 > + bool maybeComplete(PDFPlugin&); This is ok as-is. A better solution is to make this a const function because it does not modify the state of this object. > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:352 > + void completeUnconditionally(PDFPlugin&); Ditto. > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:824 > +bool PDFPlugin::ByteRangeRequest::maybeComplete(PDFPlugin& plugin) This is ok as-is. No change needed. Slightly better to take plug-in by const lvalue ref because this function does not modify it. > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:833 > +void PDFPlugin::ByteRangeRequest::completeUnconditionally(PDFPlugin& plugin) Ditto > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:858 > + ASSERT(request->streamLoader() == loader); Nullptr deref here when request is nullptr > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:883 > + request->addData(reinterpret_cast<const uint8_t*>(data), count); This is ok as-is. No change is needed. The optimal solution is to not use an early return, just write: If (auto* request = byteRangeRequestForLoader(*loader)) .... Because 1, Scopes request closer to its use 2. Less lines of code. 3. Because of (2) improves readability > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:918 > +} This ok as-is. No change needed. The parentheses are not needed because 1. -> has higher precedence than &