Bug 113372 – PDFPlugin: Enable PDFPlugin only if its dependencies exist on the system

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

Otherwise, fall back to PDFViewController/etc.

<rdar://problem/12685301>

[Tim Horton]

Created attachment 195243 [details]
patch

[EFL EWS Bot]

Comment on attachment 195243 [details]
patch

Attachment 195243 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17311441

[Tim Horton]

Comment on attachment 195243 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:42
> +#include "PDFKitImports.h"

This needs #if PLATFORM(MAC) too.

[Early Warning System Bot]

Comment on attachment 195243 [details]
patch

Attachment 195243 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17339071

[Alexey Proskuryakov]

Comment on attachment 195243 [details]
patch

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

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:42
>> +#include "PDFKitImports.h"
> 
> This needs #if PLATFORM(MAC) too.

(probably in the file itself, not around the include)

> Source/WebKit2/WebProcess/WebPage/WebPage.h:616
> +    bool pdfPluginEnabled() const;

It's not great that this function named as a simple accessor is no longer an accessor. Can we instead perform the check on setting m_pdfPluginEnabled?

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 195243 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195243&action=review
> 
> >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:42
> >> +#include "PDFKitImports.h"
> > 
> > This needs #if PLATFORM(MAC) too.
> 
> (probably in the file itself, not around the include)
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:616
> > +    bool pdfPluginEnabled() const;
> 
> It's not great that this function named as a simple accessor is no longer an accessor. Can we instead perform the check on setting m_pdfPluginEnabled?

Since checking for PDFLayerController existence requires soft-linking in PDFKit, which we previously only did when it was needed, it seems best to only link it in when we absolutely need the answer (about whether or not we have it), instead of when setting up preferences, no?

[Alexey Proskuryakov]

Good point. I'm not sure what a better name would be.

[Tim Horton]

(In reply to comment #7)
> Good point. I'm not sure what a better name would be.

Maybe I should have both pdfPluginEnabled and shouldUsePDFPlugin or something? Only the latter of which does the class check?

[Tim Horton]

http://trac.webkit.org/changeset/147067