Bug 119437 – Remove SimplePDFPlugin

• Status: RESOLVED
• Resolution: FIXED
• Component: PDF

[Tim Horton]

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.

[Tim Horton]

Created attachment 207996 [details]
patch

[WebKit Commit Bot]

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.

[Alexey Proskuryakov]

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.

[Tim Horton]

(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.

[Tim Horton]

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

[Tim Horton]

(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...

[Tim Horton]

Build fix was http://trac.webkit.org/changeset/153694

[Alexey Proskuryakov]

> I wonder if a warning doesn't make sense because of 'friend'

I can't follow, could you please elaborate?

[Tim Horton]

(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!

[Alexey Proskuryakov]

> > 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.

[Tim Horton]

Hilariously, the landed patch did NOT remove the SimplePDFPlugin sources. I believe enrica is going to fix this shortly.