[webkit-reviews] review denied: [Bug 78136] Initial upstreaming of selection handling code for BlackBerry port : [Attachment 126301] Patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Nima Ghanavatian
<nima.ghanavatian at gmail.com>'s request for review:
Bug 78136: Initial upstreaming of selection handling code for BlackBerry port
https://bugs.webkit.org/show_bug.cgi?id=78136

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=126301&action=review


Looks good, but can be cleaned up a bit more.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:131
> +    IntPoint framePosition =
m_webPage->frameOffset(m_webPage->focusedOrMainFrame());

The above can go into the if below, since if the if turns out to be false,
these declarations are not needed.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:250
> +	   if (character)

You can combine the above two statements

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:334
> +    FrameSelection* controller = focusedFrame->selection();

controller can be moved until after the if.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:380
> +    FrameSelection* controller = focusedFrame->selection();

Ditto.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:607
> +    ASSERT(m_webPage && m_webPage->focusedOrMainFrame() &&
m_webPage->focusedOrMainFrame()->selection());

Should ASSERT be moved up? We could crash in debug mode if m_webPage is null
without any help.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:678
> +    (isStartCaret) ? return !isRTL : return isRTL;

No need for parentheses.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:725
> +	   // Compare start and end and update.

end and?

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.h:2
> + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.


Once you change anything here better add 2012.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.h:24
> +#include "TextGranularity.h"

You probably can get away with doing a forward reference to TextGranularity,
and including it in the cpp.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.h:56
> +    bool findNextString(const WTF::String&, bool);

Would it be possible to use either WebString or WTF::String exclusively here?
Could make the API more consistant.

> Source/WebKit/blackberry/WebKitSupport/SelectionHandler.h:70
> +    bool lastUpdatedEndPointIsValid() const { return
m_lastUpdatedEndPointIsValid; }

Better do a new line here.