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

Mon Nov 28 00:29:04 PST 2011

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

Daniel Bates <dbates at webkit.org> changed:

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

--- Comment #5 from Daniel Bates <dbates at webkit.org>  2011-11-28 00:29:04 PST ---
(From update of attachment 116702)
View in context: https://bugs.webkit.org/attachment.cgi?id=116702&action=review

> Source/WebCore/plugins/blackberry/PluginPackageBlackBerry.cpp:191
> +unsigned int PluginPackage::hash() const

Nit: unsigned int => unsigned.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:32
> +#include "Document.h"

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

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

This include is unnecessary as PluginView.h (included above) includes this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:40
> +#include "FrameLoader.h"
> +#include "FrameTree.h"

These includes are unnecessary as Frame.h (included above) includes these files.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:54
> +#include "PlatformMouseEvent.h"

This include is unnecessary as EventHandler.h (included by Frame.h above) includes this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:59
> +#include "ScriptController.h"

This include is unnecessary as Frame.h (included above) includes this file.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:88
> +// -------

This comment doesn't add any value. Please remove it.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:90
> +class PthreadMutexLocker {
> +public:

Nit: We are underutilizing the idea of a class here since all of the member functions and instance variables are public in this class. It's sufficient to make this a struct and then we can remove the "public:" line.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:91
> +    PthreadMutexLocker(pthread_mutex_t* mutex): m_mutex(mutex)

The initialization list should be on its own line and indented. For an example of this, see section "Other Punctuation" of <http://www.webkit.org/coding/coding-style.html>.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:105
> +class PthreadReadLocker {
> +public:

Nit: We are underutilizing the idea of a class here since all of the member functions and instance variables are public in this class. It's sufficient to make this a struct and then we can remove the "public:" line.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:106
> +    PthreadReadLocker(pthread_rwlock_t* rwlock): m_rwlock(rwlock)

The initialization list should be on its own line and indented. For an example of this, see section "Other Punctuation" of <http://www.webkit.org/coding/coding-style.html>.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:120
> +class PthreadWriteLocker {
> +public:

Nit: We are underutilizing the idea of a class here since all of the member functions and instance variables are public in this class. It's sufficient to make this a struct and then we can remove the "public:" line.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:121
> +    PthreadWriteLocker(pthread_rwlock_t* rwlock): m_rwlock(rwlock)

The initialization list should be on its own line and indented. For an example of this, see section "Other Punctuation" of <http://www.webkit.org/coding/coding-style.html>.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:141
> +void npSetHolePunchHandler(void *data)

The '*' should be on the left.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:143
> +    HolePunchData* d = static_cast<HolePunchData*>(data);

Nit: I suggest using OwnPtr here since we are taking ownership of this data. Then we can omit the "delete d" (line 146) at the end of this function.

Also, can we come up with a more descriptive name for this variable? Or at the very least, call it data, instead of 'd'.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:153
> +static NPRect toNPRect(const WebCore::IntRect& rect)

Do we need the WebCore:: prefix here?

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:207
> +    PluginViewPrivate(PluginView* view):
> +        m_view(view)

This should read:

PluginViewPrivate(PluginView* view)
    : m_view(view)

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:231
> +        snprintf(uniqueId, 50, "PluginViewBB-%08x%08x-", s_counter++, (int)this);

Nit: Instead of hardcoding 50 here, we should use sizeof(uniqueId) so as to ensure we restrict snprintf to the size of the buffer, uniqueId.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:402
> +        BlackBerry::Platform::Graphics::Buffer* buffer =
> +            m_pluginBuffers[(m_pluginFrontBuffer + 1) % 2];

Nit: I would write this on one line.

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

Remove this empty line as it doesn't increase the readability of this function body.

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

Remove this empty line as it doesn't increase the readability of this function body.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:456
> +        for (int i = 0; i < 2; i++) {

Nit: Instead of harcoding the size of the m_pluginBuffer array (2) we should use WTF_ARRAY_LENGTH(), which is defined in wtf/StdLibExtras.h.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:500
> +        for (int i = 0; (i < 2) && success; i++) {

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:520
> +        for (int i = 0; i < 2; i++) {

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:537
> +        for (int i = 0; i < 2; i++) {

Ditto.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:542
> +            if (m_pluginBuffers[i]) {
> +                BlackBerry::Platform::Graphics::destroyBuffer(m_pluginBuffers[i]);
> +                m_pluginBuffers[i] = 0;
> +            }
> +        }

Nit: The code in this for-loop is identical to code in the for-loop at line 456. We should look to extract this code into a common subroutine.

> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:551
> +// -------

This comment is inane. Please remove it.

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