[webkit-reviews] review denied: [Bug 73143] Upstream BlackBerry porting of page : [Attachment 116640] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jacky Jiang
<zkjiang008 at gmail.com>'s request for review:
Bug 73143: Upstream BlackBerry porting of page
https://bugs.webkit.org/show_bug.cgi?id=73143

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116640&action=review


This change looks good. I have some minor issues.

> Source/WebCore/page/blackberry/AccessibilityObjectBlackBerry.cpp:26
> +namespace WebCore {
> +
> +} // namespace WebCore

Disregarding the #includes and copyright, this file is empty. Do we need it?

> Source/WebCore/page/blackberry/DragControllerBlackBerry.cpp:21
> +#if ENABLE(DRAG_SUPPORT)
> +#include "DragController.h"

Swap these lines.

> Source/WebCore/page/blackberry/DragControllerBlackBerry.cpp:25
> +#include "IntSize.h"

This include is unnecessary since DragController.h includes IntPoint.h that
includes this file.

> Source/WebCore/page/blackberry/DragControllerBlackBerry.cpp:32
> +// FIXME: remove this when no stubs use it anymore
> +static const WebCore::IntSize s_nullSize = WebCore::IntSize();

We should move this into the body of DragController::maxDragImageSize(). Also,
use DEFINE_STATIC_LOCAL to define this static variable. See
<http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/StdLibExtras.h?
rev=97675#L32> for more details.

> Source/WebCore/page/blackberry/DragControllerBlackBerry.cpp:34
> +const double EventHandler::TextDragDelay = 0.0;

0.0 => 0

> Source/WebCore/page/blackberry/EventHandlerBlackBerry.cpp:23
> +#include "Clipboard.h"

This include is unnecessary as ClipboardBlackBerry.h (included below) includes
this file.

> Source/WebCore/page/blackberry/EventHandlerBlackBerry.cpp:32
> +#include "PlatformMouseEvent.h"

This include is unnecessary as MouseEventWithHitTestResults.h (included above)
includes this file.

> Source/WebCore/page/blackberry/EventHandlerBlackBerry.cpp:68
> +bool
EventHandler::passWidgetMouseDownEventToWidget(MouseEventWithHitTestResults
const&)

This signature doesn't match the signature for
passWidgetMouseDownEventToWidget() in EventHandler.h:
<http://trac.webkit.org/browser/trunk/Source/WebCore/page/EventHandler.h?rev=10
0483#L298>.

> Source/WebCore/page/blackberry/EventHandlerBlackBerry.cpp:79
> +unsigned int EventHandler::accessKeyModifiers()

unsigned int => unsigned

> Source/WebCore/page/blackberry/FrameBlackBerry.cpp:26
> +void* Frame::dragImageForSelection()

void* => DragImageRef. Then typedef DragImageRef in
WebCore/platform/DragImage.h:
<http://trac.webkit.org/browser/trunk/Source/WebCore/platform/DragImage.h?rev=9
5922#L65>.