Bug 208776 – Make PDF range requests to the network

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Make PDF range requests to the network

[Brady Eidson]

Created attachment 392909 [details]
Patch

[Daniel Bates]

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.

[Brady Eidson]

Created attachment 392920 [details]
Patch

[Brady Eidson]

Created attachment 392921 [details]
Patch

[Tim Horton]

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?

[Brady Eidson]

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

[Alex Christensen]

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.

[Brady Eidson]

Created attachment 392925 [details]
Patch

[Alex Christensen]

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?

[Brady Eidson]

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

[Brady Eidson]

Created attachment 392928 [details]
Patch (fixed the indent)

[WebKit Commit Bot]

Comment on attachment 392928 [details]
Patch (fixed the indent)

Clearing flags on attachment: 392928

Committed r258096: <https://trac.webkit.org/changeset/258096>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60195730>

[Daniel Bates]

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 &