Add <extensionview>

This adds the ExtensionView to the family of GuestViews.

This...
- Wires up ExtensionView. Can apply src for navigation via inline and js call.
- Adds permission for manifests.
- Add browser tests for navigation and adding ExtensionViews to DOM.

Some code is duplicated from existing GuestViews (e.g. *ViewAttribute and some GuestViewBase implementations). Based on how GuestView code will be refactored, some of this can be consolidated.

This...
- Does not run in a detached state. (Refers to CR 845363004)
- Will have more functionality and restrictions implemented in the future.

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

[apacible, 2015-01-25 09:56:49]

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

[apacible, 2015-01-25 10:14:26]

fsamuel, PTAL. Thanks!

[Fady Samuel, 2015-01-26 12:52:02]

Some initial comments. This is awesome so far!

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/extension_view/extension_view_guest.cc
(right):

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:64:
content::SiteInstance::CreateForURL(browser_context(), GetOwnerSiteURL());
This doesn't seem right. I think you want to use the src value that you pass
along here.

create_params is the object of parameters you pass when you call
this.guest.create(createParams, ...) in JavaScript.

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:99: return
NULL;
nit: use nullptr

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:132: if
(attached() && (params.url.GetOrigin() != view_page_.GetOrigin())) {
Wow, what's going on here? You're killing the guest if it tries to navigate to a
different origin? Add a comment to document why you're doing this? I assume this
is for security reasons.

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
File
extensions/browser/guest_view/extension_view/extension_view_guest_delegate.h
(right):

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest_delegate.h:22:
class ExtensionViewGuestDelegate {
Are you using this delegate for anything?

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/guest_view/extension_view.js (right):

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:15:
this.createGuest();
This means you create the guest as soon as you create the element. In other
GuestViews, we don't have a guest until we set the src attribute. I suggest
taking that approach unless there's a specific reason why you'd like to create
the guest immediately.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:36:
this.guest.create(params, function() {
Use buildParams() which calls buildContainerParams() + some default info like
instanceId/elementWidth/elementHeight.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:45:
ExtensionViewImpl.prototype.buildAttachParams = function() {
Use buildContainerParams now. Also, given you have an attributes object,
probably easiest to iterate over that as <webview> does.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:57:
this.attributes[attributeName].ignoreMutation) {
I'd move this ignoreMutation check into SrcAttribute.handleMutation.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:65:
ExtensionViewImpl.prototype.onElementDetached = function() {
I believe the default behavior in GuestViewContainer is to destroy the guest on
detachment anyway. This isn't necessary.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/guest_view/extension_view_attributes.js
(right):

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view_attributes.js:106: if
(!this.extensionViewImpl.elementAttached || !this.getValue() ||
I'd create the guest here on first call to parse, and then navigate if we
already have a guest.

[apacible, 2015-01-26 21:55:55]

Patchset #2 (id:20001) has been deleted

[apacible, 2015-01-26 22:10:36]

Patchset #2 (id:40001) has been deleted

[apacible, 2015-01-26 22:12:33]

Thanks for the comments!

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/extension_view/extension_view_guest.cc
(right):

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:64:
content::SiteInstance::CreateForURL(browser_context(), GetOwnerSiteURL());
On 2015/01/26 12:52:02, Fady Samuel wrote:
> This doesn't seem right. I think you want to use the src value that you pass
> along here.
> 
> create_params is the object of parameters you pass when you call
> this.guest.create(createParams, ...) in JavaScript.

Done.

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:99: return
NULL;
On 2015/01/26 12:52:02, Fady Samuel wrote:
> nit: use nullptr

I cleaned up this class and end up removing the WebContentsDelegate
implementations.

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:132: if
(attached() && (params.url.GetOrigin() != view_page_.GetOrigin())) {
On 2015/01/26 12:52:02, Fady Samuel wrote:
> Wow, what's going on here? You're killing the guest if it tries to navigate to
a
> different origin? Add a comment to document why you're doing this? I assume
this
> is for security reasons.

I wanted the guest to stay on the same origin, but I'm going to remove this
since we're now looking into possibly more flexible navigation.

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
File
extensions/browser/guest_view/extension_view/extension_view_guest_delegate.h
(right):

https://codereview.chromium.org/873933002/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/extension_view/extension_view_guest_delegate.h:22:
class ExtensionViewGuestDelegate {
On 2015/01/26 12:52:02, Fady Samuel wrote:
> Are you using this delegate for anything?

Removed delegate.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/guest_view/extension_view.js (right):

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:15:
this.createGuest();
On 2015/01/26 12:52:02, Fady Samuel wrote:
> This means you create the guest as soon as you create the element. In other
> GuestViews, we don't have a guest until we set the src attribute. I suggest
> taking that approach unless there's a specific reason why you'd like to create
> the guest immediately.

Done.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:36:
this.guest.create(params, function() {
On 2015/01/26 12:52:02, Fady Samuel wrote:
> Use buildParams() which calls buildContainerParams() + some default info like
> instanceId/elementWidth/elementHeight.

Done.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:45:
ExtensionViewImpl.prototype.buildAttachParams = function() {
On 2015/01/26 12:52:02, Fady Samuel wrote:
> Use buildContainerParams now. Also, given you have an attributes object,
> probably easiest to iterate over that as <webview> does.

Done.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:57:
this.attributes[attributeName].ignoreMutation) {
On 2015/01/26 12:52:02, Fady Samuel wrote:
> I'd move this ignoreMutation check into SrcAttribute.handleMutation.

Done.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view.js:65:
ExtensionViewImpl.prototype.onElementDetached = function() {
On 2015/01/26 12:52:02, Fady Samuel wrote:
> I believe the default behavior in GuestViewContainer is to destroy the guest
on
> detachment anyway. This isn't necessary.

Keeping since I'm added to onElementDetached.

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/guest_view/extension_view_attributes.js
(right):

https://codereview.chromium.org/873933002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/guest_view/extension_view_attributes.js:106: if
(!this.extensionViewImpl.elementAttached || !this.getValue() ||
On 2015/01/26 12:52:02, Fady Samuel wrote:
> I'd create the guest here on first call to parse, and then navigate if we
> already have a guest.

Done.

[apacible, 2015-01-26 22:38:15]

apacible@chromium.org changed reviewers:
+ asvitkine@chromium.org, rockot@chromium.org

[apacible, 2015-01-26 22:39:16]

Adding OWNERS:

rockot@:
- chrome/common/extensions/permissions/permission_set_unittest.cc
- extensions/browser/BUILD.gn
- extensions/extensions.gyp
- extensions/extensions_strings.grd
- extensions/common/api/_api_features.json
- extensions/common/api/_permission_features.json
- extensions/common/api/schemas.gypi
- extensions/common/permissions/api_permission.h
- extensions/common/permissions/extensions_api_permissions.cc
- extensions/renderer/dispatcher.cc
- extensions/renderer/resources/extensions_renderer_resources.grd

asvitkine@:
- extensions/browser/extension_function_histogram_value.h

[apacible, 2015-01-26 22:45:49]

Ran git cl format browser for formatting.

[Alexei Svitkine (slow), 2015-01-26 22:54:47]

https://codereview.chromium.org/873933002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/873933002/diff/80001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:1014:
EXTENSIONVIEWINTERNAL_NAVIGATE,
Please update histograms.xml too.

[apacible, 2015-01-26 23:18:57]

https://codereview.chromium.org/873933002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/873933002/diff/80001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:1014:
EXTENSIONVIEWINTERNAL_NAVIGATE,
On 2015/01/26 22:54:47, Alexei Svitkine wrote:
> Please update histograms.xml too.

Done.

[Alexei Svitkine (slow), 2015-01-27 16:06:14]

histograms/header file lgtm, but make sure to rebase if someone beats you to it
for those values

[apacible, 2015-01-27 22:59:33]

On 2015/01/27 16:06:14, Alexei Svitkine wrote:
> histograms/header file lgtm, but make sure to rebase if someone beats you to
it
> for those values

Will do, thanks!

[Fady Samuel, 2015-01-27 22:59:57]

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/extension_view/extension_view_guest.cc
(right):

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:44: GURL
url(src);
The URL may be invalid. Check here.

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:61: if
(!create_params.GetString(extensionview::kAttributeSrc, &str))
Please call the callback with nullptr if you're going to exit early. Note to
self: I need to implement a CallbackRunner or something similar that uses RAII
to ensure that the callback is call in all early exits.

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:64: GURL
source(str);
Check if the URL is valid and exit earliy if not.

https://codereview.chromium.org/873933002/diff/100001/extensions/renderer/res...
File extensions/renderer/resources/guest_view/extension_view.js (right):

https://codereview.chromium.org/873933002/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/extension_view.js:15:
this.beforeFirstNavigation = true;
Move beforeFirstNavigation into SrcAttribute, then add a reset() method to reset
the beforeFirstNavigation.

https://codereview.chromium.org/873933002/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/extension_view.js:61:
this.beforeFirstNavigation = true;
Once you introduce a reset():

this.attributes[ExtensionViewConstants.ATTRIBUTE_SRC].reset(); here.

beforeFirstNavigation is kind of an implementation detail of SrcAttribute that
should go away from here. This needs to be fixed in WebView too. I'll take care
of that in WebView.

[apacible, 2015-01-28 02:39:37]

Patchset #6 (id:140001) has been deleted

[apacible, 2015-01-28 02:43:32]

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/extension_view/extension_view_guest.cc
(right):

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:44: GURL
url(src);
On 2015/01/27 22:59:56, Fady Samuel wrote:
> The URL may be invalid. Check here.

Done.

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:61: if
(!create_params.GetString(extensionview::kAttributeSrc, &str))
On 2015/01/27 22:59:56, Fady Samuel wrote:
> Please call the callback with nullptr if you're going to exit early. Note to
> self: I need to implement a CallbackRunner or something similar that uses RAII
> to ensure that the callback is call in all early exits.

Done.

https://codereview.chromium.org/873933002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/extension_view/extension_view_guest.cc:64: GURL
source(str);
On 2015/01/27 22:59:56, Fady Samuel wrote:
> Check if the URL is valid and exit earliy if not.

Done.

https://codereview.chromium.org/873933002/diff/100001/extensions/renderer/res...
File extensions/renderer/resources/guest_view/extension_view.js (right):

https://codereview.chromium.org/873933002/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/extension_view.js:15:
this.beforeFirstNavigation = true;
On 2015/01/27 22:59:57, Fady Samuel wrote:
> Move beforeFirstNavigation into SrcAttribute, then add a reset() method to
reset
> the beforeFirstNavigation.

Done.

https://codereview.chromium.org/873933002/diff/100001/extensions/renderer/res...
extensions/renderer/resources/guest_view/extension_view.js:61:
this.beforeFirstNavigation = true;
On 2015/01/27 22:59:57, Fady Samuel wrote:
> Once you introduce a reset():
> 
> this.attributes[ExtensionViewConstants.ATTRIBUTE_SRC].reset(); here.
> 
> beforeFirstNavigation is kind of an implementation detail of SrcAttribute that
> should go away from here. This needs to be fixed in WebView too. I'll take
care
> of that in WebView.

Done.

[apacible, 2015-01-28 07:08:08]

Patchset #6 (id:160001) has been deleted

[apacible, 2015-01-28 07:08:42]

Patchset #6 (id:180001) has been deleted

[Fady Samuel, 2015-01-29 14:10:07]

lgtm + one comment.

https://codereview.chromium.org/873933002/diff/200001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/extension_view/shim/main.html
(left):

https://codereview.chromium.org/873933002/diff/200001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/extension_view/shim/main.html:3: *
Copyright 2014 The Chromium Authors. All rights reserved.  Use of this
Could you move these files outside of the shim directory? That name is legacy
before <webview> was a custom element.

[apacible, 2015-01-29 18:05:44]

https://codereview.chromium.org/873933002/diff/200001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/extension_view/shim/main.html
(left):

https://codereview.chromium.org/873933002/diff/200001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/extension_view/shim/main.html:3: *
Copyright 2014 The Chromium Authors. All rights reserved.  Use of this
On 2015/01/29 14:10:07, Fady Samuel wrote:
> Could you move these files outside of the shim directory? That name is legacy
> before <webview> was a custom element.

Done.

[Ken Rockot(use gerrit already), 2015-01-29 19:07:05]

so the CL itself looks fine, but this is the first i've heard about
extensionview - is there an API proposal doc?

[Ken Rockot(use gerrit already), 2015-01-29 19:32:10]

extensions lgtm

[apacible, 2015-01-29 19:51:41]

The CQ bit was checked by apacible@chromium.org

[commit-bot: I haz the power, 2015-01-29 19:52:37]

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

[commit-bot: I haz the power, 2015-01-29 20:49:43]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2015-01-29 20:50:48]

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

[not at google - send to devlin, 2015-02-07 00:58:11]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-02-07 00:58:12]

This seems the same as <extensionoptions> except with a navigate() function
rather than an id property. Is there something I'm missing?

[Fady Samuel, 2015-02-07 01:14:54]

On 2015/02/07 00:58:12, kalman wrote:
> This seems the same as <extensionoptions> except with a navigate() function
> rather than an id property. Is there something I'm missing?

Ahh I think I missed something in code review:

You're not initializing the extension_function_dispatcher:

See ExtensionOptionsGuest::DidInitialize:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...