Bug 57862

Summary: WebKit2: Implement TextChecker on Windows
Product: WebKit Reporter: Jessie Berlin <jberlin>
Component: WebKit2Assignee: Jessie Berlin <jberlin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, jberlin, sfalken, webkit-ews
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: All   
OS: Windows 7   
Attachments:
Description Flags
Patch (Part 1)
none
Patch (Part 2)
none
Patch (Part 3)
none
Patch (Part 3) Take 2 - attempt to fix Qt build
none
Patch (Part 4)
none
Patch (Part 5)
none
Patch (Part 6)
none
Part 7 none

Description Jessie Berlin 2011-04-05 10:32:38 PDT
<rdar://problem/8824396>
Comment 1 Jessie Berlin 2011-04-05 13:33:05 PDT
Created attachment 88302 [details]
Patch (Part 1)
Comment 2 Anders Carlsson 2011-04-05 17:04:25 PDT
Comment on attachment 88302 [details]
Patch (Part 1)

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

> Source/WebKit2/UIProcess/API/C/win/WKTextChecker.cpp:37
> +WKTypeID WKTextCheckerGetTypeID()
> +{
> +    return toAPI(APIObject::TypeTextChecker);
> +}

This function isn't needed.
Comment 3 Jessie Berlin 2011-04-05 20:29:29 PDT
(In reply to comment #2)
> (From update of attachment 88302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88302&action=review
> 
> > Source/WebKit2/UIProcess/API/C/win/WKTextChecker.cpp:37
> > +WKTypeID WKTextCheckerGetTypeID()
> > +{
> > +    return toAPI(APIObject::TypeTextChecker);
> > +}
> 
> This function isn't needed.

Removed. Thanks for the review!
Comment 4 Jessie Berlin 2011-04-06 08:19:10 PDT
Comment on attachment 88302 [details]
Patch (Part 1)

Committed in http://trac.webkit.org/changeset/83050
Comment 5 Jessie Berlin 2011-04-06 10:57:00 PDT
Created attachment 88468 [details]
Patch (Part 2)
Comment 6 Anders Carlsson 2011-04-06 12:08:11 PDT
Comment on attachment 88468 [details]
Patch (Part 2)

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

> Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:73
> +    if (!m_client.uniqueSpellDocumentTag)

Do we really need to check for null here?

> Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:81
> +    if (!m_client.closeSpellDocumentWithTag)

Ditto.
Comment 7 Jessie Berlin 2011-04-06 12:15:24 PDT
(In reply to comment #6)
> (From update of attachment 88468 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88468&action=review
> 
> > Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:73
> > +    if (!m_client.uniqueSpellDocumentTag)
> 
> Do we really need to check for null here?
> 
> > Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:81
> > +    if (!m_client.closeSpellDocumentWithTag)
> 
> Ditto.

I talked with Anders on IRC and he said this was my call. I think I am going to leave the null checks in for parity with other clients and in case someone else ends up using it.
Comment 8 Jessie Berlin 2011-04-06 12:59:28 PDT
Comment on attachment 88468 [details]
Patch (Part 2)

Committed in http://trac.webkit.org/changeset/83085
Comment 9 Jessie Berlin 2011-04-06 18:02:46 PDT
Created attachment 88552 [details]
Patch (Part 3)
Comment 10 Early Warning System Bot 2011-04-06 18:12:48 PDT
Attachment 88552 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8347408
Comment 11 Adam Roben (:aroben) 2011-04-07 05:01:55 PDT
Looks like you need to fix Qt.
Comment 12 Jessie Berlin 2011-04-07 08:12:33 PDT
Created attachment 88640 [details]
Patch (Part 3) Take 2 - attempt to fix Qt build
Comment 13 Adam Roben (:aroben) 2011-04-07 10:36:08 PDT
Comment on attachment 88640 [details]
Patch (Part 3) Take 2 - attempt to fix Qt build

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

Seems like there are really two things going on here:

1) Add USE(UNIFIED_TEXT_CHECKING) guards where appropriate
2) Plumb checkSpellingOfString through to the UI process

Separating those into two separate patches would be slightly clearer. But I'll review as-is.

> Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:303
> +void TextChecker::checkSpellingOfString(int64_t, const UChar*, uint32_t, int32_t&, int32_t&)
> +{
> +    notImplemented();
> +}

Will this ever be called on Mac (given that WebKit2 doesn't run on Leopard or Tiger)? If not, I'd replace use ASSERT_NOT_REACHED() instead of notImplemented().

> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:401
> -void WebEditorClient::checkSpellingOfString(const UChar*, int, int*, int*)
> +void WebEditorClient::checkSpellingOfString(const UChar* text, int length, int* misspellingLocation, int* misspellingLength)

How is it possible to change this without making a similar change in the .h file?
Comment 14 Jessie Berlin 2011-04-07 10:49:57 PDT
(In reply to comment #13)
> (From update of attachment 88640 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88640&action=review
> 
> Seems like there are really two things going on here:
> 
> 1) Add USE(UNIFIED_TEXT_CHECKING) guards where appropriate
> 2) Plumb checkSpellingOfString through to the UI process
> 
> Separating those into two separate patches would be slightly clearer. But I'll review as-is.

My apologies. Thanks for reviewing!

> 
> > Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:303
> > +void TextChecker::checkSpellingOfString(int64_t, const UChar*, uint32_t, int32_t&, int32_t&)
> > +{
> > +    notImplemented();
> > +}
> 
> Will this ever be called on Mac (given that WebKit2 doesn't run on Leopard or Tiger)? If not, I'd replace use ASSERT_NOT_REACHED() instead of notImplemented().

It may, because checkSpellingOfStrings is called from a bunch of places that are not compiled out even if USE(UNIFIED_TEXT_CHECKING) is false.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:401
> > -void WebEditorClient::checkSpellingOfString(const UChar*, int, int*, int*)
> > +void WebEditorClient::checkSpellingOfString(const UChar* text, int length, int* misspellingLocation, int* misspellingLength)
> 
> How is it possible to change this without making a similar change in the .h file?

This signature matches that in the header file. I just filled in the parameter names :)
Comment 15 Jessie Berlin 2011-04-07 11:06:33 PDT
Comment on attachment 88640 [details]
Patch (Part 3) Take 2 - attempt to fix Qt build

Committed in http://trac.webkit.org/changeset/83188
Comment 16 Jessie Berlin 2011-04-07 15:50:12 PDT
Created attachment 88719 [details]
Patch (Part 4)
Comment 17 Brian Weinstein 2011-04-07 16:59:41 PDT
Comment on attachment 88719 [details]
Patch (Part 4)

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

> Source/WebKit2/Shared/APIObject.h:110
> +        TypeGrammarDetail

Is there any sorting to these? If so, GrammarDetail should go above TextChecker.

> Source/WebKit2/UIProcess/API/C/win/WKTextChecker.h:44
> +typedef void (*WKTextcheckerCheckGrammarOfString)(uint64_t tag, WKStringRef text, WKArrayRef* grammarDetails, int32_t* badGrammarLocation, int32_t* badGrammarLength, const void *clientInfo);

Missing a capital C in WKTextcheckerCheckGrammarOfString.

> Source/WebKit2/UIProcess/API/C/win/WKTextChecker.h:57
> +    WKTextcheckerCheckGrammarOfString                                       checkGrammarOfString;

Ditto about missing capital C.

> Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:307
> +    notImplemented();

A note saying where Mac implements this would be nice, since it seems like the Mac could implement it here.

> Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:102
> +{

Do you want to make sure that grammarDetails is empty when it is passed in? Or does it not matter?

> Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:103
> +    if (!m_client.checkSpellingOfString)

This should probably be if (!m_client.checkGrammarOfString)

> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:420
> +    int32_t resultLocation = -1;

Could we use the NotFound value in WTF/NotFound.h for this?
Comment 18 Jessie Berlin 2011-04-07 17:16:32 PDT
(In reply to comment #17)
> (From update of attachment 88719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88719&action=review
> 
> > Source/WebKit2/Shared/APIObject.h:110
> > +        TypeGrammarDetail
> 
> Is there any sorting to these? If so, GrammarDetail should go above TextChecker.

Not particularly (TypeView was already before TypeEditCommandProxy), but I sorted them (within each category).

> 
> > Source/WebKit2/UIProcess/API/C/win/WKTextChecker.h:44
> > +typedef void (*WKTextcheckerCheckGrammarOfString)(uint64_t tag, WKStringRef text, WKArrayRef* grammarDetails, int32_t* badGrammarLocation, int32_t* badGrammarLength, const void *clientInfo);
> 
> Missing a capital C in WKTextcheckerCheckGrammarOfString.

Whoops! Fixed.

> 
> > Source/WebKit2/UIProcess/API/C/win/WKTextChecker.h:57
> > +    WKTextcheckerCheckGrammarOfString                                       checkGrammarOfString;
> 
> Ditto about missing capital C.

Fixed.

> 
> > Source/WebKit2/UIProcess/mac/TextCheckerMac.mm:307
> > +    notImplemented();
> 
> A note saying where Mac implements this would be nice, since it seems like the Mac could implement it here.

The Mac does not implement this. Instead, it implements checkTextOfParagraph. I will add a comment here and in checkSpellingOfString.

> 
> > Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:102
> > +{
> 
> Do you want to make sure that grammarDetails is empty when it is passed in? Or does it not matter?

I don't think that matters.

> 
> > Source/WebKit2/UIProcess/win/WebTextCheckerClient.cpp:103
> > +    if (!m_client.checkSpellingOfString)
> 
> This should probably be if (!m_client.checkGrammarOfString)

Yes! Thanks for catching that. Fixed.

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:420
> > +    int32_t resultLocation = -1;
> 
> Could we use the NotFound value in WTF/NotFound.h for this?

Yes, also changed checkSpellingOfString to use this.
Comment 19 Jessie Berlin 2011-04-07 17:44:15 PDT
Comment on attachment 88719 [details]
Patch (Part 4)

Committed in http://trac.webkit.org/changeset/83232
Comment 20 Adam Roben (:aroben) 2011-04-08 06:17:01 PDT
Comment on attachment 88719 [details]
Patch (Part 4)

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

>>> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:420
>>> +    int32_t resultLocation = -1;
>> 
>> Could we use the NotFound value in WTF/NotFound.h for this?
> 
> Yes, also changed checkSpellingOfString to use this.

notFound is a size_t, not an int32_t. I wouldn't be surprised if this caused compiler warnings/errors on some platforms. But maybe it will be fine. At the very least it seems a little strange.
Comment 21 Jessie Berlin 2011-04-08 06:50:54 PDT
(In reply to comment #20)
> (From update of attachment 88719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88719&action=review
> 
> >>> Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp:420
> >>> +    int32_t resultLocation = -1;
> >> 
> >> Could we use the NotFound value in WTF/NotFound.h for this?
> > 
> > Yes, also changed checkSpellingOfString to use this.
> 
> notFound is a size_t, not an int32_t. I wouldn't be surprised if this caused compiler warnings/errors on some platforms. But maybe it will be fine. At the very least it seems a little strange.

It did on Mac, and I changed it back to -1.
Comment 22 Jessie Berlin 2011-04-08 16:19:58 PDT
Created attachment 88892 [details]
Patch (Part 5)
Comment 23 Jessie Berlin 2011-04-08 17:22:44 PDT
Comment on attachment 88892 [details]
Patch (Part 5)

Committed in http://trac.webkit.org/changeset/83363
Comment 24 Jessie Berlin 2011-04-10 18:37:48 PDT
Created attachment 88962 [details]
Patch (Part 6)
Comment 25 Jessie Berlin 2011-04-11 12:52:25 PDT
Comment on attachment 88962 [details]
Patch (Part 6)

Committed in http://trac.webkit.org/changeset/83452
Comment 26 Jessie Berlin 2011-04-11 13:40:35 PDT
Created attachment 89071 [details]
Part 7
Comment 27 Jessie Berlin 2011-04-11 14:34:25 PDT
Comment on attachment 89071 [details]
Part 7

Committed in http://trac.webkit.org/changeset/83496
Comment 28 Jessie Berlin 2011-04-11 14:35:47 PDT
Part 7 dealt with the last of the TextCheckerWin stubs. There are some remaining TextChecker bugs, but I will deal with those separately.