Bug 78136

Assign to Nima.

Created attachment 126301 [details] Patch

Comment on attachment 126301 [details] Patch 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.

Created attachment 126786 [details] Patch

Comment on attachment 126786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126786&action=review Looks good, some things still need fixing. > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:58 > +using namespace WebCore; This means you can get rid of all the WebCore:: prefixes below. > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:316 > + // Extend x to bottom without modifying x. Extend y? > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:646 > + if (renderer) You can do if (RenderObject* renderer = ... ) here > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.h:68 > + void caretPositionChanged(); caretPositionChanged should be private, it is not used outside of SelectionHandler.

Created attachment 126808 [details] Patch

Comment on attachment 126808 [details] Patch submitted wrong patch =( correct one coming shortly

Created attachment 126821 [details] Patch

Comment on attachment 126821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126821&action=review Almost there, just a few things to fix. > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:46 > +#include <BlackBerryPlatformIntRectRegion.h> already included in header. > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:141 > + Maybe remove the empty line. > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp:608 > + ASSERT(m_webPage && m_inputHandler); you probably mean m_webPage->m_inputHandler. > Source/WebKit/blackberry/WebKitSupport/SelectionHandler.h:34 > +class TextGranularity; Not needed.

Created attachment 126832 [details] Patch

Comment on attachment 126832 [details] Patch Looks good.

Comment on attachment 126832 [details] Patch Clearing flags on attachment: 126832 Committed r107674: <http://trac.webkit.org/changeset/107674>

All reviewed patches have been landed. Closing bug.