Bug 73143

Created attachment 116636 [details] Patch

Created attachment 116637 [details] Patch

Comment on attachment 116636 [details] Patch The copyright seems to lack the normal GNU section.

Comment on attachment 116637 [details] Patch I will update the copyright part.

Created attachment 116640 [details] Patch

Added LGPL license block.

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>.

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.

(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>.

Created attachment 116821 [details] Patch Updated the patch according to Daniel's advices.

Created attachment 116841 [details] Patch Update DragImageRef define and eventActivatedView.

Created attachment 116844 [details] Patch Update nullSize and the comment above.

Comment on attachment 116844 [details] Patch Thank you Jacky for updating the patch. r=me.

Comment on attachment 116844 [details] Patch Clearing flags on attachment: 116844 Committed r101319: <http://trac.webkit.org/changeset/101319>

All reviewed patches have been landed. Closing bug.