[webkit-reviews] review denied: [Bug 73397] [BlackBerry] Upstream BlackBerry porting of pluginView : [Attachment 117311] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 73397: [BlackBerry] Upstream BlackBerry porting of pluginView
https://bugs.webkit.org/show_bug.cgi?id=73397

Attachment 117311: Patch
https://bugs.webkit.org/attachment.cgi?id=117311&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=117311&action=review


> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:33
> +#include "Element.h"

This include is unnecessary as HTMLPlugInElement.h (included below) includes
HTMLFrameOwnerElement.h => HTMLElement.h => StyledElement.h, which includes
this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:38
> +#include "GraphicsContext.h"

This include is unnecessary as PlatformContextSkia.h (included below) includes
this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:42
> +#include "Image.h"

This file is unnecessary as PlatformContextSkia.h (included below)  includes
GraphicsContext.h, which includes this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:57
> +#include "Touch.h"

This file is unnecessary as TouchList.h (below) includes this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:63
> +#include <BlackBerryPlatformWindow.h>

We should move this include to after line 73 so that it's grouped with the
other BlackBerry platform headers.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:81
> +#define UNINIT_COORD 0xffffffff

This should be a C++ constant and shouldn't have an underscore in its name:

const unsigned UninitializedCoordinate = 0xffffffff;

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:103
> +static NPRect toNPRect(const IntRect& rect)
> +{
> +    NPRect npRect;
> +    npRect.top = rect.y();
> +    npRect.left = rect.x();
> +    npRect.bottom = rect.y() + rect.height();
> +    npRect.right = rect.x() + rect.width();
> +    return npRect;
> +}
> +
> +static NPRect toNPRect(const BlackBerry::Platform::IntRect& rect)
> +{
> +    NPRect npRect;
> +    npRect.top = rect.y();
> +    npRect.left = rect.x();
> +    npRect.bottom = rect.y() + rect.height();
> +    npRect.right = rect.x() + rect.width();
> +    return npRect;
> +}

This is OK AS-IS. That being said, these functions are identical up to the
datatype of their argument. Can we make this a template function?

Also, the name of this function doesn't conform to the WebKit Code Style
Guidelines per (8) of section Names. We can rename this function in separate
bug.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:106
> +// Standard plugin widget stuff.

Remove this comment. It's inane.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:146
> +    // do not call setNPWindowIfNeeded immediately, will be called on
paint()

Nit: do => Do

Also, this comment is missing a period.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:149
> +    // (i) in order to move/resize the plugin window at the same time as the


Nit: in => In

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:154
> +    // (ii) if we are running layout tests from DRT, paint() won't ever get
called

if => If

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:155
> +    // so we need to call setNPWindowIfNeeded() if window geometry has
changed

Missing a period at the end of this sentence.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:312
> +    double zoomFactorH = (double)m_private->m_pluginBufferSize.width() /
(double)frameRect().width();
> +    double zoomFactorW = (double)m_private->m_pluginBufferSize.height() /
(double)frameRect().height();

Nit: We should use C++ style casts.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:428
> +    // sanity check.

Remove this comment. It's inane. I take it this "sanity check" is unusual to do
since we have this comment. It would have been better if the comment explained
why? Regardless, we should just remove this comment.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:470
> +    if (platformKeyEvent->type() == PlatformKeyboardEvent::RawKeyDown
> +	       || platformKeyEvent->type() == PlatformKeyboardEvent::KeyDown) {


Nit: The indentation here is off. The "||" should be left-aligned with the 'p'
in platformKeyEvent on line 469.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:532
> +	   return;

This looks weird.  We don't need to fix this now, but we should eventually come
back to this code. What other mouse events are there? Does control ever reach
here? Maybe this should be an ASSERT_NOT_REACHED().

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:545
> +	   npTouchEvent.points = new NPTouchPoint[touchList->length()];

Can we use OwnArrayPtr here? Then we can remove the delete[] on line 576.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:951
> +void PluginView::setNPWindowRect(const IntRect& rc)

This argument is unused and depending on our compiler warning level may raise a
warning. We should just remove the argument name.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:987
> +	   // FLASH WORKAROUND: Only set initially. Multiple calls to
> +	   // setNPWindow() cause the plugin to crash in windowed mode.

This should read:

// Only set the width and height of the plugin content the first time
setNPWindow() is called so as to workaround
// an issue in Flash where multiple calls to setNPWindow() cause the plugin to
crash in windowed mode.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1000
> +    BlackBerry::Platform::Graphics::Window *window =
> +	   frameView->hostWindow()->platformPageClient()->platformWindow();

I would write this on one line to improve readability.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1007
> +    // FIXME: Passing zoomFactor to setwindow make windowed plugin scale
incorrectly.

Nit: I suggest adding an empty line above this FIXME comment so as to make it
stand out.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1013
> +    setCallingPlugin(false);

Nit: I suggest adding a empty line above this line so as to demarcate the end
of the zoom factor code above.

We should define a RAII object that calls setCallingPlugin(true) in its
constructor and calls setCallingPlugin(false) in its destructor. We can do this
in a follow up patch.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1025
> +    //	since the window manager should take care of that for us.

Nit: Remove the leading whitespace (before the word "since") in this comment
such that there is exactly one space after the "//" in this line.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1039
> +    FILE* fileHandle = fopen((filename.utf8()).data(), "r");

Nit: (filename.utf8()) => filename.utf8()

That is, the parentheses around "filename.utf8()" aren't necessary.