Bug 73185 - [blackberry] Upstream BlackBerry porting of plugin framework
: [blackberry] Upstream BlackBerry porting of plugin framework
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 73513
: 73144 73397
  Show dependency treegraph
 
Reported: 2011-11-27 22:26 PST by
Modified: 2011-11-30 19:53 PST (History)


Attachments
Patch (78.76 KB, patch)
2011-11-27 23:17 PST, Charles Wei
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.69 KB, patch)
2011-11-28 19:11 PST, Charles Wei
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.45 KB, patch)
2011-11-28 20:47 PST, Charles Wei
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2011-11-28 22:04 PST, Charles Wei
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.45 KB, patch)
2011-11-29 18:50 PST, Charles Wei
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.38 KB, patch)
2011-11-29 21:59 PST, Charles Wei
tonikitoo: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-27 22:26:36 PST
This bug is to upstream BlackBerry's porting of plugin-framework.
------- Comment #1 From 2011-11-27 23:17:13 PST -------
Created an attachment (id=116702) [details]
Patch
------- Comment #2 From 2011-11-27 23:24:30 PST -------
(From update of attachment 116702 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=116702&action=review

Random drive by nit:

> Source/WebCore/plugins/blackberry/PluginPackageBlackBerry.cpp:84
> +    NPP_GetMIMEDescriptionProcPtr gm = (NPP_GetMIMEDescriptionProcPtr) dlsym(m_module, "NP_GetMIMEDescription");
> +    NPP_GetValueProcPtr gv = (NPP_GetValueProcPtr) dlsym(m_module, "NP_GetValue");

This variable names are pretty short.  Usually we like complete words in variable names.
------- Comment #3 From 2011-11-27 23:25:10 PST -------
(Mostly I was just curious about this patch and that nit jumped out at me; please feel free to ignore me.)
------- Comment #4 From 2011-11-27 23:31:30 PST -------
(In reply to comment #3)
> (Mostly I was just curious about this patch and that nit jumped out at me; please feel free to ignore me.)

Thanks, Adam. I will collect all the comments and submit another patch to address them. :-)
------- Comment #5 From 2011-11-28 00:29:04 PST -------
(From update of attachment 116702 [details])
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.
------- Comment #6 From 2011-11-28 19:11:49 PST -------
Created an attachment (id=116868) [details]
Patch
------- Comment #7 From 2011-11-28 20:14:47 PST -------
(From update of attachment 116868 [details])
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.
------- Comment #8 From 2011-11-28 20:47:05 PST -------
Created an attachment (id=116879) [details]
Patch
------- Comment #9 From 2011-11-28 20:55:00 PST -------
(From update of attachment 116879 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=116879&action=review

> Source/WebCore/plugins/blackberry/PluginPackageBlackBerry.cpp:103
> +    String s = gm();

's' does not have much meaning.

nor does 'gm'

> Source/WebCore/plugins/blackberry/PluginPackageBlackBerry.cpp:110
> +            Vector<String> exts;

ditto with 'exts'
------- Comment #10 From 2011-11-28 22:04:00 PST -------
Created an attachment (id=116892) [details]
Patch
------- Comment #11 From 2011-11-28 22:13:32 PST -------
(From update of attachment 116892 [details])
Thanks Charles.
------- Comment #12 From 2011-11-28 23:39:55 PST -------
(From update of attachment 116892 [details])
Clearing flags on attachment: 116892

Committed r101331: <http://trac.webkit.org/changeset/101331>
------- Comment #13 From 2011-11-28 23:40:00 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 2011-11-29 18:39:35 PST -------
Re-open it for more patches to come.
------- Comment #15 From 2011-11-29 18:50:47 PST -------
Created an attachment (id=117097) [details]
Patch
------- Comment #16 From 2011-11-29 19:17:10 PST -------
(From update of attachment 117097 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=117097&action=review

> Source/WebCore/plugins/blackberry/PluginViewHelperBlackBerry.cpp:20
> +#include "PluginViewHelperBlackBerry.h"

HelperXXX files are not recomended in WebKit.
------- Comment #17 From 2011-11-29 19:19:34 PST -------
(In reply to comment #16)
> (From update of attachment 117097 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117097&action=review
> 
> > Source/WebCore/plugins/blackberry/PluginViewHelperBlackBerry.cpp:20
> > +#include "PluginViewHelperBlackBerry.h"
> 
> HelperXXX files are not recomended in WebKit.

Thanks, Antonio.  What do you suggest we should name it  ?
------- Comment #18 From 2011-11-29 20:43:58 PST -------
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 117097 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117097&action=review
> > 
> > > Source/WebCore/plugins/blackberry/PluginViewHelperBlackBerry.cpp:20
> > > +#include "PluginViewHelperBlackBerry.h"
> > 
> > HelperXXX files are not recomended in WebKit.
> 
> Thanks, Antonio.  What do you suggest we should name it  ?

Maybe NPCallbacksBlackBerry, Charles?
------- Comment #19 From 2011-11-29 21:59:19 PST -------
Created an attachment (id=117122) [details]
Patch
------- Comment #20 From 2011-11-29 22:02:42 PST -------
Patch re-submitted after renaming from PluginViewHelperBlackBerry to NPCallbacksBlackBerry,  following Antonio's suggestion.
------- Comment #21 From 2011-11-30 07:30:04 PST -------
(From update of attachment 117122 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=117122&action=review

there are some nits, but it looks good r+.

Please fix them before landing :)

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:77
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

maybe we could make it a helper static local function: toPluginView (or a better name)?

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:84
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and use it here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:91
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:98
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:105
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:112
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:119
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:126
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:133
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:140
> +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> +    PluginViewPrivate* viewPrivate = view->getPrivate();

... and here

> Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:156
> +NPSurface lockReadFrontBuffer(NPP instance)
> +{
> +PluginView* view = static_cast<PluginView*>(instance->ndata);
> +PluginViewPrivate* viewPrivate = view->getPrivate();
> +return viewPrivate->lockReadFrontBuffer();
> +}

... and here

ps: identation issue here!
------- Comment #22 From 2011-11-30 08:54:22 PST -------
(In reply to comment #14)
> Re-open it for more patches to come.

Please don't do this. A bug should only be re-opened if the patch has been rolled out. Instead, please file a new bug with a new patch.
------- Comment #23 From 2011-11-30 19:51:11 PST -------
> 
> there are some nits, but it looks good r+.
> 
> Please fix them before landing :)
> 
> > Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:77
> > +    PluginView* view = static_cast<PluginView*>(instance->ndata);
> > +    PluginViewPrivate* viewPrivate = view->getPrivate();
> 
> maybe we could make it a helper static local function: toPluginView (or a better name)?
> 

----  This view->getPrivate() is defined in the base class PluginView. in the upper directory, which has not been landed. So we will keep this as is for now until the PluginView.h is also upstreamed. 

> > Source/WebCore/plugins/blackberry/NPCallbacksBlackBerry.cpp:156
> > +NPSurface lockReadFrontBuffer(NPP instance)
> > +{
> > +PluginView* view = static_cast<PluginView*>(instance->ndata);
> > +PluginViewPrivate* viewPrivate = view->getPrivate();
> > +return viewPrivate->lockReadFrontBuffer();
> > +}
> > 
> ps: identation issue here!

---- This will be fixed in the new patch.

Thanks Antonio, for the review.  I am moving the thread to a new bug,  73513, for better tracking since there are too many discussions here , and as suggested, I should not re-open this bug since it's already automatically closed by the system.