Bug 99206 – [wk2] Implement PDFPlugin

• Component: Plug-ins
• Status: RESOLVED FIXED

[Tim Horton]

Work-in-progress replacement PDF plugin for Mac.

[Tim Horton]

Created attachment 168484 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/12491094>

[Radar WebKit Bug Importer]

<rdar://problem/12491096>

[WebKit Review Bot]

Attachment 168484 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1
Source/WebKit2/WebProcess/Plugins/PDF/BuiltInSimplePDFView.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:109:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:478:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 168485 [details]
this version doesn't have the BuiltInPDFView->BuiltInSimplePDFView rename, so it's easier to review but won't build.

[Tim Horton]

Fixing the change log situation.

[Tim Horton]

Comment on attachment 168485 [details]
this version doesn't have the BuiltInPDFView->BuiltInSimplePDFView rename, so it's easier to review but won't build.

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

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:46
> +    , shouldUsePDFKitPlugin(false)

This and related changes shouldn't be here. Will fix.

[Build Bot]

Comment on attachment 168485 [details]
this version doesn't have the BuiltInPDFView->BuiltInSimplePDFView rename, so it's easier to review but won't build.

Attachment 168485 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14286018

[Tim Horton]

Created attachment 168497 [details]
whole patch again, with changelogs this time

Comment on attachment 168497 [details]
whole patch again, with changelogs this time

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

Very nice! r- because I had many comments (even more comments than I ended up typing and saving).

It’s strange to have the name of a framework, PDFKit, as part of the names of so many WebKit classes. We need to distinguish between the PDFKit-based plug-in and associated code and the older CG-based plug-in, but I don’t think that’s a good way.

> Source/WebKit2/ChangeLog:11
> +

You change log doesn’t follow the conventional format of commenting on the changes to each function or method. I don’t think that’s a good thing.

> Source/WebKit2/ChangeLog:107
> +        Perform a paint into a bitmap context for zooming snapshots.

The word “zooming” here is just confusing, and “for snapshots” is implied by the function’s name.

> Source/WebKit2/ChangeLog:122
> +        Handle copy and select-all commands, and forward them to PDFKit.

I think you meant to name a class, not a framework, here.

> Source/WebKit2/ChangeLog:125
> +        * WebProcess/Plugins/PDF/BuiltInSimplePDFView.h: Renamed from Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFView.h.
> +        * WebProcess/Plugins/PDF/BuiltInSimplePDFView.mm: Renamed from Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFView.mm.

I don’t like this new name. The BuiltInSimplePDFView class primarily inherits from Plugin. Why isn’t it called SimplePDFPlugin?

> Source/WebKit2/ChangeLog:129
> +        * WebProcess/Plugins/PluginView.h:
> +        (WebKit::PluginView::shouldUseFullPagePluginScaling):
> +        Report whether or not the Plugin backed by this PluginView wants to handle scaling itself.

I think a better name for this function is one that starts with “handles”. It’s hard to come up with a name that doesn’t contain the word “page” twice if you want to express both that this is about the page scale and about full-page plug-ins. I think you could omit the full-page plug-in bit and call this handlesPageScaleFactor.

> Source/WebKit2/ChangeLog:135
> +        (WebKit::PluginView::setFullPagePluginScaleFactor):
> +        (WebKit::PluginView::fullPagePluginScaleFactor):
> +        If shouldUseFullPagePluginScaling is true, WebPage uses these to get/set its scale factor,
> +        instead of the normal mechanism.

I’d just call these pageScaleFactor and setPageScaleFactor.

> Source/WebKit2/ChangeLog:140
> +        This PageScaleFactorDidChange prevents other code from using non-1.0 scale factors as the default.

What is “This PageScaleFactorDidChange”? This is really confusing.

> Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:228
> +// Defaults to false
> +WK_EXPORT void WKPreferencesSetPDFKitPluginEnabled(WKPreferencesRef preferencesRef, bool enabled);
> +WK_EXPORT bool WKPreferencesGetPDFKitPluginEnabled(WKPreferencesRef preferencesRef);

The convention used to be that in the header, the parameter is called preferences, not preferencesRef, but I see that it’s been broken countless times.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:208
> +    virtual bool isEditingCommandEnabled(const String& commandName);

No need to name the commandName parameter.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:52
> +class BuiltInPDFKitView : public Plugin, public WebCore::ScrollableArea {

Not sure why this class has a name that ends with View if it’s a Plugin.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:59
> +    // In-process PDFViews don't support asynchronous initialization.

This comment confuses me. What are “in-process PDFViews”?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:67
> +    // Regular plug-ins don't need access to view, but we add scrollbars to embedding FrameView for proper event handling.

Did you mean “to the embedding”, or maybe “to the enclosing”?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:78
> +    // Plug-in methods

functions?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:108
> +    virtual bool isEditingCommandEnabled(const String& commandName) OVERRIDE;

Don’t need commandName here.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:116
> +    virtual void sendComplexTextInput(const String& textInput);

Don’t need textInput here.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:121
> +    virtual bool getFormValue(String& formValue);

Redundant parameter name.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:124
> +    virtual WebCore::Scrollbar* horizontalScrollbar();
> +    virtual WebCore::Scrollbar* verticalScrollbar();

Terrible (see below)

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:128
> +    // ScrollableArea methods.

functions?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:146
> +    virtual WebCore::Scrollbar* horizontalScrollbar() const OVERRIDE { return m_horizontalScrollbar.get(); }
> +    virtual WebCore::Scrollbar* verticalScrollbar() const OVERRIDE { return m_verticalScrollbar.get(); }

It’s terrible that PlugIn and ScrollableArea declare these function pairs that only differ in constness.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:148
> +    virtual bool shouldSuspendScrollAnimations() const OVERRIDE { return false; } // If we return true, ScrollAnimatorMac will keep cycling a timer forever, waiting for a good time to animate.

Bizarre comment.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:159
> +    WebCore::IntSize m_pluginSize;

Not sure what the word “plugin” adds here, since this class derives from Plugin.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:165
> +    RetainPtr<CFMutableDataRef> m_dataBuffer;

Why not m_data?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:180
> +    RetainPtr<PlatformLayer> m_containerLayer;
> +    RetainPtr<PlatformLayer> m_contentLayer;
> +    RetainPtr<PlatformLayer> m_horizontalScrollbarLayer;
> +    RetainPtr<PlatformLayer> m_verticalScrollbarLayer;
> +    RetainPtr<PlatformLayer> m_scrollCornerLayer;

I’d say CALayer here, for clarity.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:60
> +- (void)updateScrollPosition:(CGPoint) newPosition;

Extra space after the parenthesis.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:61
> +- (void)writeItemsToPasteboard:(NSArray *) items withTypes:(NSArray *)types;

Here too.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:72
> +@interface PDFLayerController (PDFKitSecretsIKnowAbout)

Please use the convention of using the name “Details” for categories such as this one.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:106
> +- (id) currentSelection;

Space after paren again.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:113
> +- (void)setHUDEnabled:(BOOL)enabled;
> +- (BOOL)hudEnabled;

It’s almost as if there is a boolean property here…

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:118
> +using namespace std;

Nope, see <http://www.webkit.org/coding/coding-style.html#using-in-cpp>.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:146
> +- (id<CAAction>)actionForKey:(NSString *)key
> +{
> +    return nil;
> +}

I’m curious: what does the superclass implementation do that we need to override here?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:187
> +    for (unsigned i = 0; i < [items count]; ++i) {

The right type to use as an index into an NSArray is NSUInteger. This needlessly sends the -count message to the array on every iteration through the loop. A better way to write this for statement would be
    for (NSUInteger i = 0, count = items.count; i < count; ++i)

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:188
> +        if ([[types objectAtIndex:i] isEqualToString:NSStringPboardType] || [[types objectAtIndex:i] isEqualToString:NSPasteboardTypeString]) {

Please use a local variable for [types objectAtIndex:i] so that you don’t call it twice.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:192
> +    }

Another way to write this loop would be using -[NSArray enumerateObjectsUsingBlock:].

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:195
> +- (void) showDefinitionForAttributedString: (NSAttributedString *) string atPoint: (CGPoint) point

Now you’re putting extra spaces after parentheses *and* after colons?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:200
> +- (void) performWebSearch: (NSString *) string

More space.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:205
> +- (void) openWithPreview

More.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:210
> +- (void) saveToPDF

And more.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:309
> +

Excellent code, but do we really need a third copy of it in the project? I think it should be moved down to WebCore soon.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:324
> +    , m_horizontalScrollbarLayer(0)
> +    , m_verticalScrollbarLayer(0)

No need to initialize a RetainPtr to 0.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:330
> +    [m_pdfLayerController.get() setDelegate:m_pdfLayerControllerDelegate.get()];
> +    [m_pdfLayerController.get() setParentLayer:m_contentLayer.get()];

Why not use property notation here?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:396
> +        IntRect scrollbarRect(IntRect(pluginView()->x() + m_pluginSize.width() - m_verticalScrollbar->width(), pluginView()->y(), m_verticalScrollbar->width(), m_pluginSize.height()));

Is the inner IntRect necessary?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:416
> +    if (m_verticalScrollbarLayer && m_verticalScrollbar) {

When is it possible for one of these to be 0 and not the other?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:501
> +    controller()->invalidate(IntRect(0, 0, m_pluginSize.width(), m_pluginSize.height()));

IntRect(IntPoint(), m_pluginSize) is shorter.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:542
> +    // Load the src URL if needed.

src? srsly?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:564
> +    [m_horizontalScrollbarLayer.get() removeFromSuperlayer];
> +    [m_verticalScrollbarLayer.get() removeFromSuperlayer];
> +    m_horizontalScrollbarLayer = 0;
> +    m_verticalScrollbarLayer = 0;

This seems redundant after destroyScrollbar().

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:568
> +    [m_containerLayer.get() removeFromSuperlayer];

Why is it this class’s responsibility to remove the container layer from its superlayer?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:575
> +void BuiltInPDFKitView::paintControlForLayerInContext(CALayer *layer, CGContextRef ctx)

I’d call the parameter “context”.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:632
> +    // This should never be called from the web process.

The comment doesn’t say why.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:645
> +        // Nothing to do.

The comment doesn’t say why.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:654
> +    [CATransaction setDisableActions:YES];

You don’t like property notation?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:655
> +    CATransform3D transform = CATransform3DMakeScale(1.0, -1.0, 1.0);

No need for “.0”s here.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:657
> +    CGFloat magnification = pluginView()->fullPagePluginScaleFactor() - [m_pdfLayerController.get() tileScaleFactor];

Subtracting scale factors is weird!

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:798
> +    static bool s_isDown = false;

We don’t use the s_ prefix for function statics.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:811
> +    if (!eventType)
> +        return false;
> +

I guess you can do that before computing the modifier flags.

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:991
> +    return IntRect(0, 0, m_pdfDocumentSize.width(), m_pdfDocumentSize.height());

IntRect(IntPoint(), m_pdfDocumentSize)?

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:996
> +    m_scrollOffset = IntSize(offset.x(), offset.y());

You can use toSize(offset).

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:997
> +    [m_pdfLayerController.get() setScrollPosition:CGPointMake(offset.x(), offset.y())];

You can use IntPoint’s operator CGPoint().

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:1063
> +    return IntPoint(m_scrollOffset.width(), m_scrollOffset.height());

You can use toPoint(m_scrollOffset).

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:1068
> +    return IntPoint(0, 0);

You can write IntPoint().

> Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:1202
> +

As much as I like these JSPDFDoc functions, having three copies of them in WebKit isn’t great. It would be good to move them to WebCore soon.

> Source/WebKit2/WebProcess/Plugins/PluginProxy.cpp:426
> +    bool isEnabled = false;

The variable name should be different.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:633
> +    if (frame->document()->isPluginDocument()) {

I’d write this with an early return.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:650
> +    PluginView* pluginView = pluginViewForFrame(frame);
> +    
> +    if (pluginView) {

You can define the local in the scope of the if statement.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:666
> +    PluginView* pluginView = pluginViewForFrame(frame);
> +    
> +    if (pluginView)

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1580
> +        PluginView* pluginView = pluginViewForFrame(frame);
> +        
> +        if (pluginView)

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:600
> +    bool pdfKitPluginEnabled() const { return m_pdfKitPluginEnabled; }
> +    void setPDFKitPluginEnabled(bool enabled) { m_pdfKitPluginEnabled = enabled; }

Why is this not inside a PLATFORM(MAC) section?

> Source/WebKit2/WebProcess/WebPage/WebPage.h:808
> +    bool m_pdfKitPluginEnabled;

And this?

[Tim Horton]

(In reply to comment #10)
> I don’t like this new name. The BuiltInSimplePDFView class primarily inherits from Plugin. Why isn’t it called SimplePDFPlugin?

I would like that name a lot better, actually. PDFPlugin and SimplePDFPlugin?

> > Source/WebKit2/ChangeLog:129
> > +        * WebProcess/Plugins/PluginView.h:
> > +        (WebKit::PluginView::shouldUseFullPagePluginScaling):
> > +        Report whether or not the Plugin backed by this PluginView wants to handle scaling itself.
> 
> I think a better name for this function is one that starts with “handles”. It’s hard to come up with a name that doesn’t contain the word “page” twice if you want to express both that this is about the page scale and about full-page plug-ins. I think you could omit the full-page plug-in bit and call this handlesPageScaleFactor.

That would make it fit in with some others, too. I like it.

> > Source/WebKit2/ChangeLog:140
> > +        This PageScaleFactorDidChange prevents other code from using non-1.0 scale factors as the default.
> 
> What is “This PageScaleFactorDidChange”? This is really confusing.

Yeah that whole part of the change is a bit confusing. I'll investigate.

> > Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:228
> > +// Defaults to false
> > +WK_EXPORT void WKPreferencesSetPDFKitPluginEnabled(WKPreferencesRef preferencesRef, bool enabled);
> > +WK_EXPORT bool WKPreferencesGetPDFKitPluginEnabled(WKPreferencesRef preferencesRef);
> 
> The convention used to be that in the header, the parameter is called preferences, not preferencesRef, but I see that it’s been broken countless times.

Yeah, that was just copy/paste. I'll fix my instance.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:52
> > +class BuiltInPDFKitView : public Plugin, public WebCore::ScrollableArea {
> 
> Not sure why this class has a name that ends with View if it’s a Plugin.

I borrowed the name from BuiltInPDFView (which I've renamed to BuiltInSimplePDFView), which has been around for a while.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:59
> > +    // In-process PDFViews don't support asynchronous initialization.
> 
> This comment confuses me. What are “in-process PDFViews”?

Hmm, I assume it means in-WebProcess, as these are (at the moment). I didn't write that comment, but that would also explain it :)

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:67
> > +    // Regular plug-ins don't need access to view, but we add scrollbars to embedding FrameView for proper event handling.
> 
> Did you mean “to the embedding”, or maybe “to the enclosing”?

Also didn't write that comment, but I'm guessing yes.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:78
> > +    // Plug-in methods
> 
> functions?

Also didn't write that comment :) Probably I'll just remove it.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:146
> > +    virtual WebCore::Scrollbar* horizontalScrollbar() const OVERRIDE { return m_horizontalScrollbar.get(); }
> > +    virtual WebCore::Scrollbar* verticalScrollbar() const OVERRIDE { return m_verticalScrollbar.get(); }
> 
> It’s terrible that PlugIn and ScrollableArea declare these function pairs that only differ in constness.

Yeah! But I don't think I should fix that here.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:148
> > +    virtual bool shouldSuspendScrollAnimations() const OVERRIDE { return false; } // If we return true, ScrollAnimatorMac will keep cycling a timer forever, waiting for a good time to animate.
> 
> Bizarre comment.

Not my comment, again :) All this is making me even more convinced that whatever-I-name-BuiltInPDFKitView should inherit from whatever-I-name-BuiltInSimplePDFView.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:72
> > +@interface PDFLayerController (PDFKitSecretsIKnowAbout)
> 
> Please use the convention of using the name “Details” for categories such as this one.

Ahh, someone mentioned the IKnowAbout convention earlier this week, but it looks like it's only used in one or two places. Details is fine with me.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:113
> > +- (void)setHUDEnabled:(BOOL)enabled;
> > +- (BOOL)hudEnabled;
> 
> It’s almost as if there is a boolean property here…

These are copied straight from the header for the SPI; I don't think I should change things like that? (except maybe I should change the SPI).

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:146
> > +- (id<CAAction>)actionForKey:(NSString *)key
> > +{
> > +    return nil;
> > +}
> 
> I’m curious: what does the superclass implementation do that we need to override here?

Crazy fades and slides and zooms and things. This is a very common approach to disabling all of CoreAnimation's implicit animation magic.

> 
> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:309
> > +
> 
> Excellent code, but do we really need a third copy of it in the project? I think it should be moved down to WebCore soon.

I was wondering the same thing. Maybe it should! Where should I put it?

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:330
> > +    [m_pdfLayerController.get() setDelegate:m_pdfLayerControllerDelegate.get()];
> > +    [m_pdfLayerController.get() setParentLayer:m_contentLayer.get()];
> 
> Why not use property notation here?

I can do that with RetainPtr? I was having trouble. I will try again!

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:416
> > +    if (m_verticalScrollbarLayer && m_verticalScrollbar) {
> 
> When is it possible for one of these to be 0 and not the other?

It isn't anymore.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:542
> > +    // Load the src URL if needed.
> 
> src? srsly?

Not my comment :\ Will fix.

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:568
> > +    [m_containerLayer.get() removeFromSuperlayer];
> 
> Why is it this class’s responsibility to remove the container layer from its super layer?

This is the class that creates, owns, and inserts the container layer into its superlayer, so why wouldn't it also be the one to remove it? Maybe container is the wrong word (it's the layer that contains the content layer which is handed to PDFKit *and* the scrollbars).

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:632
> > +    // This should never be called from the web process.
> 
> The comment doesn’t say why.
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:645
> > +        // Nothing to do.
> 
> The comment doesn’t say why.

Ugh, should I fix all these comments in both BuiltInPDFKitView and BuiltInSimplePDFView, or should I leave Simple alone?

> > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:657
> > +    CGFloat magnification = pluginView()->fullPagePluginScaleFactor() - [m_pdfLayerController.get() tileScaleFactor];
> 
> Subtracting scale factors is weird!

Everything about the magnification stuff is weird. But PDFKit adds the magnifyWithMagnification: value to tileScaleFactor, so if I want to get from where I am to the target (from tileScaleFactor to fullPagePluginScaleFactor), I have to subtract! This is probably wrong, but it's wrong everywhere.

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:600
> > +    bool pdfKitPluginEnabled() const { return m_pdfKitPluginEnabled; }
> > +    void setPDFKitPluginEnabled(bool enabled) { m_pdfKitPluginEnabled = enabled; }
> 
> Why is this not inside a PLATFORM(MAC) section?
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:808
> > +    bool m_pdfKitPluginEnabled;
> 
> And this?

Excellent questions.

[Tim Horton]

Created attachment 168567 [details]
patch with most of dan's comments

[Tim Horton]

Created attachment 168568 [details]
attached the wrong patch

[Tim Horton]

Pretend that I BuiltInPDFKitView->PDFPlugin'd in the ChangeLog bug titles too.

[WebKit Review Bot]

Attachment 168568 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/cg/PDFDocumentScripting.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/cg/PDFDocumentScripting.h:43:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/PDFDocumentScripting.h:43:  The parameter name "initialize" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/PDFDocumentScripting.h:43:  The parameter name "finalize" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/PDFDocumentScripting.h:44:  The parameter name "pdfDocument" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 168569 [details]
stylebot

(In reply to comment #11)
> (In reply to comment #10)
> > I don’t like this new name. The BuiltInSimplePDFView class primarily inherits from Plugin. Why isn’t it called SimplePDFPlugin?
> 
> I would like that name a lot better, actually. PDFPlugin and SimplePDFPlugin?

I like these names. I wonder if people would mind the loss of “built-in”. I don’t think there’s any real risk of people confusing PDFPlugin with some plug-in that’s implemented outside of WebKit.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:146
> > > +    virtual WebCore::Scrollbar* horizontalScrollbar() const OVERRIDE { return m_horizontalScrollbar.get(); }
> > > +    virtual WebCore::Scrollbar* verticalScrollbar() const OVERRIDE { return m_verticalScrollbar.get(); }
> > 
> > It’s terrible that PlugIn and ScrollableArea declare these function pairs that only differ in constness.
> 
> Yeah! But I don't think I should fix that here.

Agreed.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:148
> > > +    virtual bool shouldSuspendScrollAnimations() const OVERRIDE { return false; } // If we return true, ScrollAnimatorMac will keep cycling a timer forever, waiting for a good time to animate.
> > 
> > Bizarre comment.
> 
> Not my comment, again :) All this is making me even more convinced that whatever-I-name-BuiltInPDFKitView should inherit from whatever-I-name-BuiltInSimplePDFView.

It does look like there’s a lot of duplicated code.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:72
> > > +@interface PDFLayerController (PDFKitSecretsIKnowAbout)
> > 
> > Please use the convention of using the name “Details” for categories such as this one.
> 
> Ahh, someone mentioned the IKnowAbout convention earlier this week, but it looks like it's only used in one or two places. Details is fine with me.

“<framework>SecretsIKnowAbout” used to be what such categories were named many years ago. It’s good to know that it’s almost phased out.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:113
> > > +- (void)setHUDEnabled:(BOOL)enabled;
> > > +- (BOOL)hudEnabled;
> > 
> > It’s almost as if there is a boolean property here…
> 
> These are copied straight from the header for the SPI; I don't think I should change things like that? (except maybe I should change the SPI).

Probably not.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:309
> > > +
> > 
> > Excellent code, but do we really need a third copy of it in the project? I think it should be moved down to WebCore soon.
> 
> I was wondering the same thing. Maybe it should! Where should I put it?

We’ll have to think about this some more. Anyway, it shouldn’t be part of this change.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:568
> > > +    [m_containerLayer.get() removeFromSuperlayer];
> > 
> > Why is it this class’s responsibility to remove the container layer from its super layer?
> 
> This is the class that creates, owns, and inserts the container layer into its superlayer, so why wouldn't it also be the one to remove it? Maybe container is the wrong word (it's the layer that contains the content layer which is handed to PDFKit *and* the scrollbars).

I must have missed the place where m_containerLayer gets inserted into its superlayer.

> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:632
> > > +    // This should never be called from the web process.
> > 
> > The comment doesn’t say why.
> > 
> > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:645
> > > +        // Nothing to do.
> > 
> > The comment doesn’t say why.
> 
> Ugh, should I fix all these comments in both BuiltInPDFKitView and BuiltInSimplePDFView, or should I leave Simple alone?

Leave Simple alone!

[Tim Horton]

(In reply to comment #17)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > I don’t like this new name. The BuiltInSimplePDFView class primarily inherits from Plugin. Why isn’t it called SimplePDFPlugin?
> > 
> > I would like that name a lot better, actually. PDFPlugin and SimplePDFPlugin?
> 
> I like these names. I wonder if people would mind the loss of “built-in”. I don’t think there’s any real risk of people confusing PDFPlugin with some plug-in that’s implemented outside of WebKit.

Seems unlikely. Alexey may have a comment.

> > > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.h:148
> > > > +    virtual bool shouldSuspendScrollAnimations() const OVERRIDE { return false; } // If we return true, ScrollAnimatorMac will keep cycling a timer forever, waiting for a good time to animate.
> > > 
> > > Bizarre comment.
> > 
> > Not my comment, again :) All this is making me even more convinced that whatever-I-name-BuiltInPDFKitView should inherit from whatever-I-name-BuiltInSimplePDFView.
> 
> It does look like there’s a lot of duplicated code.

Perhaps I should try it and see how messy things get.

> > > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:309
> > > > +
> > > 
> > > Excellent code, but do we really need a third copy of it in the project? I think it should be moved down to WebCore soon.
> > 
> > I was wondering the same thing. Maybe it should! Where should I put it?
> 
> We’ll have to think about this some more. Anyway, it shouldn’t be part of this change.

Heh, whoops. I moved it into "PDFDocumentScripting" (a horrible name, I think), in WebCore, right next to PDFDocumentImage (not sure this is the right place, since this is in graphics). I can undo that.

> > > > Source/WebKit2/WebProcess/Plugins/PDF/BuiltInPDFKitView.mm:568
> > > > +    [m_containerLayer.get() removeFromSuperlayer];
> > > 
> > > Why is it this class’s responsibility to remove the container layer from its super layer?
> > 
> > This is the class that creates, owns, and inserts the container layer into its superlayer, so why wouldn't it also be the one to remove it? Maybe container is the wrong word (it's the layer that contains the content layer which is handed to PDFKit *and* the scrollbars).
> 
> I must have missed the place where m_containerLayer gets inserted into its superlayer.

Crap, you're right :)

[Tim Horton]

Created attachment 168579 [details]
Undo PDFDocumentScripting changes, and PDFPlugin can inherit from SimplePDFPlugin and be a lot smaller.

Dan, could you take another peek at some point?

[Tim Horton]

Created attachment 168580 [details]
remove more remnants of pdfdocumentscripting

Comment on attachment 168580 [details]
remove more remnants of pdfdocumentscripting

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

The only serious issue is the one in PDFPlugin::destroyScrollbar(). r=me if you fix that (or I’m wrong and it’s not an issue), but you’re welcome to address my many other comments before landing.

> Source/WebKit2/ChangeLog:47
> +        it allows us to have a preference to toggle between UIProcess' PDFView and PDFPlugin).

UIProcess’s

> Source/WebKit2/ChangeLog:126
> +        (WebKit::PDFPlugin::handleMouseEvent): Construct a custom NSEvent from the given WebMouseEvent and hand it to PDFLayerController. Mouse coordinates are in terms of m_contentLayer's origin.
> +        (WebKit::PDFPlugin::handleKeyboardEvent): Construct a custom NSEvent from the given WebKeyboardEvent and hand it to PDFLayerController. PDFLayerController currently only handles keyDown events.

NSEvent documentation uses the term “custom event” to describe an instance returned from +otherEventWithType:location:modifierFlags:timestamp:windowNumber:context:subtype:data1:data2:. The events you’re creating here are normal mouse events and keyboard events.

> Source/WebKit2/ChangeLog:130
> +        (WebKit::PDFPlugin::isEditingCommandEnabled): The 'copy' command should be enabled if there is
> +        the user has selected a part of the PDF. The 'select all' command should always be enabled.

Typo: “there is the user”.

> Source/WebKit2/PluginProcess/PluginControllerProxy.cpp:537
> +void PluginControllerProxy::handlesPageScaleFactor(bool& enabled)
> +{
> +    enabled = m_plugin->handlesPageScaleFactor();
> +}

I’d call the boolean something else.

> Source/WebKit2/PluginProcess/PluginControllerProxy.h:142
> +    void isEditingCommandEnabled(const String& commandName, bool& handled);
> +    void handlesPageScaleFactor(bool& enabled);

You can drop the parameter names here.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:211
> +    virtual bool handleEditingCommand(const String& commandName, const String& argument);
> +    virtual bool isEditingCommandEnabled(const String&);
> +    
> +    virtual bool handlesPageScaleFactor();
> +

OVERRIDE OVERRIDE OVERRIDE?

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

Why do you need to override this? The base class also returns false.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:78
> +    virtual void geometryDidChange(const WebCore::IntSize& pluginSize, const WebCore::IntRect& clipRect, const WebCore::AffineTransform& pluginToRootViewTransform)OVERRIDE;

Missing space beforeOVERRIDE.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:91
> +    JSObjectRef makeJSPDFDoc(JSContextRef);
> +    static JSValueRef jsPDFDocPrint(JSContextRef, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception);

These are not implemented in PDFPlugin.mm. Where are they implemented?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:97
> +    RetainPtr<CALayer *> m_containerLayer;
> +    RetainPtr<CALayer *> m_contentLayer;
> +    RetainPtr<CALayer *> m_horizontalScrollbarLayer;
> +    RetainPtr<CALayer *> m_verticalScrollbarLayer;
> +    RetainPtr<CALayer *> m_scrollCornerLayer;

Should be RetainPtr<CALayer>.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:106
> +- (bool)keyDown:(NSEvent *)event;

Strange that this returns a bool rather than a BOOL.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:297
> +    scrollbar = 0;

This just clears the local variable. Did you mean scrollbar to be a reference? I think you did.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:311
> +    pluginView()->setPageScaleFactor([m_pdfLayerController.get() tileScaleFactor], IntPoint(0, 0));

IntPoint() will do.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:480
> +    static bool mouseButtonIsDown = false;

No need to initialize a static to false.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:561
> +    if (commandName == "copy" && [m_pdfLayerController.get() currentSelection])
> +        return true;

You always want to return here if commandName == "copy", not continue to check commandName against other strings:

    if (commandName == "copy")
        return [m_pdfLayerController.get() currentSelection];

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1110
> +    

!

Comment on attachment 168580 [details]
remove more remnants of pdfdocumentscripting

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

Added another important comment (about clearing the delegate)

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:228
> +    m_pdfLayerController.get().delegate = m_pdfLayerControllerDelegate.get();

Even though PDFLayerController (strangely) retains its delegate, you should clear the delegate in destroy() (or in the destructor?), because WKPDFLayerControllerDelegate has a bare pointer to PDFPlugin.

[Tim Horton]

Comment on attachment 168580 [details]
remove more remnants of pdfdocumentscripting

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

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:297
>> +    scrollbar = 0;
> 
> This just clears the local variable. Did you mean scrollbar to be a reference? I think you did.

Whoops, I did, and it was, but I screwed that up during the inherit-from-SimplePDFPlugin mess yesterday.

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:480
>> +    static bool mouseButtonIsDown = false;
> 
> No need to initialize a static to false.

Hmm, I didn't know that.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1110
>> +    
> 
> !

!

Thanks, Dan!

[Tim Horton]

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

[Joseph Pecoraro]

Comment on attachment 168580 [details]
remove more remnants of pdfdocumentscripting

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

> Source/WebKit2/Configurations/FeatureDefines.xcconfig:123
> +ENABLE_PDFKIT_PLUGIN = $(ENABLE_PDFKIT_PLUGIN_$(REAL_PLATFORM_NAME));
> +ENABLE_PDFKIT_PLUGIN_macosx = $(ENABLE_PDFKIT_PLUGIN_macosx_$(TARGET_MAC_OS_X_VERSION_MAJOR));
> +ENABLE_PDFKIT_PLUGIN_macosx_1070 = ;
> +ENABLE_PDFKIT_PLUGIN_macosx_1080 = ;
> +ENABLE_PDFKIT_PLUGIN_macosx_1090 = ENABLE_PDFKIT_PLUGIN;

Just a heads up, we try to keep all JavaScriptCore / WebCore / WebKit / WebKit2 FeatureDefines.xcconfig files in sync. That prevents accidents where an ENABLE flag is enabled while building one project but not the others. Even if the feature define is only used in one project, it is easier to keep the files identical then have mismatches.

I'll add this to the other FeatureDefine files in some clean-up I'm doing. Ideally this would be more automatically enforced somehow.

[Simon Fraser (smfr)]

Why don' t we just share one FeatureDefines file between projects?