<webview>: Resolve relative paths as chrome-extension: URLs

<webview>: Resolve relative paths as chrome-extension: URLs

The CL performs URL resolution in the browser process out of convenience. If this was done in the embedder process in web_view.js then URL resolution would need to be done on both attribute mutation and initialization. BrowserPluginGuest::OnNavigateGuest gets called either way.

When updating the src attribute on loadcommit, we can no longer do a simple string comparison, as the resolved URL may not match the developer-assigned URL.

BUG=308114
Test=<webview src='guest.html'></webview> will attempt to resolve to
chrome-extension://appID/guest.html.

R=creis@chromium.org

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

[Charlie Reis, 2013-10-18 17:37:54]

https://codereview.chromium.org/27408004/diff/5001/chrome/browser/guestview/w...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/27408004/diff/5001/chrome/browser/guestview/w...
chrome/browser/guestview/webview/webview_guest.cc:561:
embedder_site_url.host().c_str()));
What will this do in Hui's case, when we have a webview embedded in a WebUI
process?

https://codereview.chromium.org/27408004/diff/5001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/27408004/diff/5001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:324: // foobar
is a privileged partition according to the manifest file.
Hmm, I'm still uncomfortable with these being called privileged partitions. 
Let's discuss that separately.

https://codereview.chromium.org/27408004/diff/5001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/27408004/diff/5001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:372: if (!opener()) {
How do we know this is true?  Couldn't we have an embedder with no opener that
hasn't attached yet?

https://codereview.chromium.org/27408004/diff/5001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:377: return
opener()->embedder_web_contents()->GetSiteInstance();
This seems confusing to me.  There's no guarantee the opener is in the same
SiteInstance in general.  It may be true for V2 apps, but it seems like we're
starting to support other use cases.

[Fady Samuel, 2013-10-25 21:10:31]

This CL was on hold as I was distracted with the declarative WebRequest API.
I've addressed your comments now. PTAL.

https://codereview.chromium.org/27408004/diff/5001/chrome/browser/guestview/w...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/27408004/diff/5001/chrome/browser/guestview/w...
chrome/browser/guestview/webview/webview_guest.cc:561:
embedder_site_url.host().c_str()));
On 2013/10/18 17:37:55, creis wrote:
> What will this do in Hui's case, when we have a webview embedded in a WebUI
> process?

I now base this on the extension_id which is always the correct value.

In the case of window.open, WebViewGuest gets an extension ID from its opener.

In the case of a webview initial navigation, WebViewGuest::Attach is always
called immediately after creation, and so WebViewGuest::ResolveURL will always
have a non-empty extension_id(). I added a DCHECK with this assertion.

https://codereview.chromium.org/27408004/diff/5001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/27408004/diff/5001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:324: // foobar
is a privileged partition according to the manifest file.
On 2013/10/18 17:37:55, creis wrote:
> Hmm, I'm still uncomfortable with these being called privileged partitions. 
> Let's discuss that separately.

Agreed. I'm supportive of changing the name.

https://codereview.chromium.org/27408004/diff/5001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/27408004/diff/5001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:372: if (!opener()) {
On 2013/10/18 17:37:55, creis wrote:
> How do we know this is true?  Couldn't we have an embedder with no opener that
> hasn't attached yet?

I've removed this method. It's no longer necessary.

https://codereview.chromium.org/27408004/diff/5001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_guest.cc:377: return
opener()->embedder_web_contents()->GetSiteInstance();
On 2013/10/18 17:37:55, creis wrote:
> This seems confusing to me.  There's no guarantee the opener is in the same
> SiteInstance in general.  It may be true for V2 apps, but it seems like we're
> starting to support other use cases.

Removed.

[Charlie Reis, 2013-10-29 21:11:38]

Thanks.  The lifetime of extension_id is a bit awkward to understand, but I
don't think it creates any problems.  Seems like it will work.

https://codereview.chromium.org/27408004/diff/5001/chrome/browser/guestview/w...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/27408004/diff/5001/chrome/browser/guestview/w...
chrome/browser/guestview/webview/webview_guest.cc:561:
embedder_site_url.host().c_str()));
On 2013/10/25 21:10:31, Fady Samuel wrote:
> On 2013/10/18 17:37:55, creis wrote:
> > What will this do in Hui's case, when we have a webview embedded in a WebUI
> > process?
> 
> I now base this on the extension_id which is always the correct value.
> 

Thanks.


> In the case of window.open, WebViewGuest gets an extension ID from its opener.
> 
> In the case of a webview initial navigation, WebViewGuest::Attach is always
> called immediately after creation, and so WebViewGuest::ResolveURL will always
> have a non-empty extension_id(). I added a DCHECK with this assertion.

Please put a comment like this on the declaration of extension_id_ in GuestView,
so that the lifetime is a bit more clear.

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
File chrome/browser/guestview/guestview.cc (right):

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
chrome/browser/guestview/guestview.cc:103: extension_id_ = extension_id;
Should we have a check that extension_id_ is either empty or equal to
extension_id?  Seems like it could be a problem if we set it to a different ID
than it was created with.

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
chrome/browser/guestview/guestview.h:45: static GuestView*
Create(content::WebContents* guest_web_contents,
This should have a comment explaining both extension_id (and what it means for
it to be empty) and view_type.  For example, explain that an empty extension ID
will be set to a real value in Attach.

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
chrome/browser/guestview/webview/webview_guest.cc:557:
DCHECK(!extension_id().empty());
nit: Just put a NOTREACHED() inside the if statement so we don't have to query
empty() twice.

https://codereview.chromium.org/27408004/diff/69001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/27408004/diff/69001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:37: #include
"content/public/browser/site_instance.h"
Is this needed?

[Fady Samuel, 2013-10-30 19:48:51]

This has been significantly simplified. PTAL.

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
File chrome/browser/guestview/guestview.cc (right):

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
chrome/browser/guestview/guestview.cc:103: extension_id_ = extension_id;
On 2013/10/29 21:11:39, creis wrote:
> Should we have a check that extension_id_ is either empty or equal to
> extension_id?  Seems like it could be a problem if we set it to a different ID
> than it was created with.

This is no longer necessary.

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
chrome/browser/guestview/guestview.h:45: static GuestView*
Create(content::WebContents* guest_web_contents,
On 2013/10/29 21:11:39, creis wrote:
> This should have a comment explaining both extension_id (and what it means for
> it to be empty) and view_type.  For example, explain that an empty extension
ID
> will be set to a real value in Attach.

I've refactored things a bit to make it so that extension_id is always passed
in. In fact, this reduces code complexity quite a bit.

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://codereview.chromium.org/27408004/diff/69001/chrome/browser/guestview/...
chrome/browser/guestview/webview/webview_guest.cc:557:
DCHECK(!extension_id().empty());
On 2013/10/29 21:11:39, creis wrote:
> nit: Just put a NOTREACHED() inside the if statement so we don't have to query
> empty() twice.

Done.

https://codereview.chromium.org/27408004/diff/69001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/27408004/diff/69001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:37: #include
"content/public/browser/site_instance.h"
On 2013/10/29 21:11:39, creis wrote:
> Is this needed?

Removed.

[Charlie Reis, 2013-10-30 20:42:20]

This is potentially much better, but I don't understand it.  Some of the
questions below should hopefully be simple explanations that I'm missing.

It also looks like you're trying to do two things here-- change the
infrastructure for how we support the guest-in-WebUI case and add support for
relative URLs.  Can we do those into separate CLs in case one needs to be
reverted?

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:872: // iframe's URL to
determine the extension.
I haven't been able to figure out why it's safe to remove this.  Looks like your
CL gets the extension ID from the SiteInstance instead, but how does it get
there in the WebUI case?

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:826: SiteInstance*
guest_site_instance,
Is this equal to guest_web_contents->GetSiteInstance()?

If so, we don't need to pass it in explicitly, since we can get it from
guest_web_contents.

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:836: const std::string&
extension_id = guest_site_url.host();
I've lost track-- are we guaranteeing that the guest's SiteInstance has an
extension host, even in the WebUI and unprivileged (for zork@) cases?

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:104: const std::string extension_id_;
I definitely like that we know the extension ID on creation in this approach.

https://codereview.chromium.org/27408004/diff/179001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_embedder.cc (left):

https://codereview.chromium.org/27408004/diff/179001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.cc:183: GURL
validated_frame_url(params.embedder_frame_url);
Are we using embedder_frame_url somewhere else?

https://codereview.chromium.org/27408004/diff/179001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/27408004/diff/179001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:96: SiteInstance*
site_instance,
It's not clear which SiteInstance this is expecting to receive.  Can you add a
comment?

https://codereview.chromium.org/27408004/diff/179001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/27408004/diff/179001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:455: guest_instance_id,
site_instance, new_contents, extra_params.Pass());
Which SiteInstance is this?  How does it get the extension ID in the WebUI case?

[Fady Samuel, 2013-10-31 17:25:17]

PTAL. This CL is much simpler and more obvious now.

The comments  below are for reference but are largely irrelevant now because the
refactor happened in a separate CL.

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (left):

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:872: // iframe's URL to
determine the extension.
On 2013/10/30 20:42:20, creis wrote:
> I haven't been able to figure out why it's safe to remove this.  Looks like
your
> CL gets the extension ID from the SiteInstance instead, but how does it get
> there in the WebUI case?

See BrowserPluginGuestManager::CreateGuest. This is essentially code
duplication. We've already determined the guest_site_instance and made the
appropriate extension ID the host of the guest site instance. There's no reason
to determine the extension ID again here when we can simply extract it from the
guest site instance.

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:826: SiteInstance*
guest_site_instance,
On 2013/10/30 20:42:20, creis wrote:
> Is this equal to guest_web_contents->GetSiteInstance()?
> 
> If so, we don't need to pass it in explicitly, since we can get it from
> guest_web_contents.

Yes, but GuestWebContentsCreated gets called prior to WebContentsImpl::Init and
so guest_web_contents->GetSiteInstance() is not yet available. This is necessary
because we need to install a GuestView prior to any kind of navigation so that
we don't miss events.

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:836: const std::string&
extension_id = guest_site_url.host();
On 2013/10/30 20:42:20, creis wrote:
> I've lost track-- are we guaranteeing that the guest's SiteInstance has an
> extension host, even in the WebUI and unprivileged (for zork@) cases?

For WebUI, this will be extracted from the iframe's URL in
BrowserPluginGuestManager::CreateGuest.

For unprivileged, we simply don't need an extension ID. Extension bindings will
not operate on BrowserPlugin. Neither WebViewGuest nor AdViewGuest will be
installed.

I expect a QuickOfficeGuest will be installed and it simply won't care about
extension IDs. We can add a check here to validate whether the host is a valid
extension ID and if not it can clear the string. I can do that now or in a
separate CL.

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/guestview...
File chrome/browser/guestview/guestview.h (right):

https://codereview.chromium.org/27408004/diff/179001/chrome/browser/guestview...
chrome/browser/guestview/guestview.h:104: const std::string extension_id_;
On 2013/10/30 20:42:20, creis wrote:
> I definitely like that we know the extension ID on creation in this approach.

:-)

https://codereview.chromium.org/27408004/diff/179001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_embedder.cc (left):

https://codereview.chromium.org/27408004/diff/179001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_embedder.cc:183: GURL
validated_frame_url(params.embedder_frame_url);
On 2013/10/30 20:42:20, creis wrote:
> Are we using embedder_frame_url somewhere else?

See BrowserPluginGuestManager::CreateGuest. The guest site instance is
constructed from the embedder's frame URL if the embedder is WebUI.

[Charlie Reis, 2013-10-31 17:35:25]

LGTM.  (Do we not need ResolveURL in AdView?)

[Fady Samuel, 2013-10-31 18:31:07]

On 2013/10/31 17:35:25, creis wrote:
> LGTM.  (Do we not need ResolveURL in AdView?)

We may at some point in the future. <adview> has not shipped and its
requirements are not yet clear. I'll leave adding <adview> URL resolution for a
future CL if the need arises.

[commit-bot: I haz the power, 2013-10-31 18:32:19]

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

[Fady Samuel, 2013-10-31 20:51:38]

Committed patchset #7 manually as r232203 (presubmit successful).