Bug 51013 - [Chromium] Should implement EditorClientImpl::requestCheckingOfString()
Summary: [Chromium] Should implement EditorClientImpl::requestCheckingOfString()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-13 22:05 PST by Hajime Morrita
Modified: 2011-02-07 17:48 PST (History)
9 users (show)

See Also:


Attachments
Patch (26.02 KB, patch)
2010-12-14 04:36 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (27.17 KB, patch)
2010-12-14 17:46 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
rebased to ToT (27.15 KB, patch)
2010-12-14 21:15 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (25.78 KB, patch)
2010-12-15 21:48 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
update to ToT (25.57 KB, patch)
2010-12-21 01:20 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Updated ToT. (25.64 KB, patch)
2011-01-04 21:19 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
updated ToT (25.65 KB, patch)
2011-01-11 21:28 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
addressed some feedback that is missed at the last patch (25.69 KB, patch)
2011-01-11 22:31 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (26.22 KB, patch)
2011-01-17 04:44 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
updated to ToT (26.18 KB, patch)
2011-01-26 01:01 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (31.66 KB, patch)
2011-02-03 03:15 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (28.60 KB, patch)
2011-02-03 20:43 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Addressed the feedback (28.65 KB, patch)
2011-02-06 20:34 PST, Hajime Morrita
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-12-13 22:05:24 PST
This is required for async spellchecking.
Comment 1 Hajime Morrita 2010-12-14 04:36:12 PST
Created attachment 76526 [details]
Patch
Comment 2 Hajime Morrita 2010-12-14 04:37:01 PST
CC levin@ because this has an WebKit API change.
Comment 3 David Levin 2010-12-14 05:22:17 PST
(In reply to comment #2)
> CC levin@ because this has an WebKit API change.
Actually that should be Darin Fisher (added).
Comment 4 David Levin 2010-12-14 05:37:30 PST
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
Comment 5 Hajime Morrita 2010-12-14 17:46:32 PST
Created attachment 76603 [details]
Patch
Comment 6 Hajime Morrita 2010-12-14 17:50:14 PST
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.
Comment 7 Hajime Morrita 2010-12-14 21:15:56 PST
Created attachment 76627 [details]
rebased to ToT
Comment 8 Darin Fisher (:fishd, Google) 2010-12-15 12:43:51 PST
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"?
Comment 9 Darin Fisher (:fishd, Google) 2010-12-15 12:44:59 PST
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/webkit-dev@lists.webkit.org/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
Comment 10 Hajime Morrita 2010-12-15 21:48:39 PST
Created attachment 76733 [details]
Patch
Comment 11 Hajime Morrita 2010-12-15 21:51:53 PST
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.
Comment 12 Hajime Morrita 2010-12-21 01:20:41 PST
Created attachment 77094 [details]
update to ToT
Comment 13 Hajime Morrita 2011-01-04 21:19:40 PST
Created attachment 77967 [details]
Updated ToT.
Comment 14 Hajime Morrita 2011-01-11 21:28:16 PST
Created attachment 78649 [details]
updated ToT
Comment 15 Hajime Morrita 2011-01-11 22:31:32 PST
Created attachment 78656 [details]
addressed some feedback that is missed at the last patch
Comment 16 Hajime Morrita 2011-01-11 22:35:53 PST
(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.
Comment 17 Hajime Morrita 2011-01-17 04:44:42 PST
Created attachment 79153 [details]
Patch
Comment 18 Hajime Morrita 2011-01-17 04:53:19 PST
Darin, I know you are busy, but would you able to take another look at this?
Comment 19 Hajime Morrita 2011-01-20 18:14:40 PST
ping?
Comment 20 Hajime Morrita 2011-01-26 01:01:14 PST
Created attachment 80175 [details]
updated to ToT
Comment 21 Hajime Morrita 2011-01-26 17:55:43 PST
Darin?
Comment 22 Darin Fisher (:fishd, Google) 2011-02-02 13:14:57 PST
(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.
Comment 23 Hajime Morrita 2011-02-03 03:15:53 PST
Created attachment 81048 [details]
Patch
Comment 24 Hajime Morrita 2011-02-03 03:18:37 PST
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.
Comment 25 Darin Fisher (:fishd, Google) 2011-02-03 14:48:42 PST
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.
Comment 26 Hajime Morrita 2011-02-03 20:43:20 PST
Created attachment 81182 [details]
Patch
Comment 27 Hajime Morrita 2011-02-03 20:48:20 PST
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.
Comment 28 Darin Fisher (:fishd, Google) 2011-02-04 10:34:23 PST
> > 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.
Comment 29 Darin Fisher (:fishd, Google) 2011-02-04 10:37:52 PST
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?
Comment 30 Hajime Morrita 2011-02-06 20:34:04 PST
Created attachment 81444 [details]
Addressed the feedback
Comment 31 Hajime Morrita 2011-02-06 20:44:30 PST
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.
Comment 32 Darin Fisher (:fishd, Google) 2011-02-07 12:54:40 PST
(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
Comment 33 Hajime Morrita 2011-02-07 16:59:38 PST
(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.
Comment 34 Hajime Morrita 2011-02-07 17:48:03 PST
Committed r77875: <http://trac.webkit.org/changeset/77875>