Bug 92160 - [BlackBerry] Support async spellcheck for the blackberry port
Summary: [BlackBerry] Support async spellcheck for the blackberry port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nima Ghanavatian
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-24 14:02 PDT by Nima Ghanavatian
Modified: 2012-07-26 16:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (20.10 KB, patch)
2012-07-24 14:29 PDT, Nima Ghanavatian
no flags Details | Formatted Diff | Diff
Patch (17.77 KB, patch)
2012-07-25 08:00 PDT, Nima Ghanavatian
no flags Details | Formatted Diff | Diff
Patch (18.05 KB, patch)
2012-07-26 11:50 PDT, Nima Ghanavatian
no flags Details | Formatted Diff | Diff
Patch (18.04 KB, patch)
2012-07-26 12:18 PDT, Nima Ghanavatian
no flags Details | Formatted Diff | Diff
Patch (18.05 KB, patch)
2012-07-26 13:26 PDT, Nima Ghanavatian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nima Ghanavatian 2012-07-24 14:02:00 PDT
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.
Comment 1 Nima Ghanavatian 2012-07-24 14:29:08 PDT
Created attachment 154142 [details]
Patch
Comment 2 Rob Buis 2012-07-24 14:51:19 PDT
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.
Comment 3 Nima Ghanavatian 2012-07-25 08:00:50 PDT
Created attachment 154355 [details]
Patch
Comment 4 Rob Buis 2012-07-25 08:21:43 PDT
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  <nghanavatian@rim.com>

You do not need this line.

> Source/WebKit/blackberry/ChangeLog:6
> +        2012-07-24  Nima Ghanavatian  <nghanavatian@rim.com>

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?
Comment 5 Nima Ghanavatian 2012-07-26 11:50:39 PDT
Created attachment 154708 [details]
Patch
Comment 6 Nima Ghanavatian 2012-07-26 11:53:23 PDT
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.
Comment 7 Rob Buis 2012-07-26 12:14:29 PDT
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
Comment 8 Nima Ghanavatian 2012-07-26 12:18:50 PDT
Created attachment 154715 [details]
Patch
Comment 9 WebKit Review Bot 2012-07-26 13:03:37 PDT
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
Comment 10 Mike Fenton 2012-07-26 13:06:42 PDT
(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.
Comment 11 Nima Ghanavatian 2012-07-26 13:26:45 PDT
Created attachment 154732 [details]
Patch
Comment 12 Rob Buis 2012-07-26 13:31:44 PDT
Comment on attachment 154732 [details]
Patch

LGTM.
Comment 13 WebKit Review Bot 2012-07-26 16:05:24 PDT
Comment on attachment 154732 [details]
Patch

Clearing flags on attachment: 154732

Committed r123809: <http://trac.webkit.org/changeset/123809>
Comment 14 WebKit Review Bot 2012-07-26 16:05:27 PDT
All reviewed patches have been landed.  Closing bug.