Towards moving guest management to chrome: Introduce GuestViewManager

Towards moving guest management to chrome: Introduce GuestViewManager

This CL moves a lot of functionality from BrowserPluginGuestManager to GuestViewManager including instance ID allocation. 

Currently GuestViewManager is a BrowserPluginGuestManagerDelegate. 

In the future, GuestViewManager will completely replace BrowserPluginGuestManager. 

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

[lazyboy, 2014-04-30 08:40:15]

Still going through the CL, sending you some comments.

https://codereview.chromium.org/258373002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/258373002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:820:
ChromeContentBrowserClient::GetGuestManagerDelegate(
nit: think you don't need spaces here.

https://codereview.chromium.org/258373002/diff/1/chrome/browser/guestview/adv...
File chrome/browser/guestview/adview/adview_guest.cc (right):

https://codereview.chromium.org/258373002/diff/1/chrome/browser/guestview/adv...
chrome/browser/guestview/adview/adview_guest.cc:16: const std::string&
extension_id)
s/extension_id/embedder_extension_id

https://codereview.chromium.org/258373002/diff/1/chrome/browser/guestview/gue...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/258373002/diff/1/chrome/browser/guestview/gue...
chrome/browser/guestview/guestview.h:151: // Returns a WeakPtr to this
GuestView.
This is the opener web_content's guest? not the the weak pointer of this
guestview.
nit: This is not a function, so shouldn't start with "Returns .."

https://codereview.chromium.org/258373002/diff/1/chrome/browser/guestview/gue...
File chrome/browser/guestview/guestview_manager.cc (right):

https://codereview.chromium.org/258373002/diff/1/chrome/browser/guestview/gue...
chrome/browser/guestview/guestview_manager.cc:26: explicit
GuestWebContentsObserver(WebContents* guest)
s/guest/guest_web_contents

https://codereview.chromium.org/258373002/diff/1/content/test/test_content_br...
File content/test/test_content_browser_client.cc (right):

https://codereview.chromium.org/258373002/diff/1/content/test/test_content_br...
content/test/test_content_browser_client.cc:16: fprintf(stderr, ">>>%s\n",
__PRETTY_FUNCTION__);
Test code.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_manager.cc (right):

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:23: class GuestWebContentsObserver
Add a one line description for this class.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:23: class GuestWebContentsObserver
Can this be inside the anonymous namespace?

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:26: explicit
GuestWebContentsObserver(WebContents* guest)
s/guest/guest_web_contents

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:44: delete this;
What if the web_contents is destroyed before we reach here? Do we leak memory?

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:79: // Add the RenderProcessHost
ID when we get one.
This will add the RPH ID ...

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:135: if (guest_instance_id >
next_instance_id_)
This sounds more like a DCHECK

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:140: if (it ==
guest_web_contents_by_instance_id_.end())
Is this a special case? This suggests this guest_instacne_id is not known.
Please add comment if/why we need to return true here.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:153:
guest_web_contents_by_instance_id_.begin();
I think you need 4 more spaces indentation here.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:171: return true;
Seems this one doesn't run |callback| for each guest, upto the first one that
return true on the callback, so need doc or need to rename the function to be
clear about this.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_manager.h (right):

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.h:68: int next_instance_id_;
From its value in .cc, seems this is current instance id rather than next
instance id.

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

https://codereview.chromium.org/258373002/diff/20001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.h:98: void
AllocateInstanceIDResponse(int request_id, int instance_id);
Keep these functions sorted by name

https://codereview.chromium.org/258373002/diff/20001/content/browser/browser_...
File content/browser/browser_plugin/test_browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/258373002/diff/20001/content/browser/browser_...
content/browser/browser_plugin/test_browser_plugin_guest_manager.h:39:
WebContents* last_guest_;
nit: last_guest_added_

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

https://codereview.chromium.org/258373002/diff/20001/content/public/browser/b...
content/public/browser/browser_plugin_guest_manager_delegate.h:23: virtual int
GetNextInstanceID();
Add description for these interface methods.

[Fady Samuel, 2014-05-01 02:04:42]

PTAL

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_manager.cc (right):

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:23: class GuestWebContentsObserver
On 2014/04/30 08:40:15, lazyboy wrote:
> Add a one line description for this class.

Done.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:23: class GuestWebContentsObserver
On 2014/04/30 08:40:15, lazyboy wrote:
> Can this be inside the anonymous namespace?

Not cleanly, as far as I can tell. I'd like it to access private innards of
GuestViewManager.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:26: explicit
GuestWebContentsObserver(WebContents* guest)
On 2014/04/30 08:40:15, lazyboy wrote:
> s/guest/guest_web_contents

Done.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:44: delete this;
On 2014/04/30 08:40:15, lazyboy wrote:
> What if the web_contents is destroyed before we reach here? Do we leak memory?

Good catch. Fixed.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:79: // Add the RenderProcessHost
ID when we get one.
On 2014/04/30 08:40:15, lazyboy wrote:
> This will add the RPH ID ...

Done.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:135: if (guest_instance_id >
next_instance_id_)
On 2014/04/30 08:40:15, lazyboy wrote:
> This sounds more like a DCHECK

No, I don't want it to be a debug only thing. If this fails, then we kill the
embedder. This is a way to try to gain access to a guestview that the embedder
shouldn't have access to.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:140: if (it ==
guest_web_contents_by_instance_id_.end())
On 2014/04/30 08:40:15, lazyboy wrote:
> Is this a special case? This suggests this guest_instacne_id is not known.
> Please add comment if/why we need to return true here.

This means that there is no existing guest for the provided instance ID. The
embedder is free to use this ID to allocate a guest.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.cc:171: return true;
On 2014/04/30 08:40:15, lazyboy wrote:
> Seems this one doesn't run |callback| for each guest, upto the first one that
> return true on the callback, so need doc or need to rename the function to be
> clear about this.

Done.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_manager.h (right):

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.h:68: int next_instance_id_;
On 2014/04/30 08:40:15, lazyboy wrote:
> From its value in .cc, seems this is current instance id rather than next
> instance id.

Done.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.h:68: int next_instance_id_;
On 2014/04/30 08:40:15, lazyboy wrote:
> From its value in .cc, seems this is current instance id rather than next
> instance id.

Done.

https://codereview.chromium.org/258373002/diff/20001/chrome/browser/guestview...
chrome/browser/guestview/guestview_manager.h:68: int next_instance_id_;
On 2014/04/30 08:40:15, lazyboy wrote:
> From its value in .cc, seems this is current instance id rather than next
> instance id.

Done.

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

https://codereview.chromium.org/258373002/diff/20001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.h:98: void
AllocateInstanceIDResponse(int request_id, int instance_id);
On 2014/04/30 08:40:15, lazyboy wrote:
> Keep these functions sorted by name

Done.

https://codereview.chromium.org/258373002/diff/20001/content/browser/browser_...
File content/browser/browser_plugin/test_browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/258373002/diff/20001/content/browser/browser_...
content/browser/browser_plugin/test_browser_plugin_guest_manager.h:39:
WebContents* last_guest_;
On 2014/04/30 08:40:15, lazyboy wrote:
> nit: last_guest_added_

Done.

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

https://codereview.chromium.org/258373002/diff/20001/content/public/browser/b...
content/public/browser/browser_plugin_guest_manager_delegate.h:23: virtual int
GetNextInstanceID();
On 2014/04/30 08:40:15, lazyboy wrote:
> Add description for these interface methods.

Done.

[lazyboy, 2014-05-01 20:06:58]

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_base.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.h:144: // Returns a WeakPtr to this
GuestView.
1) this is a member so doc shouldn't start /w "returns ..."
2) This is not the same opener WebContents as this GuestView's WebContents,
right?
This probably should say just 
// The opener guest view.
base::WeakPtr<GuestViewBase> opener_;

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_manager.cc (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:188: if (!url.is_valid() ||
!url.SchemeIs("chrome-extension")) {
Use existing kExtensionScheme instead of "chrome-extension"

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:192: const std::string&
extension_id = url.host();
The whole function just does callback.Run(GetNextInstanceID()) for now, so let's
just do that.

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.h:18: class GURL;
nit: sort

https://chromiumcodereview.appspot.com/258373002/diff/60001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.h:62: std::multiset<int>
render_process_host_id_set_;
I'd just call it render_process_host_id_multiset_, a bit ugly, but think will
make reader less confused when reading the code

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_embedder.cc:69: void
BrowserPluginEmbedder::AllocateInstanceIDResponse(int request_id,
SendAllocateInstanceIDResponse

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_embedder.cc:84:
BrowserPluginGuestManager::FromBrowserContext(
Can we keep GetBrowserPluginGuestManager() function and keep these parts
unchanged? We are repeatedly using
BrowserPluginGuestManager::FromBrowserContext(GetWebContents()->GetBrowserContext());
here and below.

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_embedder.cc:172:
weak_ptr_factory_.GetWeakPtr(),
indentation is off

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:359:
BrowserPluginGuestManager::FromBrowserContext(
Same for this file, add BrowserPluginGuest::GetBrowserPluginManager()

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.h:139: BrowserPluginGuest*
GetOpener();
Add comment

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:91: // extension.
We treat WebUI pages as a special case if they host the
This comment is stale now.

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:95: const
std::string& host = embedder_site_url.host();
Is the check for WebUI pages gone b/c of signin page not using browser-plugin
atm?

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest_manager.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest_manager.h:60: // Gets the
next available instance id.
Can you also add a comment saying 0 is an invalid ID.

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
File content/browser/browser_plugin/test_browser_plugin_guest_manager.cc
(right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/browser/b...
content/browser/browser_plugin/test_browser_plugin_guest_manager.cc:32: if
(last_guest_added_)
Set last_guest_added_ = NULL after returning the value.

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/common/br...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/common/br...
content/common/browser_plugin/browser_plugin_messages.h:101: // |request_id| is
used by BrowserPluginEmbedder to route the response back
Also add why |src| will be needed.

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/public/br...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/public/br...
content/public/browser/browser_plugin_guest_delegate.h:44: // Requests the
opener for this guest.
s/Requests/Returns

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/public/br...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/public/br...
content/public/browser/browser_plugin_guest_manager_delegate.h:26: //
TODO(fsamuel): Remove this.
A bit of description why this is temporary?

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/public/br...
content/public/browser/browser_plugin_guest_manager_delegate.h:63: //
succcessfully, then callback is called with the instance ID.
nit: .. then |callback| is called ...

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/renderer/...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/258373002/diff/60001/content/renderer/...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:39:
browser_plugin->render_view_routing_id(), request_id, src));
nit: indent

https://chromiumcodereview.appspot.com/258373002/diff/80001/content/public/br...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/258373002/diff/80001/content/public/br...
content/public/browser/browser_plugin_guest_manager_delegate.h:9: #include
"build/build_config.h"
Why do you need this? I don't see any OS_* ifdef in this file.

[Fady Samuel, 2014-05-01 21:05:27]

All comments addressed.

+jam@ for content/public review. Thanks!

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:144: // Returns a WeakPtr to this
GuestView.
On 2014/05/01 20:06:59, lazyboy wrote:
> 1) this is a member so doc shouldn't start /w "returns ..."
> 2) This is not the same opener WebContents as this GuestView's WebContents,
> right?
> This probably should say just 
> // The opener guest view.
> base::WeakPtr<GuestViewBase> opener_;

Done.

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

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:188: if (!url.is_valid() ||
!url.SchemeIs("chrome-extension")) {
On 2014/05/01 20:06:59, lazyboy wrote:
> Use existing kExtensionScheme instead of "chrome-extension"

Removed.

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:192: const std::string&
extension_id = url.host();
On 2014/05/01 20:06:59, lazyboy wrote:
> The whole function just does callback.Run(GetNextInstanceID()) for now, so
let's
> just do that.

Removed this function entirely for now.

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.h:18: class GURL;
On 2014/05/01 20:06:59, lazyboy wrote:
> nit: sort

It is sorted, isn't it?

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.h:62: std::multiset<int>
render_process_host_id_set_;
On 2014/05/01 20:06:59, lazyboy wrote:
> I'd just call it render_process_host_id_multiset_, a bit ugly, but think will
> make reader less confused when reading the code

Done.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.cc:69: void
BrowserPluginEmbedder::AllocateInstanceIDResponse(int request_id,
On 2014/05/01 20:06:59, lazyboy wrote:
> SendAllocateInstanceIDResponse

I removed this entirely.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.cc:84:
BrowserPluginGuestManager::FromBrowserContext(
On 2014/05/01 20:06:59, lazyboy wrote:
> Can we keep GetBrowserPluginGuestManager() function and keep these parts
> unchanged? We are repeatedly using
>
BrowserPluginGuestManager::FromBrowserContext(GetWebContents()->GetBrowserContext());
> here and below.

Done.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.cc:172:
weak_ptr_factory_.GetWeakPtr(),
On 2014/05/01 20:06:59, lazyboy wrote:
> indentation is off

Done.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:359:
BrowserPluginGuestManager::FromBrowserContext(
On 2014/05/01 20:06:59, lazyboy wrote:
> Same for this file, add BrowserPluginGuest::GetBrowserPluginManager()

Done.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:139: BrowserPluginGuest*
GetOpener();
On 2014/05/01 20:06:59, lazyboy wrote:
> Add comment

Done.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right):

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:91: // extension.
We treat WebUI pages as a special case if they host the
On 2014/05/01 20:06:59, lazyboy wrote:
> This comment is stale now.

Done. I removed the comment entirely.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:95: const
std::string& host = embedder_site_url.host();
On 2014/05/01 20:06:59, lazyboy wrote:
> Is the check for WebUI pages gone b/c of signin page not using browser-plugin
> atm?

Yup. Once BrowserPluginGuestManager::CreateGuest moves to the chrome later, I
might restore the check. For now, let's not allow accidental <webview> usage in
WebUI.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest_manager.h:60: // Gets the
next available instance id.
On 2014/05/01 20:06:59, lazyboy wrote:
> Can you also add a comment saying 0 is an invalid ID.

Done.

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
File content/browser/browser_plugin/test_browser_plugin_guest_manager.cc
(right):

https://codereview.chromium.org/258373002/diff/60001/content/browser/browser_...
content/browser/browser_plugin/test_browser_plugin_guest_manager.cc:32: if
(last_guest_added_)
On 2014/05/01 20:06:59, lazyboy wrote:
> Set last_guest_added_ = NULL after returning the value.

Done.

https://codereview.chromium.org/258373002/diff/60001/content/common/browser_p...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/258373002/diff/60001/content/common/browser_p...
content/common/browser_plugin/browser_plugin_messages.h:101: // |request_id| is
used by BrowserPluginEmbedder to route the response back
On 2014/05/01 20:06:59, lazyboy wrote:
> Also add why |src| will be needed.

I removed src because it's not yet relevant in this patch. I don't want to add
too much complexity upstream just yet.

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

https://codereview.chromium.org/258373002/diff/60001/content/public/browser/b...
content/public/browser/browser_plugin_guest_delegate.h:44: // Requests the
opener for this guest.
On 2014/05/01 20:06:59, lazyboy wrote:
> s/Requests/Returns

Done.

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

https://codereview.chromium.org/258373002/diff/60001/content/public/browser/b...
content/public/browser/browser_plugin_guest_manager_delegate.h:26: //
TODO(fsamuel): Remove this.
On 2014/05/01 20:06:59, lazyboy wrote:
> A bit of description why this is temporary?

Done.

https://codereview.chromium.org/258373002/diff/60001/content/public/browser/b...
content/public/browser/browser_plugin_guest_manager_delegate.h:63: //
succcessfully, then callback is called with the instance ID.
On 2014/05/01 20:06:59, lazyboy wrote:
> nit: .. then |callback| is called ...

Removed this entirely.

https://codereview.chromium.org/258373002/diff/60001/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/258373002/diff/60001/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:39:
browser_plugin->render_view_routing_id(), request_id, src));
On 2014/05/01 20:06:59, lazyboy wrote:
> nit: indent

Done.

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

https://codereview.chromium.org/258373002/diff/80001/content/public/browser/b...
content/public/browser/browser_plugin_guest_manager_delegate.h:9: #include
"build/build_config.h"
On 2014/05/01 20:06:59, lazyboy wrote:
> Why do you need this? I don't see any OS_* ifdef in this file.

Done.

[lazyboy, 2014-05-01 22:38:45]

lgtm

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/258373002/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.h:18: class GURL;
On 2014/05/01 21:05:28, Fady Samuel wrote:
> On 2014/05/01 20:06:59, lazyboy wrote:
> > nit: sort
> 
> It is sorted, isn't it?

Oh, I meant 'U'<'u'

[jam, 2014-05-02 19:09:05]

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:45: virtual WebContents*
GetOpener() const;
hmm, does this need to be in content? i haven't traced through all the usage of
this, but i'm wondering if chrome is the only place that needs to get/set this,
and it can stash it on the WC using a map or WCUD?

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/browser_plugin_guest_manager_delegate.cc (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.cc:6: #include
"ipc/ipc_listener.h"
needed?

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.h:18: class
CONTENT_EXPORT BrowserPluginGuestManagerDelegate {
nit: document

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.h:23: virtual int
GetNextInstanceID();
this and GetGuestByInstanceID can go away as well once instance ids are a chrome
concept?

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:157: BrowserContext* context);
since this is a per-browsercontext method, it should just go directly on
BrowserContext

https://codereview.chromium.org/258373002/diff/100001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/258373002/diff/100001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:38:
browser_plugin->render_view_routing_id(), request_id));
nit: leave as is, it was right before

https://codereview.chromium.org/258373002/diff/100001/content/shell/browser/b...
File content/shell/browser/browser_plugin/test_guest_manager_delegate.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/shell/browser/b...
content/shell/browser/browser_plugin/test_guest_manager_delegate.h:17: class
TestGuestManagerDelegate : public BrowserPluginGuestManagerDelegate {
this class shouldn't be in content/shell. i assume some browser tests in content
needs them? there should just be a setter on ShellBrowserContext to set a 
BrowserPluginGuestManagerDelegate, and this class should be beside the other
tests

[Fady Samuel, 2014-05-02 21:28:19]

PTAL John! Thanks!

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/browser_plugin_guest_delegate.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_delegate.h:45: virtual WebContents*
GetOpener() const;
On 2014/05/02 19:09:06, jam wrote:
> hmm, does this need to be in content? i haven't traced through all the usage
of
> this, but i'm wondering if chrome is the only place that needs to get/set
this,
> and it can stash it on the WC using a map or WCUD?

Currently, the New Window API still resides in the content layer. Once the New
Window API is moved to chrome, this will no longer be necessary. I added a TODO
above GetOpener and SetOpener.

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/browser_plugin_guest_manager_delegate.cc (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.cc:6: #include
"ipc/ipc_listener.h"
On 2014/05/02 19:09:06, jam wrote:
> needed?

No. Removed.

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/browser_plugin_guest_manager_delegate.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.h:18: class
CONTENT_EXPORT BrowserPluginGuestManagerDelegate {
On 2014/05/02 19:09:06, jam wrote:
> nit: document

Done.

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.h:23: virtual int
GetNextInstanceID();
On 2014/05/02 19:09:06, jam wrote:
> this and GetGuestByInstanceID can go away as well once instance ids are a
chrome
> concept?

GetGuestByInstanceID goes away in a subsequent patch. Added a TODO for
GetNextInstanceID.

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:157: BrowserContext* context);
On 2014/05/02 19:09:06, jam wrote:
> since this is a per-browsercontext method, it should just go directly on
> BrowserContext

Done.

https://codereview.chromium.org/258373002/diff/100001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/258373002/diff/100001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:38:
browser_plugin->render_view_routing_id(), request_id));
On 2014/05/02 19:09:06, jam wrote:
> nit: leave as is, it was right before

Done.

https://codereview.chromium.org/258373002/diff/100001/content/shell/browser/b...
File content/shell/browser/browser_plugin/test_guest_manager_delegate.h (right):

https://codereview.chromium.org/258373002/diff/100001/content/shell/browser/b...
content/shell/browser/browser_plugin/test_guest_manager_delegate.h:17: class
TestGuestManagerDelegate : public BrowserPluginGuestManagerDelegate {
On 2014/05/02 19:09:06, jam wrote:
> this class shouldn't be in content/shell. i assume some browser tests in
content
> needs them? there should just be a setter on ShellBrowserContext to set a 
> BrowserPluginGuestManagerDelegate, and this class should be beside the other
> tests

Done.

[jam, 2014-05-02 22:24:46]

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
content/public/browser/browser_context.h:173: // Returns the guest manager for
this context.
guest manager delegate?

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
content/public/browser/content_browser_client.h:88: class
BrowserPluginGuestManagerDelegate;
undo

https://codereview.chromium.org/258373002/diff/120001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/258373002/diff/120001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:38:
browser_plugin->render_view_routing_id(), request_id));
it's still here?

https://codereview.chromium.org/258373002/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/258373002/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:34:
ShellBrowserContext::guest_manager_delegate_ = guest_manager_delegate;
why is this a static method?

https://codereview.chromium.org/258373002/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:99: static
BrowserPluginGuestManagerDelegate* guest_manager_delegate_;
ditto re static

[Fady Samuel, 2014-05-03 00:16:58]

PTAL John! Thanks! :-)

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
File content/public/browser/browser_context.h (right):

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
content/public/browser/browser_context.h:173: // Returns the guest manager for
this context.
On 2014/05/02 22:24:47, jam wrote:
> guest manager delegate?

Done.

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/258373002/diff/120001/content/public/browser/...
content/public/browser/content_browser_client.h:88: class
BrowserPluginGuestManagerDelegate;
On 2014/05/02 22:24:47, jam wrote:
> undo

Done.

https://codereview.chromium.org/258373002/diff/120001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin_manager_impl.cc (right):

https://codereview.chromium.org/258373002/diff/120001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin_manager_impl.cc:38:
browser_plugin->render_view_routing_id(), request_id));
On 2014/05/02 22:24:47, jam wrote:
> it's still here?

Weird. Fixed.

https://codereview.chromium.org/258373002/diff/120001/content/shell/browser/s...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/258373002/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:34:
ShellBrowserContext::guest_manager_delegate_ = guest_manager_delegate;
On 2014/05/02 22:24:47, jam wrote:
> why is this a static method?

Made it non-static

https://codereview.chromium.org/258373002/diff/120001/content/shell/browser/s...
content/shell/browser/shell_browser_context.h:99: static
BrowserPluginGuestManagerDelegate* guest_manager_delegate_;
On 2014/05/02 22:24:47, jam wrote:
> ditto re static

Done.

[jabdelmalek, 2014-05-05 15:00:02]

lgtm

[jam, 2014-05-05 15:02:35]

lgtm

[Fady Samuel, 2014-05-05 15:07:15]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-05 15:07:36]

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

[Fady Samuel, 2014-05-05 15:54:03]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-05 15:54:16]

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

[Fady Samuel, 2014-05-05 15:58:33]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-05 15:59:08]

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

[Fady Samuel, 2014-05-05 16:36:53]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-05 16:37:29]

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

[Fady Samuel, 2014-05-05 18:36:31]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-05 18:37:16]

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

[michaelbai, 2014-05-05 21:04:40]

lgtm for android_webview

[commit-bot: I haz the power, 2014-05-05 21:56:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 01:40:45]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 03:31:34]

Change committed as 268391