Bug 92160 – [BlackBerry] Support async spellcheck for the blackberry port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Nima Ghanavatian]

Setting up support to tie in to the async spellcheck code.  This will call out to the input service and process the results in the callback methods spellCheckingRequestProcessed and spellCheckingRequestCancelled.

[Nima Ghanavatian]

Created attachment 154142 [details]
Patch

[Rob Buis]

Comment on attachment 154142 [details]
Patch

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

Looks good, still some stuff to fix.

> Source/WebKit/blackberry/ChangeLog:54
> +        (WebCore):

I think something went wrong with the ChangeLogs, one should detail the WebCore/ChangeLog changes, the other the changes in WebKit/blackberry/ChangeLog. The git commit message should do the combination of the two like above.

> Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.cpp:43
> +class SpellChecker;

You don't need this.

> Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.cpp:575
> +void EditorClientBlackBerry::checkTextOfParagraph(const UChar*, int, WebCore::TextCheckingTypeMask, WTF::Vector<WebCore::TextCheckingResult>&)

You don't need some of the prefixes here, like WTF:: for Vector.

> Source/WebKit/blackberry/WebCoreSupport/EditorClientBlackBerry.h:88
> +    virtual void checkTextOfParagraph(const UChar*, int, WebCore::TextCheckingTypeMask, WTF::Vector<WebCore::TextCheckingResult>&);

You don't need some of the prefixes here, like WTF:: for Vector.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:630
> +    if (frame) {

You can combine above two lines.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:632
> +        if (editor)

Ditto.

[Nima Ghanavatian]

Created attachment 154355 [details]
Patch

[Rob Buis]

Comment on attachment 154355 [details]
Patch

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

ChangeLogs are fine now. Please have a look at my comments and also the patch needs to be reapplied against trunk so the style checker can run.

> Source/WebCore/ChangeLog:6
> +        2012-07-24  Nima Ghanavatian  <[e-mail]>

You do not need this line.

> Source/WebKit/blackberry/ChangeLog:6
> +        2012-07-24  Nima Ghanavatian  <[e-mail]>

Ditto.

> Source/WebCore/platform/text/TextChecking.h:40
> +#if PLATFORM(MAC) && !defined(BUILDING_ON_LEOPARD) || PLATFORM(BLACKBERRY)

Better add some braces to make more clear how this should be evaluated.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:533
> +        free(checkingString);

I am not sure if it needs to be freed since malloc failed. But what about when it succeeded, does it need to be freed in that case? Are we leaking checkingString?

[Nima Ghanavatian]

Created attachment 154708 [details]
Patch

[Nima Ghanavatian]

In addition to the changes you pointed out I also reworked InputHandler::requestCheckingOfString quite a bit to call spellCheckingRequestCancelled on all early returns, and to only append the map on successful calls to checkSpellingOfStringAsync.

[Rob Buis]

Comment on attachment 154708 [details]
Patch

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

Looks good, please fix the minor issues.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:523
> +void InputHandler::requestCheckingOfString(WTF::PassRefPtr<WebCore::TextCheckingRequest> textCheckingRequest)

You can remove the WTF:: prefixes here.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:560
> +    // map sequenceId to transactionId

Add a period.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:619
> +        InputLog(LogLevelWarn, "InputHandler::spellCheckingRequestProcessed failed to processed request with sequenceId %d", sequenceId);

Typo process

[Nima Ghanavatian]

Created attachment 154715 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154715 [details]
Patch

Rejecting attachment 154715 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13342974

[Mike Fenton]

(In reply to comment #9)
> (From update of attachment 154715 [details])
> Rejecting attachment 154715 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/13342974

Rob needs to r+, or you need to add Rob to the log yourself.

[Nima Ghanavatian]

Created attachment 154732 [details]
Patch

[Rob Buis]

Comment on attachment 154732 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 154732 [details]
Patch

Clearing flags on attachment: 154732

Committed r123809: <http://trac.webkit.org/changeset/123809>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.