BrowserPlugin: Move CreateGuest to chrome

BrowserPlugin: Move CreateGuest to chrome

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

[Fady Samuel, 2014-05-02 02:05:37]

Another content/public cleanup! Yay!

[lazyboy, 2014-05-02 19:52:52]

https://chromiumcodereview.appspot.com/261013005/diff/1/content/browser/brows...
File content/browser/browser_plugin/browser_plugin_guest_manager.cc (right):

https://chromiumcodereview.appspot.com/261013005/diff/1/content/browser/brows...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:17: #include
"content/public/browser/render_process_host.h"
Remove this, no longer needed.

https://chromiumcodereview.appspot.com/261013005/diff/1/content/browser/brows...
content/browser/browser_plugin/browser_plugin_guest_manager.cc:23: #include
"net/base/escape.h"
Remove

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

https://chromiumcodereview.appspot.com/261013005/diff/20001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.cc:136: WebContents*
guest_web_contents = WebContents::Create(create_params);
just return WebContents::Create(create_params);

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

https://chromiumcodereview.appspot.com/261013005/diff/20001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.h:32: void AddGuest(int
guest_instance_id,
Description for added public methods.

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

https://chromiumcodereview.appspot.com/261013005/diff/20001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:318:
BrowserPluginHostMsg_Attach_Params attach_params;
Do we still need to create this MSG object here?

https://chromiumcodereview.appspot.com/261013005/diff/20001/content/browser/w...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/261013005/diff/20001/content/browser/w...
content/browser/web_contents/web_contents_impl.cc:443: params.guest_instance_id,
nit: params can fit in previous line.

https://chromiumcodereview.appspot.com/261013005/diff/20001/content/public/br...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/261013005/diff/20001/content/public/br...
content/public/browser/web_contents.h:111: // The guest instance to use.
Need more description about guest since it is in WebContents. Also say when we
don't care about Browser Plugins, set to 0.

[Fady Samuel, 2014-05-06 20:02:48]

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

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

https://codereview.chromium.org/261013005/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.cc:136: WebContents*
guest_web_contents = WebContents::Create(create_params);
On 2014/05/02 19:52:52, lazyboy wrote:
> just return WebContents::Create(create_params);

Done.

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

https://codereview.chromium.org/261013005/diff/20001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.h:32: void AddGuest(int
guest_instance_id,
On 2014/05/02 19:52:52, lazyboy wrote:
> Description for added public methods.

Moved to private. These need to be accessed by GuestViewBase so I made that a
friend class.

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

https://codereview.chromium.org/261013005/diff/20001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:318:
BrowserPluginHostMsg_Attach_Params attach_params;
On 2014/05/02 19:52:52, lazyboy wrote:
> Do we still need to create this MSG object here?

No removed.

https://codereview.chromium.org/261013005/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/261013005/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:443: params.guest_instance_id,
On 2014/05/02 19:52:52, lazyboy wrote:
> nit: params can fit in previous line.

Done.

https://codereview.chromium.org/261013005/diff/20001/content/public/browser/w...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/261013005/diff/20001/content/public/browser/w...
content/public/browser/web_contents.h:111: // The guest instance to use.
On 2014/05/02 19:52:52, lazyboy wrote:
> Need more description about guest since it is in WebContents. Also say when we
> don't care about Browser Plugins, set to 0.

Done.

[lazyboy, 2014-05-06 21:59:13]

lgtm

[jam, 2014-05-06 23:43:53]

https://codereview.chromium.org/261013005/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/261013005/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:430: WebContentsImpl*
WebContentsImpl::CreateWithOpener(
what is the callstack when this method is called with a non zero
guest_instance_id?

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

https://codereview.chromium.org/261013005/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.h:24: struct
StorageInfo {
why a struct for two params instead of putting them in CreateGuest?

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

https://codereview.chromium.org/261013005/diff/100001/content/public/browser/...
content/public/browser/web_contents.h:86: virtual ~CreateParams();
why virtual?

[Fady Samuel, 2014-05-07 00:07:46]

PTAL

https://codereview.chromium.org/261013005/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/261013005/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:430: WebContentsImpl*
WebContentsImpl::CreateWithOpener(
On 2014/05/06 23:43:53, jam wrote:
> what is the callstack when this method is called with a non zero
> guest_instance_id?

This bouncing from content to chrome and back should be reduced significantly in
the future. I plan to introduce a chrome extension API for "guestviews"

GuestView.allocateInstanceId

and

GuestView.createGuest

or some such API. This will allow guests to be allocated from chrome all the
time. The final step will be to create a BrowserPlugin in content via
WebContents::Create. Here is the current stack trace:

#1 0x7f38cd5b52b5 content::WebContentsImpl::CreateWithOpener()
#2 0x000001019302 GuestViewManager::CreateGuest()
#3 0x7f38cd34dd1a content::BrowserPluginGuestManager::CreateGuest()
#4 0x7f38cd344cb9 content::BrowserPluginEmbedder::OnGuestCallback()
#5 0x7f38cd34de76 content::BrowserPluginGuestByInstanceIDCallback()
#6 0x0000010194bf GuestViewManager::MaybeGetGuestByInstanceIDOrKill()
#7 0x7f38cd34de0b
content::BrowserPluginGuestManager::MaybeGetGuestByInstanceIDOrKill()
#8 0x7f38cd3446f7 content::BrowserPluginEmbedder::OnMessageReceived()

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

https://codereview.chromium.org/261013005/diff/100001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager_delegate.h:24: struct
StorageInfo {
On 2014/05/06 23:43:53, jam wrote:
> why a struct for two params instead of putting them in CreateGuest?

Done.

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

https://codereview.chromium.org/261013005/diff/100001/content/public/browser/...
content/public/browser/web_contents.h:86: virtual ~CreateParams();
On 2014/05/06 23:43:53, jam wrote:
> why virtual?

No reason. Removed virtual.

[jam, 2014-05-07 00:17:00]

lgtm

[Fady Samuel, 2014-05-07 00:17:39]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-07 00:21:21]

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

[commit-bot: I haz the power, 2014-05-07 06:25:40]

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:
  android_aosp on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 12:32:01]

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:
  android_aosp on tryserver.chromium
  android_chromium_gn_compile_rel on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  android_dbg on tryserver.chromium
  chromium_presubmit on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_clang_dbg on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium
  win_chromium_x64_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 12:37:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-07 12:37:02]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
  android_clang_dbg on tryserver.chromium
  ios_dbg_simulator on tryserver.chromium
  ios_rel_device on tryserver.chromium
  ios_rel_device_ninja on tryserver.chromium
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
  linux_chromium_chromeos_rel on tryserver.chromium
  linux_chromium_gn_rel on tryserver.chromium
  linux_chromium_rel on tryserver.chromium
  mac_chromium_compile_dbg on tryserver.chromium
  mac_chromium_rel on tryserver.chromium

[Fady Samuel, 2014-05-07 14:47:12]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-07 14:49:29]

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

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

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-07 16:00:11]

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

[Fady Samuel, 2014-05-07 18:28:33]

The CQ bit was checked by fsamuel@chromium.org

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

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

[Fady Samuel, 2014-05-07 19:17:14]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-07 19:20:33]

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

[Fady Samuel, 2014-05-07 22:08:25]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-07 22:09:14]

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

[commit-bot: I haz the power, 2014-05-08 03:37:11]

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:
  android_dbg_triggered_tests on tryserver.chromium

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium

[Fady Samuel, 2014-05-08 14:07:48]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-08 14:11:23]

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

[commit-bot: I haz the power, 2014-05-09 03:38:11]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 06:08:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 06:08:17]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Fady Samuel, 2014-05-09 11:29:58]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-09 11:31:53]

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

[Fady Samuel, 2014-05-09 17:30:10]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-09 17:31:01]

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

[commit-bot: I haz the power, 2014-05-09 22:12:26]

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:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-10 11:50:13]

Change committed as 269565