<webview>: Implement support for package-local chrome-extension:// URLs

<webview>: Implement support for package-local chrome-extension:// URLs

Add a new object to manifest.json: 

"webview": {
  'accessible_resources': [
    // List of files and URL patterns go here.
  ],
  'privileged_partitions': [
    // List of partition wildcards go here.
  ]
}
BUG=157626
Test=WebViewTest.Shim_TestChromeExtensionURL, WebViewTest.Shim_TestLoadAbortChromeExtensionURLWrongPartition

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

[Fady Samuel, 2013-08-26 16:18:55]

Not quite ready for review. Adding creis@ for informational purposes.

[Fady Samuel, 2013-09-24 15:23:29]

creis: render_view_host_manager.cc, profile_impl_io_data.cc
mpcomplete: chrome changes, this API is only exposed to dev. Will send out a
formal API review in parallel today. This API is based on a number of
discussions we had.

[Charlie Reis, 2013-09-24 19:41:55]

Nasko, I've got a question for you below.

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:597:
job_factory->SetProtocolHandler(
This doesn't make sense to me.

First, the comment suggests that the first branch is for non-guests and the
second branch is for guests.  In that case, it might make sense to selectively
create the extension protocol handler (and not the rest of
SetUpJobFactoryDefaults) here, though we should document that's what we're
doing.

Indeed, that reflects how the code was originally written:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profi...

However, the check is now about whether the partition is in_memory or not, as of
Nasko's https://chromiumcodereview.appspot.com/11147026.  That means guests
could hit either branch (based on whether the guest is persistant), and
non-guests could hit either branch.  That means this check isn't correct for
what it's being used for.

Nasko, perhaps you could help us understand what the right thing to do is?  Was
this check just a temporary thing before guests could have persistent storage?

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:600: GetExtensionInfoMap()));
nit: 598-600 all have too much indent.

https://codereview.chromium.org/22793018/diff/12001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/22793018/diff/12001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:955: if
(pending_web_ui() && !render_view_host_->GetProcess()->IsGuest())
Oops, looks like we missed this one in Hui's CL.  Can I ask you to land this
separately in case the rest needs to be reverted for some reason?

[nasko, 2013-09-24 20:02:01]

Just answered Charlie's question and haven't reviewed other code. 

One remark: We have hard time right now securing certain renderer requests,
since in extensions we allow subresources in web-accessible page to be loaded
without being explicitly listed. For web accessible resources, let's ensure that
*every* (sub)resource must be in the manifest.

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:597:
job_factory->SetProtocolHandler(
On 2013/09/24 19:41:55, creis wrote:
> This doesn't make sense to me.
> 
> First, the comment suggests that the first branch is for non-guests and the
> second branch is for guests.  In that case, it might make sense to selectively
> create the extension protocol handler (and not the rest of
> SetUpJobFactoryDefaults) here, though we should document that's what we're
> doing.
> 
> Indeed, that reflects how the code was originally written:
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profi...
> 
> However, the check is now about whether the partition is in_memory or not, as
of
> Nasko's https://chromiumcodereview.appspot.com/11147026.  That means guests
> could hit either branch (based on whether the guest is persistant), and
> non-guests could hit either branch.  That means this check isn't correct for
> what it's being used for.
> 
> Nasko, perhaps you could help us understand what the right thing to do is? 
Was
> this check just a temporary thing before guests could have persistent storage?

This indeed looks incorrect and I think it goes back to the original patch
Charlie is pointing to. The logic here (protocol handling) shouldn't be based on
whether a partition is in-memory or not. Protocol handling should be based on
whether this is for a guest process or not, so I think we should fix this here.

[Matt Perry, 2013-09-24 21:06:53]

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:343: ExtensionRendererState*
renderer_state =
Is there a reason you put this check here are the top? I think it belongs at the
bottom near the web_accessible_resources check.

https://codereview.chromium.org/22793018/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:378:
"webview_accessible_resources": {
Now that I think about it, maybe these should be grouped under a common key.
Then we could make them optional/required as needed. e.g.
  "webview": {
    "accessible_resources": [x, y, z], // required
    "privileged_partitions": [a, b] // also required?
  }

WDYT?

https://codereview.chromium.org/22793018/diff/12001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:307: var
localResource = 'chrome-extension://' + embedder.extensionId +
Does chrome.runtime.getURL() not work here?

[Fady Samuel, 2013-09-24 23:05:02]

PTAL

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:597:
job_factory->SetProtocolHandler(
On 2013/09/24 20:02:01, nasko wrote:
> On 2013/09/24 19:41:55, creis wrote:
> > This doesn't make sense to me.
> > 
> > First, the comment suggests that the first branch is for non-guests and the
> > second branch is for guests.  In that case, it might make sense to
selectively
> > create the extension protocol handler (and not the rest of
> > SetUpJobFactoryDefaults) here, though we should document that's what we're
> > doing.
> > 
> > Indeed, that reflects how the code was originally written:
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/profi...
> > 
> > However, the check is now about whether the partition is in_memory or not,
as
> of
> > Nasko's https://chromiumcodereview.appspot.com/11147026.  That means guests
> > could hit either branch (based on whether the guest is persistant), and
> > non-guests could hit either branch.  That means this check isn't correct for
> > what it's being used for.
> > 
> > Nasko, perhaps you could help us understand what the right thing to do is? 
> Was
> > this check just a temporary thing before guests could have persistent
storage?
> 
> This indeed looks incorrect and I think it goes back to the original patch
> Charlie is pointing to. The logic here (protocol handling) shouldn't be based
on
> whether a partition is in-memory or not. Protocol handling should be based on
> whether this is for a guest process or not, so I think we should fix this
here.

Looks like profile_impl_io_data doesn't have enough info to determine for
something is a guest? This looks like it needs a bunch of refactoring. It
probably doesn't make sense to do that refactoring in this CL. Shall I do it in
a separate CL and add a TODO here? Shall I write a separate CL and land that
first?

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:600: GetExtensionInfoMap()));
On 2013/09/24 19:41:55, creis wrote:
> nit: 598-600 all have too much indent.

Done.

https://codereview.chromium.org/22793018/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:378:
"webview_accessible_resources": {
On 2013/09/24 21:06:54, Matt Perry wrote:
> Now that I think about it, maybe these should be grouped under a common key.
> Then we could make them optional/required as needed. e.g.
>   "webview": {
>     "accessible_resources": [x, y, z], // required
>     "privileged_partitions": [a, b] // also required?
>   }
> 
> WDYT?

Agreed, that seems cleaner. Done!

https://codereview.chromium.org/22793018/diff/12001/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:307: var
localResource = 'chrome-extension://' + embedder.extensionId +
On 2013/09/24 21:06:54, Matt Perry wrote:
> Does chrome.runtime.getURL() not work here?

It does. I just didn't know it existed :P. Thanks! Done.

https://codereview.chromium.org/22793018/diff/12001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/22793018/diff/12001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:955: if
(pending_web_ui() && !render_view_host_->GetProcess()->IsGuest())
On 2013/09/24 19:41:55, creis wrote:
> Oops, looks like we missed this one in Hui's CL.  Can I ask you to land this
> separately in case the rest needs to be reverted for some reason?

Done.

[Matt Perry, 2013-09-24 23:09:29]

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:343: ExtensionRendererState*
renderer_state =
On 2013/09/24 21:06:54, Matt Perry wrote:
> Is there a reason you put this check here are the top? I think it belongs at
the
> bottom near the web_accessible_resources check.

ping

https://codereview.chromium.org/22793018/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/webview_accessible_resources_handler.h (right):

https://codereview.chromium.org/22793018/diff/26001/chrome/common/extensions/...
chrome/common/extensions/webview_accessible_resources_handler.h:18: struct
WebviewAccessibleResourcesInfo : public Extension::ManifestData {
Make this WebviewInfo, and update the filename and related references.

[Fady Samuel, 2013-09-24 23:35:15]

PTAL mpcomplete.

creis for profile_io_data_impl: I'm looking into a refactor for
profile_impl_io_data in another CL. I'll put this part of the change on hold
until the other CL is written, reviewed, and lands.

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/extensions...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/22793018/diff/12001/chrome/browser/extensions...
chrome/browser/extensions/extension_protocols.cc:343: ExtensionRendererState*
renderer_state =
On 2013/09/24 23:09:29, Matt Perry wrote:
> On 2013/09/24 21:06:54, Matt Perry wrote:
> > Is there a reason you put this check here are the top? I think it belongs at
> the
> > bottom near the web_accessible_resources check.
> 
> ping

Oops. I'm not sure how I missed this. It seems
content::PageTransitionIsWebTriggerable fails and so I can't put it below that.
This happens because we are doing a top level navigation and not a subframe or
link nav.

[Fady Samuel, 2013-09-24 23:35:38]

https://codereview.chromium.org/22793018/diff/26001/chrome/common/extensions/...
File chrome/common/extensions/webview_accessible_resources_handler.h (right):

https://codereview.chromium.org/22793018/diff/26001/chrome/common/extensions/...
chrome/common/extensions/webview_accessible_resources_handler.h:18: struct
WebviewAccessibleResourcesInfo : public Extension::ManifestData {
On 2013/09/24 23:09:29, Matt Perry wrote:
> Make this WebviewInfo, and update the filename and related references.

Done.

[Matt Perry, 2013-09-25 01:13:54]

couple small changes, then extensions lgtm

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:380: "extension_types":
["platform_app"]
add min_manifest_version: 2

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
File chrome/common/extensions/webview_handler.cc (right):

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
chrome/common/extensions/webview_handler.cc:42: if
(extension->manifest_version() < 2)
Rather than checking this here, let's make webview require manifest v2

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
chrome/common/extensions/webview_handler.cc:50: for (unsigned int i = 0;
use size_t

https://codereview.chromium.org/22793018/diff/34001/extensions/common/manifes...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/22793018/diff/34001/extensions/common/manifes...
extensions/common/manifest_constants.cc:586: "Invalid value
for'webview_accessible_resources'.";
webview.accessible_resources

https://codereview.chromium.org/22793018/diff/34001/extensions/common/manifes...
extensions/common/manifest_constants.cc:588: "Invalid value for
'webview_accessible_resources[*]'.";
etc

[Fady Samuel, 2013-09-25 03:29:31]

Addressed extension nits. This CL is now just pending a profile_impl_io_data
refactor. I'll send that out tomorrow.

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
chrome/common/extensions/api/_manifest_features.json:380: "extension_types":
["platform_app"]
On 2013/09/25 01:13:54, Matt Perry wrote:
> add min_manifest_version: 2

Done.

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
File chrome/common/extensions/webview_handler.cc (right):

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
chrome/common/extensions/webview_handler.cc:42: if
(extension->manifest_version() < 2)
On 2013/09/25 01:13:54, Matt Perry wrote:
> Rather than checking this here, let's make webview require manifest v2

Done.

https://codereview.chromium.org/22793018/diff/34001/chrome/common/extensions/...
chrome/common/extensions/webview_handler.cc:50: for (unsigned int i = 0;
On 2013/09/25 01:13:54, Matt Perry wrote:
> use size_t

Done.

https://codereview.chromium.org/22793018/diff/34001/extensions/common/manifes...
File extensions/common/manifest_constants.cc (right):

https://codereview.chromium.org/22793018/diff/34001/extensions/common/manifes...
extensions/common/manifest_constants.cc:586: "Invalid value
for'webview_accessible_resources'.";
On 2013/09/25 01:13:54, Matt Perry wrote:
> webview.accessible_resources

Done.

https://codereview.chromium.org/22793018/diff/34001/extensions/common/manifes...
extensions/common/manifest_constants.cc:588: "Invalid value for
'webview_accessible_resources[*]'.";
On 2013/09/25 01:13:54, Matt Perry wrote:
> etc

Done.

[Matt Perry, 2013-09-27 16:41:09]

https://codereview.chromium.org/22793018/diff/46001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/22793018/diff/46001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:592: top_job_factory =
SetUpJobFactoryDefaults(
As discussed offline, maybe we should just call this all the time. This would
set up the extension protocol handler. Investigate that first, and if it's a
dead end, then I'm OK with this patch.

[Fady Samuel, 2013-09-27 17:28:44]

On 2013/09/27 16:41:09, Matt Perry wrote:
>
https://codereview.chromium.org/22793018/diff/46001/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/22793018/diff/46001/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_impl_io_data.cc:592: top_job_factory =
> SetUpJobFactoryDefaults(
> As discussed offline, maybe we should just call this all the time. This would
> set up the extension protocol handler. Investigate that first, and if it's a
> dead end, then I'm OK with this patch.

Done. I can't see any reason not to always install the default protocol
handlers. It seems they're all disabled elsewhere. I'll investigate further as I
refactor.

[commit-bot: I haz the power, 2013-09-27 17:31:21]

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

[commit-bot: I haz the power, 2013-09-27 17:49:31]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[awong, 2013-09-27 17:51:32]

LGTM for ProfileIOData. However, as a follow up, off this CL, I'd like to
understand better what the safeguards are for ensuring future protocol handlers
are correctly disabled (or enabled) for webview tags.

[Fady Samuel, 2013-09-27 17:55:13]

On 2013/09/27 17:51:32, awong wrote:
> LGTM for ProfileIOData. However, as a follow up, off this CL, I'd like to
> understand better what the safeguards are for ensuring future protocol
handlers
> are correctly disabled (or enabled) for webview tags.

I will study this further protocol-by-protocol next week.

[commit-bot: I haz the power, 2013-09-27 17:56:29]

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

[commit-bot: I haz the power, 2013-09-27 18:23:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-27 18:27:17]

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

[commit-bot: I haz the power, 2013-09-27 19:07:42]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-27 20:26:58]

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

[commit-bot: I haz the power, 2013-09-27 21:46:46]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-27 21:58:41]

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

[commit-bot: I haz the power, 2013-09-27 23:31:56]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-27 23:37:16]

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

[commit-bot: I haz the power, 2013-09-28 00:50:17]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-28 01:06:41]

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

[commit-bot: I haz the power, 2013-09-28 01:55:26]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-28 01:56:51]

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

[commit-bot: I haz the power, 2013-09-28 02:54:28]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-28 03:11:04]

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

[commit-bot: I haz the power, 2013-09-28 03:53:23]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-28 05:21:21]

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

[commit-bot: I haz the power, 2013-09-28 06:02:01]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-09-28 13:06:27]

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

[commit-bot: I haz the power, 2013-09-28 14:27:32]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-28 15:41:52]

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

[commit-bot: I haz the power, 2013-09-28 16:46:15]

Retried try job too often on linux_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-28 17:55:05]

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

[commit-bot: I haz the power, 2013-09-28 19:16:37]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-28 20:36:51]

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

[commit-bot: I haz the power, 2013-09-28 21:36:47]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-29 15:43:00]

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

[commit-bot: I haz the power, 2013-09-29 18:05:11]

Change committed as 225896