Bug 52680 – [chromium] Notify WebViewClient when spellcheck state changes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sailesh Agrawal]

[chromium] Event for spellcheck menu

[Sailesh Agrawal]

Created attachment 79352 [details]
Patch

[Sailesh Agrawal]

This is a part of the Chromium change to enable the "Check Spelling While Typing" menu item. It adds a event to allow clients to know when the continuous spell checking state has changed. See:
http://codereview.chromium.org/6020017/

[Nico Weber]

Looks good to me, but I'm not a reviewer.

[Ryosuke Niwa]

Comment on attachment 79352 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:192
> +    // This method is called when continuous spelling is enabled or disabled.
> +    virtual void continuousSpellCheckingEnabledStateChanged() { }
> +

I don't think this should be a public member function.  It should at least be private.

[Ryosuke Niwa]

(In reply to comment #4)
> (From update of attachment 79352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79352&action=review
> 
> > Source/WebKit/chromium/public/WebViewClient.h:192
> > +    // This method is called when continuous spelling is enabled or disabled.
> > +    virtual void continuousSpellCheckingEnabledStateChanged() { }
> > +
> 
> I don't think this should be a public member function.  It should at least be private.

I take it back.  I thought this was added to WebEditorClient.h

[Ryosuke Niwa]

Comment on attachment 79352 [details]
Patch

r=me now that I understand why this change is needed.

[Ryosuke Niwa]

Comment on attachment 79352 [details]
Patch

Oops, please update ChangeLog.

[Sailesh Agrawal]

Created attachment 79353 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79353 [details]:

http/tests/appcache/fallback.html bug 52710 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 79353 [details]
Patch

Rejecting attachment 79353 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'la..." exit_code: 1

Last 500 characters of output:
n/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 76109 = 060014058f4dca178941270f8f25cad11e3c5110
r76110 = cfbc645577b3375c8578ec30aeacdbee54eb4fca
r76111 = 74f79aced1389d503321aa45d3fd024f20010694
r76112 = da68231a03735b50fd0711d1aa0d4939131b9a02
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://queues.webkit.org/results/7584193

[Sailesh Agrawal]

Created attachment 79425 [details]
Patch

[WebKit Review Bot]

Attachment 79425 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nico Weber]

Comment on attachment 79425 [details]
Patch

Usually, the commit queue fills in the reviewer automatically. I wonder why it didn't this time.

[Nico Weber]

kling on IRC tells me that the tools didn't work in this case because patch set 1 had "r+ with comments", and the follow-up patch only had cq+. The tools only work if the final patch has the r+ bit set; i.e. if you get an "r+ with comments" and a non-reviewer sets cq+, you need to "manually" fill in the reviewer when you address the comments. (There's a script that can do this for you iirc.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 79425 [details]
Patch

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

> Source/WebKit/chromium/public/WebViewClient.h:191
> +    virtual void continuousSpellCheckingEnabledStateChanged() { }

This function name is a real mouthful.  We also have a naming convention for methods like this, where they start with "didChange" instead.

So, didChangeContinuousSpellCheckingEnabledState would be more consistent.  I still find this method name to be overly verbose.

Also, this method is on WebViewClient.h, and yet the corresponding methods to query the "spellchecking" state lives on WebFrame.
shouldn't this notification method live on WebFrameClient, then?

[Darin Fisher (:fishd, Google)]

How about didToggleContinuousSpellChecking as a name?  That is nice because "toggle" isn't precise about whether the feature was enabled or disabled.  You have to query WebFrame::isContinuousSpellCheckingEnabled.  Also, "toggle" is used by the corresponding EditorClient method.

[Darin Fisher (:fishd, Google)]

One more comment:

Should WebFrameClient::didToggleContinuousSpellChecking be called in response to WebFrame::enableContinousSpellChecking?  I'm assuming that you don't need it to be, but just from studying the interfaces it is not obvious that it behaves this way.  You should add a comment explaining this subtle behavior.

[Sailesh Agrawal]

Created attachment 79452 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #17)
> One more comment:
> 
> Should WebFrameClient::didToggleContinuousSpellChecking be called in response to WebFrame::enableContinousSpellChecking?  I'm assuming that you don't need it to be, but just from studying the interfaces it is not obvious that it behaves this way.  You should add a comment explaining this subtle behavior.

I added a comment to WebFrameImpl::enableContinuousSpellChecking() noting that didToggleContinuousSpellChecking is called in EditorClientImpl.

[WebKit Commit Bot]

Comment on attachment 79452 [details]
Patch

Clearing flags on attachment: 79452

Committed r76377: <http://trac.webkit.org/changeset/76377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.