Bug 73397

Created attachment 117128 [details] Patch

Comment on attachment 117128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117128&action=review > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1054 > +bool PluginView::platformGetValueStatic(NPNVariable variable, void* value, NPError* result) Maybe a static local function? > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1092 > + *((void **) value) = (void*)&(((NPSetWindowCallbackStruct*)m_npWindow.ws_info)->zoomFactor); can we avoid the c-cast here? > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1103 > + void** v = (void**) value; ditto > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1120 > + BlackBerry::Platform::Graphics::Window *window = * position > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1124 > + void** v = (void**) value; > + *v = (void*) window->windowGroup(); ditto (c-cast) > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1144 > + void** v = (void**) value; > + *v = (void*) context; ditto > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1159 > + void** v = (void**) value; > + *v = (void*) m_private->m_pluginUniquePrefix.c_str(); ditto

Comment on attachment 117128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117128&action=review >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1054 >> +bool PluginView::platformGetValueStatic(NPNVariable variable, void* value, NPError* result) > > Maybe a static local function? This is defined in the cross-platform PluginView.h as a static member function. >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1092 >> + *((void **) value) = (void*)&(((NPSetWindowCallbackStruct*)m_npWindow.ws_info)->zoomFactor); > > can we avoid the c-cast here? Yes >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1103 >> + void** v = (void**) value; > > ditto Yes >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1120 >> + BlackBerry::Platform::Graphics::Window *window = > > * position Yes >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1124 >> + *v = (void*) window->windowGroup(); > > ditto (c-cast) window->windowGroup() returns const char*, there isn't an easy c++ way to cast that to void*, so I would suggest we keep this. >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1144 >> + *v = (void*) context; > > ditto Yes > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1158 > + void** v = (void**) value; This can be static_cast >> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1159 >> + *v = (void*) m_private->m_pluginUniquePrefix.c_str(); > > ditto No easy C++ cast from const char* to void*, so I would suggest we keep it here.

const char* to void* cast can be achieved: reinterpret_cast<void*>(const_cast<char*>()) Do we really want that?

Created attachment 117311 [details] Patch

Comment on attachment 117311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117311&action=review > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:33 > +#include "Element.h" This include is unnecessary as HTMLPlugInElement.h (included below) includes HTMLFrameOwnerElement.h => HTMLElement.h => StyledElement.h, which includes this file. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:38 > +#include "GraphicsContext.h" This include is unnecessary as PlatformContextSkia.h (included below) includes this file. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:42 > +#include "Image.h" This file is unnecessary as PlatformContextSkia.h (included below) includes GraphicsContext.h, which includes this file. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:57 > +#include "Touch.h" This file is unnecessary as TouchList.h (below) includes this file. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:63 > +#include <BlackBerryPlatformWindow.h> We should move this include to after line 73 so that it's grouped with the other BlackBerry platform headers. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:81 > +#define UNINIT_COORD 0xffffffff This should be a C++ constant and shouldn't have an underscore in its name: const unsigned UninitializedCoordinate = 0xffffffff; > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:103 > +static NPRect toNPRect(const IntRect& rect) > +{ > + NPRect npRect; > + npRect.top = rect.y(); > + npRect.left = rect.x(); > + npRect.bottom = rect.y() + rect.height(); > + npRect.right = rect.x() + rect.width(); > + return npRect; > +} > + > +static NPRect toNPRect(const BlackBerry::Platform::IntRect& rect) > +{ > + NPRect npRect; > + npRect.top = rect.y(); > + npRect.left = rect.x(); > + npRect.bottom = rect.y() + rect.height(); > + npRect.right = rect.x() + rect.width(); > + return npRect; > +} This is OK AS-IS. That being said, these functions are identical up to the datatype of their argument. Can we make this a template function? Also, the name of this function doesn't conform to the WebKit Code Style Guidelines per (8) of section Names. We can rename this function in separate bug. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:106 > +// Standard plugin widget stuff. Remove this comment. It's inane. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:146 > + // do not call setNPWindowIfNeeded immediately, will be called on paint() Nit: do => Do Also, this comment is missing a period. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:149 > + // (i) in order to move/resize the plugin window at the same time as the Nit: in => In > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:154 > + // (ii) if we are running layout tests from DRT, paint() won't ever get called if => If > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:155 > + // so we need to call setNPWindowIfNeeded() if window geometry has changed Missing a period at the end of this sentence. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:312 > + double zoomFactorH = (double)m_private->m_pluginBufferSize.width() / (double)frameRect().width(); > + double zoomFactorW = (double)m_private->m_pluginBufferSize.height() / (double)frameRect().height(); Nit: We should use C++ style casts. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:428 > + // sanity check. Remove this comment. It's inane. I take it this "sanity check" is unusual to do since we have this comment. It would have been better if the comment explained why? Regardless, we should just remove this comment. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:470 > + if (platformKeyEvent->type() == PlatformKeyboardEvent::RawKeyDown > + || platformKeyEvent->type() == PlatformKeyboardEvent::KeyDown) { Nit: The indentation here is off. The "||" should be left-aligned with the 'p' in platformKeyEvent on line 469. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:532 > + return; This looks weird. We don't need to fix this now, but we should eventually come back to this code. What other mouse events are there? Does control ever reach here? Maybe this should be an ASSERT_NOT_REACHED(). > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:545 > + npTouchEvent.points = new NPTouchPoint[touchList->length()]; Can we use OwnArrayPtr here? Then we can remove the delete[] on line 576. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:951 > +void PluginView::setNPWindowRect(const IntRect& rc) This argument is unused and depending on our compiler warning level may raise a warning. We should just remove the argument name. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:987 > + // FLASH WORKAROUND: Only set initially. Multiple calls to > + // setNPWindow() cause the plugin to crash in windowed mode. This should read: // Only set the width and height of the plugin content the first time setNPWindow() is called so as to workaround // an issue in Flash where multiple calls to setNPWindow() cause the plugin to crash in windowed mode. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1000 > + BlackBerry::Platform::Graphics::Window *window = > + frameView->hostWindow()->platformPageClient()->platformWindow(); I would write this on one line to improve readability. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1007 > + // FIXME: Passing zoomFactor to setwindow make windowed plugin scale incorrectly. Nit: I suggest adding an empty line above this FIXME comment so as to make it stand out. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1013 > + setCallingPlugin(false); Nit: I suggest adding a empty line above this line so as to demarcate the end of the zoom factor code above. We should define a RAII object that calls setCallingPlugin(true) in its constructor and calls setCallingPlugin(false) in its destructor. We can do this in a follow up patch. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1025 > + // since the window manager should take care of that for us. Nit: Remove the leading whitespace (before the word "since") in this comment such that there is exactly one space after the "//" in this line. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1039 > + FILE* fileHandle = fopen((filename.utf8()).data(), "r"); Nit: (filename.utf8()) => filename.utf8() That is, the parentheses around "filename.utf8()" aren't necessary.

Created attachment 117347 [details] Patch

Comment on attachment 117347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117347&action=review > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:528 > + npTouchEvent.points = new NPTouchPoint[touchList->length()]; Can we use OwnArrayPtr here?

(In reply to comment #8) > (From update of attachment 117347 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117347&action=review > > > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:528 > > + npTouchEvent.points = new NPTouchPoint[touchList->length()]; > > Can we use OwnArrayPtr here? I would rather not, npTouchEvent.points is defined as raw pointer, change it to OwnArrayPtr changes too many platform-specific code.

Comment on attachment 117347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117347&action=review This patch is better. There are still a few minor things that we could improve. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:68 > +#include <BlackBerryPlatformPrimitives.h> This include is unnecessary as BlackBerryPlatformGraphics.h (above) includes this file. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:390 > +#else > +#warning "Implement me!" > + ASSERT_NOT_REACHED(); > +#endif We always build with Skia enabled. Therefore remove this code and the #if USE(SKIA) on line 268. >>> Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:528 >>> + npTouchEvent.points = new NPTouchPoint[touchList->length()]; >> >> Can we use OwnArrayPtr here? > > I would rather not, npTouchEvent.points is defined as raw pointer, change it to OwnArrayPtr changes too many platform-specific code. I don't understand why we would need to modify the declaration of NPTouchEvent::points to be of type OwnArrayPtr. Can't we just define a local variable in this function of type OwnArrayPtr<NPTouchPoint> called touchPoints. Then set npTouchEvent.points = touchPoints.get() and remove "delete[] npTouchEvent.points" (line 559). The code would have the following form: ... OwnArrayPtr<NPTouchPoint> touchPoints; unsigned numberOfTouchPoints = touchList->length(); if (numberOfTouchPoints) { touchPoints = adoptArrayPtr(new NPTouchPoint[numberOfTouchPoints]); for (unsigned i = 0; i < numberOfTouchPoints; ++i) { Touch* touchItem = touchList->item(i); touchPoints[i].touchId = touchItem->identifier(); ... touchPoints[i].pageY = touchItem->pageY(); } } npTouchEvent.points = touchPoints.get(); NPEvent npEvent; npEvent.type = NP_TouchEvent; npEvent.data = &npTouchEvent; if (dispatchNPEvent(npEvent)) { ... } else if (npTouchEvent.type == TOUCH_EVENT_DOUBLETAP) { ... } > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1070 > + default: > + return false; > + } NPNVariable is an enum type. Instead of having a default statement we should list all of the other enums values here and fall through to a return false, like: case NPNVxDisplay: case NPNVxtAppContext: ... case NPNVprivateModeBool: return false; Then add an ASSERT_NOT_REACHED() after the switch statement and "return false;". By using this approach we'll get a compiler error should someone define a new enum value in NPNVariable. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:1158 > + default: > + return platformGetValueStatic(variable, value, result); > + // FIXME: Should we return false? > + } We should use a similar approach here.

Created attachment 119141 [details] Patch

Created attachment 119167 [details] Patch V5 Resubmit the patch and set the review flag and commit-queue flag.

Comment on attachment 119167 [details] Patch V5 View in context: https://bugs.webkit.org/attachment.cgi?id=119167&action=review Looks good to me. > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:680 > + // Clip against any frames that the widget is inside. Note that if the frames are also clipped > + // by a div, that will not be included in this calculation. That is an improvement that still > + // needs to be made. It should be a FIXME > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:704 > + while (current->parent() && visible) { > + // Determine if it is visible in this scrollview. > + visibleContentRect = current->parent()->visibleContentRect(); > + > + // Special case for the root ScrollView. Its size does not match the actual window size. > + if (current->parent() == root()) > + visibleContentRect.setSize(windowSize); > + > + contentRect.intersect(visibleContentRect); > + visible = !contentRect.isEmpty(); > + > + // Offset to visible coordinates in scrollview widget's coordinate system (except in the case of > + // the top scroll view). > + if (current->parent()->parent()) > + contentRect.move(-visibleContentRect.x(), -visibleContentRect.y()); > + > + current = current->parent(); > + > + // Don't include the offset for the root window or we get the wrong coordinates. > + if (current->parent()) { > + // Move content rect into the parent scrollview's coordinates. > + IntRect curFrameRect = current->frameRect(); > + contentRect.move(curFrameRect.x(), curFrameRect.y()); This whole block could be simplified, I think. see WebPagePrivate::getRecursiveVisibleWindowRect. Maybe in a follow up though

Comment on attachment 119167 [details] Patch V5 View in context: https://bugs.webkit.org/attachment.cgi?id=119167&action=review > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:532 > + npTouchEvent.points[i].touchId = touchItem->identifier(); > + npTouchEvent.points[i].clientX = touchItem->pageX() - frameRect().x(); > + npTouchEvent.points[i].clientY = touchItem->pageY() - frameRect().y(); > + npTouchEvent.points[i].screenX = touchItem->screenX(); > + npTouchEvent.points[i].screenY = touchItem->screenY(); > + npTouchEvent.points[i].pageX = touchItem->pageX(); > + npTouchEvent.points[i].pageY = touchItem->pageY(); Although the compiler will probably cache npTouchEvent.points, you may want to consider dereferencing touchPoints (i.e. touchPoints[i]) instead of dereferencing points and then dereferencing the ith point. That is, I would have written these lines as: touchPoints[i]. touchId = = touchItem->identifier(); touchPoints[i].clientX = touchItem->pageX() - frameRect().x(); ... touchPoints[i].pageY = touchItem->pageY();

Committed r103019: <http://trac.webkit.org/changeset/103019>

(In reply to comment #14) > (From update of attachment 119167 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=119167&action=review > > > Source/WebCore/plugins/blackberry/PluginViewBlackBerry.cpp:532 > > + npTouchEvent.points[i].touchId = touchItem->identifier(); > > + npTouchEvent.points[i].clientX = touchItem->pageX() - frameRect().x(); > > + npTouchEvent.points[i].clientY = touchItem->pageY() - frameRect().y(); > > + npTouchEvent.points[i].screenX = touchItem->screenX(); > > + npTouchEvent.points[i].screenY = touchItem->screenY(); > > + npTouchEvent.points[i].pageX = touchItem->pageX(); > > + npTouchEvent.points[i].pageY = touchItem->pageY(); > > Although the compiler will probably cache npTouchEvent.points, you may want to consider dereferencing touchPoints (i.e. touchPoints[i]) instead of dereferencing points and then dereferencing the ith point. That is, I would have written these lines as: > > touchPoints[i]. touchId = = touchItem->identifier(); > touchPoints[i].clientX = touchItem->pageX() - frameRect().x(); > ... > touchPoints[i].pageY = touchItem->pageY(); Thanks, Daniel. Your comments have been addressed and the patch has been landed. Thanks again for your review and comments.