[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