[webkit-reviews] review denied: [Bug 73397] [BlackBerry] Upstream BlackBerry porting of pluginView : [Attachment 117347] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 73397: [BlackBerry] Upstream BlackBerry porting of pluginView
https://bugs.webkit.org/show_bug.cgi?id=73397

Attachment 117347: Patch
https://bugs.webkit.org/attachment.cgi?id=117347&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.