GuestView: Move GuestViewManager extension dependencies to ExtensionsGuestViewManagerDelegate

GuestView: Move GuestViewManager extension dependencies to ExtensionsGuestViewManagerDelegate

The CL does the following:

1. Replace GuestViewManager::FromBrowserContext with GuestViewManager::CreateWithDelegate.
2. Rename GuestViewManager::FromBrowserContextIfAvailable to GuestViewManager::FromBrowserContxt.
3. Creates a GuestViewManagerDelegate interface and ExtensionsGuestViewManagerDelegate.
4. GuestViewManager is created with a particular delegate implementation.
5. GuestView permission checks and registration is moved to the ExtensionsGuestViewDelegate.
6. owner_extension_id() is renamed to owner_host().

After this patch the following classes can be moved to Components (once GuestViewEvent is decoupled from EventRouter):

1. GuestViewManager
2. GuestViewManagerDelegate
3. GuestViewBase
4. GuestViewMessageFilter
5. GuestView

BUG=444869
TBR=noms@chromium.org for chrome/browser/profiles, nkostylev@chromium.org chrome/browser/chromeos/login, thestig@chromium.org for chrome/browser/printing.

Committed: https://crrev.com/9a3c81efb05232657d8df6a11db2ef8f8f64fd7f
Cr-Commit-Position: refs/heads/master@{#326774}

[Fady Samuel, 2015-04-21 23:09:55]

fsamuel@chromium.org changed reviewers:
+ lazyboy@chromium.org

[Fady Samuel, 2015-04-22 00:42:48]

PTAL

[Fady Samuel, 2015-04-22 00:42:59]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 00:43:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/60001

[commit-bot: I haz the power, 2015-04-22 01:08:08]

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

[commit-bot: I haz the power, 2015-04-22 01:08:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lazyboy, 2015-04-22 15:16:30]

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/extensions_guest_view_manager_delegate.h
(right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extensions_guest_view_manager_delegate.h:21: class
ExtensionsGuestViewManagerDelegate :
: goes next line

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extensions_guest_view_manager_delegate.h:21: class
ExtensionsGuestViewManagerDelegate :
Short description of this class.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager.cc:263: // static
not static

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager.h:76: // Indicates whether the
|guest| provided is owned by an extension or Chrome
nit: "provided" is redundant

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_manager_delegate.h (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:17: //
responsibilities to other modules in chromium. Different builds of chromium
Shortly, also describe what responsibilities they are.
Explain "different builds of chromium", different platforms?

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:21: virtual
~GuestViewManagerDelegate() {}
indent this file.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:23: // Indicates
whether the provided |guest| can be used in the context it has
nit: here and below, "the provided" is redundant since you're using |guest|
form, which already specifies which param you're talking about.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:31: // Registers
additional GuestView types that the GuestViewManager can create.
... GuestView types that our delegator (GuestViewManager) can create.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:977: }
indent

[Fady Samuel, 2015-04-22 23:09:48]

PTAL I've addressed your comments and hopefully fixed builds. Running a dry run
now.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/extensions_guest_view_manager_delegate.h
(right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extensions_guest_view_manager_delegate.h:21: class
ExtensionsGuestViewManagerDelegate :
On 2015/04/22 15:16:29, lazyboy wrote:
> Short description of this class.

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/extensions_guest_view_manager_delegate.h:21: class
ExtensionsGuestViewManagerDelegate :
On 2015/04/22 15:16:29, lazyboy wrote:
> : goes next line

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_manager.cc (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager.cc:263: // static
On 2015/04/22 15:16:29, lazyboy wrote:
> not static

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager.h:76: // Indicates whether the
|guest| provided is owned by an extension or Chrome
On 2015/04/22 15:16:29, lazyboy wrote:
> nit: "provided" is redundant

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/guest_view_manager_delegate.h (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:17: //
responsibilities to other modules in chromium. Different builds of chromium
On 2015/04/22 15:16:29, lazyboy wrote:
> Shortly, also describe what responsibilities they are.
> Explain "different builds of chromium", different platforms?

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:21: virtual
~GuestViewManagerDelegate() {}
On 2015/04/22 15:16:29, lazyboy wrote:
> indent this file.

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:23: // Indicates
whether the provided |guest| can be used in the context it has
On 2015/04/22 15:16:29, lazyboy wrote:
> nit: here and below, "the provided" is redundant since you're using |guest|
> form, which already specifies which param you're talking about.

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/guest_view_manager_delegate.h:31: // Registers
additional GuestView types that the GuestViewManager can create.
On 2015/04/22 15:16:29, lazyboy wrote:
> ... GuestView types that our delegator (GuestViewManager) can create.

Done.

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1066563006/diff/60001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:977: }
On 2015/04/22 15:16:29, lazyboy wrote:
> indent

Done.

[Fady Samuel, 2015-04-22 23:09:55]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-22 23:10:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/80001

[Fady Samuel, 2015-04-23 02:01:01]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 02:02:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/100001

[Fady Samuel, 2015-04-23 03:03:40]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 03:04:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/120001

[Fady Samuel, 2015-04-23 03:44:52]

Some notes to my future self.

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/app_view_browsertest.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/app_view_browsertest.cc:11: #include
"extensions/browser/guest_view/extensions_guest_view_manager_delegate.h"
Note to self: This can be removed.

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/signin_header_helper.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/signin/...
chrome/browser/signin/signin_header_helper.cc:253: if (is_guest &&
webview_info.owner_host.empty())
Note to self: Replace owner_host with is_owned_by_extension

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_apitest.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_apitest.cc:379: fprintf(stderr,
">>%s\n", __PRETTY_FUNCTION__);
Note to self: Delete this.

[commit-bot: I haz the power, 2015-04-23 04:02:36]

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

[commit-bot: I haz the power, 2015-04-23 04:02:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2015-04-23 13:05:24]

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/guest_view_event.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/guest_view_event.cc:21: void
GuestViewEvent::Dispatch(GuestViewBase* guest, int instance_id) {
Another note to my near future self: Move code in here to
ExtensionsGuestViewManagerDelegate.

GuestViewEvent::Dispatch calls GuestViewManager::DispatchEvent which calls
ExtensionsGuestViewManagerDelegate::DispatchEvent.

This abstracts away how event dispatch actually happens. GuestViewEvent merely
stores information about the event to be dispatched so that it can be queued for
attachment.

[lazyboy, 2015-04-23 17:16:43]

One more question.

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1297:
NavigateGuest(url::kAboutBlankURL, false /* force_navigation */);
What is this change?

[Fady Samuel, 2015-04-24 00:11:31]

Addressed my past self's comments. PTAL Istiaque!

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/app_view_browsertest.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/app_view_browsertest.cc:11: #include
"extensions/browser/guest_view/extensions_guest_view_manager_delegate.h"
On 2015/04/23 03:44:52, Fady Samuel wrote:
> Note to self: This can be removed.

Done.

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/signin/...
File chrome/browser/signin/signin_header_helper.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/chrome/browser/signin/...
chrome/browser/signin/signin_header_helper.cc:253: if (is_guest &&
webview_info.owner_host.empty())
On 2015/04/23 03:44:52, Fady Samuel wrote:
> Note to self: Replace owner_host with is_owned_by_extension

Doesn't look like this is possible because it is used elsewhere.

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/guest_view_event.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/guest_view_event.cc:21: void
GuestViewEvent::Dispatch(GuestViewBase* guest, int instance_id) {
On 2015/04/23 13:05:24, Fady Samuel wrote:
> Another note to my near future self: Move code in here to
> ExtensionsGuestViewManagerDelegate.
> 
> GuestViewEvent::Dispatch calls GuestViewManager::DispatchEvent which calls
> ExtensionsGuestViewManagerDelegate::DispatchEvent.
> 
> This abstracts away how event dispatch actually happens. GuestViewEvent merely
> stores information about the event to be dispatched so that it can be queued
for
> attachment.

Done.

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_apitest.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_apitest.cc:379: fprintf(stderr,
">>%s\n", __PRETTY_FUNCTION__);
On 2015/04/23 03:44:52, Fady Samuel wrote:
> Note to self: Delete this.

Done.

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1066563006/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1297:
NavigateGuest(url::kAboutBlankURL, false /* force_navigation */);
On 2015/04/23 17:16:43, lazyboy wrote:
> What is this change?

Undone.

[lazyboy, 2015-04-24 00:25:31]

lgtm

[Fady Samuel, 2015-04-24 00:26:42]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-24 00:27:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/140001

[Fady Samuel, 2015-04-24 01:31:24]

Some notes to self.

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc
(right):

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc:22:
return static_cast<extensions::TestGuestViewManager*>(
Implement Create a TestGuestViewManager if one doesn't exist.

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:57:
GetGuestViewManager()->WaitForAllGuestsDeleted();
Note to self: browser() may be undefined at this point. Get rid of this

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/guest_view_manager_unittest.cc (left):

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/guest_view_manager_unittest.cc:22: class
TestGuestViewManager : public GuestViewManager {
Get rid of TestGuestViewManager here.

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1055:
NavigateGuest(new_window_info.url.spec(), true /* force_navigation */);
Restore this.

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1297:
NavigateGuest(url::kAboutBlankURL, false /* force_navigation */);
Restore this.

[commit-bot: I haz the power, 2015-04-24 01:59:00]

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

[commit-bot: I haz the power, 2015-04-24 01:59:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2015-04-24 04:32:14]

Patchset #10 (id:180001) has been deleted

[Fady Samuel, 2015-04-24 04:37:48]

Addressed my self-comments.

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc
(right):

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc:22:
return static_cast<extensions::TestGuestViewManager*>(
On 2015/04/24 01:31:23, Fady Samuel wrote:
> Implement Create a TestGuestViewManager if one doesn't exist.

Done.

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1066563006/diff/140001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:57:
GetGuestViewManager()->WaitForAllGuestsDeleted();
On 2015/04/24 01:31:24, Fady Samuel wrote:
> Note to self: browser() may be undefined at this point. Get rid of this

Done.

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/guest_view_manager_unittest.cc (left):

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/guest_view_manager_unittest.cc:22: class
TestGuestViewManager : public GuestViewManager {
On 2015/04/24 01:31:24, Fady Samuel wrote:
> Get rid of TestGuestViewManager here.

Done.

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1055:
NavigateGuest(new_window_info.url.spec(), true /* force_navigation */);
On 2015/04/24 01:31:24, Fady Samuel wrote:
> Restore this.

Done.

https://codereview.chromium.org/1066563006/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:1297:
NavigateGuest(url::kAboutBlankURL, false /* force_navigation */);
On 2015/04/24 01:31:24, Fady Samuel wrote:
> Restore this.

Done.

[Fady Samuel, 2015-04-24 04:38:13]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[Fady Samuel, 2015-04-24 04:38:13]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1066563006/#ps200001
(title: "Fixed NavigateGuest")

[commit-bot: I haz the power, 2015-04-24 04:38:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/200001

[commit-bot: I haz the power, 2015-04-24 05:50:25]

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

[commit-bot: I haz the power, 2015-04-24 05:50:26]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2015-04-24 12:27:49]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2015-04-24 12:28:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1066563006/200001

[commit-bot: I haz the power, 2015-04-24 12:30:32]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-04-24 12:31:49]

Patchset 10 (id:??) landed as
https://crrev.com/9a3c81efb05232657d8df6a11db2ef8f8f64fd7f
Cr-Commit-Position: refs/heads/master@{#326774}