| Summary: | Remove SimplePDFPlugin | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||
| Component: | Assignee: | Tim Horton <thorton> | |||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | andersca, ap, commit-queue | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Tim Horton
2013-08-02 03:09:08 PDT
Created attachment 207996 [details]
patch
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 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. (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. (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... Build fix was http://trac.webkit.org/changeset/153694 > I wonder if a warning doesn't make sense because of 'friend'
I can't follow, could you please elaborate?
(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! > > 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.
Hilariously, the landed patch did NOT remove the SimplePDFPlugin sources. I believe enrica is going to fix this shortly. |