Bug 57862 – WebKit2: Implement TextChecker on Windows

• Component: WebKit2
• Status: RESOLVED FIXED

[Jessie Berlin]

<rdar://problem/8824396>

[Jessie Berlin]

Created attachment 88302 [details]
Patch (Part 1)

[Anders Carlsson]

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.

[Jessie Berlin]

(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!

[Jessie Berlin]

Comment on attachment 88302 [details]
Patch (Part 1)

Committed in http://trac.webkit.org/changeset/83050

[Jessie Berlin]

Created attachment 88468 [details]
Patch (Part 2)

[Anders Carlsson]

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.

[Jessie Berlin]

(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.

[Jessie Berlin]

Comment on attachment 88468 [details]
Patch (Part 2)

Committed in http://trac.webkit.org/changeset/83085

[Jessie Berlin]

Created attachment 88552 [details]
Patch (Part 3)

[Early Warning System Bot]

Attachment 88552 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8347408

[Adam Roben (:aroben)]

Looks like you need to fix Qt.

[Jessie Berlin]

Created attachment 88640 [details]
Patch (Part 3) Take 2 - attempt to fix Qt build

[Adam Roben (:aroben)]

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?

[Jessie Berlin]

(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 :)

[Jessie Berlin]

Comment on attachment 88640 [details]
Patch (Part 3) Take 2 - attempt to fix Qt build

Committed in http://trac.webkit.org/changeset/83188

[Jessie Berlin]

Created attachment 88719 [details]
Patch (Part 4)

[Brian Weinstein]

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?

[Jessie Berlin]

(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.

[Jessie Berlin]

Comment on attachment 88719 [details]
Patch (Part 4)

Committed in http://trac.webkit.org/changeset/83232

[Adam Roben (:aroben)]

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.

[Jessie Berlin]

(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.

[Jessie Berlin]

Created attachment 88892 [details]
Patch (Part 5)

[Jessie Berlin]

Comment on attachment 88892 [details]
Patch (Part 5)

Committed in http://trac.webkit.org/changeset/83363

[Jessie Berlin]

Created attachment 88962 [details]
Patch (Part 6)

[Jessie Berlin]

Comment on attachment 88962 [details]
Patch (Part 6)

Committed in http://trac.webkit.org/changeset/83452

[Jessie Berlin]

Created attachment 89071 [details]
Part 7

[Jessie Berlin]

Comment on attachment 89071 [details]
Part 7

Committed in http://trac.webkit.org/changeset/83496

[Jessie Berlin]

Part 7 dealt with the last of the TextCheckerWin stubs. There are some remaining TextChecker bugs, but I will deal with those separately.