[webkit-reviews] review denied: [Bug 99206] [wk2] Implement BuiltInPDFKitView : [Attachment 168497] whole patch again, with changelogs this time

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 12 23:29:19 PDT 2012 

mitz at webkit.org <mitz at webkit.org> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 99206: [wk2] Implement BuiltInPDFKitView
https://bugs.webkit.org/show_bug.cgi?id=99206

Attachment 168497: whole patch again, with changelogs this time
https://bugs.webkit.org/attachment.cgi?id=168497&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
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
> + at 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?

More information about the webkit-reviews mailing list