Implement a useful context menu for the blocked plug-in frame:...

Implement a useful context menu for the blocked plug-in frame:
- Offers option to run plugin (clickjack-proof).
- Offers option to hide blocking placeholder.
- Retains Inspect Element; it's also useful to see the <embed>.

BUG=NONE
TEST=NONE


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68719

[Chris Evans, 2010-12-07 01:48:59]

Finally got it working to my satisfaction :)

[brettw, 2010-12-07 06:23:56]

In the future please be sure to add somebody who's an expert on RenderView when
submitting a patch like this.

Mostly I have style nits:

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.cc
File chrome/renderer/blocked_plugin.cc (right):

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:92: WebContextMenuData menuData;
Local variables should be lower_case_with_underscores.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:93: WebVector<WebMenuItemInfo>
customItems(static_cast<size_t>(4));
I think you can just use custom_items(4) here? If not, try 4u.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:94: {
Normally we wouldn't use {} here. Just separate the blocks with newlines.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.h
File chrome/renderer/blocked_plugin.h (right):

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.h:32: void menuOptionSelected(unsigned id);
This should be capitalized. I'd probably also put a blank line abofe this since
it's not at all related to plugin().

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.h:62: string16 name_;
Can you provide a comment for this? It's very generic.

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

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:816: if (plugin_menu_event_listener_ == dead) {
Don't use {} for single-line conditionals to match surrouding code.

http://codereview.chromium.org/5639004/diff/1/webkit/glue/plugins/webview_plu...
File webkit/glue/plugins/webview_plugin.h (right):

http://codereview.chromium.org/5639004/diff/1/webkit/glue/plugins/webview_plu...
webkit/glue/plugins/webview_plugin.h:45: virtual void ShowContextMenu(const
WebKit::WebMouseEvent&) = 0;
This can be forward-declared like WebPreferences to avoid having to include
WebInputEvent.h in this header.

[Chris Evans, 2010-12-07 15:36:14]

Thanks for the review, Brett. Got it re: render_view.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.cc
File chrome/renderer/blocked_plugin.cc (right):

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:92: WebContextMenuData menuData;
On 2010/12/07 06:23:56, brettw wrote:
> Local variables should be lower_case_with_underscores.

Done.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:93: WebVector<WebMenuItemInfo>
customItems(static_cast<size_t>(4));
On 2010/12/07 06:23:56, brettw wrote:
> I think you can just use custom_items(4) here? If not, try 4u.

4u doesn't work either (on 64-bit); the constructor in question is marked
explicit. 4UL works, but I think there are 64-bit platforms where
sizeof(long)==4 so I think the explicit cast to size_t is safest long term.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:94: {
On 2010/12/07 06:23:56, brettw wrote:
> Normally we wouldn't use {} here. Just separate the blocks with newlines.

Done.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.h
File chrome/renderer/blocked_plugin.h (right):

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.h:32: void menuOptionSelected(unsigned id);
On 2010/12/07 06:23:56, brettw wrote:
> This should be capitalized. I'd probably also put a blank line abofe this
since
> it's not at all related to plugin().

Done.

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.h:62: string16 name_;
On 2010/12/07 06:23:56, brettw wrote:
> Can you provide a comment for this? It's very generic.

Done.

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

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:816: if (plugin_menu_event_listener_ == dead) {
On 2010/12/07 06:23:56, brettw wrote:
> Don't use {} for single-line conditionals to match surrouding code.

Done.

http://codereview.chromium.org/5639004/diff/1/webkit/glue/plugins/webview_plu...
File webkit/glue/plugins/webview_plugin.h (right):

http://codereview.chromium.org/5639004/diff/1/webkit/glue/plugins/webview_plu...
webkit/glue/plugins/webview_plugin.h:45: virtual void ShowContextMenu(const
WebKit::WebMouseEvent&) = 0;
On 2010/12/07 06:23:56, brettw wrote:
> This can be forward-declared like WebPreferences to avoid having to include
> WebInputEvent.h in this header.

I don't believe this is possible, because WebMouseEvent is in the WebKit
namespace. WebPreferences can only be forward declared because it is outside a
namespace.

[cevans, 2010-12-07 15:41:40]

On Tue, Dec 7, 2010 at 7:36 AM, <cevans@chromium.org> wrote:

> Thanks for the review, Brett. Got it re: render_view.
>
>
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.cc
> File chrome/renderer/blocked_plugin.cc (right):
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
> chrome/renderer/blocked_plugin.cc:92: WebContextMenuData menuData;
> On 2010/12/07 06:23:56, brettw wrote:
>
>> Local variables should be lower_case_with_underscores.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
> chrome/renderer/blocked_plugin.cc:93: WebVector<WebMenuItemInfo>
> customItems(static_cast<size_t>(4));
> On 2010/12/07 06:23:56, brettw wrote:
>
>> I think you can just use custom_items(4) here? If not, try 4u.
>>
>
> 4u doesn't work either (on 64-bit); the constructor in question is
> marked explicit. 4UL works, but I think there are 64-bit platforms where
> sizeof(long)==4 so I think the explicit cast to size_t is safest long
> term.
>
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
> chrome/renderer/blocked_plugin.cc:94: {
> On 2010/12/07 06:23:56, brettw wrote:
>
>> Normally we wouldn't use {} here. Just separate the blocks with
>>
> newlines.
>
> Done.
>
>
>
> http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.h
> File chrome/renderer/blocked_plugin.h (right):
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
> chrome/renderer/blocked_plugin.h:32: void menuOptionSelected(unsigned
> id);
> On 2010/12/07 06:23:56, brettw wrote:
>
>> This should be capitalized. I'd probably also put a blank line abofe
>>
> this since
>
>> it's not at all related to plugin().
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
> chrome/renderer/blocked_plugin.h:62: string16 name_;
> On 2010/12/07 06:23:56, brettw wrote:
>
>> Can you provide a comment for this? It's very generic.
>>
>
> Done.
>
>
>
> http://codereview.chromium.org/5639004/diff/1/chrome/renderer/render_view.cc
> File chrome/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/render_view.cc#...
> chrome/renderer/render_view.cc:816: if (plugin_menu_event_listener_ ==
> dead) {
> On 2010/12/07 06:23:56, brettw wrote:
>
>> Don't use {} for single-line conditionals to match surrouding code.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5639004/diff/1/webkit/glue/plugins/webview_plu...
> File webkit/glue/plugins/webview_plugin.h (right):
>
>
>
http://codereview.chromium.org/5639004/diff/1/webkit/glue/plugins/webview_plu...
> webkit/glue/plugins/webview_plugin.h:45: virtual void
> ShowContextMenu(const WebKit::WebMouseEvent&) = 0;
> On 2010/12/07 06:23:56, brettw wrote:
>
>> This can be forward-declared like WebPreferences to avoid having to
>>
> include
>
>> WebInputEvent.h in this header.
>>
>
> I don't believe this is possible, because WebMouseEvent is in the WebKit
> namespace. WebPreferences can only be forward declared because it is
> outside a namespace.


Ah, I see that gcc accepts
namespace WebKit {
  struct WebMouseEvent;
}

I didn't know that was valid C++. gcc likes it, so I'll run it through the
Win + Mac try bots and use it if successful.

Cheers
Chris


>
> http://codereview.chromium.org/5639004/
>

[brettw, 2010-12-07 16:44:17]

> Ah, I see that gcc accepts
> namespace WebKit {
>   struct WebMouseEvent;
> }
> 
> I didn't know that was valid C++. gcc likes it, so I'll run it through the
> Win + Mac try bots and use it if successful.

Yes, that's legal. Don't indent the stuff inside the {} for namespaces (Google
style).

[brettw, 2010-12-07 16:53:51]

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.cc
File chrome/renderer/blocked_plugin.cc (right):

http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
chrome/renderer/blocked_plugin.cc:93: WebVector<WebMenuItemInfo>
customItems(static_cast<size_t>(4));
4u should be OK. size_t is equal to or larger than an unsigned so the compiler
can convert if necessary without you making it explicit. Explicit doesn't affect
type conversion, that just prevents a size_t from being implicitly converted to
a WebVector when e.g. calling a function. Since you're calling the constructor
explicitly here, the "explicit" condition is met.

[Bernhard Bauer, 2010-12-07 17:03:32]

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

http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:812: plugin_menu_event_listener_ = listening;
I'm not happy that we add a generic menu event listener interface here, and then
make it depend on something as specific as BlockedPlugin. How do regular plugins
or extensions do it?

[cevans, 2010-12-07 21:12:37]

On Tue, Dec 7, 2010 at 8:44 AM, <brettw@chromium.org> wrote:

> Ah, I see that gcc accepts
>> namespace WebKit {
>>   struct WebMouseEvent;
>> }
>>
>
>  I didn't know that was valid C++. gcc likes it, so I'll run it through the
>> Win + Mac try bots and use it if successful.
>>
>
> Yes, that's legal. Don't indent the stuff inside the {} for namespaces
> (Google
> style).


Done, thanks. I was confused between namespaces and nested classes.


>
>
> http://codereview.chromium.org/5639004/
>

[cevans, 2010-12-07 21:20:10]

On Tue, Dec 7, 2010 at 8:53 AM, <brettw@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin.cc
> File chrome/renderer/blocked_plugin.cc (right):
>
>
>
http://codereview.chromium.org/5639004/diff/1/chrome/renderer/blocked_plugin....
> chrome/renderer/blocked_plugin.cc:93: WebVector<WebMenuItemInfo>
> customItems(static_cast<size_t>(4));
> 4u should be OK. size_t is equal to or larger than an unsigned so the
> compiler can convert if necessary without you making it explicit.
>

4u does not compile, 64-bit Linux on Lucid:
In file included from
./third_party/WebKit/WebKit/chromium/public/WebPluginParams.h:36,
                 from ./chrome/renderer/blocked_plugin.h:11,
                 from chrome/renderer/blocked_plugin.cc:5:
./third_party/WebKit/WebKit/chromium/public/WebVector.h: In constructor
‘WebKit::WebVector<T>::WebVector(const C&) [with C = unsigned int, T =
WebKit::WebMenuItemInfo]’:
chrome/renderer/blocked_plugin.cc:93:   instantiated from here
./third_party/WebKit/WebKit/chromium/public/WebVector.h:84: error:
subscripted value is neither array nor pointer
./third_party/WebKit/WebKit/chromium/public/WebVector.h:84: error: request
for member ‘size’ in ‘other’, which is of non-class type ‘const unsigned
int’
./third_party/WebKit/WebKit/chromium/public/WebVector.h:84: error: request
for member ‘size’ in ‘other’, which is of non-class type ‘const unsigned
int’

Explicit doesn't affect type conversion, that just prevents a size_t
> from being implicitly converted to a WebVector when e.g. calling a
> function. Since you're calling the constructor explicitly here, the
> "explicit" condition is met.
>
>
> http://codereview.chromium.org/5639004/
>

[cevans, 2010-12-07 21:35:10]

On Tue, Dec 7, 2010 at 9:03 AM, <bauerb@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view.cc
> File chrome/renderer/render_view.cc (right):
>
>
>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view...
> chrome/renderer/render_view.cc:812: plugin_menu_event_listener_ =
> listening;
> I'm not happy that we add a generic menu event listener interface here,
> and then make it depend on something as specific as BlockedPlugin.


How about I add a simple interface, which BlockedPlugin happens to
implement? An early revision of the patch had this; I probably shouldn't
have removed it.

How
> do regular plugins or extensions do it?


I spent a couple of days poking around at this, but couldn't find anything
more useful or cleaner. The standard plugin handler, for example, simply
swallows context menu events. Any NPAPI plugin that show you a context menu
are probably doing it via a native window, I'd guess?

One possibility might be to hack up WebKit/chromium/public/WebView to expose
more of WebKit's menu handling, but I rejected that as complicated and not
even guaranteed to work :)


Cheers
Chris



>
> http://codereview.chromium.org/5639004/
>

[Bernhard Bauer, 2010-12-07 23:48:09]

On Tue, Dec 7, 2010 at 22:35, Chris Evans <cevans@google.com> wrote:
> On Tue, Dec 7, 2010 at 9:03 AM, <bauerb@chromium.org> wrote:
>>
>>
>>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view.cc
>> File chrome/renderer/render_view.cc (right):
>>
>>
>>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view...
>> chrome/renderer/render_view.cc:812: plugin_menu_event_listener_ =
>> listening;
>> I'm not happy that we add a generic menu event listener interface here,
>> and then make it depend on something as specific as BlockedPlugin.
>
> How about I add a simple interface, which BlockedPlugin happens to
> implement? An early revision of the patch had this; I probably shouldn't
> have removed it.
>>
>> How
>> do regular plugins or extensions do it?
>
> I spent a couple of days poking around at this, but couldn't find anything
> more useful or cleaner. The standard plugin handler, for example, simply
> swallows context menu events. Any NPAPI plugin that show you a context menu
> are probably doing it via a native window, I'd guess?

Ugh. That's totally ugly, and at the same time frighteningly likely ;-)

Something like a more generic context menu interface might be nice.

> One possibility might be to hack up WebKit/chromium/public/WebView to expose
> more of WebKit's menu handling, but I rejected that as complicated and not
> even guaranteed to work :)
>
> Cheers
> Chris
>
>>
>>
>> http://codereview.chromium.org/5639004/
>
>

[cevans, 2010-12-08 02:00:53]

On Tue, Dec 7, 2010 at 3:47 PM, Bernhard Bauer <bauerb@chromium.org> wrote:

> On Tue, Dec 7, 2010 at 22:35, Chris Evans <cevans@google.com> wrote:
> > On Tue, Dec 7, 2010 at 9:03 AM, <bauerb@chromium.org> wrote:
> >>
> >>
> >>
>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view.cc
> >> File chrome/renderer/render_view.cc (right):
> >>
> >>
> >>
>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view...
> >> chrome/renderer/render_view.cc:812: plugin_menu_event_listener_ =
> >> listening;
> >> I'm not happy that we add a generic menu event listener interface here,
> >> and then make it depend on something as specific as BlockedPlugin.
> >
> > How about I add a simple interface, which BlockedPlugin happens to
> > implement? An early revision of the patch had this; I probably shouldn't
> > have removed it.
> >>
> >> How
> >> do regular plugins or extensions do it?
> >
> > I spent a couple of days poking around at this, but couldn't find
> anything
> > more useful or cleaner. The standard plugin handler, for example, simply
> > swallows context menu events. Any NPAPI plugin that show you a context
> menu
> > are probably doing it via a native window, I'd guess?
>
> Ugh. That's totally ugly, and at the same time frighteningly likely ;-)
>
> Something like a more generic context menu interface might be nice.
>

Ok, done and uploaded.

I also:
- Fixed 2 lint errors.
- Fixed a compile warning / error on Windows, due to forward declaring a
class as a struct.

Cheers
Chris


> > One possibility might be to hack up WebKit/chromium/public/WebView to
> expose
> > more of WebKit's menu handling, but I rejected that as complicated and
> not
> > even guaranteed to work :)
> >
> > Cheers
> > Chris
> >
> >>
> >>
> >> http://codereview.chromium.org/5639004/
> >
> >
>

[brettw, 2010-12-08 06:58:48]

On Tue, Dec 7, 2010 at 1:20 PM, Chris Evans <cevans@google.com> wrote:
> 4u does not compile, 64-bit Linux on Lucid:
> In file included from
> ./third_party/WebKit/WebKit/chromium/public/WebPluginParams.h:36,
>                  from ./chrome/renderer/blocked_plugin.h:11,
>                  from chrome/renderer/blocked_plugin.cc:5:
> ./third_party/WebKit/WebKit/chromium/public/WebVector.h: In constructor
> ‘WebKit::WebVector<T>::WebVector(const C&) [with C = unsigned int, T =
> WebKit::WebMenuItemInfo]’:
> chrome/renderer/blocked_plugin.cc:93:   instantiated from here
> ./third_party/WebKit/WebKit/chromium/public/WebVector.h:84: error:
> subscripted value is neither array nor pointer
> ./third_party/WebKit/WebKit/chromium/public/WebVector.h:84: error: request
> for member ‘size’ in ‘other’, which is of non-class type ‘const unsigned
> int’
> ./third_party/WebKit/WebKit/chromium/public/WebVector.h:84: error: request
> for member ‘size’ in ‘other’, which is of non-class type ‘const unsigned
> int’

Gargh, sorry. It looks like it's getting confused because of the
different overloads of this function. Normally it works though! :)

Brett

[brettw, 2010-12-08 07:06:17]

On Tue, Dec 7, 2010 at 3:47 PM, Bernhard Bauer <bauerb@chromium.org> wrote:
> On Tue, Dec 7, 2010 at 22:35, Chris Evans <cevans@google.com> wrote:
>> On Tue, Dec 7, 2010 at 9:03 AM, <bauerb@chromium.org> wrote:
>>>
>>>
>>>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view.cc
>>> File chrome/renderer/render_view.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/5639004/diff/19001/chrome/renderer/render_view...
>>> chrome/renderer/render_view.cc:812: plugin_menu_event_listener_ =
>>> listening;
>>> I'm not happy that we add a generic menu event listener interface here,
>>> and then make it depend on something as specific as BlockedPlugin.
>>
>> How about I add a simple interface, which BlockedPlugin happens to
>> implement? An early revision of the patch had this; I probably shouldn't
>> have removed it.
>>>
>>> How
>>> do regular plugins or extensions do it?
>>
>> I spent a couple of days poking around at this, but couldn't find anything
>> more useful or cleaner. The standard plugin handler, for example, simply
>> swallows context menu events. Any NPAPI plugin that show you a context menu
>> are probably doing it via a native window, I'd guess?
>
> Ugh. That's totally ugly, and at the same time frighteningly likely ;-)

Yes, non-windowed plugins grab the native window handle from our
window and show their own menu, running a nested event loop. This
sucks big time. The context menu for windowless Flash is disabled
because it hangs the whole plugin for unknown reasons (GTK or X gets
confused somehow) :(

Brett

[brettw, 2010-12-08 07:20:13]

This is a lot nicer. We can re-use this for Pepper plugin context menus in the
future (we want to add an API to allow them to specify a custom menu).

LGTM with the nits fixed. Be sure to wait for Bernhard's review, he may have
more ideas.

http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/custommenul...
File chrome/renderer/custommenulistener.h (right):

http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/custommenul...
chrome/renderer/custommenulistener.h:5: #ifndef
CHROME_RENDERER_CUSTOMMENULISTENER_H_
We would normally name this file (and the define) with underscores like:
custom_menu_listener.h / ..._CUSTOM_MENU_LISTENER_H_

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

http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/render_view...
chrome/renderer/render_view.h:342: // context menu events.
Can you add a quick extra sentence here about how to show a context menu. I'm
thinking like "To show a manu call chowContextMenu and then immediately
afterward call CustomMenuListenerInstall to install yourself as the receiver for
the the asynchronous reply."

[Chris Evans, 2010-12-08 16:33:52]

Thanks!

http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/custommenul...
File chrome/renderer/custommenulistener.h (right):

http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/custommenul...
chrome/renderer/custommenulistener.h:5: #ifndef
CHROME_RENDERER_CUSTOMMENULISTENER_H_
On 2010/12/08 07:20:13, brettw wrote:
> We would normally name this file (and the define) with underscores like:
> custom_menu_listener.h / ..._CUSTOM_MENU_LISTENER_H_

Done.

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

http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/render_view...
chrome/renderer/render_view.h:342: // context menu events.
On 2010/12/08 07:20:13, brettw wrote:
> Can you add a quick extra sentence here about how to show a context menu. I'm
> thinking like "To show a manu call chowContextMenu and then immediately
> afterward call CustomMenuListenerInstall to install yourself as the receiver
for
> the the asynchronous reply."

Done.

[Bernhard Bauer, 2010-12-09 08:52:17]

 LGTM, sorry for the holdup.

On Wed, Dec 8, 2010 at 17:33,  <cevans@chromium.org> wrote:
> Thanks!
>
>
>
http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/custommenul...
> File chrome/renderer/custommenulistener.h (right):
>
>
http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/custommenul...
> chrome/renderer/custommenulistener.h:5: #ifndef
> CHROME_RENDERER_CUSTOMMENULISTENER_H_
> On 2010/12/08 07:20:13, brettw wrote:
>>
>> We would normally name this file (and the define) with underscores
>
> like:
>>
>> custom_menu_listener.h / ..._CUSTOM_MENU_LISTENER_H_
>
> Done.
>
>
http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/render_view.h
> File chrome/renderer/render_view.h (right):
>
>
http://codereview.chromium.org/5639004/diff/52001/chrome/renderer/render_view...
> chrome/renderer/render_view.h:342: // context menu events.
> On 2010/12/08 07:20:13, brettw wrote:
>>
>> Can you add a quick extra sentence here about how to show a context
>
> menu. I'm
>>
>> thinking like "To show a manu call chowContextMenu and then
>
> immediately
>>
>> afterward call CustomMenuListenerInstall to install yourself as the
>
> receiver for
>>
>> the the asynchronous reply."
>
> Done.
>
> http://codereview.chromium.org/5639004/
>