BrowserPlugin: Simplify content/public API

BrowserPlugin: Simplify content/public API

BUG=364141, 330264
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268753

[lazyboy, 2014-05-01 23:27:58]

Cool, nice CL.
Please change the description to contain a bit detail on how public interface is
changing. Thanks.

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.cc:124: content::WebContents*
GuestViewManager::GetGuestByInstanceID(
Rearrange placement of this function so it matches decl order in .h

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.h:38: bool
CanEmbedderAccessInstanceID(int embedder_render_process_id,
Exposing this publicly defeats the purpose since it can be misused? Don' think
we need to make it public.

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.h:60: content::WebContents*
GetGuestByInstanceID(
Doc, also say when it returns NULL etc.

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_embedder.h:100: void
GuestCallback(int instance_id,
OnGuestCallback() or CallGuestCallback()
Also add desc what this does.

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:124: static void
BrowserPluginGuestByInstanceIDCallback(
Again, OnGuestRetrieved might be better describing this function's name.

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:139: if
(!GetDelegate())
Consider callback.Run(NULL) to be more consistent, if we do not get killed, then
we expect the callback to be run.

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.h:81: typedef
base::Callback<void(BrowserPluginGuest*)> GuestByInstanceIDCallback;
optional: GuestByInstanceIDCallback can be improved to OnGuestRetrievedCallback
or something similar, so it's more readable.

https://codereview.chromium.org/264943006/diff/1/content/public/browser/brows...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://codereview.chromium.org/264943006/diff/1/content/public/browser/brows...
content/public/browser/browser_plugin_guest_manager_delegate.h:37: virtual void
MaybeGetGuestByInstanceIDOrKill(
Need description here. Also note when |callback| is not expected to be called
and so on.

[Fady Samuel, 2014-05-06 16:51:35]

jam@: Please review content/public
lazyboy@: Please take another look at everything else.

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.cc:124: content::WebContents*
GuestViewManager::GetGuestByInstanceID(
On 2014/05/01 23:27:58, lazyboy wrote:
> Rearrange placement of this function so it matches decl order in .h

Done.

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.h:38: bool
CanEmbedderAccessInstanceID(int embedder_render_process_id,
On 2014/05/01 23:27:58, lazyboy wrote:
> Exposing this publicly defeats the purpose since it can be misused? Don' think
> we need to make it public.

Done.

https://codereview.chromium.org/264943006/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.h:60: content::WebContents*
GetGuestByInstanceID(
On 2014/05/01 23:27:58, lazyboy wrote:
> Doc, also say when it returns NULL etc.

I documented the public method.

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_embedder.h:100: void
GuestCallback(int instance_id,
On 2014/05/01 23:27:58, lazyboy wrote:
> OnGuestCallback() or CallGuestCallback()
> Also add desc what this does.

Done.

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:124: static void
BrowserPluginGuestByInstanceIDCallback(
On 2014/05/01 23:27:58, lazyboy wrote:
> Again, OnGuestRetrieved might be better describing this function's name.

Skipping. This file is going away very soon.

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:139: if
(!GetDelegate())
On 2014/05/01 23:27:58, lazyboy wrote:
> Consider callback.Run(NULL) to be more consistent, if we do not get killed,
then
> we expect the callback to be run.

Done.

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.h:81: typedef
base::Callback<void(BrowserPluginGuest*)> GuestByInstanceIDCallback;
On 2014/05/01 23:27:58, lazyboy wrote:
> optional: GuestByInstanceIDCallback can be improved to
OnGuestRetrievedCallback
> or something similar, so it's more readable.

This whole file is going away in a couple of CLs anyway, so I'm skipping this
optional suggestion.

https://codereview.chromium.org/264943006/diff/1/content/public/browser/brows...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://codereview.chromium.org/264943006/diff/1/content/public/browser/brows...
content/public/browser/browser_plugin_guest_manager_delegate.h:37: virtual void
MaybeGetGuestByInstanceIDOrKill(
On 2014/05/01 23:27:58, lazyboy wrote:
> Need description here. Also note when |callback| is not expected to be called
> and so on.

Done.

[lazyboy, 2014-05-06 18:03:54]

lgtm

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

https://codereview.chromium.org/264943006/diff/1/content/browser/browser_plug...
content/browser/browser_plugin/browser_plugin_guest_manager.h:81: typedef
base::Callback<void(BrowserPluginGuest*)> GuestByInstanceIDCallback;
On 2014/05/06 16:51:36, Fady Samuel wrote:
> On 2014/05/01 23:27:58, lazyboy wrote:
> > optional: GuestByInstanceIDCallback can be improved to
> OnGuestRetrievedCallback
> > or something similar, so it's more readable.
> 
> This whole file is going away in a couple of CLs anyway, so I'm skipping this
> optional suggestion.

SG.

https://codereview.chromium.org/264943006/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/264943006/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:150: void
GuestViewManager::AddRenderProcessHostID(int render_process_host_id) {
nit: try to match declaration and definition order

https://codereview.chromium.org/264943006/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/264943006/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.h:100: // Called by the
content embedder when a guest is found with the provided
s/is found/exists

[Fady Samuel, 2014-05-06 18:13:03]

Fixed nit.

https://codereview.chromium.org/264943006/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/264943006/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:150: void
GuestViewManager::AddRenderProcessHostID(int render_process_host_id) {
On 2014/05/06 18:03:54, lazyboy wrote:
> nit: try to match declaration and definition order

It is. AddRenderProcessHostID (which is private) is placed after ForEachGuest
(which is public)

https://codereview.chromium.org/264943006/diff/40001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/264943006/diff/40001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.h:100: // Called by the
content embedder when a guest is found with the provided
On 2014/05/06 18:03:54, lazyboy wrote:
> s/is found/exists

Done.

[lazyboy, 2014-05-06 18:16:54]

SLGTM

https://codereview.chromium.org/264943006/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/264943006/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:150: void
GuestViewManager::AddRenderProcessHostID(int render_process_host_id) {
On 2014/05/06 18:13:03, Fady Samuel wrote:
> On 2014/05/06 18:03:54, lazyboy wrote:
> > nit: try to match declaration and definition order
> 
> It is. AddRenderProcessHostID (which is private) is placed after ForEachGuest
> (which is public)

Oh sorry, I got confused with the other GetGuest* method's relative order.

[jam, 2014-05-06 18:21:49]

https://codereview.chromium.org/264943006/diff/60001/content/public/browser/b...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://codereview.chromium.org/264943006/diff/60001/content/public/browser/b...
content/public/browser/browser_plugin_guest_manager_delegate.h:46: // and the
|callback| will not be called.
regarding this last sentance; if content is requiring embedders to do this logic
(kill if the given process isn't the owner), why not do this in content? it
seems simpler from an api pov, and also in not requiring all embedders to do
something that content could itself do.

[jam, 2014-05-06 19:39:10]

lgtm

[Fady Samuel, 2014-05-06 19:43:24]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-06 19:44:57]

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

[commit-bot: I haz the power, 2014-05-07 14:37:41]

Change committed as 268753