Bug 73143 – Upstream BlackBerry porting of page

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jacky Jiang]

Blackberry's implementation of page.
PR 118400.

[Jacky Jiang]

Created attachment 116636 [details]
Patch

[Jacky Jiang]

Created attachment 116637 [details]
Patch

[Rob Buis]

Comment on attachment 116636 [details]
Patch

The copyright seems to lack the normal GNU section.

[Jacky Jiang]

Comment on attachment 116637 [details]
Patch

I will update the copyright part.

[Jacky Jiang]

Created attachment 116640 [details]
Patch

[Jacky Jiang]

Added LGPL license block.

[Daniel Bates]

Comment on attachment 116640 [details]
Patch

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=100483#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=95922#L65>.

[Jacky Jiang]

Comment on attachment 116640 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116640&action=review

>> Source/WebCore/page/blackberry/DragControllerBlackBerry.cpp:21
>> +#include "DragController.h"
> 
> Swap these lines.

Thanks for your review Dan, I will follow your advices. But I don't get why do we need to swap these lines? Seems we don't need to include "DragController.h" if we don't define DRAG_SUPPORT.

[Daniel Bates]

(In reply to comment #8)
> (From update of attachment 116640 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116640&action=review
> 
> >> Source/WebCore/page/blackberry/DragControllerBlackBerry.cpp:21
> >> +#include "DragController.h"
> > 
> > Swap these lines.
> 
> Thanks for your review Dan, I will follow your advices. But I don't get why do we need to swap these lines? Seems we don't need to include "DragController.h" if we don't define DRAG_SUPPORT.

I spoke with Jacky on IRC today about this. I suggest swapping these lines so as to be consistent with both the WebKit Code Style Guidelines (*) and the form used in DragControllerMac.mm. That being said, is this guard even needed in this file? Is it necessary to even compile this file without drag support? I assume that the callers of these functions are guarded with ENABLE(DRAG_SUPPORT) and hence we don't need to even compile is file when building without drag support. Notice that the DragController implementation file for all non-Mac ports omit such a DRAG_SUPPORT guard.

(*) See (2) of section "#include Statements" of <http://www.webkit.org/coding/coding-style.html>.

[Jacky Jiang]

Created attachment 116821 [details]
Patch

Updated the patch according to Daniel's advices.

[Jacky Jiang]

Created attachment 116841 [details]
Patch

Update DragImageRef define and eventActivatedView.

[Jacky Jiang]

Created attachment 116844 [details]
Patch

Update nullSize and the comment above.

[Daniel Bates]

Comment on attachment 116844 [details]
Patch

Thank you Jacky for updating the patch.

r=me.

[WebKit Review Bot]

Comment on attachment 116844 [details]
Patch

Clearing flags on attachment: 116844

Committed r101319: <http://trac.webkit.org/changeset/101319>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.