Bug 73185 - [blackberry] Upstream BlackBerry porting of plugin framework
Summary: [blackberry] Upstream BlackBerry porting of plugin framework
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 73513
Blocks: 73144 73397
  Show dependency treegraph
 
Reported: 2011-11-27 22:26 PST by Charles Wei
Modified: 2011-11-30 19:53 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Wei 2011-11-27 22:26:36 PST
This bug is to upstream BlackBerry's porting of plugin-framework.
Comment 1 Charles Wei 2011-11-27 23:17:13 PST
Created attachment 116702 [details]
Patch
Comment 2 Adam Barth 2011-11-27 23:24:30 PST
Comment on attachment 116702 [details]
Patch

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 Adam Barth 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 Charles Wei 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 Daniel Bates 2011-11-28 00:29:04 PST
Comment on attachment 116702 [details]
Patch

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 Charles Wei 2011-11-28 19:11:49 PST
Created attachment 116868 [details]
Patch
Comment 7 Daniel Bates 2011-11-28 20:14:47 PST
Comment on attachment 116868 [details]
Patch

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 Charles Wei 2011-11-28 20:47:05 PST
Created attachment 116879 [details]
Patch
Comment 9 Antonio Gomes 2011-11-28 20:55:00 PST
Comment on attachment 116879 [details]
Patch

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 Charles Wei 2011-11-28 22:04:00 PST
Created attachment 116892 [details]
Patch
Comment 11 Daniel Bates 2011-11-28 22:13:32 PST
Comment on attachment 116892 [details]
Patch

Thanks Charles.
Comment 12 WebKit Review Bot 2011-11-28 23:39:55 PST
Comment on attachment 116892 [details]
Patch

Clearing flags on attachment: 116892

Committed r101331: <http://trac.webkit.org/changeset/101331>
Comment 13 WebKit Review Bot 2011-11-28 23:40:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Charles Wei 2011-11-29 18:39:35 PST
Re-open it for more patches to come.
Comment 15 Charles Wei 2011-11-29 18:50:47 PST
Created attachment 117097 [details]
Patch
Comment 16 Antonio Gomes 2011-11-29 19:17:10 PST
Comment on attachment 117097 [details]
Patch

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 Charles Wei 2011-11-29 19:19:34 PST
(In reply to comment #16)
> (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.

Thanks, Antonio.  What do you suggest we should name it  ?
Comment 18 Antonio Gomes 2011-11-29 20:43:58 PST
(In reply to comment #17)
> (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  ?

Maybe NPCallbacksBlackBerry, Charles?
Comment 19 Charles Wei 2011-11-29 21:59:19 PST
Created attachment 117122 [details]
Patch
Comment 20 Charles Wei 2011-11-29 22:02:42 PST
Patch re-submitted after renaming from PluginViewHelperBlackBerry to NPCallbacksBlackBerry,  following Antonio's suggestion.
Comment 21 Antonio Gomes 2011-11-30 07:30:04 PST
Comment on attachment 117122 [details]
Patch

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 Daniel Bates 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 Charles Wei 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.