[webkit-reviews] review granted: [Bug 99206] [wk2] Implement PDFPlugin : [Attachment 168580] remove more remnants of pdfdocumentscripting

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 14 10:08:05 PDT 2012


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

Attachment 168580: remove more remnants of pdfdocumentscripting
https://bugs.webkit.org/attachment.cgi?id=168580&action=review

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

!


More information about the webkit-reviews mailing list