Integrate the Spelling service to Chrome.

Integrate the Spelling service to Chrome.
This change integrates the Spelling service (a.k.a. "Did you mean") to a context-menu of Chrome so we can see the spell-check result retrieved from the Spelling service. This change sends a JSON-RPC request to the service in the background and update a context-menu item while showing to prevent junkiness.

BUG=29191, 93746
TEST=right-click a misspelled word and see it is updated.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99633

[Hironori Bono, 2011-08-23 10:14:47]

Greetings,

Unfortunately, this change becomes huge and needs approvals of several
reviewers. To describe the overview of this change, it roughly consists of four
parts:
* Adding an interface (RenderViewContextMenuDelegate) that allows updating
context-menu items while showing.
* Adding an observer (RenderViewContextMenuObserver) that allows adding
context-menu items without changing the RenderViewContextMenu class.
* Implementing the new classes for the Spelling service.
* Adding an option page that enables/disables this feature.
It would be definitely helpful to review this change and give me your comments.

Regards,

Hironori Bono

[Ben Goodger (Google), 2011-08-23 16:12:52]

I'm going to let others with more recent experience with the contents world
(e.g. Avi) review the functionality here, but a couple of comments:

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:535: Loading spelling suggestion
append an ellipsis ...

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8848: Use a web serviece to help resolve
spelling errors
service

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/spelling_menu_observer.cc (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.cc:43: #if
defined(GOOGLE_CHROME_BUILD)
I do wonder if this code shouldn't be ifdefed like this but rather controlled by
a preference that is set to true in a GOOGLE_CHROME_BUILD.

That way it's easier for developers to test and debug this code in typical
Chromium builds just by flipping the bit, rather than having to do a complete
new build with the official build flag set.

[Avi (use Gerrit), 2011-08-23 18:38:19]

Your approach looks reasonable. Noted spelling errors, but I have a naming
question.

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:534: <message
name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message
shown while the Spelling servie is checking text">
typo: service

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:734: <message
name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message
shown while the Spelling servie is checking text">
same typo here

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:40: // This interface is
designed mainly for controling the instance while showing
typo: controlling

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:63: //       bool enabled
= respose == 200;
response

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:89: class
RenderViewContextMenuDelegate {
This naming seems backwards from most usage of the term "delegate". An
XXXXDelegate is implemented by a class that "owns" the XXXX, and is consulted by
the XXXX for policy decisions/upward delegation.

For example, see TabContents/TabContentsDelegate,
RenderViewHost/RenderViewHostDelegate,
RenderViewHostManager/RenderViewHostManager::Delegate, etc. I'm not aware of an
XXXXDelegate interface that the XXXX itself implements.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:94: // Update the stauts
and text of the specified context-menu item.
status

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu_mac.mm (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu_mac.mm:145: [item
setEnabled:enabled ? YES : NO];
It's safe to just pass in enabled here; no need to convert it.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu_observer.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu_observer.h:27: //
RenderViewContetMenu class directly.)
typo in class name

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu_observer.h:89: // listens
the specified command ID. If this function returns true, the
listens for the specified...

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/spelling_menu_observer.cc (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.cc:46: // Exit if we are not
in an ediable element because we add a menu item only
s/ediable/editable/

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.cc:123: // Format the JSON
request to be sent to the Speling service.
s/Speling/Spelling/

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/spelling_menu_observer.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:20: // An observer that
listens events from the RenderViewContextMenu class and show
...listens to events... and shows...

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:25: // *
URLFethcer::Delegate
URLFetcher

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:28: // these interfaces
allows this class to make a JSON-RPC call to the Spelling
Capitalize "these". allows->allow.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:29: // service in the
background and updates the context menu while showing. The
s/updates/update/

[Miranda Callahan, 2011-08-23 18:57:49]

On 2011/08/23 18:38:19, Avi wrote:
> Your approach looks reasonable. Noted spelling errors, but I have a naming
> question.
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:534: <message
> name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message
> shown while the Spelling servie is checking text">
> typo: service
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:734: <message
> name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message
> shown while the Spelling servie is checking text">
> same typo here
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/render_view_context_menu.h (right):
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu.h:40: // This interface
is
> designed mainly for controling the instance while showing
> typo: controlling
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu.h:63: //       bool
enabled
> = respose == 200;
> response
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu.h:89: class
> RenderViewContextMenuDelegate {
> This naming seems backwards from most usage of the term "delegate". An
> XXXXDelegate is implemented by a class that "owns" the XXXX, and is consulted
by
> the XXXX for policy decisions/upward delegation.
> 
> For example, see TabContents/TabContentsDelegate,
> RenderViewHost/RenderViewHostDelegate,
> RenderViewHostManager/RenderViewHostManager::Delegate, etc. I'm not aware of
an
> XXXXDelegate interface that the XXXX itself implements.
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu.h:94: // Update the
stauts
> and text of the specified context-menu item.
> status
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/render_view_context_menu_mac.mm (right):
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu_mac.mm:145: [item
> setEnabled:enabled ? YES : NO];
> It's safe to just pass in enabled here; no need to convert it.
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/render_view_context_menu_observer.h (right):
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu_observer.h:27: //
> RenderViewContetMenu class directly.)
> typo in class name
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/render_view_context_menu_observer.h:89: // listens
> the specified command ID. If this function returns true, the
> listens for the specified...
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/spelling_menu_observer.cc (right):
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/spelling_menu_observer.cc:46: // Exit if we are
not
> in an ediable element because we add a menu item only
> s/ediable/editable/
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/spelling_menu_observer.cc:123: // Format the JSON
> request to be sent to the Speling service.
> s/Speling/Spelling/
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/spelling_menu_observer.h (right):
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/spelling_menu_observer.h:20: // An observer that
> listens events from the RenderViewContextMenu class and show
> ...listens to events... and shows...
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/spelling_menu_observer.h:25: // *
> URLFethcer::Delegate
> URLFetcher
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/spelling_menu_observer.h:28: // these interfaces
> allows this class to make a JSON-RPC call to the Spelling
> Capitalize "these". allows->allow.
> 
>
http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/spelling_menu_observer.h:29: // service in the
> background and updates the context menu while showing. The
> s/updates/update/

LGTM as Profiles OWNER -- adding the pref looks fine (though I like beng's
suggestion above about avoiding #ifdefs...)

[Hironori Bono, 2011-08-25 11:18:58]

Greetings Ben, Avi, and Miranda,

Thank you so much for your review and comments. (It is embarrassing for me that
a spellchecker change has lots of misspellings.)
By the way, even though I have removed most of ifdefs, I assume our server
cannot (probably) afford to provide this Spelling service both to Chrome and to
Chromium at the beginning. To avoid server issues, this change provides the
server information only to Chrome until our server can afford to provide this
service also to Chromium. Sorry for your inconvenience.

Regards,

Hironori Bono

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:534: <message
name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message
shown while the Spelling servie is checking text">
On 2011/08/23 18:38:19, Avi wrote:
> typo: service

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:535: Loading spelling suggestion
On 2011/08/23 16:12:52, Ben Goodger (Google) wrote:
> append an ellipsis ...

This change appends an ellipsis in
SpellingMenuObserver::OnAnimationTimerExpired() for 'Loading...' animation. (The
current test server sometimes takes several seconds to send a response because
of its QPS limitation.) Is it better to add an ellipsis here and use better
animation?

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:734: <message
name="IDS_CONTENT_CONTEXT_SPELLING_CHECKING" desc="The place-holder message
shown while the Spelling servie is checking text">
On 2011/08/23 18:38:19, Avi wrote:
> same typo here

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8848: Use a web serviece to help resolve
spelling errors
On 2011/08/23 16:12:52, Ben Goodger (Google) wrote:
> service

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:40: // This interface is
designed mainly for controling the instance while showing
On 2011/08/23 18:38:19, Avi wrote:
> typo: controlling

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:63: //       bool enabled
= respose == 200;
On 2011/08/23 18:38:19, Avi wrote:
> response

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:89: class
RenderViewContextMenuDelegate {
On 2011/08/23 18:38:19, Avi wrote:
> This naming seems backwards from most usage of the term "delegate". An
> XXXXDelegate is implemented by a class that "owns" the XXXX, and is consulted
by
> the XXXX for policy decisions/upward delegation.
> 
> For example, see TabContents/TabContentsDelegate,
> RenderViewHost/RenderViewHostDelegate,
> RenderViewHostManager/RenderViewHostManager::Delegate, etc. I'm not aware of
an
> XXXXDelegate interface that the XXXX itself implements.

Thank you for your description. This is just caused by my poor naming sense.
Even though I have renamed this interface to RenderViewContextMenuProxy, I'm
wondering if it is good. Is it possible to give me suggestions if it does not
make sense to you?

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:94: // Update the stauts
and text of the specified context-menu item.
On 2011/08/23 18:38:19, Avi wrote:
> status

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu_mac.mm (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu_mac.mm:145: [item
setEnabled:enabled ? YES : NO];
On 2011/08/23 18:38:19, Avi wrote:
> It's safe to just pass in enabled here; no need to convert it.

Done. Thank you for noticing it. I misunderstood our coding style.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu_observer.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu_observer.h:27: //
RenderViewContetMenu class directly.)
On 2011/08/23 18:38:19, Avi wrote:
> typo in class name

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu_observer.h:89: // listens
the specified command ID. If this function returns true, the
On 2011/08/23 18:38:19, Avi wrote:
> listens for the specified...

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/spelling_menu_observer.cc (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.cc:43: #if
defined(GOOGLE_CHROME_BUILD)
On 2011/08/23 16:12:52, Ben Goodger (Google) wrote:
> I do wonder if this code shouldn't be ifdefed like this but rather controlled
by
> a preference that is set to true in a GOOGLE_CHROME_BUILD.
> 
> That way it's easier for developers to test and debug this code in typical
> Chromium builds just by flipping the bit, rather than having to do a complete
> new build with the official build flag set.

Done. I have removed all ifdefs except the above code which includes an internal
header and the default value of the new profile. (The above header file includes
the URL to our test server and its access key.)

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.cc:46: // Exit if we are not
in an ediable element because we add a menu item only
On 2011/08/23 18:38:19, Avi wrote:
> s/ediable/editable/

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.cc:123: // Format the JSON
request to be sent to the Speling service.
On 2011/08/23 18:38:19, Avi wrote:
> s/Speling/Spelling/

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/spelling_menu_observer.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:20: // An observer that
listens events from the RenderViewContextMenu class and show
On 2011/08/23 18:38:19, Avi wrote:
> ...listens to events... and shows...

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:25: // *
URLFethcer::Delegate
On 2011/08/23 18:38:19, Avi wrote:
> URLFetcher

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:28: // these interfaces
allows this class to make a JSON-RPC call to the Spelling
On 2011/08/23 18:38:19, Avi wrote:
> Capitalize "these". allows->allow.

Done.

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/spelling_menu_observer.h:29: // service in the
background and updates the context menu while showing. The
On 2011/08/23 18:38:19, Avi wrote:
> s/updates/update/

Done.

[Avi (use Gerrit), 2011-08-25 14:59:11]

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:89: class
RenderViewContextMenuDelegate {
That's better. I'm struggling to come up with a good name; what do you call an
interface that implements a menu?

[Avi (use Gerrit), 2011-08-25 18:20:56]

In any case, LGTM from me.

[Hironori Bono, 2011-08-29 08:17:04]

Greetings Avi,

Thank you for your review. Even though I have not updated this change, I have
added my comment to reply your question.

Ben and Erik,

Would it be possible to review my views change and resource change,
respectively? (Needless to say, it would be helpful for you to review other
parts as well.)

Regards,

Hironori Bono

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/7713033/diff/1008/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.h:89: class
RenderViewContextMenuDelegate {
On 2011/08/25 14:59:12, Avi wrote:
> That's better. I'm struggling to come up with a good name; what do you call an
> interface that implements a menu?

It is a good question. I initially tried to split this RenderViewContextMenu
into a model, a view, and a controller so we can denoted this interface as
RenderViewContextMenuController. Unfortunately, while implementing this change,
I wondered if this is really a controller and started finding better names. This
is the beginning of my naming struggle.

[Avi (use Gerrit), 2011-08-29 12:54:42]

Well, LGTM with the name you've got now.

We're moving pieces of the view into content (rwhv and friends) and I have a
feeling that this class may need to be broken up and parts moved in there
too. We'll get back to it, and probably sooner than later.

Avi

[Ben Goodger (Google), 2011-08-30 17:40:35]

LGTM for the views btis

http://codereview.chromium.org/7713033/diff/20001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h
(right):

http://codereview.chromium.org/7713033/diff/20001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:37: //
RenderViewContextMenuDelegate implementation.
Match the header style of the other implementations in this file, or
(preferably) make the other implementations match this style...

http://codereview.chromium.org/7713033/diff/20001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:43: //
RenderViewContextMenu implementation --------------------------------------
nix the ----

[Hironori Bono, 2011-09-01 10:33:05]

Greetings Ben,

Thank you for your review and comments. I have updated the styles of the other
override functions as you suggested.

Chris and Evan,

Would it be possible to review the "options" change of this change? (It just
adds an option that disables this feature.)

Regards,

Hironori Bono

http://codereview.chromium.org/7713033/diff/20001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h
(right):

http://codereview.chromium.org/7713033/diff/20001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:37: //
RenderViewContextMenuDelegate implementation.
On 2011/08/30 17:40:35, Ben Goodger (Google) wrote:
> Match the header style of the other implementations in this file, or
> (preferably) make the other implementations match this style...

Done. Thanks you for noticing it. I have made the other override functions to
match it. (I have also added OVERRIDE to them.)

http://codereview.chromium.org/7713033/diff/20001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h:43: //
RenderViewContextMenu implementation --------------------------------------
On 2011/08/30 17:40:35, Ben Goodger (Google) wrote:
> nix the ----

Done.

[csilv, 2011-09-01 16:42:13]

Options related changes, LGTM.