Bug 208599

Summary: Lay initial groundwork for new PDF loading model
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, cdumez, cmarcelo, commit-queue, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
achristensen: review+
PFL
none
PFL
none
PFL none

Description Brady Eidson 2020-03-04 13:47:07 PST
Lay initial groundwork for new PDF loading model
Comment 1 Brady Eidson 2020-03-04 14:08:44 PST
Created attachment 392478 [details]
Patch
Comment 2 Alex Christensen 2020-03-04 14:14:59 PST
Comment on attachment 392478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392478&action=review

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:41
> +// For now, disable new PDF APIs by default even on platforms where otherwise enabled.

FIXME: Enable this?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:126
> +    void getResourceBytesAtPosition(size_t count, off_t position, WTF::Function<void(const uint8_t*, size_t count)>&& completionHandler);

Will it be only called once?  CompletionHandler?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:328
> +        WTF::Function<void(const uint8_t*, size_t count)> completionHandler;

WTF:: is unnecessary.
CompletionHandler?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:585
> +    , m_pdfThread(Thread::create("PDF document thread", [protectedThis = makeRef(*this), this] { threadEntry(); }))

This looks like a ref cycle to me.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:714
> +    unsigned long long dataLength = m_data ? (unsigned long long)CFDataGetLength(m_data.get()) : 0;

static_cast

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1184
> +        if ((unsigned long long)CFDataGetLength(m_data.get()) >= request.position + request.count) {

If this case is needed, could it be static_cast<uint64_t>?
Comment 3 Brady Eidson 2020-03-04 15:15:33 PST
Created attachment 392493 [details]
Patch
Comment 4 Alex Christensen 2020-03-04 15:22:25 PST
Comment on attachment 392493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392493&action=review

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.h:328
> +        size_t count;

It could prevent use of uninitialized memory to give these default initializers.
Comment 5 Brady Eidson 2020-03-04 15:58:57 PST
Created attachment 392500 [details]
PFL
Comment 6 Brady Eidson 2020-03-04 16:00:19 PST
Created attachment 392501 [details]
PFL
Comment 7 Brady Eidson 2020-03-04 16:42:42 PST
Created attachment 392508 [details]
PFL
Comment 8 WebKit Commit Bot 2020-03-04 21:26:19 PST
Comment on attachment 392508 [details]
PFL

Clearing flags on attachment: 392508

Committed r257900: <https://trac.webkit.org/changeset/257900>
Comment 9 WebKit Commit Bot 2020-03-04 21:26:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2020-03-04 21:27:39 PST
<rdar://problem/60071098>