BrowserPlugin: Implemented BrowserPluginGuestManager::ForEachGuest

BrowserPlugin: Implemented BrowserPluginGuestManager::ForEachGuest

Use this pattern to replace existing methods in BrowserPluginGuestManager. This CL is just a refactor, there is no change in functionality.

BUG=330843
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243893

[nasko, 2014-01-08 17:58:42]

It is almost there. Just a couple of nits and one note.

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_embedder.h:103: bool
UnlockMouseIfNecessaryCallback(const NativeWebKeyboardEvent& event,
nit: I'd put the guest parameter first, as it is what the callback operates on.

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right):

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:236: if
(callback.Run(guest))
nit: I see that this fits in how HandleInputEvent works, but it seems
unintuitive to return false on successful execution (for example
DidSendScreenRects).

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
File content/browser/browser_plugin/browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.h:91: GuestCallback
callback);
Why not pass the callback as a const reference? An example is
FrameTree::ForEach.

[Fady Samuel, 2014-01-08 18:22:39]

PTAL

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_embedder.h:103: bool
UnlockMouseIfNecessaryCallback(const NativeWebKeyboardEvent& event,
On 2014/01/08 17:58:42, nasko wrote:
> nit: I'd put the guest parameter first, as it is what the callback operates
on.

I don't think this is possible because we need to bind parameters in order.

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right):

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:236: if
(callback.Run(guest))
On 2014/01/08 17:58:42, nasko wrote:
> nit: I see that this fits in how HandleInputEvent works, but it seems
> unintuitive to return false on successful execution (for example
> DidSendScreenRects).

Agreed, but I can't think of a better way to do this :(

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
File content/browser/browser_plugin/browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/128563002/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.h:91: GuestCallback
callback);
On 2014/01/08 17:58:42, nasko wrote:
> Why not pass the callback as a const reference? An example is
> FrameTree::ForEach.

Done.

[nasko, 2014-01-08 18:27:01]

LGTM with one nit.

https://codereview.chromium.org/128563002/diff/130001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/128563002/diff/130001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_embedder.cc:87:
guest->GetWebContents()->GetRenderViewHost())->SendScreenRects();
nit: two more spaces for indent.

[Fady Samuel, 2014-01-08 18:29:24]

Addressed nit: CQ'ing.

https://codereview.chromium.org/128563002/diff/130001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/128563002/diff/130001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_embedder.cc:87:
guest->GetWebContents()->GetRenderViewHost())->SendScreenRects();
On 2014/01/08 18:27:01, nasko wrote:
> nit: two more spaces for indent.

Done.

[commit-bot: I haz the power, 2014-01-08 18:31:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/128563002/180001

[commit-bot: I haz the power, 2014-01-08 22:30:26]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-09 13:15:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/128563002/180001

[commit-bot: I haz the power, 2014-01-09 14:19:38]

Change committed as 243893