Bug 78136 – Initial upstreaming of selection handling code for BlackBerry port

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

[Nima Ghanavatian]

Upstreaming SelectionHandler.cpp and SelectionHandler.h for the BlackBerry port

[Rob Buis]

Assign to Nima.

[Nima Ghanavatian]

Created attachment 126301 [details]
Patch

[Rob Buis]

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.

[Nima Ghanavatian]

Created attachment 126786 [details]
Patch

[Rob Buis]

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.

[Nima Ghanavatian]

Created attachment 126808 [details]
Patch

[Nima Ghanavatian]

Comment on attachment 126808 [details]
Patch

submitted wrong patch =(
correct one coming shortly

[Nima Ghanavatian]

Created attachment 126821 [details]
Patch

[Rob Buis]

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.

[Nima Ghanavatian]

Created attachment 126832 [details]
Patch

[Rob Buis]

Comment on attachment 126832 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 126832 [details]
Patch

Clearing flags on attachment: 126832

Committed r107674: <http://trac.webkit.org/changeset/107674>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.