Bug 52680

Created attachment 79352 [details] Patch

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/

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

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.

(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

Comment on attachment 79352 [details] Patch r=me now that I understand why this change is needed.

Comment on attachment 79352 [details] Patch Oops, please update ChangeLog.

Created attachment 79353 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 79353 [details]: http/tests/appcache/fallback.html bug 52710 (authors: ap@webkit.org and michaeln@google.com) The commit-queue is continuing to process your patch.

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

Created attachment 79425 [details] Patch

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.

Comment on attachment 79425 [details] Patch Usually, the commit queue fills in the reviewer automatically. I wonder why it didn't this time.

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.)

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?

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.

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.

Created attachment 79452 [details] Patch

(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.

Comment on attachment 79452 [details] Patch Clearing flags on attachment: 79452 Committed r76377: <http://trac.webkit.org/changeset/76377>

All reviewed patches have been landed. Closing bug.