Bug 113372

Summary: PDFPlugin: Enable PDFPlugin only if its dependencies exist on the system
Product: WebKit Reporter: Tim Horton <thorton>
Component: PDFAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, webkit-ews
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch ap: review+, eflews.bot: commit-queue-

Description Tim Horton 2013-03-27 01:34:20 PDT
Otherwise, fall back to PDFViewController/etc.

<rdar://problem/12685301>
Comment 1 Tim Horton 2013-03-27 01:37:06 PDT
Created attachment 195243 [details]
patch
Comment 2 EFL EWS Bot 2013-03-27 01:44:08 PDT
Comment on attachment 195243 [details]
patch

Attachment 195243 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17311441
Comment 3 Tim Horton 2013-03-27 01:44:52 PDT
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.
Comment 4 Early Warning System Bot 2013-03-27 01:46:45 PDT
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
Comment 5 Alexey Proskuryakov 2013-03-27 08:48:40 PDT
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?
Comment 6 Tim Horton 2013-03-27 10:58:22 PDT
(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?
Comment 7 Alexey Proskuryakov 2013-03-27 11:34:11 PDT
Good point. I'm not sure what a better name would be.
Comment 8 Tim Horton 2013-03-27 11:36:28 PDT
(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?
Comment 9 Tim Horton 2013-03-27 23:23:29 PDT
http://trac.webkit.org/changeset/147067