[Webkit-unassigned] [Bug 73185] [blackberry] Upstream BlackBerry porting of plugin framework

Mon Nov 28 20:14:47 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=73185

Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116868|review?                     |review-
               Flag|                            |

--- Comment #7 from Daniel Bates <dbates at webkit.org>  2011-11-28 20:14:47 PST ---
(From update of attachment 116868)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.