[webkit-reviews] review denied: [Bug 73185] [blackberry] Upstream BlackBerry porting of plugin framework : [Attachment 116868] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 28 20:14:47 PST 2011


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

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

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


We need to iterate on these files and clean them up. Notice,
PluginViewBlackBerry.cpp is almost 2000 lines long. We need to look to break
out the functionality into this file into separate files as well as look to
decompose some of the larger functions in this file, say paint(), into smaller
functions so as to improve the readability of our plugin code. This refactoring
can be done in separate bugs.

> Source/WebCore/ChangeLog:8
> +	   This patch is to upstream BlackBerry porting of Plugin framework.

This change log is very long. I suggest removing the list of functions and just
leave the list of added files since this patch is only adding files.

> Source/WebCore/plugins/blackberry/PluginDataBlackBerry.cpp:44
> +

Nit: I suggest removing this empty line since it doesn't improve the
readability of this for-loop.

> Source/WebCore/plugins/blackberry/PluginDataBlackBerry.cpp:48
> +

Ditto.

> Source/WebCore/plugins/blackberry/PluginDataBlackBerry.cpp:51
> +

Nit: I suggest removing this empty line since it doesn't improve the
readability of this function.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:546
> +   
///////////////////////////////////////////////////////////////////////////////
/
> +    // Static NPAPI Callbacks
> +   
///////////////////////////////////////////////////////////////////////////////
/

Nit: This kind of stylized comment is excessive. Also, the use of the word
"static" seems a bit disingenuous here. From my understanding, functions within
an anonymous namespace have external linkage compared to static functions which
have internal linkage. I take it that the word "static" here is meant to
describe the non-member, non-friend quality of these functions. Regardless, I
don't think the word "static" provides much value and hence I suggest removing
it.

Therefore, I would change this comment block so that it reads:

// NPAPI callbacks

OR

// NPAPI callback functions

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:549
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Can we use static_cast?

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:556
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:563
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:570
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:577
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:584
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:591
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:598
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:605
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:612
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:619
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:626
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:633
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:640
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:647
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:654
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:661
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:668
> +	   WebCore::PluginView* pv = (WebCore::PluginView *)instance->ndata;

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:674
> +    {

This brace should be on the previous line (line 673).

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:753
> +    if (m_clipRect.isEmpty()
> +	   || (platformPluginWidget() && (m_windowRect != oldWindowRect ||
m_clipRect != oldClipRect || zoomFactorChanged)))
> +	   setNPWindowIfNeeded();

Because the condition expression of this if-statement spans multiple lines, we
need to use braces for this if-statement.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:831
> +void PluginView::paint(GraphicsContext* context, const IntRect& rect)

Wow. This function is 179 lines long. We should look to break out the painting
into subroutines. We can do this in a follow up patch.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1034
> +    setCallingPlugin(true);
> +
> +    bool accepted = m_plugin->pluginFuncs()->event(m_instance, &event);
> +
> +    setCallingPlugin(false);

This pattern of calling setCallingPlugin(true) and setCallingPlugin(false)
comes up throughout this patch. Instead of balancing setCallingPlugin(true) and
setCallingPlugin(false) we should define a RAII object that on construction
calls setCallingPlugin(true) and on destruction calls setCallingPlugin(false).
We can do this in a separate bug.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1044
> +    const PlatformKeyboardEvent *platformKeyEvent = event->keyEvent();

The '*' should be on the left.

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


Nit: I would just write this on one line. Then we can remove the curly braces.

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

We should use OwnArrayPtr here. Then we can remove the delete[] on line 1172.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1800
> +    IntRect r(rect->left, rect->top, rect->right - rect->left, rect->bottom
- rect->top);
> +    invalidateRect(r);

Nit: The variable r is only being referenced on line 1800. I suggest we just
inline the IntRect constructor call into line 1800.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1920
> +    NPScreenWindowHandles* screenWinHandles =
(NPScreenWindowHandles*)valPtr;

We should use static_cast instead of C-style casts.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1939
> +    BlackBerry::Platform::Graphics::Buffer* buffer =
> +	   m_private->lockReadFrontBufferInternal();

Nit: I would write this on one line.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1951
> +

Nit: I suggest removing this empty line since it doesn't help improve the
readability of this function body, which is already short.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1960
> +

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1969
> +

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1978
> +    if (m_private->m_isBackgroundPlaying != value) {

Nit: I suggest using an early-return style here so that we can remove one level
of indentation.


More information about the webkit-reviews mailing list