Refactor GuestView.

1. Handle the case of empty embedder_extension_id in    guestview.h.

2. Introduce a template class GuestView<T> to handle the    *guestview-specific functions in guestview.h:
   . Rename GuestView to GuestViewBase, introduce GuestView<T> class, and move *ViewGuest-specific functions to     GuestView<T>:
        -- FromWebContents<T>(...)
        -- From<T>(...).
   . Introduce a template member function As() in      GuestViewBase to replace all of the As*View functions.

3. In each derived *ViewGuest class , e.g., WebViewGuest &    AdViewGuest, define a static variable to show its view type    (string).


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

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

[Xi Han, 2014-04-15 21:35:37]

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

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/gue...
chrome/browser/guestview/guestview.h:86: template <class T>
We do not want a bunch of As*View functions, e.g.,AsAdView(), AsWebView() ect.
After introducing this template functions, we can obtain an *GuestView pointer
by using guest->As<*ViewGuest>() if this guest view is of the same type as the
given guest view.

[Fady Samuel, 2014-04-16 02:47:44]

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/web...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/web...
chrome/browser/guestview/webview/webview_guest.cc:295: GuestView::Type
WebViewGuest::GetViewType() const { return Type(); }
I've been thinking. We can probably move GetViewType() and Type() to a templated
base class.

GuestViewType<WebViewGuest>

What if the relationships were:

WebViewGuest derives from GuestViewType<WebViewGuest> derives from GuestView?

[Xi Han, 2014-04-16 13:33:32]

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/web...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/web...
chrome/browser/guestview/webview/webview_guest.cc:295: GuestView::Type
WebViewGuest::GetViewType() const { return Type(); }
Does it worth to introduce another layer just for these two functions? It also
introduces complexity to the code.


On 2014/04/16 02:47:44, Fady Samuel wrote:
> I've been thinking. We can probably move GetViewType() and Type() to a
templated
> base class.
> 
> GuestViewType<WebViewGuest>
> 
> What if the relationships were:
> 
> WebViewGuest derives from GuestViewType<WebViewGuest> derives from GuestView?

[Xi Han, 2014-04-16 17:42:19]

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/web...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/237533008/diff/1/chrome/browser/guestview/web...
chrome/browser/guestview/webview/webview_guest.cc:295: GuestView::Type
WebViewGuest::GetViewType() const { return Type(); }
As discussed offline, I will introduce a template class GuestViewType and move
related functions to this class.

On 2014/04/16 02:47:44, Fady Samuel wrote:
> I've been thinking. We can probably move GetViewType() and Type() to a
templated
> base class.
> 
> GuestViewType<WebViewGuest>
> 
> What if the relationships were:
> 
> WebViewGuest derives from GuestViewType<WebViewGuest> derives from GuestView?

[Fady Samuel, 2014-04-23 21:48:53]

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.h (left):

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:11: #include "base/values.h"
Do you need this?

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:13: #include
"content/public/browser/web_contents.h"
Do you need this?

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_base.h (right):

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
chrome/browser/guestview/guestview_base.h:42: if (GetViewType().compare(T::Type)
== 0) {
if (GetViewType() == T::Type) {

[Xi Han, 2014-04-24 15:40:58]

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.h (left):

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:11: #include "base/values.h"
On 2014/04/23 21:48:54, Fady Samuel wrote:
> Do you need this?

Done.

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:13: #include
"content/public/browser/web_contents.h"
On 2014/04/23 21:48:54, Fady Samuel wrote:
> Do you need this?

Done.

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
File chrome/browser/guestview/guestview_base.h (right):

https://codereview.chromium.org/237533008/diff/10001/chrome/browser/guestview...
chrome/browser/guestview/guestview_base.h:42: if (GetViewType().compare(T::Type)
== 0) {
On 2014/04/23 21:48:54, Fady Samuel wrote:
> if (GetViewType() == T::Type) {

Done.

[Fady Samuel, 2014-04-24 15:42:18]

guestview lgtm.

Please add sky@ for chrome OWNER review. Thanks.

[Xi Han, 2014-04-24 15:46:20]

sky: Please take a look at the changes in the following files:
1. chrome/browser/chrome_content_browser_client.cc
2. chrome/chrome_browser.gypi

Thanks!

[sky, 2014-04-24 16:31:24]

https://codereview.chromium.org/237533008/diff/30001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/237533008/diff/30001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:11: class GuestView : public GuestViewBase
{
Noticed this in looking at gyp changes. This file should be named
guest_view_base. Comment likely applies to other files in this directory.

[Xi Han, 2014-04-24 18:14:49]

https://codereview.chromium.org/237533008/diff/30001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/237533008/diff/30001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:11: class GuestView : public GuestViewBase
{
On 2014/04/24 16:31:24, sky wrote:
> Noticed this in looking at gyp changes. This file should be named
> guest_view_base. Comment likely applies to other files in this directory.

Done.

[Fady Samuel, 2014-04-24 18:16:33]

Still guest_view lgtm! Thanks for the mass rename! :-)

[sky, 2014-04-24 18:17:20]

LGTM

[Xi Han, 2014-04-24 18:17:43]

sky: please take another look at the renaming. Thanks!

[Xi Han, 2014-04-24 18:49:33]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:51:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/50001

[commit-bot: I haz the power, 2014-04-24 23:52:57]

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

[commit-bot: I haz the power, 2014-04-24 23:52:57]

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

[Xi Han, 2014-04-25 15:22:09]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-25 21:44:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/50001

[commit-bot: I haz the power, 2014-04-26 00:11:24]

Change committed as 266297

[Dan Beam, 2014-04-26 03:00:10]

this has added static initializers and broken sizes on the tree

[Dan Beam, 2014-04-26 03:04:40]

https://codereview.chromium.org/237533008/diff/50001/chrome/browser/guest_vie...
File chrome/browser/guest_view/ad_view/ad_view_guest.cc (right):

https://codereview.chromium.org/237533008/diff/50001/chrome/browser/guest_vie...
chrome/browser/guest_view/ad_view/ad_view_guest.cc:22: const std::string&
AdViewGuest::Type = "adview";
^ this just needs to be a char[] or something

[Xi Han, 2014-04-28 22:08:33]

I keep Dan's changes and add the missing parts in the web_view_guest.h(cc).

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/ad_view/ad_view_guest.cc (right):

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/ad_view/ad_view_guest.cc:22: const char
AdViewGuest::Type[] = "adview";
Initialization.

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/ad_view/ad_view_guest.h (right):

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/ad_view/ad_view_guest.h:25: static const char Type[];
change to char[].

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view.h (right):

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view.h:36: std::string type_;
Add a private variable which can be initialized by constructor.

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.cc:165: const char
WebViewGuest::Type[] = "webview";
Update initialization.

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/237533008/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/web_view/web_view_guest.h:44: static const char
Type[];
Also update the other derived class web_view_guest.

[Xi Han, 2014-04-28 22:46:19]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:46:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/80001

[commit-bot: I haz the power, 2014-04-28 23:35:27]

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

[commit-bot: I haz the power, 2014-04-28 23:35:27]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit
  tryserver.chromium on win_chromium_rel

[Xi Han, 2014-04-28 23:54:24]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:56:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/80001

[commit-bot: I haz the power, 2014-04-29 00:18:16]

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

[commit-bot: I haz the power, 2014-04-29 00:18:17]

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

[Xi Han, 2014-04-29 00:58:19]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:59:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/80001

[commit-bot: I haz the power, 2014-04-29 01:42:51]

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

[commit-bot: I haz the power, 2014-04-29 01:42:52]

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

[Xi Han, 2014-04-29 16:30:17]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-29 16:30:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/80001

[commit-bot: I haz the power, 2014-04-29 16:30:51]

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

[commit-bot: I haz the power, 2014-04-29 16:30:52]

Failed to apply patch for chrome/browser/chrome_content_browser_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chrome_content_browser_client.cc
  Hunk #3 FAILED at 837.
  Hunk #4 succeeded at 846 (offset -2 lines).
  Hunk #5 succeeded at 869 (offset -2 lines).
  Hunk #6 succeeded at 941 (offset -2 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/chrome_content_browser_client.cc.rej

Patch:       chrome/browser/chrome_content_browser_client.cc
Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc
b/chrome/browser/chrome_content_browser_client.cc
index
024a98615eff9fb329985c499d54a3d64a0502c2..5d0bf4ff9216bbb11d482eb45ebbdc60f135f03b
100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -41,10 +41,10 @@
 #include "chrome/browser/extensions/suggest_permission_util.h"
 #include "chrome/browser/geolocation/chrome_access_token_store.h"
 #include "chrome/browser/google/google_util.h"
-#include "chrome/browser/guestview/adview/adview_guest.h"
-#include "chrome/browser/guestview/guestview.h"
-#include "chrome/browser/guestview/guestview_constants.h"
-#include "chrome/browser/guestview/webview/webview_guest.h"
+#include "chrome/browser/guest_view/ad_view/ad_view_guest.h"
+#include "chrome/browser/guest_view/guest_view_base.h"
+#include "chrome/browser/guest_view/guest_view_constants.h"
+#include "chrome/browser/guest_view/web_view/web_view_guest.h"
 #include "chrome/browser/media/cast_transport_host_filter.h"
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
 #include "chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h"
@@ -771,7 +771,7 @@ void
ChromeContentBrowserClient::GetStoragePartitionConfigForSite(
   partition_name->clear();
   *in_memory = false;
 
-  bool success = GuestView::GetGuestPartitionConfigForSite(
+  bool success = GuestViewBase::GetGuestPartitionConfigForSite(
       site, partition_domain, partition_name, in_memory);
 
   if (!success && site.SchemeIs(extensions::kExtensionScheme)) {
@@ -837,10 +837,11 @@ void ChromeContentBrowserClient::GuestWebContentsCreated(
     return;
   }
 
-  /// TODO(fsamuel): In the future, certain types of GuestViews won't require
-  // extension bindings. At that point, we should clear |extension_id| instead
-  // of exiting early.
-  if (!service->GetExtensionById(extension_id, false) &&
+  /// TODO(fsamuel): In the future, certain types of GuestViewBases won't
+  // require extension bindings. At that point, we should clear |extension_id|
+  // instead of exiting early.
+  if (!extension_id.empty() &&
+      !service->GetExtensionById(extension_id, false) &&
       !CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserPluginForAllViewTypes)) {
     NOTREACHED();
@@ -848,17 +849,15 @@ void ChromeContentBrowserClient::GuestWebContentsCreated(
   }
 
   if (opener_web_contents) {
-    GuestView* guest = GuestView::FromWebContents(opener_web_contents);
+    GuestViewBase* guest = GuestViewBase::FromWebContents(opener_web_contents);
     if (!guest) {
       NOTREACHED();
       return;
     }
 
-    // Create a new GuestView of the same type as the opener.
-    *guest_delegate =
-        GuestView::Create(guest_web_contents,
-                          extension_id,
-                          guest->GetViewType());
+    // Create a new GuestViewBase of the same type as the opener.
+    *guest_delegate = GuestViewBase::Create(
+        guest_web_contents, extension_id, guest->GetViewType());
     return;
   }
 
@@ -873,17 +872,14 @@ void ChromeContentBrowserClient::GuestWebContentsCreated(
     return;
 
   *guest_delegate =
-      GuestView::Create(guest_web_contents,
-                        extension_id,
-                        GuestView::GetViewTypeFromString(api_type));
+      GuestViewBase::Create(guest_web_contents, extension_id, api_type);
 }
 
 void ChromeContentBrowserClient::GuestWebContentsAttached(
     WebContents* guest_web_contents,
     WebContents* embedder_web_contents,
     const base::DictionaryValue& extra_params) {
-
-  GuestView* guest = GuestView::FromWebContents(guest_web_contents);
+  GuestViewBase* guest = GuestViewBase::FromWebContents(guest_web_contents);
   if (!guest) {
     // It's ok to return here, since we could be running a browser plugin
     // outside an extension, and don't need to attach a
@@ -948,7 +944,8 @@ void ChromeContentBrowserClient::RenderProcessWillLaunch(
 
   RendererContentSettingRules rules;
   if (host->IsGuest()) {
-    GuestView::GetDefaultContentSettingRules(&rules,
profile->IsOffTheRecord());
+    GuestViewBase::GetDefaultContentSettingRules(&rules,
+                                                 profile->IsOffTheRecord());
   } else {
     GetRendererContentSettingRules(
         profile->GetHostContentSettingsMap(), &rules);

[Xi Han, 2014-04-29 17:59:59]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-04-29 18:01:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/100001

[commit-bot: I haz the power, 2014-04-29 23:16:38]

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

[commit-bot: I haz the power, 2014-04-29 23:16:39]

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

[Fady Samuel, 2014-04-30 00:37:39]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-04-30 00:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hanxi@chromium.org/237533008/100001

[commit-bot: I haz the power, 2014-04-30 05:10:38]

Change committed as 267079