Bug 113372 - PDFPlugin: Enable PDFPlugin only if its dependencies exist on the system
Summary: PDFPlugin: Enable PDFPlugin only if its dependencies exist on the system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: PDF (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-27 01:34 PDT by Tim Horton
Modified: 2013-03-27 23:23 PDT (History)
2 users (show)

See Also:


Attachments
patch (2.20 KB, patch)
2013-03-27 01:37 PDT, Tim Horton
ap: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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