Fix the Declarative WebRequest API for <webview>

Fix the Declarative WebRequest API for <webview>

The SendMessageToExtension action was not working because <webview> originally cast it as a WebRequestEvent which it is not. This CL fires a filtered webview.onMessage event if the action is being operated upon a webview guest.

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

[Fady Samuel, 2014-05-27 22:52:12]

Hi Dominic,

I still need to write a test for this but I thought I'd send it to you to get
your thoughts on the web_request_api change. Does it look reasonable to you? We
basically send a different message event for webview guests. This event is
targeted at a particular webview.

Thanks,

Fady

[battre, 2014-05-28 13:24:45]

I think I currently miss some background. I have never heard of webviews. Do you
have any documentation that you can point me to?

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/web_request/web_request_api.cc:178:
ExtensionRendererState::WebViewInfo* webview_info) {
I think this function should be called Get[something] to indicate that it
modifies webview_info. Also some documentation would be useful.

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/web_request/web_request_api.cc:388: //
|event_argument| is passed to the event listener.
I guess this needs to be updated.

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/web_request/web_request_api.cc:392: int is_guest,
What does |is_guest| mean?

Would you mind adding some documentation to c/b/guest_view as well. E.g. to
guest_view.h. It would be tremendously helpful as an entry point for somebody
who does not have the slightest clue what a guest view is. ;-)

[Fady Samuel, 2014-05-28 14:18:23]

PTAL

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/web_request/web_request_api.cc:178:
ExtensionRendererState::WebViewInfo* webview_info) {
On 2014/05/28 13:24:45, battre wrote:
> I think this function should be called Get[something] to indicate that it
> modifies webview_info. Also some documentation would be useful.

Done.

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/web_request/web_request_api.cc:388: //
|event_argument| is passed to the event listener.
On 2014/05/28 13:24:45, battre wrote:
> I guess this needs to be updated.

Done.

https://codereview.chromium.org/307543005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/web_request/web_request_api.cc:392: int is_guest,
On 2014/05/28 13:24:45, battre wrote:
> What does |is_guest| mean?
> 
> Would you mind adding some documentation to c/b/guest_view as well. E.g. to
> guest_view.h. It would be tremendously helpful as an entry point for somebody
> who does not have the slightest clue what a guest view is. ;-)

Done.

[battre, 2014-05-28 15:15:21]

chrome/browser/extensions/api/web_request/web_request_api.cc LGTM after
addressing the remaining issues.

For the web_view.js changes, I'd appreciate a review by a member of the
extensions team. This code has changed quite a lot since I last looked at it.

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:401: int
is_web_view_guest,
bool?

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:415:
extensions::EventFilteringInfo event_filtering_info;
More stuff that could use some documentation. ;-)

No need to do it here, but I think that at least service_type_ and instance_id_
in EventfilteringInfo could use some comments.

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:416:
event_filtering_info.SetURL(GURL());
This looks crude... You set a URL such that EventFilteringInfo::has_url_ becomes
true but then url().is_valid() returns false? I think this is semantics that
should be described in EventFilteringInfo.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (left):

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1224:
WebViewInternal.prototype.maybeAttachWebRequestEventToObject = function() {};
I have no idea about this code. It would be great to get a reviewer who is
familiar with this.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:995: // TODO(lazyboy): When do
we dispose this listener?
Isn't this taken care of by
ExtensionWebRequestEventRouter::RemoveWebViewEventListeners?

If that's the case, please add a comment.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:999: }, {instanceId:
opt_webViewInstanceId || 0});
I am lacking the details here, but do you allow one webview to listen for
request messages of another webview?

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1029: // The onMessage event
gets a special event type.
Document why?

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1030: var EventObj =
webRequestEvent.name === 'onMessage' ?
EventClass?

[Fady Samuel, 2014-05-29 15:13:27]

+lazyboy@: Istiaque could you please take a look? Thanks!

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:401: int
is_web_view_guest,
On 2014/05/28 15:15:21, battre wrote:
> bool?

Done.

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:415:
extensions::EventFilteringInfo event_filtering_info;
On 2014/05/28 15:15:21, battre wrote:
> More stuff that could use some documentation. ;-)
> 
> No need to do it here, but I think that at least service_type_ and
instance_id_
> in EventfilteringInfo could use some comments.

I have no clue what service_type_ is but instance_id belongs to guest_view. I
added a comment.

https://codereview.chromium.org/307543005/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_api.cc:416:
event_filtering_info.SetURL(GURL());
On 2014/05/28 15:15:21, battre wrote:
> This looks crude... You set a URL such that EventFilteringInfo::has_url_
becomes
> true but then url().is_valid() returns false? I think this is semantics that
> should be described in EventFilteringInfo.

This is actually unnecessary. I've removed it.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (left):

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1224:
WebViewInternal.prototype.maybeAttachWebRequestEventToObject = function() {};
On 2014/05/28 15:15:21, battre wrote:
> I have no idea about this code. It would be great to get a reviewer who is
> familiar with this.

Yes, will do.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:995: // TODO(lazyboy): When do
we dispose this listener?
On 2014/05/28 15:15:21, battre wrote:
> Isn't this taken care of by
> ExtensionWebRequestEventRouter::RemoveWebViewEventListeners?
> 
> If that's the case, please add a comment.

No. This is a very crude way to do cleanup. These events are cleaned up when he
embedder goes away, not when the webview goes away. A webview's lifetime may be
significantly shorter than an embedder's lifetime and so we might not want a
bunch of event listeners hanging around. I don't have a good solution for this
yet but we're working on it.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:999: }, {instanceId:
opt_webViewInstanceId || 0});
On 2014/05/28 15:15:21, battre wrote:
> I am lacking the details here, but do you allow one webview to listen for
> request messages of another webview?

No. Hence the filter by instance ID. Each webview has a unique instance ID. We
don't want the developer to worry about that detail though (it's an
implementation detail).

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1029: // The onMessage event
gets a special event type.
On 2014/05/28 15:15:21, battre wrote:
> Document why?

Done.

https://codereview.chromium.org/307543005/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1030: var EventObj =
webRequestEvent.name === 'onMessage' ?
On 2014/05/28 15:15:21, battre wrote:
> EventClass?

Done.

[lazyboy, 2014-05-29 17:50:13]

https://chromiumcodereview.appspot.com/307543005/diff/60002/chrome/browser/ex...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://chromiumcodereview.appspot.com/307543005/diff/60002/chrome/browser/ex...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:495:
process_id, route_id, &webview_info)) {
Indent 4 more spaces.

https://chromiumcodereview.appspot.com/307543005/diff/60002/chrome/renderer/r...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/307543005/diff/60002/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view.js:1049: request,
Doesn't this need to go to web_view_experimental?

[Fady Samuel, 2014-05-29 18:44:45]

PTAL

https://codereview.chromium.org/307543005/diff/60002/chrome/browser/extension...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://codereview.chromium.org/307543005/diff/60002/chrome/browser/extension...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:495:
process_id, route_id, &webview_info)) {
On 2014/05/29 17:50:14, lazyboy wrote:
> Indent 4 more spaces.

Done.

https://codereview.chromium.org/307543005/diff/60002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/307543005/diff/60002/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1049: request,
On 2014/05/29 17:50:14, lazyboy wrote:
> Doesn't this need to go to web_view_experimental?

This will be used by products that will presumably need to work on stable so I'm
moving this to stable.

[lazyboy, 2014-05-29 19:01:39]

lgtm

https://codereview.chromium.org/307543005/diff/60002/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/307543005/diff/60002/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view.js:1049: request,
On 2014/05/29 18:44:45, Fady Samuel wrote:
> On 2014/05/29 17:50:14, lazyboy wrote:
> > Doesn't this need to go to web_view_experimental?
> 
> This will be used by products that will presumably need to work on stable so
I'm
> moving this to stable.

Ok.

[fsamuel, 2014-05-29 19:04:52]

The CQ bit was checked by fsamuel@google.com

[commit-bot: I haz the power, 2014-05-29 19:07:04]

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

[Fady Samuel, 2014-05-29 21:03:46]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-05-29 21:05:35]

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

[commit-bot: I haz the power, 2014-05-30 04:00:32]

Change committed as 273715