Bug 74071 – [Chromium] Chromium should have EditorClientImpl::checkTextOfParagraph

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Shinya Kawanaka]

Currently Chromium does not implement necessary methods to work with USE_UNIFIED_TEXT_CHECKING.
This should be implemented.

[Shinya Kawanaka]

Currently SpellChecker on Mac uses more sophisticated interface for spellchecking (requestTextOfParagraph).
If Windows and Linux have the same interface, the spellchecking code will be clearer, and easier to change.

[Shinya Kawanaka]

Created attachment 118740 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Shinya Kawanaka]

s/requestTextOfParagraph/checkTextOfParagraph/g

[Darin Fisher (:fishd, Google)]

Comment on attachment 118740 [details]
Patch

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

> Source/WebKit/chromium/public/WebSpellCheckClient.h:44
>  class WebSpellCheckClient {

By the way, it seems like WebSpellCheckClient is misnamed.  It should probably
be called WebTextChecker.  Such a renaming is out-of-scope for this patch, but
I just wanted to call attention to the issue.

> Source/WebKit/chromium/public/WebSpellCheckClient.h:57
> +    virtual void checkTextOfParagraph(const WebString& text,

This appears to run synchronously, like spellCheck.  Should we have an asynchronous
version like requestCheckingOfText?  Also, what about the optionalSuggestions out-
param of spellCheck?  You don't need a way to provide suggestions with this new API?

It seems like WebSpellCheckClient is a bit of a mess :-(

> Source/WebKit/chromium/public/WebTextChecking.h:31
> +#ifndef WebTextChecking_h

nit: the file name should match the type name, so WebTextCheckingType.h would be better.

> Source/WebKit/chromium/public/WebTextChecking.h:36
> +enum WebTextCheckingType {

WebTextCheckingResult has a similar enum.  Presumably, that one should be deleted
in favor of this one?

> Source/WebKit/chromium/src/EditorClientImpl.cpp:769
> +static void convertCheckingResult(const WebKit::WebTextCheckingResult& webResult,

nit: please do not break up the flow of class method definitions with non-class method definitions.
please move static helper functions to the top of the file.

nit: you are already in the WebKit namespace, so no need for WebKit:: here.  also,
there is a 'using namespace WebCore' at the top of the file, so no need for WebCore::
here either.

nit: consider putting a conversion operator from WebTextCheckingResult to TextCheckingResult
on WebTextCheckingResult in a #if WEBKIT_IMPLEMENTATION section.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:793
> +

perhaps you should assert that webMask is not 0 after this?  it seems like TextCheckingTypeMask
could have other bits set too.

> Source/WebKit/chromium/src/EditorClientImpl.cpp:795
> +    m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults);

do we need to concern ourselves with the cost of constructing the WebString here?
it seems like if we are doing a lot of spell checking that we might prefer to
avoid the heap allocation here.  have you measured this to see if it matters?

[Shinya Kawanaka]

Created attachment 118995 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #5)
> (From update of attachment 118740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118740&action=review
> 
> > Source/WebKit/chromium/public/WebSpellCheckClient.h:44
> >  class WebSpellCheckClient {
> 
> By the way, it seems like WebSpellCheckClient is misnamed.  It should probably
> be called WebTextChecker.  Such a renaming is out-of-scope for this patch, but
> I just wanted to call attention to the issue.

I'll do this another patch if necessary.

> 
> > Source/WebKit/chromium/public/WebSpellCheckClient.h:57
> > +    virtual void checkTextOfParagraph(const WebString& text,
> 
> This appears to run synchronously, like spellCheck.  Should we have an asynchronous
> version like requestCheckingOfText?  Also, what about the optionalSuggestions out-
> param of spellCheck?  You don't need a way to provide suggestions with this new API?

I have a plan to add asynchronous version after this patch is landed.
I want to have the same interface WebCore::TextCheckerClient::checkTextOfParagraph. So optional suggestions is not mandatory. But we can specify adding auto correction list by WebTextCheckingTypeMask.

> 
> It seems like WebSpellCheckClient is a bit of a mess :-(
> 
> > Source/WebKit/chromium/public/WebTextChecking.h:31
> > +#ifndef WebTextChecking_h
> 
> nit: the file name should match the type name, so WebTextCheckingType.h would be better.

Done.

> 
> > Source/WebKit/chromium/public/WebTextChecking.h:36
> > +enum WebTextCheckingType {
> 
> WebTextCheckingResult has a similar enum.  Presumably, that one should be deleted
> in favor of this one?

Yeah, I want to keep WebTextCheckingType. So the previous enum should be removed. But removing it this time will break chromium build, so after this patch is landed, I'll change chromium code, then come back to WebKit to remove it.

> 
> > Source/WebKit/chromium/src/EditorClientImpl.cpp:769
> > +static void convertCheckingResult(const WebKit::WebTextCheckingResult& webResult,
> 
> nit: please do not break up the flow of class method definitions with non-class method definitions.
> please move static helper functions to the top of the file.
> 
> nit: you are already in the WebKit namespace, so no need for WebKit:: here.  also,
> there is a 'using namespace WebCore' at the top of the file, so no need for WebCore::
> here either.
> 
> nit: consider putting a conversion operator from WebTextCheckingResult to TextCheckingResult
> on WebTextCheckingResult in a #if WEBKIT_IMPLEMENTATION section.

I've added conversion operator.

> 
> > Source/WebKit/chromium/src/EditorClientImpl.cpp:793
> > +
> 
> perhaps you should assert that webMask is not 0 after this?  it seems like TextCheckingTypeMask
> could have other bits set too.

Done.

> > Source/WebKit/chromium/src/EditorClientImpl.cpp:795
> > +    m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults);
> 
> do we need to concern ourselves with the cost of constructing the WebString here?
> it seems like if we are doing a lot of spell checking that we might prefer to
> avoid the heap allocation here.  have you measured this to see if it matters?

Hmm... Currently SpellCheckClient uses WebString interface instead of UChar* and int. If it is important, we should change all the interface of SpellCheckClient, I guess. Currently it does not matter, I guess.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> > By the way, it seems like WebSpellCheckClient is misnamed.  It should probably
> > be called WebTextChecker.  Such a renaming is out-of-scope for this patch, but
> > I just wanted to call attention to the issue.
> 
> I'll do this another patch if necessary.

OK

> > This appears to run synchronously, like spellCheck.  Should we have an asynchronous
> > version like requestCheckingOfText?  Also, what about the optionalSuggestions out-
> > param of spellCheck?  You don't need a way to provide suggestions with this new API?
> 
> I have a plan to add asynchronous version after this patch is landed.
> I want to have the same interface WebCore::TextCheckerClient::checkTextOfParagraph. So optional suggestions is not mandatory. But we can specify adding auto correction list by WebTextCheckingTypeMask.

OK


> > > Source/WebKit/chromium/public/WebTextChecking.h:36
> > > +enum WebTextCheckingType {
> > 
> > WebTextCheckingResult has a similar enum.  Presumably, that one should be deleted
> > in favor of this one?
> 
> Yeah, I want to keep WebTextCheckingType. So the previous enum should be removed. But removing it this time will break chromium build, so after this patch is landed, I'll change chromium code, then come back to WebKit to remove it.

OK


> > > Source/WebKit/chromium/src/EditorClientImpl.cpp:795
> > > +    m_webView->spellCheckClient()->checkTextOfParagraph(WebString(text, length), webMask, &webResults);
> > 
> > do we need to concern ourselves with the cost of constructing the WebString here?
> > it seems like if we are doing a lot of spell checking that we might prefer to
> > avoid the heap allocation here.  have you measured this to see if it matters?
> 
> Hmm... Currently SpellCheckClient uses WebString interface instead of UChar* and int. If it is important, we should change all the interface of SpellCheckClient, I guess. Currently it does not matter, I guess.

OK... perhaps this is just something worth investigating to see if it matters.
does not have to block this patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118995 [details]
Patch

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

> Source/WebKit/chromium/public/WebTextCheckingResult.h:47
> +struct WebGrammarDetail {

webkit API rule is to have one type definition per file

[Shinya Kawanaka]

Created attachment 119126 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #9)
> (From update of attachment 118995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118995&action=review
> 
> > Source/WebKit/chromium/public/WebTextCheckingResult.h:47
> > +struct WebGrammarDetail {
> 
> webkit API rule is to have one type definition per file

Done. Thanks!!

[Shinya Kawanaka]

Darin, could you look this?

[Darin Fisher (:fishd, Google)]

Comment on attachment 119126 [details]
Patch

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

> Source/WebKit/chromium/public/WebTextCheckingResult.h:48
> +    // TODO(shinyak): Should be removed after we confirm Chromium does not use it.

nit: WebKit style is to use FIXME instead of TODO(user)

> Source/WebKit/chromium/public/WebTextCheckingResult.h:94
> +    Error error; // TODO(shinyak): Should be removed after we confirm Chromium does not use it.

TODO(user) -> FIXME

> Source/WebKit/chromium/public/WebTextCheckingResult.h:98
> +    WebVector<WebGrammarDetail> details;

it seems like there is potentially a lot of copying of data here.  have you
considered transforming WebCore::GrammarDetail and WebCore::TextCheckingResult
to be reference counted so that we can just create WebPrivatePtr<>-style wrappers
for them at the API boundary?

[Shinya Kawanaka]

> > Source/WebKit/chromium/public/WebTextCheckingResult.h:98
> > +    WebVector<WebGrammarDetail> details;
> 
> it seems like there is potentially a lot of copying of data here.  have you
> considered transforming WebCore::GrammarDetail and WebCore::TextCheckingResult
> to be reference counted so that we can just create WebPrivatePtr<>-style wrappers
> for them at the API boundary?

Darin,

I didn't know that... I think it should be better than the current implementation. I'll change the code to use it.

[Shinya Kawanaka]

Created attachment 119809 [details]
Patch

[Shinya Kawanaka]

Darin, 

> I didn't know that... I think it should be better than the current implementation. I'll change the code to use it.

Since GrammarDetails is a value struct, so I decided not to make it RefCounted.
However, since currently we don't have a plan to use GrammarDetails for now, we have decided to omit it. I hope you to review it again.

[Shinya Kawanaka]

(In reply to comment #13)
> (From update of attachment 119126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119126&action=review
> 
> > Source/WebKit/chromium/public/WebTextCheckingResult.h:48
> > +    // TODO(shinyak): Should be removed after we confirm Chromium does not use it.
> 
> nit: WebKit style is to use FIXME instead of TODO(user)
> 
> > Source/WebKit/chromium/public/WebTextCheckingResult.h:94
> > +    Error error; // TODO(shinyak): Should be removed after we confirm Chromium does not use it.
> 
> TODO(user) -> FIXME

There were DONE.

[Darin Fisher (:fishd, Google)]

Comment on attachment 119809 [details]
Patch

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

> Source/WebKit/chromium/src/EditorClientImpl.cpp:770
> +                                            WebCore::TextCheckingTypeMask mask,

nit: no need to mention WebCore:: or WTF:: in this .cpp file.  notice the using directive at the top of the file for WebCore, and the WTF headers have a using directive.

[Darin Fisher (:fishd, Google)]

nit: you probably want to bump your copyrights to 2012

[Shinya Kawanaka]

Created attachment 123242 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123242 [details]
Patch for landing

Clearing flags on attachment: 123242

Committed r105491: <http://trac.webkit.org/changeset/105491>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.