Mac: Enable "Check Spelling While Typing" in Edit menu...

Mac: Enable "Check Spelling While Typing" in Edit menu

Currently the "Edit -> Spelling and Grammar -> Check Spelling While Typing" menu item is always disabled.

The problem is that the logic to enable / disable this item lives in the render process and not in the UI.

This patch implements a generic message that the render process can send to the browser to update the state of view commands. This is used to enable and disable the "Check Spelling While Typing" menu item.

BUG=38440
TEST=Clicked in an edit box and verified that the "Check Spelling While Typing" menu item was enabled. Verified that changing the value from the context menu correctly updates the Edit menu as well.

[pink (ping after 24hrs), 2011-01-07 15:24:18]

Looks reasonable to me, you should probably talk to darin and brett about this
approach. I'm not sure what they think about caching all the commands in that
way. 

This should probably be helpful for cut/copy/paste enabling as well.

[sail, 2011-01-07 20:30:44]

Hi Darin and Brett, could you take a quick look at this patch and let you know
what you think. I'm trying to find a way to enable/disable menu items in the UI
process. My approach is for the render process to send an update state message
to the UI anytime the command's state changes.

For reference, here's how this problem is handled by the WebKit2 UI process:
http://trac.webkit.org/browser/trunk/WebKit2/UIProcess/API/mac/WKView.mm#L443

[sail, 2011-01-07 20:37:41]

I forgot to mention that this patch requires adding an extra message to
WebKit/WebKit/chromium/public/WebViewClient.h and the corresponding
EditorClientImpl.cpp file.

[brettw, 2011-01-09 00:06:06]

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host.cc:366: std::map<std::string,
CommandState>::const_iterator it =
A couple of comments on this: where does the command name come from? Can it be
some kind of enum?

It also seems weird to me that this defaults to "not enabled". This means if I
call GetStateForCommand("reload") it will return false which is probably not
what we want. This is another thing that would be helped by an enum.

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host.cc:367:
command_states_.find(command_name);
Style nit: wrapped lines get indented 4 spaces

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1921:
renderWidgetHostView_->render_widget_host_)->
Ditto for 4 space indents (below as well).

http://codereview.chromium.org/6020017/diff/8001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/renderer/render_view....
chrome/renderer/render_view.h:447: virtual void
continuousSpellCheckingEnabledStateChanged();
What do you key off of in WebKit to call this function?

[sail, 2011-01-11 18:45:51]

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host.cc:366: std::map<std::string,
CommandState>::const_iterator it =
On 2011/01/09 00:06:06, brettw wrote:
> A couple of comments on this: where does the command name come from? Can it be
> some kind of enum?
Enum sounds good. Should I put it in render_messages_internal.h?

> It also seems weird to me that this defaults to "not enabled". This means if I
> call GetStateForCommand("reload") it will return false which is probably not
> what we want. This is another thing that would be helped by an enum.
How about not enabled by default and assert if an unknown command type is sent.
If the render process hasn't update the command state yet then it would be weird
to allow the user to execute the command.

http://codereview.chromium.org/6020017/diff/8001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/renderer/render_view....
chrome/renderer/render_view.h:447: virtual void
continuousSpellCheckingEnabledStateChanged();
On 2011/01/09 00:06:06, brettw wrote:
> What do you key off of in WebKit to call this function?

Currently I'm only calling this in
EditorClientImpl::toggleContinuousSpellChecking().

[brettw, 2011-01-11 23:30:39]

Let's put it in chrome/common/command_state or something (match the enum name).
We don't want it in render_messages[_internal].h because we'll need the enum
defined in the header of various classes like RenderView, and we don't want the
render_messages to be included from the header (then the giant file gets
included implicitly in hundreds of files across the project). Plus, this command
state is used by other parts of the project that don't care about IPC.

I'm on the fence whether it's worthwhile writing a ParamTraits for this new
enum, or passing and int and static casting on both ends. The disadvantage of
the static case is that you'll need to be careful to validate it on the
receiving end so that a compromised renderer can't exploit you.

The "default to off but assert on invalid enum" approach sounds fine.

[sail, 2011-01-12 01:26:36]

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host.cc:366: std::map<std::string,
CommandState>::const_iterator it =
On 2011/01/09 00:06:06, brettw wrote:
> A couple of comments on this: where does the command name come from? Can it be
> some kind of enum?
> 
> It also seems weird to me that this defaults to "not enabled". This means if I
> call GetStateForCommand("reload") it will return false which is probably not
> what we want. This is another thing that would be helped by an enum.

Done.

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host.cc:367:
command_states_.find(command_name);
On 2011/01/09 00:06:06, brettw wrote:
> Style nit: wrapped lines get indented 4 spaces

Done.

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6020017/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1921:
renderWidgetHostView_->render_widget_host_)->
On 2011/01/09 00:06:06, brettw wrote:
> Ditto for 4 space indents (below as well).

Done.

[brettw, 2011-01-13 05:59:50]

Thanks, I like the enums better.

http://codereview.chromium.org/6020017/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:365: switch (command) {
Any reason this isn't an if statement? It will be fewer lines and if we end up
with a bunch of them, we'll probably end up writing some kind of range check
rather than individually listing them all anyway.

http://codereview.chromium.org/6020017/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:2048: switch (command) {
I'd probably use an if for these switches as well (you could do the cast at the
end when you actually use it) and it would be almost half as long:

if (checked_state != COMMAND_CHECKED_STATE_UNCHECKED &&
    checked_state != COMMAND_CHECKED_STATE_CHECKED &&
    checked_state != COMMAND_CHECKED_STATE_MIXED) {
  NOTREACHED() << "Invalid checked state " << checked_state;
  return;
}

http://codereview.chromium.org/6020017/diff/21001/chrome/common/render_view_c...
File chrome/common/render_view_commands.h (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/common/render_view_c...
chrome/common/render_view_commands.h:10: enum RenderViewCommand {
Can you add a comment here about what this is for and why it's here? Something
about the renderer process needing to send enabled/disabled states to the
browser process so the Mac menus can stay in sync will help people looking at
this file.

http://codereview.chromium.org/6020017/diff/21001/chrome/common/render_view_c...
chrome/common/render_view_commands.h:14: enum CommandCheckedState {
Can you call this RenderViewCommandCheckedState? It's kind of long, but I think
CommandCheckedState is too generic.

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2551: if
(frame->isContinuousSpellCheckingEnabled()) {
Use {} for single-line conditionals consistently. Normally this means we don't
use {}

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view...
chrome/renderer/render_view.h:1099: bool GetFocusedNode(WebKit::WebNode& node)
const;
Never use refs for out params. This should probable return a WebNode* and it
should have a comment that mentions that it will return NULL in <list cases that
it can return NULL>.

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view...
chrome/renderer/render_view.h:1157: void UpdateToggleSpellCheckCommandState();
Comment?

[sail, 2011-01-13 23:38:15]

Addressed review comments.

http://codereview.chromium.org/6020017/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:365: switch (command) {
On 2011/01/13 05:59:55, brettw wrote:
> Any reason this isn't an if statement? It will be fewer lines and if we end up
> with a bunch of them, we'll probably end up writing some kind of range check
> rather than individually listing them all anyway.

Done.

http://codereview.chromium.org/6020017/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:2048: switch (command) {
On 2011/01/13 05:59:55, brettw wrote:
> I'd probably use an if for these switches as well (you could do the cast at
the
> end when you actually use it) and it would be almost half as long:
> 
> if (checked_state != COMMAND_CHECKED_STATE_UNCHECKED &&
>     checked_state != COMMAND_CHECKED_STATE_CHECKED &&
>     checked_state != COMMAND_CHECKED_STATE_MIXED) {
>   NOTREACHED() << "Invalid checked state " << checked_state;
>   return;
> }

Done.

http://codereview.chromium.org/6020017/diff/21001/chrome/common/render_view_c...
File chrome/common/render_view_commands.h (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/common/render_view_c...
chrome/common/render_view_commands.h:10: enum RenderViewCommand {
On 2011/01/13 05:59:55, brettw wrote:
> Can you add a comment here about what this is for and why it's here? Something
> about the renderer process needing to send enabled/disabled states to the
> browser process so the Mac menus can stay in sync will help people looking at
> this file.

Done.

http://codereview.chromium.org/6020017/diff/21001/chrome/common/render_view_c...
chrome/common/render_view_commands.h:14: enum CommandCheckedState {
On 2011/01/13 05:59:55, brettw wrote:
> Can you call this RenderViewCommandCheckedState? It's kind of long, but I
think
> CommandCheckedState is too generic.

Done.

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:2551: if
(frame->isContinuousSpellCheckingEnabled()) {
On 2011/01/13 05:59:55, brettw wrote:
> Use {} for single-line conditionals consistently. Normally this means we don't
> use {}

Done.

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6020017/diff/21001/chrome/renderer/render_view...
chrome/renderer/render_view.h:1099: bool GetFocusedNode(WebKit::WebNode& node)
const;
On 2011/01/13 05:59:55, brettw wrote:
> Never use refs for out params. This should probable return a WebNode* and it
> should have a comment that mentions that it will return NULL in <list cases
that
> it can return NULL>.

Done.

[brettw, 2011-01-18 18:25:43]

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:4621: linked_ptr<WebNode>
RenderView::GetFocusedNode() const {
I've never seen us use linked_ptr in this context. In this case, I'd just return
the WebNode by value, since it has nice and efficient copy constructors, and
then the memory management is easier. We generally encourage returning by value
except on perf-critical paths.

[sail, 2011-01-18 18:41:12]

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:4621: linked_ptr<WebNode>
RenderView::GetFocusedNode() const {
On 2011/01/18 18:25:43, brettw wrote:
> I've never seen us use linked_ptr in this context. In this case, I'd just
return
> the WebNode by value, since it has nice and efficient copy constructors, and
> then the memory management is easier. We generally encourage returning by
value
> except on perf-critical paths.

Hm.. but then I wouldn't be able to indicate that there's no focused node.

[brettw, 2011-01-18 18:44:16]

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:4621: linked_ptr<WebNode>
RenderView::GetFocusedNode() const {
It has an IsNull function and you can make isNull ones with the default
constructor. It looks like WebNode is really just a smart pointer around a
WebCore::Node.

[sail, 2011-01-18 19:09:17]

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6020017/diff/32001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:4621: linked_ptr<WebNode>
RenderView::GetFocusedNode() const {
On 2011/01/18 18:44:17, brettw wrote:
> It has an IsNull function and you can make isNull ones with the default
> constructor. It looks like WebNode is really just a smart pointer around a
> WebCore::Node.

Ahh, that's awesome. I should have seen that earlier. Fixed.

[brettw, 2011-01-18 19:17:01]

LGTM with one nit on everything except the objective C part, which I'm not
qualified to review. You should probably check with pink that this is OK before
checkin.

http://codereview.chromium.org/6020017/diff/45001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6020017/diff/45001/chrome/renderer/render_view...
chrome/renderer/render_view.h:1103: WebKit::WebNode GetFocusedNode() const;
Can you provide a comment that says the node will be isNull if there is no
currently focused node?

[pink (ping after 24hrs), 2011-01-18 19:27:55]

obj-c parts LGTM

[sail, 2011-01-18 20:46:48]

http://codereview.chromium.org/6020017/diff/45001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6020017/diff/45001/chrome/renderer/render_view...
chrome/renderer/render_view.h:1103: WebKit::WebNode GetFocusedNode() const;
On 2011/01/18 19:17:01, brettw wrote:
> Can you provide a comment that says the node will be isNull if there is no
> currently focused node?

Done.

[sail, 2011-01-26 00:19:00]

Submitted as r72570