<rdar://problem/8824396>
Created attachment 88302 [details] Patch (Part 1)
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.
(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 on attachment 88302 [details] Patch (Part 1) Committed in http://trac.webkit.org/changeset/83050
Created attachment 88468 [details] Patch (Part 2)
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.
(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 on attachment 88468 [details] Patch (Part 2) Committed in http://trac.webkit.org/changeset/83085
Created attachment 88552 [details] Patch (Part 3)
Attachment 88552 [details] did not build on qt: Build output: http://queues.webkit.org/results/8347408
Looks like you need to fix Qt.
Created attachment 88640 [details] Patch (Part 3) Take 2 - attempt to fix Qt build
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?
(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 on attachment 88640 [details] Patch (Part 3) Take 2 - attempt to fix Qt build Committed in http://trac.webkit.org/changeset/83188
Created attachment 88719 [details] Patch (Part 4)
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?
(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 on attachment 88719 [details] Patch (Part 4) Committed in http://trac.webkit.org/changeset/83232
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.
(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.
Created attachment 88892 [details] Patch (Part 5)
Comment on attachment 88892 [details] Patch (Part 5) Committed in http://trac.webkit.org/changeset/83363
Created attachment 88962 [details] Patch (Part 6)
Comment on attachment 88962 [details] Patch (Part 6) Committed in http://trac.webkit.org/changeset/83452
Created attachment 89071 [details] Part 7
Comment on attachment 89071 [details] Part 7 Committed in http://trac.webkit.org/changeset/83496
Part 7 dealt with the last of the TextCheckerWin stubs. There are some remaining TextChecker bugs, but I will deal with those separately.