Refactor GuestViewContainer to split out WebView related logic and MimeHandlerView related logic.

Refactor GuestViewContainer to split out WebView related logic and MimeHandlerView related logic.

This refactors GuestViewContainer to separate WebView related logic and MimeHandlerView
related logic. Two subclasses are created to contain the logic related to each. The
content API has to be modified to allow creating each of the two types of container.

BUG=303491
Committed: https://crrev.com/78e3d60d83366c66e82ec147a061be200088a656
Cr-Commit-Position: refs/heads/master@{#303118}

[raymes, 2014-11-03 06:38:04]

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

[Fady Samuel, 2014-11-03 11:03:38]

Here are some of my initial thoughts. Thanks for writing this! :-)

https://codereview.chromium.org/702433002/diff/20001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/702433002/diff/20001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:1587:
ChromeContentRendererClient::CreateWebViewContainer(
Content should not know the distinction between MimeHandlrViewContainer and
ExtensionsGuestViewContainer (or whatever else name you choose, WebViewContainer
is inappropriate because there are multiple types of GuestViews).

https://codereview.chromium.org/702433002/diff/20001/content/public/renderer/...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/702433002/diff/20001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:106: virtual
BrowserPluginDelegate* CreateWebViewContainer(
This exposes the concept of WebView to the content module which is not
appropriate. Please restore the code here.

https://codereview.chromium.org/702433002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/702433002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1620:
GetContentClient()->renderer()->CreateWebViewContainer(this));
Please restore.

https://codereview.chromium.org/702433002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1637:
GetContentClient()->renderer()->CreateMimeHandlerViewContainer(
Please restore.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (left):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:9: #include
"content/public/renderer/render_view.h"
Is this necessary?

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:11: #include
"extensions/common/guest_view/guest_view_constants.h"
Is this necessary?

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:7: #include
"base/pickle.h"
Is this necessary?

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (left):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:5: #ifndef
CHROME_RENDERER_GUEST_VIEW_GUEST_VIEW_CONTAINER_H_
EXTENSIONS_RENDERER_GUEST_VIEW_CONTAINER_H_

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/mime_handler_view_container.h (right):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/mime_handler_view_container.h:5: #ifndef
CHROME_RENDERER_GUEST_VIEW_MIME_HANDLER_VIEW_CONTAINER_H_
This should say:

EXTESNIONS_RENDERER_GUEST_VIEW_MIME_HANDLER_VIEW_CONTAINER_H_

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/web_view_container.h (right):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/web_view_container.h:5: #ifndef
CHROME_RENDERER_GUEST_VIEW_WEB_VIEW_CONTAINER_H_
This should be updated to say extensions.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/web_view_container.h:18: class WebViewContainer :
public GuestViewContainer {
Multiple guest types need this container type. I would call this maybe
ExtensionsGuestViewContainer or CustomGuestViewContainer

[raymes, 2014-11-03 22:51:32]

Thanks

https://codereview.chromium.org/702433002/diff/20001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/702433002/diff/20001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:1587:
ChromeContentRendererClient::CreateWebViewContainer(
On 2014/11/03 11:03:37, Fady Samuel wrote:
> Content should not know the distinction between MimeHandlrViewContainer and
> ExtensionsGuestViewContainer (or whatever else name you choose,
WebViewContainer
> is inappropriate because there are multiple types of GuestViews).

Done.

https://codereview.chromium.org/702433002/diff/20001/content/public/renderer/...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/702433002/diff/20001/content/public/renderer/...
content/public/renderer/content_renderer_client.h:106: virtual
BrowserPluginDelegate* CreateWebViewContainer(
On 2014/11/03 11:03:37, Fady Samuel wrote:
> This exposes the concept of WebView to the content module which is not
> appropriate. Please restore the code here.

Done.

https://codereview.chromium.org/702433002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/702433002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1620:
GetContentClient()->renderer()->CreateWebViewContainer(this));
On 2014/11/03 11:03:37, Fady Samuel wrote:
> Please restore.

Done.

https://codereview.chromium.org/702433002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1637:
GetContentClient()->renderer()->CreateMimeHandlerViewContainer(
On 2014/11/03 11:03:37, Fady Samuel wrote:
> Please restore.

Done.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (left):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:9: #include
"content/public/renderer/render_view.h"
On 2014/11/03 11:03:37, Fady Samuel wrote:
> Is this necessary?

Done.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:11: #include
"extensions/common/guest_view/guest_view_constants.h"
This one is still used.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.cc (right):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.cc:7: #include
"base/pickle.h"
On 2014/11/03 11:03:37, Fady Samuel wrote:
> Is this necessary?

Done.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (left):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:5: #ifndef
CHROME_RENDERER_GUEST_VIEW_GUEST_VIEW_CONTAINER_H_
On 2014/11/03 11:03:38, Fady Samuel wrote:
> EXTENSIONS_RENDERER_GUEST_VIEW_CONTAINER_H_

Done.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/mime_handler_view_container.h (right):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/mime_handler_view_container.h:5: #ifndef
CHROME_RENDERER_GUEST_VIEW_MIME_HANDLER_VIEW_CONTAINER_H_
On 2014/11/03 11:03:38, Fady Samuel wrote:
> This should say:
> 
> EXTESNIONS_RENDERER_GUEST_VIEW_MIME_HANDLER_VIEW_CONTAINER_H_

Done.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
File extensions/renderer/guest_view/web_view_container.h (right):

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/web_view_container.h:5: #ifndef
CHROME_RENDERER_GUEST_VIEW_WEB_VIEW_CONTAINER_H_
On 2014/11/03 11:03:38, Fady Samuel wrote:
> This should be updated to say extensions.

Done.

https://codereview.chromium.org/702433002/diff/20001/extensions/renderer/gues...
extensions/renderer/guest_view/web_view_container.h:18: class WebViewContainer :
public GuestViewContainer {
On 2014/11/03 11:03:38, Fady Samuel wrote:
> Multiple guest types need this container type. I would call this maybe
> ExtensionsGuestViewContainer or CustomGuestViewContainer

Done.

[Fady Samuel, 2014-11-04 11:47:50]

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:30: protected:
I'd prefer if these member variables are private and we instead provide public
const accessors. That way we know only this base class and modify these
variables.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:46:
ExtensionsGuestViewContainer* web_view_container =
change this back to guest_view_container.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:52: if
(!web_view_container)
guest_view_container

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:75:
web_view_container->AttachGuest(request);
guest_view_container

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
File extensions/renderer/guest_view/mime_handler_view_container.cc (right):

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/mime_handler_view_container.cc:35: void
MimeHandlerViewContainer::Ready() {}
If you're not using this you can get rid of it.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/mime_handler_view_container.cc:61: }
If you're not using this, you can get rid of it.

[raymes, 2014-11-04 22:56:09]

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_container.h (right):

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_container.h:30: protected:
On 2014/11/04 11:47:50, Fady Samuel wrote:
> I'd prefer if these member variables are private and we instead provide public
> const accessors. That way we know only this base class and modify these
> variables.

Done.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
File extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc
(right):

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:46:
ExtensionsGuestViewContainer* web_view_container =
On 2014/11/04 11:47:50, Fady Samuel wrote:
> change this back to guest_view_container.

Done.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:52: if
(!web_view_container)
On 2014/11/04 11:47:50, Fady Samuel wrote:
> guest_view_container

Done.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/guest_view_internal_custom_bindings.cc:75:
web_view_container->AttachGuest(request);
On 2014/11/04 11:47:50, Fady Samuel wrote:
> guest_view_container

Done.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
File extensions/renderer/guest_view/mime_handler_view_container.cc (right):

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/mime_handler_view_container.cc:35: void
MimeHandlerViewContainer::Ready() {}
I'm about to use it in a followup CL, but I've removed them for now.

https://codereview.chromium.org/702433002/diff/60001/extensions/renderer/gues...
extensions/renderer/guest_view/mime_handler_view_container.cc:61: }
On 2014/11/04 11:47:50, Fady Samuel wrote:
> If you're not using this, you can get rid of it.

Done.

[Fady Samuel, 2014-11-04 23:01:42]

lgtm

[raymes, 2014-11-04 23:12:29]

raymes@chromium.org changed reviewers:
+ benwells@chromium.org, dcheng@chromium.org, jam@chromium.org

[raymes, 2014-11-04 23:12:30]

+OWNERS

benwells:
extensions/extensions.gyp
extensions/renderer/BUILD.gn
extensions/shell/renderer/shell_content_renderer_client.cc

jam:
chrome/renderer/chrome_content_renderer_client.cc
content/public/renderer/browser_plugin_delegate.h
content/renderer/render_frame_impl.cc

dcheng:
extensions/common/extension_messages.h (NOTE: I only removed an unused param
from an IPC).

[dcheng, 2014-11-04 23:14:15]

rs lgtm for extensions/common/extension_messages.h

[benwells, 2014-11-05 03:53:57]

lgtm

[raymes, 2014-11-06 00:56:26]

raymes@chromium.org changed reviewers:
+ davidben@chromium.org, thestig@chromium.org
- jam@chromium.org

[raymes, 2014-11-06 00:56:27]

-jam

+OWNERS

thestig:
  chrome/renderer/chrome_content_renderer_client.cc

davidben:
  content/public/renderer/browser_plugin_delegate.h
  content/renderer/render_frame_impl.cc

[Lei Zhang, 2014-11-06 01:04:50]

chrome/renderer/chrome_content_renderer_client.cc lgtm here too

[davidben, 2014-11-06 15:30:30]

content lgtm

[raymes, 2014-11-06 22:30:10]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-11-06 22:31:59]

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

[commit-bot: I haz the power, 2014-11-06 23:25:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-06 23:26:54]

Patchset 7 (id:??) landed as
https://crrev.com/78e3d60d83366c66e82ec147a061be200088a656
Cr-Commit-Position: refs/heads/master@{#303118}