[Webkit-unassigned] [Bug 73397] [BlackBerry] Upstream BlackBerry porting of pluginView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73397


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #117347|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #10 from Daniel Bates <dbates at webkit.org>  2011-12-08 19:24:23 PST ---
(From update of attachment 117347)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.