Model <webview> and <appview> script injection after <extensionoptions>

Model <webview> and <appview> script injection after <extensionoptions>

BUG=407833
Committed: https://crrev.com/bb30da9b554a5b46cc72145800da0ed10b98cf6b
Cr-Commit-Position: refs/heads/master@{#292224}

[Fady Samuel, 2014-08-26 22:08:38]

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

[not at google - send to devlin, 2014-08-26 22:23:26]

Nice patch. A couple of things to fix, and a chance to make an even nicer
cleanup if you'd like :)

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:286: if
(extensions::GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV ||
You could clean this up by adding a vacuous webViewExperimental API here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

Mark it internal (maybe call it webViewExperimentalInternal just for
consistency?) - but has the channel and the whitelist. Then you can replace all
of this with just GetAvailability("webViewExperimentalInternal").is_available().

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:293: } else
{
I think you should have else-if BLESSED or UNBLESSED extension context. We don't
want to deny webview in web contents or content scripts.

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:300: } else
{
ditto (though just BLESSED in this case).

[Fady Samuel, 2014-08-26 23:30:36]

PTAL Ben! :-)

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:286: if
(extensions::GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV ||
On 2014/08/26 22:23:26, kalman wrote:
> You could clean this up by adding a vacuous webViewExperimental API here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> 
> Mark it internal (maybe call it webViewExperimentalInternal just for
> consistency?) - but has the channel and the whitelist. Then you can replace
all
> of this with just
GetAvailability("webViewExperimentalInternal").is_available().

Done.

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:293: } else
{
On 2014/08/26 22:23:26, kalman wrote:
> I think you should have else-if BLESSED or UNBLESSED extension context. We
don't
> want to deny webview in web contents or content scripts.

Done.

https://codereview.chromium.org/512543002/diff/1/chrome/renderer/extensions/c...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:300: } else
{
On 2014/08/26 22:23:26, kalman wrote:
> ditto (though just BLESSED in this case).

Done.

[not at google - send to devlin, 2014-08-27 01:15:59]

Nice :)

https://codereview.chromium.org/512543002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/512543002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:807:
"webViewExperimentalInternal": {
:D

Mention crbug.com/264852 here though. That seems to be the relevant bug which
relates to this whitelist?

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:281: } else
{
why not "} else if {"?

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:283:
context_type == extensions::Feature::UNBLESSED_EXTENSION_CONTEXT) {
I was thinking about this - it's probably enough to just have BLESSED here, not
UNBLESSED. IIRC UNBLESSED is just to make component extensions get access to
this for WebUI. Normal extensions don't need that.

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:291: } else
{
Likewise: "} else if {" ?

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:292: if
(context_type == extensions::Feature::BLESSED_EXTENSION_CONTEXT)
Sheesh I wish this file was in the extensions namespace. So much redundancy in
here :(

[Fady Samuel, 2014-08-27 01:33:36]

PTAL

https://codereview.chromium.org/512543002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/512543002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:807:
"webViewExperimentalInternal": {
On 2014/08/27 01:15:59, kalman wrote:
> :D
> 
> Mention crbug.com/264852 here though. That seems to be the relevant bug which
> relates to this whitelist?

That comment is stale. The Files App is no longer in the whitelist. I actually
don't know what the Apps are that are in the whitelist unfortunately.

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
File chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc
(right):

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:281: } else
{
On 2014/08/27 01:15:59, kalman wrote:
> why not "} else if {"?

Done.

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:283:
context_type == extensions::Feature::UNBLESSED_EXTENSION_CONTEXT) {
On 2014/08/27 01:15:59, kalman wrote:
> I was thinking about this - it's probably enough to just have BLESSED here,
not
> UNBLESSED. IIRC UNBLESSED is just to make component extensions get access to
> this for WebUI. Normal extensions don't need that.

Done.

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:291: } else
{
On 2014/08/27 01:15:59, kalman wrote:
> Likewise: "} else if {" ?

Done.

https://codereview.chromium.org/512543002/diff/20001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc:292: if
(context_type == extensions::Feature::BLESSED_EXTENSION_CONTEXT)
On 2014/08/27 01:15:59, kalman wrote:
> Sheesh I wish this file was in the extensions namespace. So much redundancy in
> here :(

This code is moving into extensions soon.

[not at google - send to devlin, 2014-08-27 01:44:17]

https://codereview.chromium.org/512543002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/512543002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:807:
"webViewExperimentalInternal": {
On 2014/08/27 01:33:35, Fady Samuel wrote:
> On 2014/08/27 01:15:59, kalman wrote:
> > :D
> > 
> > Mention crbug.com/264852 here though. That seems to be the relevant bug
which
> > relates to this whitelist?
> 
> That comment is stale. The Files App is no longer in the whitelist. I actually
> don't know what the Apps are that are in the whitelist unfortunately.

Bleh. Can you file a bug on this then, and reference it here? CC myself,
asargent, miket.

[not at google - send to devlin, 2014-08-27 01:44:18]

lgtm

[Fady Samuel, 2014-08-27 01:49:33]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-08-27 01:50:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/512543002/40001

[commit-bot: I haz the power, 2014-08-27 03:21:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-27 04:03:57]

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

[commit-bot: I haz the power, 2014-08-27 04:03:58]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[not at google - send to devlin, 2014-08-27 20:01:05]

https://codereview.chromium.org/512543002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/512543002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:818: },
This needs to be:

"webViewExperimentalInternal": [{
  "internal": true,
  "channel": "dev",
  "contexts": ["blessed_extension"],
  "dependencies": ["permission:webview"]
},{
  "internal": true,
  "channel": "stable",
  "contexts": ["blessed_extension"],
  "dependencies": ["permission:webview"],
  "whitelist": [...]
}]

[Fady Samuel, 2014-08-27 20:13:43]

https://codereview.chromium.org/512543002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/512543002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/_api_features.json:818: },
On 2014/08/27 20:01:04, kalman wrote:
> This needs to be:
> 
> "webViewExperimentalInternal": [{
>   "internal": true,
>   "channel": "dev",
>   "contexts": ["blessed_extension"],
>   "dependencies": ["permission:webview"]
> },{
>   "internal": true,
>   "channel": "stable",
>   "contexts": ["blessed_extension"],
>   "dependencies": ["permission:webview"],
>   "whitelist": [...]
> }]

Done. CQ'ing. Thanks! :-)

[Fady Samuel, 2014-08-27 20:14:05]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-08-27 20:14:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/512543002/60001

[Fady Samuel, 2014-08-27 20:18:35]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2014-08-27 20:19:26]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-08-27 20:20:56]

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

[commit-bot: I haz the power, 2014-08-27 21:20:37]

Committed patchset #5 (id:80001) as a933723f212f34837045beac0f4260445f1ec94e

[commit-bot: I haz the power, 2014-09-10 02:54:05]

Patchset 5 (id:??) landed as
https://crrev.com/bb30da9b554a5b46cc72145800da0ed10b98cf6b
Cr-Commit-Position: refs/heads/master@{#292224}