Bug 119437 - Remove SimplePDFPlugin
Summary: Remove SimplePDFPlugin
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:
Depends on:
Blocks:
 
Reported: 2013-08-02 03:09 PDT by Tim Horton
Modified: 2013-11-21 17:27 PST (History)
3 users (show)

See Also:


Attachments
patch (89.57 KB, patch)
2013-08-02 03:12 PDT, Tim Horton
ap: review+
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-08-02 03:09:08 PDT
PDFPlugin has replaced SimplePDFPlugin everywhere, so we can merge the required parts of SimplePDFPlugin into PDFPlugin and get rid of unnecessary bits.

Nightlies on some outdated versions of OS X (a fully-updated Lion/Mountain Lion will work fine) will lose the ability to view PDFs (unless you install an external plugin), but everything should still build fine.
Comment 1 Tim Horton 2013-08-02 03:12:50 PDT
Created attachment 207996 [details]
patch
Comment 2 WebKit Commit Bot 2013-08-02 03:15:23 PDT
Attachment 207996 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/WebProcessProxy.cpp', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.h', u'Source/WebKit2/WebProcess/Plugins/PDF/SimplePDFPlugin.mm', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1
Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:119:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexey Proskuryakov 2013-08-02 10:06:30 PDT
Comment on attachment 207996 [details]
patch

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

It's sad how huge the class has become. Does it have any responsibilities that can be logically separated?

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:51
> +#if PLATFORM(MAC) && ENABLE(PDFKIT_PLUGIN)
>  #include "PDFPlugin.h"

I think that at least the ENABLE check should be in the header. It's so ugly and fragile to have ifdefs around every include site.

Also, I don't understand why we need a PLATFORM check. ENABLE should only be enabled on appropriate platforms.

> Source/WebKit2/UIProcess/WebProcessProxy.cpp:318
> +#if PLATFORM(MAC) && ENABLE(PDFKIT_PLUGIN)

Same concern about ENABLE.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:66
> +class PDFPlugin FINAL : public Plugin, protected WebCore::ScrollableArea {

Protected base class doesn't make sense in a FINAL class. It should just be private (and clang should have a warning).

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:157
> +    // In-process PDFViews don't support asynchronous initialization.
> +    virtual bool isBeingAsynchronouslyInitialized() const OVERRIDE { return false; }

Not sure what this comment means.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:166
>  #if PLATFORM(MAC)
> -#include "SimplePDFPlugin.h"
>  #if ENABLE(PDFKIT_PLUGIN)

Same concerns about ifdefs.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:570
> -#if PLATFORM(MAC)
> +#if PLATFORM(MAC) && ENABLE(PDFKIT_PLUGIN)

Ditto.
Comment 4 Tim Horton 2013-08-03 18:13:37 PDT
(In reply to comment #3)
> (From update of attachment 207996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207996&action=review
> 
> It's sad how huge the class has become. Does it have any responsibilities that can be logically separated?

Sad but not super surprising. I'd like to do something about this in the future, definitely, but I'm not totally sure how yet.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:51
> > +#if PLATFORM(MAC) && ENABLE(PDFKIT_PLUGIN)
> >  #include "PDFPlugin.h"
> 
> I think that at least the ENABLE check should be in the header. It's so ugly and fragile to have ifdefs around every include site.
> 
> Also, I don't understand why we need a PLATFORM check. ENABLE should only be enabled on appropriate platforms.

Quite right on both counts.

> > Source/WebKit2/UIProcess/WebProcessProxy.cpp:318
> > +#if PLATFORM(MAC) && ENABLE(PDFKIT_PLUGIN)
> 
> Same concern about ENABLE.
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:66
> > +class PDFPlugin FINAL : public Plugin, protected WebCore::ScrollableArea {
> 
> Protected base class doesn't make sense in a FINAL class. It should just be private (and clang should have a warning).

Ooh. I wonder if a warning doesn't make sense because of 'friend', but other than that agreed.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:157
> > +    // In-process PDFViews don't support asynchronous initialization.
> > +    virtual bool isBeingAsynchronouslyInitialized() const OVERRIDE { return false; }
> 
> Not sure what this comment means.

I assumed you wrote it, but never checked :) I think it means exactly what it says; we'll never use the async plugin init code, so we can never be being-asynchronously-initialized. I'm going to remove it because you're like the third person to complain about it and I don't think it's useful.
Comment 5 Tim Horton 2013-08-03 20:51:27 PDT
http://trac.webkit.org/changeset/153693
Comment 6 Tim Horton 2013-08-03 21:03:30 PDT
(In reply to comment #3)
> Also, I don't understand why we need a PLATFORM check. ENABLE should only be enabled on appropriate platforms.

The enable check is in the header, but I put the PLATFORM checks back for now because of http://build.webkit.org/builders/Qt%20Linux%20MIPS32R2%20LE%20Release/builds/17113. I'm not sure why Qt can't find the header… maybe it not being included in their project files means they can't find it? (no idea how their build system works).

I guess I could add it to their project files...
Comment 7 Tim Horton 2013-08-03 21:03:47 PDT
Build fix was http://trac.webkit.org/changeset/153694
Comment 8 Alexey Proskuryakov 2013-08-04 10:59:00 PDT
> I wonder if a warning doesn't make sense because of 'friend'

I can't follow, could you please elaborate?
Comment 9 Tim Horton 2013-08-04 12:21:31 PDT
(In reply to comment #8)
> > I wonder if a warning doesn't make sense because of 'friend'
> 
> I can't follow, could you please elaborate?

I was thinking that protected methods on final classes still made some sense (and therefore shouldn't have a warning) because friend classes could still access them, but friend classes can also access private methods so I guess it doesn't make sense :) ignore me!
Comment 10 Alexey Proskuryakov 2013-08-05 10:36:03 PDT
> > Not sure what this comment means.
> I assumed you wrote it, but never checked :)

Probably. I guess I was thinking about moving PDF rendering to a separate process then, in which case async initialization could make sense.
Comment 11 Tim Horton 2013-11-21 17:27:20 PST
Hilariously, the landed patch did NOT remove the SimplePDFPlugin sources. I believe enrica is going to fix this shortly.