Browser tag shim

Shim that simulates a <browser> tag via Mutation Observers.

The actual tag is implemented via the browser plugin. The internals of this
are hidden via Shadow DOM.

Also fixes a bug with Shadow DOM not being enabled for all extensions views.

BUG=133379
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145229

[Mihai Parparita -not on Chrome, 2012-06-21 01:30:11]

Arv: please look at custom tag swizzling in browser_tag.js and see if I did this
right.
Adam: extension_set.cc changes.
Aaron: extension bits, and overall reviewer.

Thanks!

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/common/exten...
File chrome/common/extensions/extension_set.cc (right):

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_set.cc:89: // Additionally, the url will
initialy be null during a window.open call
Adam, you expressed a wish to always use the origin in http://crrev.com/112655,
so this seems like a move in the right direction.

The place where the URL was null during window.open is at
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/chrome_ren....
Is this because the load isn't committed at that point? Should I be passing in
the data source URL instead?

[dglazkov, 2012-06-21 16:22:06]

This looks awesome!

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:59: this.shadowRoot_ = new
WebKitShadowRoot(this);
Is it ok for these members to be enumerable? They seem like a UA implementation
detail.

[Fady Samuel, 2012-06-21 23:14:46]

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:63:
this.objectNode_.style.webkitTransform = 'translateZ(0)';
This should be unnecessary. We just landed a patch to force compositing on
guests, and pepper takes care of the magic in the embedder.

[Mihai Parparita -not on Chrome, 2012-06-25 20:29:43]

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:59: this.shadowRoot_ = new
WebKitShadowRoot(this);
On 2012/06/21 16:22:07, Dimitri Glazkov wrote:
> Is it ok for these members to be enumerable? They seem like a UA
implementation
> detail. 

Switched to using Object.defineProperty so that they're not enumerable.

https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:63:
this.objectNode_.style.webkitTransform = 'translateZ(0)';
On 2012/06/21 23:14:46, Fady Samuel wrote:
> This should be unnecessary. We just landed a patch to force compositing on
> guests, and pepper takes care of the magic in the embedder.

Removed.

[Mihai Parparita -not on Chrome, 2012-06-25 20:39:29]

Also, ping to Aaron/Adam/Erik about reviewing this.

Mihai

On Mon, Jun 25, 2012 at 1:29 PM, <mihaip@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10598006/diff/**
>
2001/chrome/renderer/**resources/extensions/browser_**tag.js<https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/resources/extensions/browser_tag.js>
> File chrome/renderer/resources/**extensions/browser_tag.js (right):
>
> https://chromiumcodereview.**appspot.com/10598006/diff/**
>
2001/chrome/renderer/**resources/extensions/browser_**tag.js#newcode59<https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/resources/extensions/browser_tag.js#newcode59>
> chrome/renderer/resources/**extensions/browser_tag.js:59: this.shadowRoot_
> = new WebKitShadowRoot(this);
> On 2012/06/21 16:22:07, Dimitri Glazkov wrote:
>
>> Is it ok for these members to be enumerable? They seem like a UA
>>
> implementation
>
>> detail.
>>
>
> Switched to using Object.defineProperty so that they're not enumerable.
>
>
> https://chromiumcodereview.**appspot.com/10598006/diff/**
>
2001/chrome/renderer/**resources/extensions/browser_**tag.js#newcode63<https://chromiumcodereview.appspot.com/10598006/diff/2001/chrome/renderer/resources/extensions/browser_tag.js#newcode63>
> chrome/renderer/resources/**extensions/browser_tag.js:63:
> this.objectNode_.style.**webkitTransform = 'translateZ(0)';
> On 2012/06/21 23:14:46, Fady Samuel wrote:
>
>> This should be unnecessary. We just landed a patch to force
>>
> compositing on
>
>> guests, and pepper takes care of the magic in the embedder.
>>
>
> Removed.
>
>
https://chromiumcodereview.**appspot.com/10598006/<https://chromiumcodereview...
>

[arv (Not doing code reviews), 2012-06-25 21:02:24]

hawt

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:12: var
BROWSER_TAG_ATTRIBUTES = ['src', 'width', 'height'];
You might want to wrap all of this code in a closure.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:16: var browserNodes =
document.body.getElementsByTagName('browser');
querySelectorAll is better here. getElementsByTagName slows DOM changes down.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:55: var decorate =
function() {
function decorate() {

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:58: this.objectNode_ =
document.createElement('object');
How about making objectNode_ non enumerable?

Maybe even consider making it truly encapsulated?

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:59:
this.objectNode_.setAttribute('type', 'application/browser-plugin');
.type = ...

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:65:
mutations.forEach(handleMutation);
-4 indent

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:71:
Object.defineProperty(BrowserTag.prototype, 'decorate_', {value: decorate});
I feel like we could probably make this truly transparent if we wanted to.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:73: var handleMutation =
function() {
function handleMutation() {

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:76:
this.objectNode_.postMessage(this.getAttribute('src'));
You should also add accessors for src, width and height.

[Mihai Parparita -not on Chrome, 2012-06-26 00:58:24]

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
File chrome/renderer/resources/extensions/browser_tag.js (right):

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:12: var
BROWSER_TAG_ATTRIBUTES = ['src', 'width', 'height'];
On 2012/06/25 21:02:24, arv wrote:
> You might want to wrap all of this code in a closure.

The extension module system does this already:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/module_sys...

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:16: var browserNodes =
document.body.getElementsByTagName('browser');
On 2012/06/25 21:02:24, arv wrote:
> querySelectorAll is better here. getElementsByTagName slows DOM changes down.

Done.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:58: this.objectNode_ =
document.createElement('object');
On 2012/06/25 21:02:24, arv wrote:
> How about making objectNode_ non enumerable?
> 
> Maybe even consider making it truly encapsulated?

I decided that making BrowserTag into an HTML element subclass wasn't worth it
for now, since it became too annoying to hide all of the implementation details.
I changed BrowserTag to be a wrapper for the DOM node, so that there is no
leakage from the DOM side.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:59:
this.objectNode_.setAttribute('type', 'application/browser-plugin');
On 2012/06/25 21:02:24, arv wrote:
> .type = ...

Done.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:65:
mutations.forEach(handleMutation);
On 2012/06/25 21:02:24, arv wrote:
> -4 indent

Done.

https://chromiumcodereview.appspot.com/10598006/diff/8001/chrome/renderer/res...
chrome/renderer/resources/extensions/browser_tag.js:76:
this.objectNode_.postMessage(this.getAttribute('src'));
On 2012/06/25 21:02:24, arv wrote:
> You should also add accessors for src, width and height.

Done.

[Mihai Parparita -not on Chrome, 2012-06-28 06:03:15]

Ping to arv@ and abart@ (aa@ is off the hook for now)

[Aaron Boodman, 2012-07-02 20:52:35]

http://codereview.chromium.org/10598006/diff/24001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:40: "extension_types":
["platform_app"]
Before we launch all this stuff, we should talk about philosophy about what goes
in platform apps only.

Right now I am having trouble imagining anything that should be platform-apps
only... but maybe there is some reason why that should not be the case?

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:505:
extension->HasAPIPermission(APIPermission::kBrowserTag);
Is it possible to just check for the browser permission when you care about the
browser permission?

This will end up enabling other stuff that the developer did not intend, just
because they have the browser permission.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:733: if (extension &&
extension->HasAPIPermission(APIPermission::kBrowserTag)) {
1) No braces needed on 1-line if statements :)

2) It seems like just checking HasAPIPermission(kBrowserTag) outside
IsWithinPlatformApp() should be sufficient, no? Or are you worried about the
sandboxed origin case again?

3) I think we should find a way to normalize the sandboxed origin case. This is
not going to work for cases where we want features to work for both platform
apps and extensions. Perhaps that context type should be
UNBLESSED_EXTENSION_CONTEXT. Or perhaps we should add a new context type.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/renderer_r...
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/renderer_r...
chrome/renderer/renderer_resources.grd:71: <include name="IDR_BROWSER_TAG_JS"
file="resources\extensions\browser_tag.js" type="BINDATA" />
I don't think that this should be categorized as 'platform app support'. It's
just an API. What to expose it to is a high-level policy decision that we should
be able to change without touching lots of code.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/browser_tag.js (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:5: // Shim that simulates a
<browser> tag via Mutation Observers.
Is the plan to actually implement a <browser> tag at some point? Just curious.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:10: var
BROWSER_TAG_ATTRIBUTES = ['src', 'width', 'height'];
I can't remember, but I think that these names might be exposed in the pages
that this script runs in, which isn't what you want.

http://codereview.chromium.org/10598006/diff/24001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/shadow_dom/empty.html (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/shadow_dom/empty.html:4: <title>Just an
empty window</title>
weird indent

[arv (Not doing code reviews), 2012-07-02 21:04:28]

LGTM with nits

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/browser_tag.js (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:37: this.shadowRoot_ = new
WebKitShadowRoot(node);
No need to make this a property

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:90: }
missing semicolon

[Mihai Parparita -not on Chrome, 2012-07-02 22:05:13]

http://codereview.chromium.org/10598006/diff/24001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:40: "extension_types":
["platform_app"]
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> Before we launch all this stuff, we should talk about philosophy about what
goes
> in platform apps only.
> 
> Right now I am having trouble imagining anything that should be platform-apps
> only... but maybe there is some reason why that should not be the case?

I don't have a strong preference. The rationale with some of the other APIs
(socket, serial) was that they were scary enough that we'd want to roll them out
gradually. That doesn't seem applicable here.

I've enabled this for extensions too.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:505:
extension->HasAPIPermission(APIPermission::kBrowserTag);
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> Is it possible to just check for the browser permission when you care about
the
> browser permission?
> 
> This will end up enabling other stuff that the developer did not intend, just
> because they have the browser permission.

I don't see an easy way to do this (enable it for bindings code only), given
that we're in the same world as the app.

For now, the check for the browser tag permission is not necessary, since all
apps have the experimental permission anyway. I could remove the browser tag
check, and just leave the check for experimental?

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:733: if (extension &&
extension->HasAPIPermission(APIPermission::kBrowserTag)) {
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> 1) No braces needed on 1-line if statements :)
> 
> 2) It seems like just checking HasAPIPermission(kBrowserTag) outside
> IsWithinPlatformApp() should be sufficient, no? Or are you worried about the
> sandboxed origin case again?
> 
> 3) I think we should find a way to normalize the sandboxed origin case. This
is
> not going to work for cases where we want features to work for both platform
> apps and extensions. Perhaps that context type should be
> UNBLESSED_EXTENSION_CONTEXT. Or perhaps we should add a new context type.

Actually, isn't making this work in BLESSED_EXTENSION_CONTEXT only the right
thing? That includes apps and extensions, but not sandboxed pages.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/renderer_r...
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/renderer_r...
chrome/renderer/renderer_resources.grd:71: <include name="IDR_BROWSER_TAG_JS"
file="resources\extensions\browser_tag.js" type="BINDATA" />
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> I don't think that this should be categorized as 'platform app support'. It's
> just an API. What to expose it to is a high-level policy decision that we
should
> be able to change without touching lots of code.

Moved to the general bindings list.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/browser_tag.js (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:5: // Shim that simulates a
<browser> tag via Mutation Observers.
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> Is the plan to actually implement a <browser> tag at some point? Just curious.

Perhaps, but I don't think Apple would be too receptive to WebKit changes for
new tags that 1) are not standardized 2) are exposed to apps only. Perhaps if we
can get an agreement with Mozilla on the <browser> API it'd be worth fighting
that battle?

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:10: var
BROWSER_TAG_ATTRIBUTES = ['src', 'width', 'height'];
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> I can't remember, but I think that these names might be exposed in the pages
> that this script runs in, which isn't what you want.

The module system wrap everything in an anonymous function:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/module_sys...

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:37: this.shadowRoot_ = new
WebKitShadowRoot(node);
On 2012/07/02 21:04:28, arv wrote:
> No need to make this a property

Done. The observer didn't need to be a property either.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:90: }
On 2012/07/02 21:04:28, arv wrote:
> missing semicolon

Done.

http://codereview.chromium.org/10598006/diff/24001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/shadow_dom/empty.html (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/shadow_dom/empty.html:4: <title>Just an
empty window</title>
On 2012/07/02 20:52:35, Aaron Boodman wrote:
> weird indent

Stray tab, fixed.

[Aaron Boodman, 2012-07-02 23:04:37]

http://codereview.chromium.org/10598006/diff/24001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:40: "extension_types":
["platform_app"]
On 2012/07/02 22:05:14, Mihai Parparita wrote:
> On 2012/07/02 20:52:35, Aaron Boodman wrote:
> > Before we launch all this stuff, we should talk about philosophy about what
> goes
> > in platform apps only.
> > 
> > Right now I am having trouble imagining anything that should be
platform-apps
> > only... but maybe there is some reason why that should not be the case?
> 
> I don't have a strong preference. The rationale with some of the other APIs
> (socket, serial) was that they were scary enough that we'd want to roll them
out
> gradually. That doesn't seem applicable here.
> 
> I've enabled this for extensions too.

Agree about the rolling out gradually sentiment :). But I think that eventually
we probably want serial, socket, etc to work in extensions.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:505:
extension->HasAPIPermission(APIPermission::kBrowserTag);
On 2012/07/02 22:05:14, Mihai Parparita wrote:
> On 2012/07/02 20:52:35, Aaron Boodman wrote:
> > Is it possible to just check for the browser permission when you care about
> the
> > browser permission?
> > 
> > This will end up enabling other stuff that the developer did not intend,
just
> > because they have the browser permission.
> 
> I don't see an easy way to do this (enable it for bindings code only), given
> that we're in the same world as the app.
> 
> For now, the check for the browser tag permission is not necessary, since all
> apps have the experimental permission anyway. I could remove the browser tag
> check, and just leave the check for experimental?

Oh, I see the issue. My earlier comment was mistaken.

How about this:
- Move the check for webui into allowWebComponents()
- Move the check for browser permission into allowWebComponents()
- Rename IsExperimentalWebFeaturesAllowed() to something like
IsExperimentalExtensionPermissionEnabled(const WebDocument&)

I'm trying to set us up so that features are enabled specifically when needed;
and we don't end up with an IsSpecialStuffEnabled() which enables lots of them.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:733: if (extension &&
extension->HasAPIPermission(APIPermission::kBrowserTag)) {
On 2012/07/02 22:05:14, Mihai Parparita wrote:
> On 2012/07/02 20:52:35, Aaron Boodman wrote:
> > 1) No braces needed on 1-line if statements :)
> > 
> > 2) It seems like just checking HasAPIPermission(kBrowserTag) outside
> > IsWithinPlatformApp() should be sufficient, no? Or are you worried about the
> > sandboxed origin case again?
> > 
> > 3) I think we should find a way to normalize the sandboxed origin case. This
> is
> > not going to work for cases where we want features to work for both platform
> > apps and extensions. Perhaps that context type should be
> > UNBLESSED_EXTENSION_CONTEXT. Or perhaps we should add a new context type.
> 
> Actually, isn't making this work in BLESSED_EXTENSION_CONTEXT only the right
> thing? That includes apps and extensions, but not sandboxed pages.

It depends on what you want. Looking at the code, I thought you were trying to
make the <browser> tag work in sandboxed frames.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/browser_tag.js (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:5: // Shim that simulates a
<browser> tag via Mutation Observers.
On 2012/07/02 22:05:14, Mihai Parparita wrote:
> On 2012/07/02 20:52:35, Aaron Boodman wrote:
> > Is the plan to actually implement a <browser> tag at some point? Just
curious.
> 
> Perhaps, but I don't think Apple would be too receptive to WebKit changes for
> new tags that 1) are not standardized 2) are exposed to apps only. Perhaps if
we
> can get an agreement with Mozilla on the <browser> API it'd be worth fighting
> that battle?

I don't think so. I wondered if you were considering alternate approaches such
as adding hooks to the parser to allow these kind of tags to be implemented. But
I think way you have it makes most sense.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/browser_tag.js:10: var
BROWSER_TAG_ATTRIBUTES = ['src', 'width', 'height'];
On 2012/07/02 22:05:14, Mihai Parparita wrote:
> On 2012/07/02 20:52:35, Aaron Boodman wrote:
> > I can't remember, but I think that these names might be exposed in the pages
> > that this script runs in, which isn't what you want.
> 
> The module system wrap everything in an anonymous function:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/module_sys...

Cool.

[Mihai Parparita -not on Chrome, 2012-07-02 23:23:45]

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/chrome_ren...
chrome/renderer/chrome_render_view_observer.cc:505:
extension->HasAPIPermission(APIPermission::kBrowserTag);
On 2012/07/02 23:04:37, Aaron Boodman wrote:
> How about this:
> - Move the check for webui into allowWebComponents()
> - Move the check for browser permission into allowWebComponents()
> - Rename IsExperimentalWebFeaturesAllowed() to something like
> IsExperimentalExtensionPermissionEnabled(const WebDocument&)
> 
> I'm trying to set us up so that features are enabled specifically when needed;
> and we don't end up with an IsSpecialStuffEnabled() which enables lots of
them.

Did something along these lines.

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10598006/diff/24001/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:733: if (extension &&
extension->HasAPIPermission(APIPermission::kBrowserTag)) {
On 2012/07/02 23:04:37, Aaron Boodman wrote:
> It depends on what you want. Looking at the code, I thought you were trying to
> make the <browser> tag work in sandboxed frames.

Nope, that wasn't the plan. I had started this CL before Evan added
IsWithinPlatformApp, and didn't quite think things through when merging in his
changes.

[Aaron Boodman, 2012-07-02 23:27:03]

LGTM

[commit-bot: I haz the power, 2012-07-02 23:27:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/10598006/21003

[commit-bot: I haz the power, 2012-07-03 00:43:35]

Change committed as 145229