Bug 73185

Created attachment 116702 [details] Patch

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.

(Mostly I was just curious about this patch and that nit jumped out at me; please feel free to ignore me.)

(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 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.

Created attachment 116868 [details] Patch

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.

Created attachment 116879 [details] Patch

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'

Created attachment 116892 [details] Patch

Comment on attachment 116892 [details] Patch Thanks Charles.

Comment on attachment 116892 [details] Patch Clearing flags on attachment: 116892 Committed r101331: <http://trac.webkit.org/changeset/101331>

All reviewed patches have been landed. Closing bug.

Re-open it for more patches to come.

Created attachment 117097 [details] Patch

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.

(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 ?

(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?

Created attachment 117122 [details] Patch

Patch re-submitted after renaming from PluginViewHelperBlackBerry to NPCallbacksBlackBerry, following Antonio's suggestion.

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!

(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.

> > 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.