Make BlockedPlugin implement RenderViewObserver so that RenderView doesn't ha...

Make BlockedPlugin implement RenderViewObserver so that RenderView doesn't have to know about it.

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

[Chris Evans, 2011-01-17 13:50:30]

Broadly LGTM, with one concern to resolve. And thanks for the clean-up.

You might want to add Bernhard as another reviewer.

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
File chrome/renderer/blocked_plugin.cc (right):

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
chrome/renderer/blocked_plugin.cc:119: render_view()->showContextMenu(NULL,
menu_data);
I don't see where this ever gets set to false? Doesn't that mean that
::OnMessageReceived() will start swallowing messages inappropriately?

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
chrome/renderer/blocked_plugin.cc:127: &message, this, this,
&BlockedPlugin::OnMenuItemSelected);
Perhaps we could guarantee unique IDs for custom menu items, and then only
"grab" this event if it's for one of the IDs we know about? Would avoid worrying
about maintaining correct state for "custom_menu_showing_".

[jam, 2011-01-17 18:45:19]

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
File chrome/renderer/blocked_plugin.cc (right):

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
chrome/renderer/blocked_plugin.cc:119: render_view()->showContextMenu(NULL,
menu_data);
On 2011/01/17 13:50:30, Chris Evans wrote:
> I don't see where this ever gets set to false? Doesn't that mean that
> ::OnMessageReceived() will start swallowing messages inappropriately?

This is the same behavior as before, where CustomMenuListenerInstall was called
after showContextMenu, and it would keep swallowing messages forever.

My intent in this change is to clean things up, fixing possible existing bugs is
better to be done in separate changes, since that really helps with tracking
down regressions (i.e. having them be done in different cls).

[jam, 2011-01-17 18:47:46]

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
File chrome/renderer/blocked_plugin.cc (right):

http://codereview.chromium.org/6312004/diff/13001/chrome/renderer/blocked_plu...
chrome/renderer/blocked_plugin.cc:119: render_view()->showContextMenu(NULL,
menu_data);
On 2011/01/17 18:45:19, John Abd-El-Malek wrote:
> On 2011/01/17 13:50:30, Chris Evans wrote:
> > I don't see where this ever gets set to false? Doesn't that mean that
> > ::OnMessageReceived() will start swallowing messages inappropriately?
> 
> This is the same behavior as before, where CustomMenuListenerInstall was
called
> after showContextMenu, and it would keep swallowing messages forever.
> 
> My intent in this change is to clean things up, fixing possible existing bugs
is
> better to be done in separate changes, since that really helps with tracking
> down regressions (i.e. having them be done in different cls).

oh, wait, nm, I think I understand what you meant now.  before showContextMenu
would reset the listener each time showContextMenu is closed.  let me look into
it, thanks for bringing it up

[jam, 2011-01-17 19:34:51]

Fixed, I now watch the message for menu closing

[Chris Evans, 2011-01-17 21:32:13]

On 2011/01/17 19:34:51, John Abd-El-Malek wrote:
> Fixed, I now watch the message for menu closing

LGTM, but there seems to be a compile error. Possibly a missing ::ID ?