Bug 51013 – [Chromium] Should implement EditorClientImpl::requestCheckingOfString()

• Component: Platform
• Status: RESOLVED FIXED

[Hajime Morrita]

This is required for async spellchecking.

[Hajime Morrita]

Created attachment 76526 [details]
Patch

[Hajime Morrita]

CC levin@ because this has an WebKit API change.

[David Levin]

(In reply to comment #2)
> CC levin@ because this has an WebKit API change.
Actually that should be Darin Fisher (added).

[David Levin]

Comment on attachment 76526 [details]
Patch

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

Here's a few things that I noticed when I looked at this change quickly.

> WebKit/chromium/public/WebTextCheckingResponse.h:40
> +// Note that type() is opaque from the client

This looks like it should be a comment in the class (near the type() method).

> WebKit/chromium/public/WebTextCheckingResponse.h:43
> +class WebTextCheckingResult {

I thought we did one class per header in public and the file would be named the same as the class but Darin would know absolutely. (If so, this class should move into its own header file.)

> WebKit/chromium/public/WebTextCheckingResponse.h:46
> +    static WebTextCheckingResult makeBadGrammar(int location, int length);

It isn't clear to me what these methods do. Perhaps that could be fixed by better methods names. (The method names don't make sense right now.)

> WebKit/chromium/public/WebTextCheckingResponse.h:68
> +    explicit WebTextCheckingResponse(int sequence, const WebVector<WebTextCheckingResult>& results)

explicit shouldn't be here.

> WebKit/chromium/public/WebViewClient.h:176
> +    // Requests asyncronous spell and grammar checking. The result should be returned by 

misspelled: asyncronous

> WebKit/chromium/src/EditorClientImpl.cpp:892
> +        coreResults.append(SpellCheckingResult(static_cast<DocumentMarker::MarkerType>(results[i].type()), results[i].location(), results[0].length()));

Shouldn't results[0].length() be results[i].length()?

> WebKitTools/ChangeLog:47
> +

There are a lot of things wrong here. I think you'll see them when you look at it.  You may find it helpful if you review the diff of your changes before getting others to do it.

> WebKitTools/DumpRenderTree/chromium/WebViewHost.h:213
> +    // Spellcheck realted helper APIs

spelling: realted

[Hajime Morrita]

Created attachment 76603 [details]
Patch

[Hajime Morrita]

Hi Levin, I'm sorry for the bad CC ... and thank you for reviewing anyway!

> Here's a few things that I noticed when I looked at this change quickly.
> 
> > WebKit/chromium/public/WebTextCheckingResponse.h:40
> > +// Note that type() is opaque from the client
Done.

> 
> This looks like it should be a comment in the class (near the type() method).
> 
> > WebKit/chromium/public/WebTextCheckingResponse.h:43
> > +class WebTextCheckingResult {
> 
> I thought we did one class per header in public and the file would be named the same as the class but Darin would know absolutely. (If so, this class should move into its own header file.)
Gave his own header and source files.

> 
> > WebKit/chromium/public/WebTextCheckingResponse.h:46
> > +    static WebTextCheckingResult makeBadGrammar(int location, int length);
> 
> It isn't clear to me what these methods do. Perhaps that could be fixed by better methods names. (The method names don't make sense right now.)
Tried another name. I'm not sure even the new name is sufficient though...

> 
> > WebKit/chromium/public/WebTextCheckingResponse.h:68
> > +    explicit WebTextCheckingResponse(int sequence, const WebVector<WebTextCheckingResult>& results)
> 
> explicit shouldn't be here.
Removed.

> 
> > WebKit/chromium/public/WebViewClient.h:176
> > +    // Requests asyncronous spell and grammar checking. The result should be returned by 
> 
> misspelled: asyncronous
Done.

> 
> > WebKit/chromium/src/EditorClientImpl.cpp:892
> > +        coreResults.append(SpellCheckingResult(static_cast<DocumentMarker::MarkerType>(results[i].type()), results[i].location(), results[0].length()));
> 
> Shouldn't results[0].length() be results[i].length()?
Oops. Done.

> 
> > WebKitTools/ChangeLog:47
> > +
> 
> There are a lot of things wrong here. I think you'll see them when you look at it.  You may find it helpful if you review the diff of your changes before getting others to do it.
I totally missed about this file...Thank you for pointing this. Fixed.

> 
> > WebKitTools/DumpRenderTree/chromium/WebViewHost.h:213
> > +    // Spellcheck realted helper APIs
> 
> spelling: realted
Done.

[Hajime Morrita]

Created attachment 76627 [details]
rebased to ToT

[Darin Fisher (:fishd, Google)]

Comment on attachment 76627 [details]
rebased to ToT

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

> WebKit/chromium/public/WebViewClient.h:178
> +    // Requests asynchronous spell and grammar checking. The result should be returned by 
> +    // WebView::respondCheckingOfString()
> +    virtual void requestTextCheck(int sequence, const WebString&) { }

nit: find-in-page APIs are somewhat similar in that there are methods on WebFrame
and WebFrameClient that work asynchronously in unison.  in that API, we use the
term "identifier" instead of "sequence", so for consistency it would be better to
do the same thing here.

another idea though is to follow the model used by WebViewClient::runFileChooser.
it defines a WebFileChooserCompletion interface that gets invoked by the embedder
when the embedder wishes to push results back into WebKit.  this model allows you
to avoid the concept of "identifier" altogether.  (it actually ends up pushing it
up to the Chromium layer where we generate IPCs, but that isn't a bad thing.)

so, going with that model, you'd end up with an API that might look like:

  WebViewClient::requestTextCheck(const WebString&, WebTextCheckCompletion*)

then,

  class WebTextCheckCompletion {
  public:
      virtual void didCheckText(const WebVector<WebTextCheckResult>&) = 0;
  protected:
      virtual ~WebTextCheckCompletion() {}
  };

the WebTextCheckCompletion object would be deleted by the didCheckText method.

one more comment:  i find the term "text check" to be quite awkward.  i think we
can do better.  maybe "languageCheck"?

[Darin Fisher (:fishd, Google)]

Comment on attachment 76627 [details]
rebased to ToT

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

> WebKit/chromium/public/WebTextCheckingResult.h:43
> +    static WebTextCheckingResult misspellingAt(int location, int length);
> +    static WebTextCheckingResult badGrammarAt(int location, int length);

these static methods need the WEBKIT_API prefix so that they can be exported from WebKit when it is built as a DLL.

please review:
http://www.mail-archive.com/[e-mail]/msg13648.html

> WebKit/chromium/public/WebTextCheckingResult.h:46
> +    int type() const { return m_type; }

it is generally better to define an enum for things like this.  then add code to
AssertMatchingEnums.cpp to ensure that the API defined enums do not get out of sync
with the WebCore defined ones.

> WebKit/chromium/public/WebTextCheckingResult.h:47
> +    int location() const { return m_location; }

nit: i think it is more common to use the term "position" or "offset" in cases like this.

> WebKit/chromium/public/WebViewClient.h:177
> +    // WebView::respondCheckingOfString()

nit: this comment is incorrect

[Hajime Morrita]

Created attachment 76733 [details]
Patch

[Hajime Morrita]

Hi Darin, thank you for reviewing!
I updated the patch. Could you take another look?

> > WebKit/chromium/public/WebTextCheckingResult.h:43
> > +    static WebTextCheckingResult misspellingAt(int location, int length);
> > +    static WebTextCheckingResult badGrammarAt(int location, int length);
> 
> these static methods need the WEBKIT_API prefix so that they can be exported from WebKit when it is built as a DLL.
Remove thsese, defining an enum instead.
> > WebKit/chromium/public/WebTextCheckingResult.h:46
> > +    int type() const { return m_type; }
> 
> it is generally better to define an enum for things like this.  then add code to
> AssertMatchingEnums.cpp to ensure that the API defined enums do not get out of sync
> with the WebCore defined ones.
Defined an enum. I didn't noticed AssertMatchingEnums.cpp. Thank you for pointing this.

> 
> > WebKit/chromium/public/WebTextCheckingResult.h:47
> > +    int location() const { return m_location; }
> 
> nit: i think it is more common to use the term "position" or "offset" in cases like this.
Renamed to position()

> 
> > WebKit/chromium/public/WebViewClient.h:177
> > +    // WebView::respondCheckingOfString()
> 
> nit: this comment is incorrect
Fixed.

[Hajime Morrita]

Created attachment 77094 [details]
update to ToT

[Hajime Morrita]

Created attachment 77967 [details]
Updated ToT.

[Hajime Morrita]

Created attachment 78649 [details]
updated ToT

[Hajime Morrita]

Created attachment 78656 [details]
addressed some feedback that is missed at the last patch

[Hajime Morrita]

(In reply to comment #8)
The last patch missed this feedback. I updated the patch to handle this.

> (From update of attachment 76627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76627&action=review
> 
> > WebKit/chromium/public/WebViewClient.h:178
> > +    // Requests asynchronous spell and grammar checking. The result should be returned by 
> > +    // WebView::respondCheckingOfString()
> > +    virtual void requestTextCheck(int sequence, const WebString&) { }
> 
> nit: find-in-page APIs are somewhat similar in that there are methods on WebFrame
> and WebFrameClient that work asynchronously in unison.  in that API, we use the
> term "identifier" instead of "sequence", so for consistency it would be better to
> do the same thing here.
Renamed "sequence" to "identifier"

> 
> another idea though is to follow the model used by WebViewClient::runFileChooser.
> it defines a WebFileChooserCompletion interface that gets invoked by the embedder
> when the embedder wishes to push results back into WebKit.  this model allows you
> to avoid the concept of "identifier" altogether.  (it actually ends up pushing it
> up to the Chromium layer where we generate IPCs, but that isn't a bad thing.)
> 
> so, going with that model, you'd end up with an API that might look like:
> 
>   WebViewClient::requestTextCheck(const WebString&, WebTextCheckCompletion*)
This makes sense. But in this case, async spellcheck will cross the process boundary 
and the pointer-as-an-identifier approach doesn't look better than the plain identifier.
I'd like to keep current design.

[Hajime Morrita]

Created attachment 79153 [details]
Patch

[Hajime Morrita]

Darin, I know you are busy, but would you able to take another look at this?

[Hajime Morrita]

ping?

[Hajime Morrita]

Created attachment 80175 [details]
updated to ToT

[Hajime Morrita]

Darin?

[Darin Fisher (:fishd, Google)]

(In reply to comment #16)
> > so, going with that model, you'd end up with an API that might look like:
> > 
> >   WebViewClient::requestTextCheck(const WebString&, WebTextCheckCompletion*)
> This makes sense. But in this case, async spellcheck will cross the process boundary 
> and the pointer-as-an-identifier approach doesn't look better than the plain identifier.
> I'd like to keep current design.

Well, I think that our WebKit API should not have baked in assumptions about being used in a multi-process browser.  It seems like you could just as easily put the map from identifier to WebTextCheckCompletion* outside in Chromium instead of within WebKit.

[Hajime Morrita]

Created attachment 81048 [details]
Patch

[Hajime Morrita]

Hi Darin, thank you for your feedback!
I updated the patch to address it.

> Well, I think that our WebKit API should not have baked in assumptions about being used in a multi-process browser.  It seems like you could just as easily put the map from identifier to WebTextCheckCompletion* outside in Chromium instead of within WebKit.
Sounds making sense. So I switched to take the completion callback pattern.

[Darin Fisher (:fishd, Google)]

Comment on attachment 81048 [details]
Patch

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

> Source/WebKit/chromium/public/WebTextCheckCompletion.h:43
> +class WebTextCheckCompletion {

nit: how about WebTextCheckingCompletion for consistency with the other interfaces, whose names begin with WebTextChecking?

> Source/WebKit/chromium/public/WebTextCheckCompletion.h:45
> +    virtual int identifier() const = 0;

it seems like identifier should just be a parameter of requestCheckingOfText just as it
is a parameter to requestCheckingOfString.

or, can you tell me why you need to expose this identifier to Chromium?  you could also
put WebTextCheckingCompletion objects in a base::IDMap within Chromium, and use the ID
vended from that for IPC purposes.  It seems like the internal identifier used by the
editing code could be replaced with an interface like WebTextCheckingCompletion instead.

> Source/WebKit/chromium/public/WebTextCheckCompletion.h:46
> +    virtual void didRespondTextCheck(const WebVector<WebTextCheckingResult>&) = 0;

nit: how about didFinishCheckingText?

> Source/WebKit/chromium/public/WebTextCheckingResult.h:43
> +        Misspelling = 1 << 0,

nit: how about a more specific enum name, like "Error"?

then you can have:

  enum Error {
      ErrorSpelling = 1 << 0,
      ErrorGrammar  = 1 << 1
  };

And the field can be:

  Error error() const { return m_error; }

> Source/WebKit/chromium/public/WebViewClient.h:182
> +    // Requests asynchronous spell and grammar checking. The result should be returned by 

nit: "spelling and grammar checking"

> Source/WebKit/chromium/public/WebViewClient.h:183
> +    // WebView::respondTextCheck()

i think this comment is now wrong.  there is no respondTextCheck method.

> Source/WebKit/chromium/public/WebViewClient.h:184
> +    virtual void requestTextCheck(const WebString&, WebTextCheckCompletion*) { }

requestTextCheck -> requestCheckingOfText perhaps?

> Source/WebKit/chromium/src/EditorClientImpl.h:194
> +

nit: no extra line here

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:414
> +static void invokeRespondLastTextCheck(void* context)

please put static helper methods at the top of the file, so that you don't
interrupt the flow of member function implementations.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:436
> +        m_spellcheck.spellCheckWord(WebString(text.characters(), text.length()), &misspelledPosition, &misspelledLength);

I think you should be able to just pass a String here and the conversion to
WebString will happen automatically.

[Hajime Morrita]

Created attachment 81182 [details]
Patch

[Hajime Morrita]

Hi Darin, Thank you for reviewing again. 
I addressed the points.

> 
> > Source/WebKit/chromium/public/WebTextCheckCompletion.h:43
> > +class WebTextCheckCompletion {
> 
> nit: how about WebTextCheckingCompletion for consistency with the other interfaces, whose names begin with WebTextChecking?
Sure. Renamed

> 
> > Source/WebKit/chromium/public/WebTextCheckCompletion.h:45
> > +    virtual int identifier() const = 0;
> 
> it seems like identifier should just be a parameter of requestCheckingOfText just as it
> is a parameter to requestCheckingOfString.
> 
> or, can you tell me why you need to expose this identifier to Chromium?  you could also
> put WebTextCheckingCompletion objects in a base::IDMap within Chromium, and use the ID
> vended from that for IPC purposes.  It seems like the internal identifier used by the
> editing code could be replaced with an interface like WebTextCheckingCompletion instead.
Good point. I thought re-numbering ID is a bit redundant. But IDMap class helps.
Thank you for pointing this. I didn't know that.
Now removed identifier() method.

> 
> > Source/WebKit/chromium/public/WebTextCheckCompletion.h:46
> > +    virtual void didRespondTextCheck(const WebVector<WebTextCheckingResult>&) = 0;
> 
> nit: how about didFinishCheckingText?
> 
Renamed.

> > Source/WebKit/chromium/public/WebTextCheckingResult.h:43
> > +        Misspelling = 1 << 0,
> 
> nit: how about a more specific enum name, like "Error"?
> 
> then you can have:
> 
>   enum Error {
>       ErrorSpelling = 1 << 0,
>       ErrorGrammar  = 1 << 1
>   };
> 
> And the field can be:
> 
>   Error error() const { return m_error; }
Done.

> 
> > Source/WebKit/chromium/public/WebViewClient.h:182
> > +    // Requests asynchronous spell and grammar checking. The result should be returned by 
> 
> nit: "spelling and grammar checking"
Fixed.

> 
> > Source/WebKit/chromium/public/WebViewClient.h:183
> > +    // WebView::respondTextCheck()
> 
> i think this comment is now wrong.  there is no respondTextCheck method.
Sure. removed.

> 
> > Source/WebKit/chromium/public/WebViewClient.h:184
> > +    virtual void requestTextCheck(const WebString&, WebTextCheckCompletion*) { }
> 
> requestTextCheck -> requestCheckingOfText perhaps?
Done.

> 
> > Source/WebKit/chromium/src/EditorClientImpl.h:194
> > +
> 
> nit: no extra line here
Done.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:414
> > +static void invokeRespondLastTextCheck(void* context)
> 
> please put static helper methods at the top of the file, so that you don't
> interrupt the flow of member function implementations.
I'm sorry for that. Moved.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:436
> > +        m_spellcheck.spellCheckWord(WebString(text.characters(), text.length()), &misspelledPosition, &misspelledLength);
> 
> I think you should be able to just pass a String here and the conversion to
> WebString will happen automatically.
I tried, but it didn't happen. It looks that WEKIT_IMPLEMENTATION is not defined in DumpRenderTree.

[Darin Fisher (:fishd, Google)]

> > I think you should be able to just pass a String here and the conversion to
> > WebString will happen automatically.
> I tried, but it didn't happen. It looks that WEKIT_IMPLEMENTATION is not defined in DumpRenderTree.

Ah, that's right!  Of course it won't work then.  Thanks for trying.

[Darin Fisher (:fishd, Google)]

Comment on attachment 81182 [details]
Patch

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

> Source/WebKit/chromium/public/WebTextCheckingCompletion.h:35
> +#include "WebTextCheckingResult.h"

nit: i think you can just forward declare WebTextCheckingResult and WebVector here instead.

> Source/WebKit/chromium/public/WebTextCheckingResult.h:35
> +#include "WebVector.h"

nit: no need for WebVector include here

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:232
> +    WebViewHost* wvh = static_cast<WebViewHost*>(context);

how do you know that the WebViewHost hasn't been deleted by the time this function is called?

[Hajime Morrita]

Created attachment 81444 [details]
Addressed the feedback

[Hajime Morrita]

Darin, thanks for your review again.
I addressed the points.

> (From update of attachment 81182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81182&action=review
> 
> > Source/WebKit/chromium/public/WebTextCheckingCompletion.h:35
> > +#include "WebTextCheckingResult.h"
> 
> nit: i think you can just forward declare WebTextCheckingResult and WebVector here instead.
Sure. Fixed.

> 
> > Source/WebKit/chromium/public/WebTextCheckingResult.h:35
> > +#include "WebVector.h"
> 
> nit: no need for WebVector include here
Done.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:232
> > +    WebViewHost* wvh = static_cast<WebViewHost*>(context);
> 
> how do you know that the WebViewHost hasn't been deleted by the time this function is called?
No.
I found there are some other place use similar trick in WebViewHost.
So I filed Bug 53899 to address them separately.
My plan is to track live WebViewHost instances and verify passed one's liveness before using it.

[Darin Fisher (:fishd, Google)]

(In reply to comment #31)
> > how do you know that the WebViewHost hasn't been deleted by the time this function is called?
> No.
> I found there are some other place use similar trick in WebViewHost.
> So I filed Bug 53899 to address them separately.
> My plan is to track live WebViewHost instances and verify passed one's liveness before using it.

OK

[Hajime Morrita]

(In reply to comment #32)
> (In reply to comment #31)
> > > how do you know that the WebViewHost hasn't been deleted by the time this function is called?
> > No.
> > I found there are some other place use similar trick in WebViewHost.
> > So I filed Bug 53899 to address them separately.
> > My plan is to track live WebViewHost instances and verify passed one's liveness before using it.
> 
> OK

Thanks! will land shortly.

[Hajime Morrita]

Committed r77875: <http://trac.webkit.org/changeset/77875>