[Webkit-unassigned] [Bug 237513] [GTK][WPE] Add initial support for PDF documents using PDF.js

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=237513

--- Comment #10 from Tim Nguyen (:ntim) <ntim at apple.com> ---
Comment on attachment 453949
  --> https://bugs.webkit.org/attachment.cgi?id=453949
Patch

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

>> Source/WebCore/loader/ResourceLoader.cpp:872
>> +#if PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE)
> 
> No need for #if here: we have a runtime setting PdfJSViewerEnabled, so should check that instead.

The goal here is to target platforms that implement the webkit-pdfjs-viewer scheme, which may be correlated to PdfJSViewerEnabled, but not necessarily. Perhaps we should have a WEBKIT-PDFJS-VIEWER-SCHEME macro?

>>> Source/WebCore/page/SecurityOrigin.cpp:124
>>> +        || url.protocolIs("webkit-pdfjs-viewer")
>> 
>> This is surely required for all ports using PDF.js, not just GTK and WPE. Better check PdfJSViewerEnabled instead of using #if.
> 
> It's enabled unconditionally for GTK and WPE. We can discuss if we want to add api to enable/disable it, though. But for now, I think it's easier this way. Cocoa doesn't do this because they have :
> 
> // Allow images to load.
> response.addHTTPHeaderField(HTTPHeaderName::AccessControlAllowOrigin, "*");
> 
> in the bundle resource loader. I  did it this way for consistency with gresources, since webkit-pdfjs-viewer are gresources too in the end.

If we can make this consistent across platforms, that would be great. I'm open to removing response.addHTTPHeaderField(HTTPHeaderName::AccessControlAllowOrigin, "*"); too.

Can you add a comment either way to explain why PDF.js needs this?

>>> Source/WebCore/platform/LegacySchemeRegistry.cpp:148
>>>  #endif
>> 
>> If you change this from std::array to std::vector, then you can check the runtime setting PdfJSViewerEnabled here too.
> 
> And same here.

ditto here, maybe a macro type thing.

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:1915
>>> +        return true;
>> 
>> @ntim: Should we have done this for our port? Will this break something for Cocoa ports if we choose not to ship with it enabled by default yet?
> 
> Note: this requires owner approval to land.

Brent, Cocoa is covered by the 5 lines above:

#if PLATFORM(COCOA)
// On Mac, we can show PDFs.
if (MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType) && !WebProcessPool::omitPDFSupport()
  return true;
#endif // PLATFORM(COCOA)

I think we should add this, but put it above the Cocoa check, although I'm not sure if respecting the `WebProcessPool::omitPDFSupport()` pref is important in the context of PDF.js.

>>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6460
>>> +        return true;
>> 
>> Ditto:
> 
> This too.

I'm not sure what this adds? Can you describe why you're adding this?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220308/d6e5f3a8/attachment.htm>