Fix for WebView accessible resources.

This patch allows the webview.partitions.accessible_resources entry in chrome app manifests to work fully as intended.

Previously, web pages attempting to use resources specified as webview accessible would fail to load those resources. This happened because a check in the renderer would stop the resource request from being made before the request ever reached the browser process where the webview accessible resources are checked in the manifest. This patch corrects this problem by propagating webview partition IDs to webview guest renderer processes, so that they can check the correct entry in the manifest for accessible resources and allow valid requests to go through to the browser process.

BUG=460797
Committed: https://crrev.com/ad727fc6c70d04cdd0a9a7d14c4aa68b4c0e8e72
Cr-Commit-Position: refs/heads/master@{#347936}

[lfg, 2015-08-26 16:44:38]

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

[lfg, 2015-08-26 16:44:38]

Some comments.

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/resource_request_policy.cc:73: return true;
I think this is probably wrong. This has different meaning for web accessible
resources, which were doing the checks below.

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/resource_request_policy.h (right):

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/resource_request_policy.h:8: #include <string>
Why do you need this?

[paulmeyer, 2015-08-27 17:28:48]

PTAL

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/resource_request_policy.cc:73: return true;
On 2015/08/26 16:44:38, lfg wrote:
> I think this is probably wrong. This has different meaning for web accessible
> resources, which were doing the checks below.

This should be identical to the way it was before except that I have:

1) Negated the check for IsResourceWebAccessible so that I can make the if
statement an early out (preferred in chromium) instead of a big block.
2) Added another check for IsResourceWebviewAccessible so that requests for
webview accessible resources are also let through.

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/resource_request_policy.h (right):

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/resource_request_policy.h:8: #include <string>
On 2015/08/26 16:44:38, lfg wrote:
> Why do you need this?

It was needed at some point but not anymore. Removed.

[lfg, 2015-08-27 17:31:46]

lgtm

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/1/chrome/renderer/extensions/...
chrome/renderer/extensions/resource_request_policy.cc:73: return true;
On 2015/08/27 17:28:48, Paul Meyer wrote:
> On 2015/08/26 16:44:38, lfg wrote:
> > I think this is probably wrong. This has different meaning for web
accessible
> > resources, which were doing the checks below.
> 
> This should be identical to the way it was before except that I have:
> 
> 1) Negated the check for IsResourceWebAccessible so that I can make the if
> statement an early out (preferred in chromium) instead of a big block.
> 2) Added another check for IsResourceWebviewAccessible so that requests for
> webview accessible resources are also let through.

Ah, sorry, makes sense now.

[paulmeyer, 2015-08-27 18:24:48]

The CQ bit was checked by paulmeyer@chromium.org

[commit-bot: I haz the power, 2015-08-27 18:25:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312653003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312653003/20001

[commit-bot: I haz the power, 2015-08-27 18:36:55]

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

[commit-bot: I haz the power, 2015-08-27 18:36:56]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[paulmeyer, 2015-08-27 19:19:20]

paulmeyer@chromium.org changed reviewers:
+ kalman@chromium.org, kenrb@chromium.org

[paulmeyer, 2015-08-27 19:19:21]

+kenrb@ for extension_messages.h

+kalman@ for the rest

[not at google - send to devlin, 2015-08-27 20:01:25]

https://codereview.chromium.org/1312653003/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.cc:66:
extension->GetManifestData(manifest_keys::kWebviewAccessibleResources));
Usually there would be a WebviewInfo::Get(extension) method. Could you add that?

https://codereview.chromium.org/1312653003/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.cc:67: if
(WebAccessibleResourcesInfo::IsResourceWebAccessible(
Is there a way to improve this diff? Like, go back to the old code which didn't
have the early-return. Yes it's a bit less neat (?) but it's hard to see what
you've changed.

https://codereview.chromium.org/1312653003/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1312653003/diff/20001/extensions/common/exten...
extensions/common/extension_messages.h:433: std::string /* webview_partition_id
*/)
This seems like an arbitrary IPC to add this to. It should be in its own IPC.

https://codereview.chromium.org/1312653003/diff/20001/extensions/renderer/ren...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1312653003/diff/20001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:39: void
SetWebviewPartitionID(const std::string& partition_id);
This class is a set of extensions. Knowledge about webview doesn't belong here.

[paulmeyer, 2015-08-31 15:27:10]

Patchset #3 (id:40001) has been deleted

[paulmeyer, 2015-08-31 15:31:23]

Patchset #3 (id:60001) has been deleted

[paulmeyer, 2015-08-31 15:32:55]

PTAL

https://codereview.chromium.org/1312653003/diff/20001/chrome/renderer/extensi...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.cc:66:
extension->GetManifestData(manifest_keys::kWebviewAccessibleResources));
On 2015/08/27 20:01:25, kalman wrote:
> Usually there would be a WebviewInfo::Get(extension) method. Could you add
that?

Done.

https://codereview.chromium.org/1312653003/diff/20001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.cc:67: if
(WebAccessibleResourcesInfo::IsResourceWebAccessible(
On 2015/08/27 20:01:25, kalman wrote:
> Is there a way to improve this diff? Like, go back to the old code which
didn't
> have the early-return. Yes it's a bit less neat (?) but it's hard to see what
> you've changed.

You're right, it's hard to tell. Done.

https://codereview.chromium.org/1312653003/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1312653003/diff/20001/extensions/common/exten...
extensions/common/extension_messages.h:433: std::string /* webview_partition_id
*/)
On 2015/08/27 20:01:25, kalman wrote:
> This seems like an arbitrary IPC to add this to. It should be in its own IPC.

Done.

https://codereview.chromium.org/1312653003/diff/20001/extensions/renderer/ren...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1312653003/diff/20001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:39: void
SetWebviewPartitionID(const std::string& partition_id);
On 2015/08/27 20:01:25, kalman wrote:
> This class is a set of extensions. Knowledge about webview doesn't belong
here.

Okay, I will instead store the partition ID right in ResourceRequestPolicy,
which is where it is actually used.

[not at google - send to devlin, 2015-08-31 18:31:44]

https://codereview.chromium.org/1312653003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1050:
extensions::WebViewGuest::GetPartitionID(host)));
You should only need to tell host renderers about their partition IDs, not all
renderers, so this should be unnecessary.

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.cc:30:
LAZY_INSTANCE_INITIALIZER;
It would be better to make ResourceRequestPolicy a singleton (i.e. a
LazyInstance) with a Get() method, rather than making every method static then
having a LazyInstance for everything that happens to be stateful.

SetWebViewPartitionID would be a method on ResourceRequestPolicy.

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
File chrome/renderer/extensions/resource_request_policy.h (right):

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.h:36: friend class
::ChromeExtensionsDispatcherDelegate;
Just make SetWebViewPartitionID part of the public interface.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:275:
renderer_state->GetPartitionID(process_id, &partition_id);
There is a threading mismatch here. WebViewRendererState says onto to access it
on the IO thread, but I see UI thread accessors.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:22: struct
ExtensionMsg_Loaded_Params;
Not used?

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:66: // represented by
|render_process_host|, if any. Otherwise, nullptr is
This doesn't return a nullptr, it returns an empty string.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/rend...
File extensions/browser/renderer_startup_helper.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/rend...
extensions/browser/renderer_startup_helper.cc:76: process->Send(new
ExtensionMsg_SetWebViewPartitionID(
You should only need to send it if it's actually a webview host.

and in fact, in light of the IO thread issues, could you pull this into webview
code somewhere?

https://codereview.chromium.org/1312653003/diff/80001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/common/exten...
extensions/common/extension_messages.h:806: // Report the WebView parition ID to
the WebView guest renderer process.
You've added this to the "messages sent from renderer to browser" section, which
isn't right.

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:1061:
delegate_->SetWebViewPartitionID(parition_id);
Despite my earlier comments on the renderer, you might as well just store the
webview partition ID on the Dispatcher, then have the resourcerequestpolicy read
it from there, rather than setting up the delegate structure.

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.h (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.h:58: static std::string
GetWebViewPartitionID();
unused?

[paulmeyer, 2015-08-31 20:48:10]

Patchset #4 (id:100001) has been deleted

[paulmeyer, 2015-08-31 21:40:08]

PTAL

https://codereview.chromium.org/1312653003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1050:
extensions::WebViewGuest::GetPartitionID(host)));
On 2015/08/31 18:31:44, kalman wrote:
> You should only need to tell host renderers about their partition IDs, not all
> renderers, so this should be unnecessary.

Done.

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.cc:30:
LAZY_INSTANCE_INITIALIZER;
On 2015/08/31 18:31:44, kalman wrote:
> It would be better to make ResourceRequestPolicy a singleton (i.e. a
> LazyInstance) with a Get() method, rather than making every method static then
> having a LazyInstance for everything that happens to be stateful.
> 
> SetWebViewPartitionID would be a method on ResourceRequestPolicy.

I'll leave this class alone since I won't be adding the partition ID in here
anymore.

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
File chrome/renderer/extensions/resource_request_policy.h (right):

https://codereview.chromium.org/1312653003/diff/80001/chrome/renderer/extensi...
chrome/renderer/extensions/resource_request_policy.h:36: friend class
::ChromeExtensionsDispatcherDelegate;
On 2015/08/31 18:31:44, kalman wrote:
> Just make SetWebViewPartitionID part of the public interface.

No longer necessary since I'll be taking partition ID out of here.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:275:
renderer_state->GetPartitionID(process_id, &partition_id);
On 2015/08/31 18:31:44, kalman wrote:
> There is a threading mismatch here. WebViewRendererState says onto to access
it
> on the IO thread, but I see UI thread accessors.

You may have been looking at an out-of-date version of WebViewRendererState. It
now allows access from UI or IO threads (as of last week).

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:22: struct
ExtensionMsg_Loaded_Params;
On 2015/08/31 18:31:44, kalman wrote:
> Not used?

Done.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:66: // represented by
|render_process_host|, if any. Otherwise, nullptr is
On 2015/08/31 18:31:44, kalman wrote:
> This doesn't return a nullptr, it returns an empty string.

Done.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/rend...
File extensions/browser/renderer_startup_helper.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/rend...
extensions/browser/renderer_startup_helper.cc:76: process->Send(new
ExtensionMsg_SetWebViewPartitionID(
On 2015/08/31 18:31:44, kalman wrote:
> You should only need to send it if it's actually a webview host.
> 
> and in fact, in light of the IO thread issues, could you pull this into
webview
> code somewhere?

You're right that I should put a guard on here for whether the renderer is
actually a guest, but there should be no thread issues for leaving it here.

https://codereview.chromium.org/1312653003/diff/80001/extensions/common/exten...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/common/exten...
extensions/common/extension_messages.h:806: // Report the WebView parition ID to
the WebView guest renderer process.
On 2015/08/31 18:31:44, kalman wrote:
> You've added this to the "messages sent from renderer to browser" section,
which
> isn't right.

I've moved it to the correct section. I've also made the comments about IPC
direction more obvious, similar to:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/rend...

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:1061:
delegate_->SetWebViewPartitionID(parition_id);
On 2015/08/31 18:31:44, kalman wrote:
> Despite my earlier comments on the renderer, you might as well just store the
> webview partition ID on the Dispatcher, then have the resourcerequestpolicy
read
> it from there, rather than setting up the delegate structure.

Done.

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.h (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.h:58: static std::string
GetWebViewPartitionID();
On 2015/08/31 18:31:44, kalman wrote:
> unused?

Yes. Removed.

[not at google - send to devlin, 2015-08-31 21:52:51]

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:275:
renderer_state->GetPartitionID(process_id, &partition_id);
On 2015/08/31 21:40:08, Paul Meyer wrote:
> On 2015/08/31 18:31:44, kalman wrote:
> > There is a threading mismatch here. WebViewRendererState says onto to access
> it
> > on the IO thread, but I see UI thread accessors.
> 
> You may have been looking at an out-of-date version of WebViewRendererState.
It
> now allows access from UI or IO threads (as of last week).

I'm not looking at an old version, I'm looking at git trunk:

https://chromium.googlesource.com/chromium/src/+/master/extensions/browser/gu...

That file says the equivalent of "call on IO thread" 5 times, in this case:

  // Looks up the partition info for the embedder <webview> for a given guest
  // process. Called on the IO thread.

https://codereview.chromium.org/1312653003/diff/120001/chrome/renderer/extens...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/resource_request_policy.cc:28:
ResourceRequestPolicy::ResourceRequestPolicy() {}
Just chuck DISALLOW_COPY_AND_ASSIGN on the ResourceRequestPolicy declaration in
the header. Shouldn't need to change this.

https://codereview.chromium.org/1312653003/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/resource_request_policy.cc:73: extension,
extensions::Dispatcher::GetWebViewPartitionID(),
This file is already in the extensions namespace.

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:270: const
RenderProcessHost* render_process_host) {
I actually don't even see the point of this method. The call site can just as
easily use WebViewRnedererState itself.

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/ren...
File extensions/browser/renderer_startup_helper.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/ren...
extensions/browser/renderer_startup_helper.cc:80: }
Take care where you put new code. In the original code, lines 59-75 are to do
with sending the ExtensionMsg_Loaded IPC, and this new code interrupts that. I'd
put it before the new code, and please comment it.

https://codereview.chromium.org/1312653003/diff/120001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:225: LAZY_INSTANCE_INITIALIZER;
This can be a member of Dispatcher, and note that when you do, the accessor
would be webview_partition_id()

[paulmeyer, 2015-09-02 13:43:57]

kalman@: PTAL.

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:275:
renderer_state->GetPartitionID(process_id, &partition_id);
On 2015/08/31 21:52:51, kalman wrote:
> On 2015/08/31 21:40:08, Paul Meyer wrote:
> > On 2015/08/31 18:31:44, kalman wrote:
> > > There is a threading mismatch here. WebViewRendererState says onto to
access
> > it
> > > on the IO thread, but I see UI thread accessors.
> > 
> > You may have been looking at an out-of-date version of WebViewRendererState.
> It
> > now allows access from UI or IO threads (as of last week).
> 
> I'm not looking at an old version, I'm looking at git trunk:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/extensions/browser/gu...
> 
> That file says the equivalent of "call on IO thread" 5 times, in this case:
> 
>   // Looks up the partition info for the embedder <webview> for a given guest
>   // process. Called on the IO thread.

Oh, I see. Those comments are stale, so I'll remove them.

https://codereview.chromium.org/1312653003/diff/120001/chrome/renderer/extens...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/resource_request_policy.cc:28:
ResourceRequestPolicy::ResourceRequestPolicy() {}
On 2015/08/31 21:52:51, kalman wrote:
> Just chuck DISALLOW_COPY_AND_ASSIGN on the ResourceRequestPolicy declaration
in
> the header. Shouldn't need to change this.

Added the macro, but I still need this constructor now that I am giving it a
pointer to the extensions dispatcher.

https://codereview.chromium.org/1312653003/diff/120001/chrome/renderer/extens...
chrome/renderer/extensions/resource_request_policy.cc:73: extension,
extensions::Dispatcher::GetWebViewPartitionID(),
On 2015/08/31 21:52:51, kalman wrote:
> This file is already in the extensions namespace.

You're right. The other uses of "extensions::" threw me off. I'll remove them as
well.

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:270: const
RenderProcessHost* render_process_host) {
On 2015/08/31 21:52:51, kalman wrote:
> I actually don't even see the point of this method. The call site can just as
> easily use WebViewRnedererState itself.

It could, but I thought that it would be neater if all of this WebView-specific
code was hidden in WebViewGuest. It could also be used in other locations in the
future. If you don't think that is necessary, then I can move it out.

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/ren...
File extensions/browser/renderer_startup_helper.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/extensions/browser/ren...
extensions/browser/renderer_startup_helper.cc:80: }
On 2015/08/31 21:52:51, kalman wrote:
> Take care where you put new code. In the original code, lines 59-75 are to do
> with sending the ExtensionMsg_Loaded IPC, and this new code interrupts that.
I'd
> put it before the new code, and please comment it.

Done.

https://codereview.chromium.org/1312653003/diff/120001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1312653003/diff/120001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:225: LAZY_INSTANCE_INITIALIZER;
On 2015/08/31 21:52:51, kalman wrote:
> This can be a member of Dispatcher, and note that when you do, the accessor
> would be webview_partition_id()

Done.

[kenrb, 2015-09-02 14:43:02]

ipc lgtm

[not at google - send to devlin, 2015-09-03 20:17:54]

Just nits left for the code, so that lg.

But this is missing a test.

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1293:
!(resource_request_policy_ &&
I don't think it's possible for this to be null?

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/extens...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/extens...
chrome/renderer/extensions/resource_request_policy.cc:71:
webview_info->IsResourceWebviewAccessible(
See point about WebviewInfo::IsResourceWebviewAccessible(...)

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:62: using
content::RenderProcessHost;
This is content::RenderProcessHost everywhere in this file. I wouldn't bother
with the "using", but if you really want, at least replace every occurrence of
content::RenderProcessHost with RenderProcessHost in this file.

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_renderer_state.h (left):

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_renderer_state.h:28: // All
methods should be called on the IO thread.
You might actually want to keep a comment, just have it "Keeps track of
<webview> renderer state. Can be used on any thread."

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_renderer_state.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_renderer_state.h:43: // Looks up
the information for the embedder <webview> for a given render
I don't like the way these comments (for all functions) match the
implementations. It should be along the lines of:

"Looks up the information for the embedder <webview> for a RenderViewHost given
its process and view ID. Returns true and writes the information to
|web_view_info| if found, otherwise returns false."

it's a mystery why this doesn't just return a scoped_ptr<WebViewInfo> but
whatever.

it should also take a RenderFrameHost* rather than this ID pair, but one step at
a time.

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/exte...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/exte...
extensions/common/extension_messages.h:390: //
-----------------------------------------------------------------------------
Please don't add these --------------- comments, but it probably should be
"renderer:" not "renderer."

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/mani...
File extensions/common/manifest_handlers/webview_info.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/mani...
extensions/common/manifest_handlers/webview_info.h:32: bool
IsResourceWebviewAccessible(const Extension* extension,
you know, it's actually silly that this isn't static and that callers can't just
do WebWebInfo::IsResourceWebviewAccessible(extension, ...) because needing to
grab the WebviewInfo object from an extension, then turning around and passing
it back in, is odd.

so I'd do that rather than adding a Get() method, after all that.

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:1061: DCHECK(webview_partition_id_.empty() ||
Make this a CHECK, then you don't need the extra "if
(webview_partition_id_.empty())" guard. just

CHECK(webview_partition_id_.empty() || webview_partition_id_ == id);
webview_partition_id_ = id;

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.h:86: std::string webview_partition_id() { return
webview_partition_id_; }
return a const std::string&

[paulmeyer, 2015-09-08 18:51:02]

Patchset #6 (id:160001) has been deleted

[paulmeyer, 2015-09-08 18:51:36]

kalman@: PTAL

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1293:
!(resource_request_policy_ &&
On 2015/09/03 20:17:53, kalman wrote:
> I don't think it's possible for this to be null?

I didn't think so either, but I thought it would be safer to use this check
anyways in case someone changes something in the future that makes it possible
to be null. I'll remove it though.

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/extens...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/extens...
chrome/renderer/extensions/resource_request_policy.cc:71:
webview_info->IsResourceWebviewAccessible(
On 2015/09/03 20:17:54, kalman wrote:
> See point about WebviewInfo::IsResourceWebviewAccessible(...)

Done.

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:62: using
content::RenderProcessHost;
On 2015/09/03 20:17:54, kalman wrote:
> This is content::RenderProcessHost everywhere in this file. I wouldn't bother
> with the "using", but if you really want, at least replace every occurrence of
> content::RenderProcessHost with RenderProcessHost in this file.

Done.

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_renderer_state.h (left):

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_renderer_state.h:28: // All
methods should be called on the IO thread.
On 2015/09/03 20:17:54, kalman wrote:
> You might actually want to keep a comment, just have it "Keeps track of
> <webview> renderer state. Can be used on any thread."

I removed this comment because the same info is already at the top of the file.

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_renderer_state.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_renderer_state.h:43: // Looks up
the information for the embedder <webview> for a given render
On 2015/09/03 20:17:54, kalman wrote:
> I don't like the way these comments (for all functions) match the
> implementations. It should be along the lines of:
> 
> "Looks up the information for the embedder <webview> for a RenderViewHost
given
> its process and view ID. Returns true and writes the information to
> |web_view_info| if found, otherwise returns false."
> 
> it's a mystery why this doesn't just return a scoped_ptr<WebViewInfo> but
> whatever.
> 
> it should also take a RenderFrameHost* rather than this ID pair, but one step
at
> a time.

Updated the comments.

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/exte...
File extensions/common/extension_messages.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/exte...
extensions/common/extension_messages.h:390: //
-----------------------------------------------------------------------------
On 2015/09/03 20:17:54, kalman wrote:
> Please don't add these --------------- comments, but it probably should be
> "renderer:" not "renderer."

Done.

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/mani...
File extensions/common/manifest_handlers/webview_info.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/mani...
extensions/common/manifest_handlers/webview_info.h:32: bool
IsResourceWebviewAccessible(const Extension* extension,
On 2015/09/03 20:17:54, kalman wrote:
> you know, it's actually silly that this isn't static and that callers can't
just
> do WebWebInfo::IsResourceWebviewAccessible(extension, ...) because needing to
> grab the WebviewInfo object from an extension, then turning around and passing
> it back in, is odd.
> 
> so I'd do that rather than adding a Get() method, after all that.

I'll make it static, but I can keep the Get() function anyways and use it in the
static IsResourceWebviewAccessible() function.

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.cc:1061: DCHECK(webview_partition_id_.empty() ||
On 2015/09/03 20:17:54, kalman wrote:
> Make this a CHECK, then you don't need the extra "if
> (webview_partition_id_.empty())" guard. just
> 
> CHECK(webview_partition_id_.empty() || webview_partition_id_ == id);
> webview_partition_id_ = id;

Done.

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/renderer/di...
extensions/renderer/dispatcher.h:86: std::string webview_partition_id() { return
webview_partition_id_; }
On 2015/09/03 20:17:54, kalman wrote:
> return a const std::string&

Done.

[not at google - send to devlin, 2015-09-08 20:10:24]

lgtm

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1312653003/diff/140001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1293:
!(resource_request_policy_ &&
On 2015/09/08 18:51:36, Paul Meyer wrote:
> On 2015/09/03 20:17:53, kalman wrote:
> > I don't think it's possible for this to be null?
> 
> I didn't think so either, but I thought it would be safer to use this check
> anyways in case someone changes something in the future that makes it possible
> to be null. I'll remove it though.

If somebody changes it to be null, they can deal with it (... and the crash it
will cause). By default nothing should be null.

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/mani...
File extensions/common/manifest_handlers/webview_info.h (right):

https://codereview.chromium.org/1312653003/diff/140001/extensions/common/mani...
extensions/common/manifest_handlers/webview_info.h:32: bool
IsResourceWebviewAccessible(const Extension* extension,
On 2015/09/08 18:51:36, Paul Meyer wrote:
> On 2015/09/03 20:17:54, kalman wrote:
> > you know, it's actually silly that this isn't static and that callers can't
> just
> > do WebWebInfo::IsResourceWebviewAccessible(extension, ...) because needing
to
> > grab the WebviewInfo object from an extension, then turning around and
passing
> > it back in, is odd.
> > 
> > so I'd do that rather than adding a Get() method, after all that.
> 
> I'll make it static, but I can keep the Get() function anyways and use it in
the
> static IsResourceWebviewAccessible() function.

If the only caller is IsResourceWebviewAccessible then there's no point having
Get. But if it isn't, then ok.

[paulmeyer, 2015-09-08 22:21:48]

paulmeyer@chromium.org changed reviewers:
+ sky@chromium.org

[paulmeyer, 2015-09-08 22:21:48]

+sky@ for OWNER review of plugin_info_message_filter.cc and
chrome_content_renderer_client.h/cc

[sky, 2015-09-08 23:46:52]

LGTM

[paulmeyer, 2015-09-09 00:00:10]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-09-09 00:00:11]

The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org,
kenrb@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1312653003/#ps200001
(title: "Removed WebviewInfo::Get().")

[commit-bot: I haz the power, 2015-09-09 00:00:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312653003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312653003/200001

[commit-bot: I haz the power, 2015-09-09 00:17:51]

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

[commit-bot: I haz the power, 2015-09-09 00:17:52]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[paulmeyer, 2015-09-09 14:00:57]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-09-09 14:00:57]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
lfg@chromium.org, sky@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1312653003/#ps220001
(title: "Small fix. Rebased.")

[commit-bot: I haz the power, 2015-09-09 14:01:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312653003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312653003/220001

[commit-bot: I haz the power, 2015-09-09 15:30:06]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2015-09-09 15:30:46]

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