| Summary: | PDFPlugin: Enable PDFPlugin only if its dependencies exist on the system | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||
| Component: | Assignee: | 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
Tim Horton
2013-03-27 01:34:20 PDT
Created attachment 195243 [details]
patch
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 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 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 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? (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? Good point. I'm not sure what a better name would be. (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? |