Bug 73971 – [Chromium] WebFrame should have an interface to invoke spellchecking in arbitrarily

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

[Shinya Kawanaka]

We should have an interface to invoke spellchecking to implement the following chromium issue.
http://code.google.com/p/chromium/issues/detail?id=21225

[Shinya Kawanaka]

Created attachment 118737 [details]
Patch

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 118737 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:421
> +    virtual void requestTextChecking() = 0;

This looks a lot like the WebSpellCheckClient::requestCheckingOfText method,
although I realize that they are totally different.  It seems unfortunate
for those two methods, which are different, to have such similar names.

This seems to be a fairly high-level function.  It appears to initiate a
spelling and grammar check of the selected editable text.  What if there
is no selection?  Does it spell check anything in that case?  What if
there is nothing editable in the current selection?  Maybe answering
some of these questions could help us consider a better name for this
function?

[Shinya Kawanaka]

Created attachment 118994 [details]
Patch

[Shinya Kawanaka]

> This looks a lot like the WebSpellCheckClient::requestCheckingOfText method,
> although I realize that they are totally different.  It seems unfortunate
> for those two methods, which are different, to have such similar names.

The method of WebspellCheckClient is used to request spellchecking from WebKit to chromium, and my method is used to request spellchecking from chromium to WebKit. The former method is just to spellchecking, and the latter method does spellchecking and adding misspelling markers.

> This seems to be a fairly high-level function. 

Yeah, I'm now thinking so, too... So now I changed the code to give a node to the method. 

> It appears to initiate a
> spelling and grammar check of the selected editable text.  What if there
> is no selection?  Does it spell check anything in that case?  What if
> there is nothing editable in the current selection?  Maybe answering
> some of these questions could help us consider a better name for this
> function?

Actually what I want to solve by this patch is:
http://code.google.com/p/chromium/issues/detail?id=21225
Just for your information, I uploaded Chromium side patch in
http://codereview.chromium.org/8907016/

So what I want to do in this function is to spellcheck some area in the document (not just selected text).
If there is no node to spellcheck, this method should return immediately.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118994 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrame.h:421
> +    virtual void requestTextChecking(const WebNode&) = 0;

maybe this should take a WebElement instead?

from the chrome side, how do you know which WebFrame to call this
function on?  what if the focused editable content is actually in
a different frame?  are you using WebView::focusedFrame() for that?

> Source/WebKit/chromium/public/WebNode.h:113
> +    WEBKIT_EXPORT WebElement rootEditableElement() const;

it seems wrong for WebNode to know about elements.  maybe this should be a function
on WebElement?

[Shinya Kawanaka]

(In reply to comment #6)
> (From update of attachment 118994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118994&action=review
> 
> > Source/WebKit/chromium/public/WebFrame.h:421
> > +    virtual void requestTextChecking(const WebNode&) = 0;
> 
> maybe this should take a WebElement instead?

Thanks. It seems better.

> 
> from the chrome side, how do you know which WebFrame to call this
> function on?  what if the focused editable content is actually in
> a different frame?  are you using WebView::focusedFrame() for that?

Yes. Does it have any problem? If so, please let me know.

> 
> > Source/WebKit/chromium/public/WebNode.h:113
> > +    WEBKIT_EXPORT WebElement rootEditableElement() const;
> 
> it seems wrong for WebNode to know about elements.  maybe this should be a function
> on WebElement?

When considering TextNode, it seems natural that it knows rootEditableElement, I think.

[Shinya Kawanaka]

Created attachment 119825 [details]
Patch

[Eric Seidel (no email)]

Darin is your man.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> > from the chrome side, how do you know which WebFrame to call this
> > function on?  what if the focused editable content is actually in
> > a different frame?  are you using WebView::focusedFrame() for that?
> 
> Yes. Does it have any problem? If so, please let me know.

It should be fine.  I just wanted to make sure I understood the full picture.


> > > Source/WebKit/chromium/public/WebNode.h:113
> > > +    WEBKIT_EXPORT WebElement rootEditableElement() const;
> > 
> > it seems wrong for WebNode to know about elements.  maybe this should be a function
> > on WebElement?
> 
> When considering TextNode, it seems natural that it knows rootEditableElement, I think.

OK, I see.  That makes sense.

[Darin Fisher (:fishd, Google)]

Comment on attachment 119825 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1306
> +    RefPtr<Range> rangeToCheck = rangeOfContents(static_cast<PassRefPtr<Element> >(webElem).get());

nit: use the WebNode::unwrap helper function here.

  RefPtr<Range> rangeToCheck = rangeOfContents(webElem.unwrap<Element>.get());

[Shinya Kawanaka]

Created attachment 123239 [details]
Patch for landing

[Shinya Kawanaka]

Created attachment 123241 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123241 [details]
Patch for landing

Clearing flags on attachment: 123241

Committed r105489: <http://trac.webkit.org/changeset/105489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.