<webview>: First stab at implementing media permission request for guests.

<webview>: First stab at implementing media permission request for guests.

We allow/deny the media permission on embedder's WebContents when a guest's WebContents requests for the permission.

The embedder js api looks like:
1.
<webview id="webview1" src=...></webview>
webview1.addEventListener('permissionrequest', function(e) {
  if (e.permission == 'media') e.request.allow();  // Or e.request.deny();
  // if no listener call allow/deny, the request is auto denied.
});

2. If embedder wants to defer on deciding, they must call preventDefault();
<webview ...>
webview.addEventListener('permissionrequest', function(e) {
  e.preventDefault();
  setTimeout(function() { e.request.allow(); }, 10000);
});

3. If there are no listeners attached, request is auto denied.

4. If there are multiple listeners attached and each are trying to call allow/deny, we implement 'first call wins', subsequent calls to allow/deny will throw exception.
<webview ...>
webview.addEventListener('permissionrequest', function(e) {
  // assume this listener fires first.
  e.request.allow();
});
webview.addEventListener('permissionrequest', function(e) {
  // Assume this listener fires later.
  // Calling e.request.allow() or e.request.deny() will throw exception.
});

5. Deferring on calling allow/deny can be done indefinitely and once the event.request object goes out of scope, we auto deny if decision has not yet been made:
<webview ...>
webview.addEventListener('permissionrequest', function(e) {
  e.preventDefault();
  setTimeout(function() {
    if (e.type == 'something') do_something_else; // so 'e.request' is still live.
  }, 2000); // After 2000ms, e/e.request goes out of scope == auto deny on next gc.
});


When the guest requests a permission (BrowserPluginGuest.RequestMediaAccessPermission()), we send the info to the renderer BrowserPlugin, which runs the embedder js api function as above.


BUG=141197, 153540
TEST=Added browser_tests: WebViewTest.MediaAccess*


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

[lazyboy, 2012-10-11 21:21:01]

(Adding Fady for taking initial look)

[Fady Samuel, 2012-10-11 21:29:28]

From an overall perspective, this is really cool!

What happens in Chrome Apps though?

[Fady Samuel, 2012-10-11 21:41:00]

http://codereview.chromium.org/11093080/diff/5001/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11093080/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:77: if (instance_id != -1 &&
request_id != -1) {
I feel like these should be DCHECKs rather than if?

[lazyboy, 2012-10-11 23:05:07]

Adding creis@ for review.

https://chromiumcodereview.appspot.com/11093080/diff/5001/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/5001/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:77: if (instance_id != -1 &&
request_id != -1) {
On 2012/10/11 21:41:00, Fady Samuel wrote:
> I feel like these should be DCHECKs rather than if?

Done.

[Charlie Reis, 2012-10-17 06:18:45]

Sorry for the long delay here, but things got crazy right around the time of my
vacation day on Friday.  Very nice start.

I like having a detailed CL description, but you can probably trim that down
quite a bit.  One thing to note: I think it's worth it to continue firing the
event to the embedder even if the app doesn't have the permission.  This will
let the app put up some kind of message to the user that such and such
permission is not supported, if it wants to.  (CL description nit: "requests a
permission," not "requests for a permission")

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:110: // Allows or
denies browser plugin's media access request.
nit: s/browser plugin's/a guest's/

Also add "after the embedder has decided."

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:111: void
AllowMediaAccess(int instance_id, int request_id, bool allow);
nit: should_allow

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:139: static int
request_count = 0;
Do we need globally unique request counts for the browser process?  Your comment
says it only needs to be unique per guest.  Maybe this should just be a member
variable.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:187: WebContentsImpl*
embedder_web_contents_impl = static_cast<
nit: Push the cast to the next line.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:189: // Re-route the
request to the embedder's WebContents, the guest gets the
nit: semicolon

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:174: // Allows or denies
media access request.
nit: Allows or denies a media access request, after the embedder has had a
chance to decide.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:177: bool allow);
nit: should_allow

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/common/bro...
content/common/browser_plugin_messages.h:182: // When the guest requests for
media access, the browser process forwards this
nit: requests media access

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:58: const char
kPermissionRequestEventName[] = "permissionRequest";
nit: permission < persist

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:519: // TODO(lazyboy):
Automatically deny request when there is no listener.
Can we go ahead and do this?

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:73: // Requests the embedder
(browser process) for media access permission.
nit: Requests media access permission from the embedder.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:75: // Informs the
BrowserPlugin that the guest's request to media access has been
Heh, in this case you do want "for" rather than "to".

[lazyboy, 2012-10-17 09:09:53]

I've trimmed the CL description a bit.
I will remove the long TEST=description after I've finished adding tests, I will
be adding them next.
Thanks.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:110: // Allows or
denies browser plugin's media access request.
On 2012/10/17 06:18:45, creis wrote:
> nit: s/browser plugin's/a guest's/
> 
> Also add "after the embedder has decided."

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.h:111: void
AllowMediaAccess(int instance_id, int request_id, bool allow);
On 2012/10/17 06:18:45, creis wrote:
> nit: should_allow

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:139: static int
request_count = 0;
On 2012/10/17 06:18:45, creis wrote:
> Do we need globally unique request counts for the browser process?  Your
comment
> says it only needs to be unique per guest.  Maybe this should just be a member
> variable.

Right, made it member var instead.
Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:187: WebContentsImpl*
embedder_web_contents_impl = static_cast<
On 2012/10/17 06:18:45, creis wrote:
> nit: Push the cast to the next line.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:189: // Re-route the
request to the embedder's WebContents, the guest gets the
On 2012/10/17 06:18:45, creis wrote:
> nit: semicolon

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:174: // Allows or denies
media access request.
On 2012/10/17 06:18:45, creis wrote:
> nit: Allows or denies a media access request, after the embedder has had a
> chance to decide.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:177: bool allow);
On 2012/10/17 06:18:45, creis wrote:
> nit: should_allow

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/common/bro...
content/common/browser_plugin_messages.h:182: // When the guest requests for
media access, the browser process forwards this
On 2012/10/17 06:18:45, creis wrote:
> nit: requests media access

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:58: const char
kPermissionRequestEventName[] = "permissionRequest";
On 2012/10/17 06:18:45, creis wrote:
> nit: permission < persist

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:519: // TODO(lazyboy):
Automatically deny request when there is no listener.
On 2012/10/17 06:18:45, creis wrote:
> Can we go ahead and do this?

Ah, it is easy to do so now.
Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:73: // Requests the embedder
(browser process) for media access permission.
On 2012/10/17 06:18:45, creis wrote:
> nit: Requests media access permission from the embedder.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/11001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:75: // Informs the
BrowserPlugin that the guest's request to media access has been
On 2012/10/17 06:18:45, creis wrote:
> Heh, in this case you do want "for" rather than "to".

Done.

[lazyboy, 2012-10-19 19:52:49]

I've updated the CL to *not* expose the permission api flow.

Video/webcam also works now b/c Fady fixed the compositing flag recently!

PTAL.

[Fady Samuel, 2012-11-16 03:42:59]

I haven't done a thorough review yet (will do so tomorrow), but this looks cool.
This pretty much matches what we discussed the other day.

https://chromiumcodereview.appspot.com/11093080/diff/37001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:148:
objectNode.setMediaPermission(request_id, false /* allow */);
This looks good. Looks like what we discussed.

[sadrul, 2012-11-16 03:59:53]

I looked at the shim and the code in renderer/browser_plugin/. I think this
looks good. A couple of minor nit-picky comments:

https://chromiumcodereview.appspot.com/11093080/diff/37001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:132:
WebView.prototype.setupPermissionRequestEvent_ = function(mutation) {
Can setupEvent_ be reused for this? setupEvent_ can take an optional third param
which is a callback that prepares the events. For example:

   setupEvent_('permissionrequest', ['reason', ...], prepareEvent);

where prepareEvent is a function:

   function prepareEvent(custom_event, webview_event) {
     if (/* custom_event.request_id is not undefined */) {
       webview_event.reason = custom_event.reason;
       webview_event.allow = ...;
       webview_event.deny = ...;
     }
   }

and setupEvent_ will call into this just before it dispatches the event.

The reason I am proposing this is that the internal details of the event
dispatching from the <object> plugin to the <webview> shim can change (e.g. we
decide at some future time that this custom event business is not what we want,
and we use postMessage or some other mechanism for this). So the main code that
dispatches the event to the <webview> consumers happens in a single place.

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:62: const char
kPermissionRequestEventName[] = "permissionrequest";
General comment (definitely not scope for this CL): the event names and the
attrib names are inter-mingled here. It is difficult to get the list of events
at a glance. It would be good to list the event names and the various attrib
names separately.

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:122: void AllowMediaAccess(int
request_id, bool allow);
Perhaps call this RespondMediaAccess or something similar. It feels a bit
awkward to call AllowMediaAccess when the request is denied!

[lazyboy, 2012-11-16 06:27:09]

https://chromiumcodereview.appspot.com/11093080/diff/37001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:132:
WebView.prototype.setupPermissionRequestEvent_ = function(mutation) {
On 2012/11/16 03:59:53, sadrul wrote:
> Can setupEvent_ be reused for this? setupEvent_ can take an optional third
param
> which is a callback that prepares the events. For example:
> 
>    setupEvent_('permissionrequest', ['reason', ...], prepareEvent);
> 
> where prepareEvent is a function:
> 
>    function prepareEvent(custom_event, webview_event) {
>      if (/* custom_event.request_id is not undefined */) {
>        webview_event.reason = custom_event.reason;
>        webview_event.allow = ...;
>        webview_event.deny = ...;
>      }
>    }
> 
> and setupEvent_ will call into this just before it dispatches the event.
> 
> The reason I am proposing this is that the internal details of the event
> dispatching from the <object> plugin to the <webview> shim can change (e.g. we
> decide at some future time that this custom event business is not what we
want,
> and we use postMessage or some other mechanism for this). So the main code
that
> dispatches the event to the <webview> consumers happens in a single place.

Yes that makes sense.
Done.

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:62: const char
kPermissionRequestEventName[] = "permissionrequest";
On 2012/11/16 03:59:53, sadrul wrote:
> General comment (definitely not scope for this CL): the event names and the
> attrib names are inter-mingled here. It is difficult to get the list of events
> at a glance. It would be good to list the event names and the various attrib
> names separately.

Agreed, I'll fix that separately so I don't have to worry abt merge conflicts in
this CL.

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/37001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:122: void AllowMediaAccess(int
request_id, bool allow);
On 2012/11/16 03:59:53, sadrul wrote:
> Perhaps call this RespondMediaAccess or something similar. It feels a bit
> awkward to call AllowMediaAccess when the request is denied!

Done.
(the msg is named AllowFoo still, we shd probably change that too?)

[lazyboy, 2012-11-19 21:17:15]

Charlie, this should be good to review now.
I'm not having much luck running the tests I've added on bots yet, I'm not even
sure if tests requiring media permission can run properly on buildbots, I'll
keep working on that.
Ty.

[Charlie Reis, 2012-11-20 01:54:00]

This is looking pretty good!  A few comments and questions below.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:124: ['reason'],
The API called for 'type', but if that's not available, I'm wondering if
something like 'permission' is better than 'reason'.  Can we find similar APIs
in extensions to model after?

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:139:
WebView.prototype.setupEvent_ = function(eventname, attribs, opt_prepareEvent) {
I'm not familiar with our JS style guide rules.  Are optional arguments
permitted?

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:162: // webview specs).
Are we still happy with the spec, or should it be updated?  (Sounds like we're
changing at least one name, but I'm guessing the URL still makes sense.  Is
details useful?)

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:166:
objectNode.setMediaPermission(requestId, true /* allow */);
nit: Don't need the /* allow */, since it's clear from surrounding context.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:17:
// not have acccess to media. Permission is denied until we expose js API to
I don't understand this part:
"Permission is denied until we expose js API to let embedder allow/deny such
requests."

Isn't that what this CL is implementing?

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:29:
// at this point, but will change once permission API is implmented.
nit: implemented
Again, aren't we implementing the permission API?

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:33:
//' src="' + embedder.guestURL + '"' +
Commented out?

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:54:

nit: Remove extra line.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:18:
// let embedder allow/deny such requests.
Same comments.

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:305: // a given
BrowserPluginGuest.
I wonder if we should put a (generous) cap on this map's size.  Otherwise a web
page could presumably generate requests in a loop to allocate arbitrary amounts
of memory in the browser process, right?

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:57: const char
kEventRequestPermission[] = "permissionrequest";
nit: Alphabetize

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:73: const char kRequestIdKey[]
= "request_id";
What's the difference between having a "Key" suffix or not?

[lazyboy, 2012-11-20 02:55:20]

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:124: ['reason'],
On 2012/11/20 01:54:00, creis wrote:
> The API called for 'type', but if that's not available, I'm wondering if
> something like 'permission' is better than 'reason'.  Can we find similar APIs
> in extensions to model after?

This should be easy to change later (before m25 cut of course) once we decide.

Related apps API is chrome.permissions.* API:
http://developer.chrome.com/extensions/permissions.html
It seems different enough from our model, but interesting.

I was trying to check if mozilla's BrowserAPI's permission section got updated,
it still says pending in the status page:
https://wiki.mozilla.org/WebAPI/BrowserAPI.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:139:
WebView.prototype.setupEvent_ = function(eventname, attribs, opt_prepareEvent) {
On 2012/11/20 01:54:00, creis wrote:
> I'm not familiar with our JS style guide rules.  Are optional arguments
> permitted?
I don't know about chromium js style guide. Google internal js heavily uses opt_
args.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:162: // webview specs).
On 2012/11/20 01:54:00, creis wrote:
> Are we still happy with the spec, or should it be updated?  (Sounds like we're
> changing at least one name, but I'm guessing the URL still makes sense.  Is
> details useful?)

Details can be made useful, probably noting that what type of media was
requested: audio or video or both.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:166:
objectNode.setMediaPermission(requestId, true /* allow */);
On 2012/11/20 01:54:00, creis wrote:
> nit: Don't need the /* allow */, since it's clear from surrounding context.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:17:
// not have acccess to media. Permission is denied until we expose js API to
On 2012/11/20 01:54:00, creis wrote:
> I don't understand this part:
> "Permission is denied until we expose js API to let embedder allow/deny such
> requests."
> 
> Isn't that what this CL is implementing?

Yes it is.
The comments are stale from previous round, where I removed exposing the
permission-request events. I didn't update the test comments properly yet since
I'm a bit skeptical about checking-in these tests (I have a feeling
bots/builders won't like this one), I'm planning to have to trials on bots too
see how things go.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:29:
// at this point, but will change once permission API is implmented.
On 2012/11/20 01:54:00, creis wrote:
> nit: implemented
> Again, aren't we implementing the permission API?

Removed, same as above.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:33:
//' src="' + embedder.guestURL + '"' +
On 2012/11/20 01:54:00, creis wrote:
> Commented out?

Removed.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:54:

On 2012/11/20 01:54:00, creis wrote:
> nit: Remove extra line.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:18:
// let embedder allow/deny such requests.
On 2012/11/20 01:54:00, creis wrote:
> Same comments.

Addressed all of them similar to other file.

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.h:305: // a given
BrowserPluginGuest.
On 2012/11/20 01:54:00, creis wrote:
> I wonder if we should put a (generous) cap on this map's size.  Otherwise a
web
> page could presumably generate requests in a loop to allocate arbitrary
amounts
> of memory in the browser process, right?

That is true, added check for limit, we deny media permissions if there are more
than thousand requests pending, although that might seem wrong if we already
allowed permission once and suddenly stopped allowing permissions. But hopefully
that would happen when the guest is trying to do some funny business. So it
should be OK to deny.
Thanks for catching this, I'll keep this in mind for similar code paths we're
adding for browser plugin.

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:57: const char
kEventRequestPermission[] = "permissionrequest";
On 2012/11/20 01:54:00, creis wrote:
> nit: Alphabetize

Ah bad merge, Done.

https://chromiumcodereview.appspot.com/11093080/diff/37003/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:73: const char kRequestIdKey[]
= "request_id";
On 2012/11/20 01:54:00, creis wrote:
> What's the difference between having a "Key" suffix or not?

yes that's confusing, all string in this section are keys to an object, removed
key suffix.

[lazyboy, 2012-12-04 23:08:28]

Moved the permission api behind flag/dev channel as we discussed, should be good
to be reviewed.
Still no luck on trybot when the embedder allows permission.

[Fady Samuel, 2012-12-05 22:14:32]

You mentioned that one of the tests don't work on the bots. Could you please
find out why?

Also, Charlie, are you happy with us putting permissions API as a "dev"
permission until M26?

https://codereview.chromium.org/11093080/diff/55001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view_permission_api.js (right):

https://codereview.chromium.org/11093080/diff/55001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_permission_api.js:30: //
TODO(lazyboy): Also fill in webViewEvt.url and webViewEvt.details (see
Do it now?

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:31:
// Timeout is required otherwise event registration on <webview> does not
This should not be necessary any more.

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:28:
webview.setAttribute('src', embedder.guestURL);
Out of curiosity, is there any reason why this is not a part of the string
above?

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:31:
// work.
This timeout should no longer be required after Sadrul's changes to events.

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:44:
webview.addEventListener('loadcommit', onWebViewLoadCommit);
Make this loadstop. I believe loadstop happens after loadcommit. We don't really
provide hard guarantees on when contentWindow will become available. I should
look into that to avoid flaky tests.

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/manifest.json
(right):

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/manifest.json:7:
"webview.permissionAPI",
So what happens when this goes stable? Do we just get rid fo this permission at
that point and cut-and-paste web_view_permission_api.js into web_view.js?

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html
(right):

https://codereview.chromium.org/11093080/diff/55001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html:47:
requestAudioMedia();
This code might perhaps be a bit simpler if we delay the media access until we
get the embedderWindowChannel?

https://codereview.chromium.org/11093080/diff/55001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11093080/diff/55001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.h:121: // been allowed or denied.
...allowed or denied by the embedder.

[Fady Samuel, 2012-12-05 22:47:31]

https://codereview.chromium.org/11093080/diff/55001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11093080/diff/55001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:80:
media_requests_map_.clear();
I believe this is unnecessary.

[lazyboy, 2012-12-06 00:05:52]

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view_permission_api.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_permission_api.js:30: //
TODO(lazyboy): Also fill in webViewEvt.url and webViewEvt.details (see
On 2012/12/05 22:14:32, Fady Samuel wrote:
> Do it now?

I'm not sure about the values, making the url guest's url fine?
for 'details', we need to decide in API spec, and can be done once we have
decide.

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:31:
// Timeout is required otherwise event registration on <webview> does not
On 2012/12/05 22:14:32, Fady Samuel wrote:
> This should not be necessary any more.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:28:
webview.setAttribute('src', embedder.guestURL);
On 2012/12/05 22:14:32, Fady Samuel wrote:
> Out of curiosity, is there any reason why this is not a part of the string
> above?
I think this was here because of the old way of doing things in shim. setting it
directly seems to work now.

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:31:
// work.
On 2012/12/05 22:14:32, Fady Samuel wrote:
> This timeout should no longer be required after Sadrul's changes to events.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:44:
webview.addEventListener('loadcommit', onWebViewLoadCommit);
On 2012/12/05 22:14:32, Fady Samuel wrote:
> Make this loadstop. I believe loadstop happens after loadcommit. We don't
really
> provide hard guarantees on when contentWindow will become available. I should
> look into that to avoid flaky tests. 

Done.

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/manifest.json
(right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/manifest.json:7:
"webview.permissionAPI",
On 2012/12/05 22:14:32, Fady Samuel wrote:
> So what happens when this goes stable? Do we just get rid fo this permission
at
> that point and cut-and-paste web_view_permission_api.js into web_view.js?

Yes.

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html
(right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html:47:
requestAudioMedia();
On 2012/12/05 22:14:32, Fady Samuel wrote:
> This code might perhaps be a bit simpler if we delay the media access until we
> get the embedderWindowChannel?
Done, thanks.

https://chromiumcodereview.appspot.com/11093080/diff/55001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:80:
media_requests_map_.clear();
On 2012/12/05 22:47:31, Fady Samuel wrote:
> I believe this is unnecessary.

Removed.

https://chromiumcodereview.appspot.com/11093080/diff/55001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/55001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:121: // been allowed or denied.
On 2012/12/05 22:14:32, Fady Samuel wrote:
> ...allowed or denied by the embedder.

Done.

[lazyboy, 2012-12-06 20:28:42]

Charlie, this should be good to take another look.

[Charlie Reis, 2012-12-07 19:16:25]

Thanks.  Can you further update the CL description?  We should change "type" to
"permission," and drop all the stuff about repro'ing in a regular page (since
that's not a supported use case outside the dangerous command line flag).

I'm definitely happy with this being behind a 'dev' permission for M25, and then
exposing it in M26.  Should we just use 'experimental' instead of a new one?

https://codereview.chromium.org/11093080/diff/37003/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/11093080/diff/37003/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:139:
WebView.prototype.setupEvent_ = function(eventname, attribs, opt_prepareEvent) {
On 2012/11/20 02:55:20, lazyboy wrote:
> On 2012/11/20 01:54:00, creis wrote:
> > I'm not familiar with our JS style guide rules.  Are optional arguments
> > permitted?
> I don't know about chromium js style guide. Google internal js heavily uses
opt_
> args.

Ok.  Make sure you get a chrome/ reviewer to approve this, then.

https://codereview.chromium.org/11093080/diff/71001/chrome/common/extensions/...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/11093080/diff/71001/chrome/common/extensions/...
chrome/common/extensions/permissions/api_permission.h:117:
kWebViewPermissionAPI,
Hmm, this requires developers to request a permission?  We don't want that in
the long term, right?  Perhaps we can just gate this stuff on kExperimental
instead?

https://codereview.chromium.org/11093080/diff/71001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view_permission_api.js (right):

https://codereview.chromium.org/11093080/diff/71001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_permission_api.js:10: // but we
want keep permission API in <webview> only available in DEV channel.
nit: Let's be consistent: 'stable' / 'dev'

https://codereview.chromium.org/11093080/diff/71001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_permission_api.js:20:
this.preparePremissionEvent_.bind(this));
Typo: Premission

https://codereview.chromium.org/11093080/diff/71001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_permission_api.js:25: * @param
{!Object} detail The event details, originated from <object>.
I don't know JavaScript coding style very well.  What does !Object mean?

https://codereview.chromium.org/11093080/diff/71001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_permission_api.js:29: if
(detail.reason == 'media' && detail.request_id !== undefined) {
From your links, it sounds like we should change this from 'reason' to
'permission'.

https://codereview.chromium.org/11093080/diff/71001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view_permission_api.js:31: // webview
specs).
Can you add a bug number (141197) here?  (Is it tricky to fix in this CL?)

https://codereview.chromium.org/11093080/diff/71001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/71001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:32:
chrome.test.log('Embedder notified on permissionRequest');
Can we verify that e.permission is "media"?

https://codereview.chromium.org/11093080/diff/71001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/71001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:31:
chrome.test.log('Embedder notified on permissionRequest');
Can we verify that e.permission is "media"?

https://codereview.chromium.org/11093080/diff/71001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html
(right):

https://codereview.chromium.org/11093080/diff/71001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html:17:
// Should not happen.
This file gets used for both tests, right?  So it should happen in one of them.

https://codereview.chromium.org/11093080/diff/71001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11093080/diff/71001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:321:
BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
Should we have a security check here, ensuring that the embedder has this
permission to grant?  We wouldn't want some other future caller of this to leak
the permission inadvertently.  (Or is there something else that should prevent
that?)

https://codereview.chromium.org/11093080/diff/71001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11093080/diff/71001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:346:
embedder_web_contents_impl->RequestMediaAccessPermission(
Ah, is the security check in here, then?  Maybe my earlier comment isn't as
important if this fails when the embedder lacks the permission.

https://codereview.chromium.org/11093080/diff/71001/content/common/browser_pl...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11093080/diff/71001/content/common/browser_pl...
content/common/browser_plugin_messages.h:236: // denied.
Might want to note that this occurs before the security check, so it can say
"allowed" in cases where the permission will end up being denied.  (Is that
right?)

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:672: // listeners for
permissionrequest. Right now it is not possible to know about
So what happens now if there's no listeners?  Do we end up with lots of
outstanding requests that are never cleaned up?

Also, how are we handling multiple listeners?  I found our email discussion
("API design question: policy functions") and it seems like we were settling on
requiring all listeners to call allow() to grant the permission.  Is that
possible if we don't know how many listeners there are?

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:675: // listener registered on
the plugin from the shim.
One permissionrequest listener?  I'm not sure I follow.

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:47: const char
kMethodSetMediaPermission[] = "setMediaPermission";
What is this for?  I don't remember it in the API discussions.

If this is just an implementation detail, can it be hidden from actual pages? 
Seems like it would allow bypassing the listeners.

[lazyboy, 2012-12-07 22:50:44]

Updated the CL description.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/common/exte...
File chrome/common/extensions/permissions/api_permission.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/common/exte...
chrome/common/extensions/permissions/api_permission.h:117:
kWebViewPermissionAPI,
On 2012/12/07 19:16:25, creis wrote:
> Hmm, this requires developers to request a permission?  We don't want that in
> the long term, right?  Perhaps we can just gate this stuff on kExperimental
> instead?
Yes that requires permission and we don't want them in long term.
Looking at kExperimental, that is is marked as 'stable'
(chrome/common/extensions/api/_permission_features.json) and feels a bit too
broad for us.
Also we can rename this to kWebViewExperimental, so we use this as a bucket for
all experimental webview features.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view_permission_api.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_permission_api.js:10: // but we
want keep permission API in <webview> only available in DEV channel.
On 2012/12/07 19:16:25, creis wrote:
> nit: Let's be consistent: 'stable' / 'dev'

Done.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_permission_api.js:20:
this.preparePremissionEvent_.bind(this));
On 2012/12/07 19:16:25, creis wrote:
> Typo: Premission

Done.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_permission_api.js:25: * @param
{!Object} detail The event details, originated from <object>.
On 2012/12/07 19:16:25, creis wrote:
> I don't know JavaScript coding style very well.  What does !Object mean?

!Object == non-nullable Object (@param of every non primitive type is nullable
by default).

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_permission_api.js:29: if
(detail.reason == 'media' && detail.request_id !== undefined) {
On 2012/12/07 19:16:25, creis wrote:
> From your links, it sounds like we should change this from 'reason' to
> 'permission'.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view_permission_api.js:31: // webview
specs).
On 2012/12/07 19:16:25, creis wrote:
> Can you add a bug number (141197) here?  (Is it tricky to fix in this CL?)

For 'details', we need to decide what they would be (for other permission
requests too).
I've filled in the 'url' part + test. Pending: a test that tries to access media
from a sub-frame within guest.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_no_permission/embedder.js:32:
chrome.test.log('Embedder notified on permissionRequest');
On 2012/12/07 19:16:25, creis wrote:
> Can we verify that e.permission is "media"?

Done.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/embedder_has_permission/embedder.js:31:
chrome.test.log('Embedder notified on permissionRequest');
On 2012/12/07 19:16:25, creis wrote:
> Can we verify that e.permission is "media"?

Done.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/test/data/e...
File
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html
(right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/test/data/e...
chrome/test/data/extensions/platform_apps/web_view_media_access/media_access_guest.html:17:
// Should not happen.
On 2012/12/07 19:16:25, creis wrote:
> This file gets used for both tests, right?  So it should happen in one of
them.

Right. Fixed.

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:321:
BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
On 2012/12/07 19:16:25, creis wrote:
> Should we have a security check here, ensuring that the embedder has this
> permission to grant?  We wouldn't want some other future caller of this to
leak
> the permission inadvertently.  (Or is there something else that should prevent
> that?)

(see next comment)

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:346:
embedder_web_contents_impl->RequestMediaAccessPermission(
On 2012/12/07 19:16:25, creis wrote:
> Ah, is the security check in here, then?  Maybe my earlier comment isn't as
> important if this fails when the embedder lacks the permission.

OK.

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/common/bro...
content/common/browser_plugin_messages.h:236: // denied.
On 2012/12/07 19:16:25, creis wrote:
> Might want to note that this occurs before the security check, so it can say
> "allowed" in cases where the permission will end up being denied.  (Is that
> right?)
Correct. Added notes.

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:672: // listeners for
permissionrequest. Right now it is not possible to know about
On 2012/12/07 19:16:25, creis wrote:
> So what happens now if there's no listeners?  Do we end up with lots of
> outstanding requests that are never cleaned up?
> 
> Also, how are we handling multiple listeners?  I found our email discussion
> ("API design question: policy functions") and it seems like we were settling
on
> requiring all listeners to call allow() to grant the permission.  Is that
> possible if we don't know how many listeners there are?

When there's no listeners, yes we end up with lost/outstanding requests. I am
attempting to figure this out (https://chromiumcodereview.appspot.com/11446071/)
so we automatically deny, but bad thing about that is this code here needs to be
shim aware. The other bad thing is even if we have listeners, we can end up with
outstanding request, if the embedder ignores the event by not calling
allow/deny.

Multiple listeners are not handled in this CL at all. Only the first one to call
allow/deny wins (and that clears the request), so the subsequent calls to
allow/deny are ignored. I am not sure how to approach this yet, webkit doesn't
seem to expose anything about # of event listeners registered.

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:675: // listener registered on
the plugin from the shim.
On 2012/12/07 19:16:25, creis wrote:
> One permissionrequest listener?  I'm not sure I follow.

This is the one (and only one) '-internal-permissionrequest' listener attached
to <object> from <webview> shim. This one always gets attached, whether or not
there are actual event listeners registered on the <webview> tag.

Where user facing ones are the one on object (that embedder/developer creates)
<webview>.addEventListener('permissionrequest', functionA);
<webview>.addEventListener('permissionrequest', functionB);
<webview>.addEventListener('permissionrequest', functionC);

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:47: const char
kMethodSetMediaPermission[] = "setMediaPermission";
On 2012/12/07 19:16:25, creis wrote:
> What is this for?  I don't remember it in the API discussions.
> 
> If this is just an implementation detail, can it be hidden from actual pages? 
> Seems like it would allow bypassing the listeners.

This is just an impl details, right. It is only exposed in the <object>, not the
<webview> tag.

I'm tyring to think what kind of (unexpected) bypassing might happen even if
this somehow gets exposed... Do you have a case where this can trigger
unexpected allow/deny?

[lazyboy, 2012-12-10 22:21:37]

Hey,
I've updated the CL to handle multiple event listeners as we discussed. PTAL.

Couple points:

1. This requires 2 webkit changes:
a. browser_plugin.GetNumberOfListeners(str);
b. WebNode.isShadowRoot(); and WebNode.shadowHost();

2. I still don't know how to handle tracking multiple e.allow() calls from a
listeners. In short the following can incorrectly end up allowing permission:
<webview>.addEventListener('permissionrequest', function(e) {
  e.allow();
  e.allow();
});
<webview>.addEventListener('permissionrequest', function(e) {
  e.deny();
});

[Charlie Reis, 2012-12-13 00:48:04]

On 2012/12/10 22:21:37, lazyboy wrote:
> Hey,
> I've updated the CL to handle multiple event listeners as we discussed. PTAL.
> 
> Couple points:
> 
> 1. This requires 2 webkit changes:
> a. browser_plugin.GetNumberOfListeners(str);
> b. WebNode.isShadowRoot(); and WebNode.shadowHost();
> 
> 2. I still don't know how to handle tracking multiple e.allow() calls from a
> listeners. In short the following can incorrectly end up allowing permission:
> <webview>.addEventListener('permissionrequest', function(e) {
>   e.allow();
>   e.allow();
> });
> <webview>.addEventListener('permissionrequest', function(e) {
>   e.deny();
> });

Thanks.  Sorry this is taking me a bit to get to, but things are in crunch mode
with the branch.  Since this is destined for M26 anyway, is it ok if I get to
this review next week, or is it blocking you?

[Charlie Reis, 2012-12-17 22:28:09]

The code looks mostly ready, but there are still two big questions about how an
app could bypass its own security checks.  We'd have to change the API if we
couldn't promise these things.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/common/exte...
File chrome/common/extensions/permissions/api_permission.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/common/exte...
chrome/common/extensions/permissions/api_permission.h:117:
kWebViewPermissionAPI,
On 2012/12/07 22:50:44, lazyboy wrote:
> On 2012/12/07 19:16:25, creis wrote:
> > Hmm, this requires developers to request a permission?  We don't want that
in
> > the long term, right?  Perhaps we can just gate this stuff on kExperimental
> > instead?
> Yes that requires permission and we don't want them in long term.
> Looking at kExperimental, that is is marked as 'stable'
> (chrome/common/extensions/api/_permission_features.json) and feels a bit too
> broad for us.
> Also we can rename this to kWebViewExperimental, so we use this as a bucket
for
> all experimental webview features.

Experimental is allowed in stable, but apps that use it can't be uploaded to the
web store, which accomplishes what we want.  I don't know if there's a similar
restriction for permissions marked dev.  If we did go this route,
kWebViewExperimental does seem like a more general name for future issues like
this.

Anyway, I'll defer on this, so please have someone from the apps team review it
to make sure they're ok with it.

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:47: const char
kMethodSetMediaPermission[] = "setMediaPermission";
On 2012/12/07 22:50:44, lazyboy wrote:
> On 2012/12/07 19:16:25, creis wrote:
> > What is this for?  I don't remember it in the API discussions.
> > 
> > If this is just an implementation detail, can it be hidden from actual
pages? 
> > Seems like it would allow bypassing the listeners.
> 
> This is just an impl details, right. It is only exposed in the <object>, not
the
> <webview> tag.
> 
> I'm tyring to think what kind of (unexpected) bypassing might happen even if
> this somehow gets exposed... Do you have a case where this can trigger
> unexpected allow/deny?

It certainly looks like the embedder could call
someObjectTag.setMediaPermission(42, true) to bypass the existing event
listeners, if it could guess the request_id (e.g., 42).  That would defeat app
developers who try to install security policies to restrict their own app's
behavior, which isn't ok.

https://chromiumcodereview.appspot.com/11093080/diff/85001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/85001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:792: // incorrectly allow the
request.
Yeah, that's no good.  Ideally there would be some way to match up the responses
to the functions we handed out, like keeping a reference to each allow/deny
function.  An unforgeable ID (e.g., in the function's closure) would also be
sufficient.  I don't know how we can get TriggerEvent to hand out a different
version of the function to each listener, though.

We may need some advice on this, since it's an important aspect of the API if we
stick with this design.  Not sure I'm comfortable leaving it as a TODO without a
plan for how to fix it.

[Fady Samuel, 2013-02-05 17:53:26]

Some nits I noticed. This doesn't match the API we discussed with abarth@ quite
yet. I'll move the newwindow event to experimental as well.

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11093080/diff/71001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin_bindings.cc:47: const char
kMethodSetMediaPermission[] = "setMediaPermission";
On 2012/12/07 19:16:25, creis wrote:
> What is this for?  I don't remember it in the API discussions.
> 
> If this is just an implementation detail, can it be hidden from actual pages? 
> Seems like it would allow bypassing the listeners.

This is not forwarded from the shim to the BrowserPlugin.

https://codereview.chromium.org/11093080/diff/95001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/web_view.js (left):

https://codereview.chromium.org/11093080/diff/95001/chrome/renderer/resources...
chrome/renderer/resources/extensions/web_view.js:175: node.dispatchEvent(evt);
If true then we can deny... because preventDefault was not called.

https://codereview.chromium.org/11093080/diff/95001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11093080/diff/95001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:367:
BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
No need for this. Simply add BrowserPluginHostMsg_AllowMediaAccess to
ShouldForwardToBrowserPluginGuest, and it'll automagically do the forwarding for
you.

https://codereview.chromium.org/11093080/diff/95001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11093080/diff/95001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:708: LOG(INFO) << "Not a
valid request id";
nit: Not a valid request ID.

https://codereview.chromium.org/11093080/diff/95001/content/renderer/browser_...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/95001/content/renderer/browser_...
content/renderer/browser_plugin/browser_plugin.cc:544: bool
BrowserPlugin::HasEventListeners(const std::string& event_name) {
I think you'd need the ancestor list here too, as a more general solution that
supports bubble.

[lazyboy, 2013-02-07 04:38:42]

Fady: Can you take a look again? I've included the garbage collection stuff for
event.request object. Thanks.

> Fady Samuel	
> Some nits I noticed. This doesn't match the API we > discussed with abarth@
quite
> yet. I'll move the newwindow event to experimental as well.
Ya, I didn't update the CL then.

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/common/exte...
File chrome/common/extensions/permissions/api_permission.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/71001/chrome/common/exte...
chrome/common/extensions/permissions/api_permission.h:117:
kWebViewPermissionAPI,
On 2012/12/17 22:28:09, creis wrote:
> On 2012/12/07 22:50:44, lazyboy wrote:
> > On 2012/12/07 19:16:25, creis wrote:
> > > Hmm, this requires developers to request a permission?  We don't want that
> in
> > > the long term, right?  Perhaps we can just gate this stuff on
kExperimental
> > > instead?
> > Yes that requires permission and we don't want them in long term.
> > Looking at kExperimental, that is is marked as 'stable'
> > (chrome/common/extensions/api/_permission_features.json) and feels a bit too
> > broad for us.
> > Also we can rename this to kWebViewExperimental, so we use this as a bucket
> for
> > all experimental webview features.
> 
> Experimental is allowed in stable, but apps that use it can't be uploaded to
the
> web store, which accomplishes what we want.  I don't know if there's a similar
> restriction for permissions marked dev.  If we did go this route,
> kWebViewExperimental does seem like a more general name for future issues like
> this.
> 
> Anyway, I'll defer on this, so please have someone from the apps team review
it
> to make sure they're ok with it.

Ah I see, 'experimental' should take care of our needs then.
Made web_view_experimental_api.js only served when 'experimental' permission is
available.

https://chromiumcodereview.appspot.com/11093080/diff/95001/chrome/renderer/re...
File chrome/renderer/resources/extensions/web_view.js (left):

https://chromiumcodereview.appspot.com/11093080/diff/95001/chrome/renderer/re...
chrome/renderer/resources/extensions/web_view.js:175: node.dispatchEvent(evt);
On 2013/02/05 17:53:27, Fady Samuel wrote:
> If true then we can deny... because preventDefault was not called.

Updated CL.

https://chromiumcodereview.appspot.com/11093080/diff/95001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/95001/content/browser/br...
content/browser/browser_plugin/browser_plugin_embedder.cc:367:
BrowserPluginGuest* guest = GetGuestByInstanceID(instance_id);
On 2013/02/05 17:53:27, Fady Samuel wrote:
> No need for this. Simply add BrowserPluginHostMsg_AllowMediaAccess to
> ShouldForwardToBrowserPluginGuest, and it'll automagically do the forwarding
for
> you.

Yep, we also have embedder_web_contents_ directly in guest, which makes it work.
Done.

https://chromiumcodereview.appspot.com/11093080/diff/95001/content/browser/br...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/95001/content/browser/br...
content/browser/browser_plugin/browser_plugin_guest.cc:708: LOG(INFO) << "Not a
valid request id";
On 2013/02/05 17:53:27, Fady Samuel wrote:
> nit: Not a valid request ID.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/95001/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/95001/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:544: bool
BrowserPlugin::HasEventListeners(const std::string& event_name) {
On 2013/02/05 17:53:27, Fady Samuel wrote:
> I think you'd need the ancestor list here too, as a more general solution that
> supports bubble.
Deferring it for now, added todo, I'll fix once we're happy with the rest.

[Fady Samuel, 2013-02-07 15:40:34]

This looks great! I'm really excited by this. This API is beautiful thanks to
you, Charlie, Darin, Erik, and Adam. :-)

The general theme of my comments is generalize...

Let's avoid writing seemingly media access specifics when code can be
generalized for other permissions and window.open.

https://codereview.chromium.org/11093080/diff/112002/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_no_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/112002/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_no_permission/embedder.js:44:
webview.contentWindow.frames.postMessage(
webview.contentWindow.postMessage should work.

https://codereview.chromium.org/11093080/diff/112002/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/112002/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js:54:
webview.contentWindow.frames.postMessage(
webview.contentWindow.postMessage should now work.

https://codereview.chromium.org/11093080/diff/112002/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11093080/diff/112002/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:689: // TODO(lazyboy): We
should use timeouts to clear this pending requests if
Do we still care about this if we're tying pending requests to garbage
collection?

https://codereview.chromium.org/11093080/diff/112002/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:699:
SendMessageToEmbedder(new BrowserPluginMsg_RequestMediaAccess(
How about generalizing this? BrowserPluginMsg_RequestPermission?

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:839: void
BrowserPlugin::PersistRequestObject(const NPVariant* request, int id) {
Can we make this more general to allow for window.open's "window" object? Also
to allow for other permission types. This code to avoid referring directly to
media access as it will be used for other permissions and window.open.

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:842: int *new_request_id = new
int(id);
why are you allocating memory on the heap here? We should be able to store ints
and pairs below directly in the map and pass a reference within the map to
MakeWeak.

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:861:
static_cast<std::pair<int*, BrowserPlugin*>*>(param);
We're leaking memory here. Seem my suggestion above. It should be possible to do
this without leaking memory or needing to explicitly allocate memory on the
heap. We can use a map to manage our memory for us.

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:941: new
BrowserPluginHostMsg_AllowMediaAccess(
Given the review overhead for introducing new IPCs, I'm wondering if we can
generalize permission request IPCs?  BrowserPluginHostMsg_AllowPermissionRequest
perhaps?

This way we won't have to constantly request IPC audits as we add permissions.

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_bindings.cc:343: class
BrowserPluginBindingPersistRequestObject
Get rid of "request" to allow this to be used by window.open.

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_bindings.cc:402:
BrowserPluginBindingSetMediaPermission()
To reduce the number of new methods we introduce, how about a more general
SetPermissionResponse(request_id, allow).

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_constants.cc (right):

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_constants.cc:19: const char
kMethodInternalPersistObject[] = "-internal-persistObject";
This is a good idea! Perhaps we should group internal methods together and away
from other methods as well to make it clear.

https://codereview.chromium.org/11093080/diff/112002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_constants.cc:61: const char
kRequestId[] = "request_id";
requestId?

[lazyboy, 2013-02-07 21:24:41]

https://chromiumcodereview.appspot.com/11093080/diff/112002/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_no_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/112002/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_no_permission/embedder.js:44:
webview.contentWindow.frames.postMessage(
On 2013/02/07 15:40:34, Fady Samuel wrote:
> webview.contentWindow.postMessage should work.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/112002/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/112002/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js:54:
webview.contentWindow.frames.postMessage(
On 2013/02/07 15:40:34, Fady Samuel wrote:
> webview.contentWindow.postMessage should now work.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:689: // TODO(lazyboy): We
should use timeouts to clear this pending requests if
On 2013/02/07 15:40:34, Fady Samuel wrote:
> Do we still care about this if we're tying pending requests to garbage
> collection?
Hmmm, probably not; I'll let Charlie comment on this.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:699:
SendMessageToEmbedder(new BrowserPluginMsg_RequestMediaAccess(
On 2013/02/07 15:40:34, Fady Samuel wrote:
> How about generalizing this? BrowserPluginMsg_RequestPermission?

request.security_origin might be a problem here to generalize, not all
permissions would have this.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:839: void
BrowserPlugin::PersistRequestObject(const NPVariant* request, int id) {
On 2013/02/07 15:40:34, Fady Samuel wrote:
> Can we make this more general to allow for window.open's "window" object? Also
> to allow for other permission types. This code to avoid referring directly to
> media access as it will be used for other permissions and window.open.

We need a centralized id generation in that case, as discussed offline, we can
generalize when we have > 1 use case.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:842: int *new_request_id = new
int(id);
On 2013/02/07 15:40:34, Fady Samuel wrote:
> why are you allocating memory on the heap here? We should be able to store
ints
> and pairs below directly in the map and pass a reference within the map to
> MakeWeak.

I thought reference to map element is not guaranteed to stay the same if the map
was mutated, but the spec looks like it emphasizes that it should stay the same
(for associative containers) if I read this correctly:
http://gcc.gnu.org/onlinedocs/libstdc++/ext/lwg-active.html, search for "Modify
23.2.4 [associative.reqmts], p8:"
My assumption was based on std::vector probably.
I asked on IRC, no response yet I'll ask chromium-dev or awong@ to make sure.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:861:
static_cast<std::pair<int*, BrowserPlugin*>*>(param);
On 2013/02/07 15:40:34, Fady Samuel wrote:
> We're leaking memory here. Seem my suggestion above. It should be possible to
do
> this without leaking memory or needing to explicitly allocate memory on the
> heap. We can use a map to manage our memory for us.
See above, fixed hopefully.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:941: new
BrowserPluginHostMsg_AllowMediaAccess(
On 2013/02/07 15:40:34, Fady Samuel wrote:
> Given the review overhead for introducing new IPCs, I'm wondering if we can
> generalize permission request IPCs? 
BrowserPluginHostMsg_AllowPermissionRequest
> perhaps?
> 
> This way we won't have to constantly request IPC audits as we add permissions.

This part should be straight forward.
Done.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin_bindings.cc:343: class
BrowserPluginBindingPersistRequestObject
On 2013/02/07 15:40:34, Fady Samuel wrote:
> Get rid of "request" to allow this to be used by window.open.

See my comments in .cc file.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin_bindings.cc:402:
BrowserPluginBindingSetMediaPermission()
On 2013/02/07 15:40:34, Fady Samuel wrote:
> To reduce the number of new methods we introduce, how about a more general
> SetPermissionResponse(request_id, allow).

Done.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
File content/renderer/browser_plugin/browser_plugin_constants.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin_constants.cc:19: const char
kMethodInternalPersistObject[] = "-internal-persistObject";
On 2013/02/07 15:40:34, Fady Samuel wrote:
> This is a good idea! Perhaps we should group internal methods together and
away
> from other methods as well to make it clear.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/112002/content/renderer/...
content/renderer/browser_plugin/browser_plugin_constants.cc:61: const char
kRequestId[] = "request_id";
On 2013/02/07 15:40:34, Fady Samuel wrote:
> requestId?

Done, thanks.

[Fady Samuel, 2013-02-07 21:41:24]

https://codereview.chromium.org/11093080/diff/115007/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/115007/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:856: void
BrowserPlugin::WeakCallbackForPersistObject(
It just occurred to me: what happens if BrowserPlugin is removed from the DOM
before we garbage collect? :-(

[lazyboy, 2013-02-07 22:59:27]

https://codereview.chromium.org/11093080/diff/115007/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/115007/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:856: void
BrowserPlugin::WeakCallbackForPersistObject(
On 2013/02/07 21:41:24, Fady Samuel wrote:
> It just occurred to me: what happens if BrowserPlugin is removed from the DOM
> before we garbage collect? :-(

Good catch here.
Using WeakPtr and allocating map item with new().
I am able to repro this case confidently, so hopefully this is good now.

[lazyboy, 2013-02-07 23:48:47]

Charlie, can you take a look again?
The API now is aligned with what we discussed in the previous meetings.

You had couple overall concerns on the last round of review:
1. Putting permission API behind experimental, which is addressed.
2. Exposing non-API functions to <object> but not to <webview> which can bypass
the whole api by calling these functions directly (+ guessing the request id);
The solution Fady and I discussed here is to reject instantiating browser plugin
and its bindings, if <object> is not directly under <webview> tag. We will do
the change separately soon.
Thanks.

[Charlie Reis, 2013-02-09 01:20:18]

On 2013/02/07 23:48:47, lazyboy wrote:
> Charlie, can you take a look again?
> The API now is aligned with what we discussed in the previous meetings.
> 
> You had couple overall concerns on the last round of review:
> 1. Putting permission API behind experimental, which is addressed.
> 2. Exposing non-API functions to <object> but not to <webview> which can
bypass
> the whole api by calling these functions directly (+ guessing the request id);
> The solution Fady and I discussed here is to reject instantiating browser
plugin
> and its bindings, if <object> is not directly under <webview> tag. We will do
> the change separately soon.
> Thanks.

I'll try to take a close look on Monday.  In the meantime, can you update the CL
description to reflect what the new API looks like, following the discussion
with Adam?  That'll help me follow along, since I'm not sure which doc is
current.  Thanks!

[lazyboy, 2013-02-09 03:01:51]

On 2013/02/09 01:20:18, creis wrote:
> On 2013/02/07 23:48:47, lazyboy wrote:
> > Charlie, can you take a look again?
> > The API now is aligned with what we discussed in the previous meetings.
> > 
> > You had couple overall concerns on the last round of review:
> > 1. Putting permission API behind experimental, which is addressed.
> > 2. Exposing non-API functions to <object> but not to <webview> which can
> bypass
> > the whole api by calling these functions directly (+ guessing the request
id);
> > The solution Fady and I discussed here is to reject instantiating browser
> plugin
> > and its bindings, if <object> is not directly under <webview> tag. We will
do
> > the change separately soon.
> > Thanks.
> 
> I'll try to take a close look on Monday.  In the meantime, can you update the
CL
> description to reflect what the new API looks like, following the discussion
> with Adam?  That'll help me follow along, since I'm not sure which doc is
> current.  Thanks!
I've updated the CL description so it matches the new API. (A bit verbose than
required probably, but we can fix that later). Ty.

[Charlie Reis, 2013-02-11 22:20:56]

Making some good progress.  Main high level question: have you thought about how
this will generalize to other types of permissions?  Would be nice to share some
code between permission types if it's possible.

https://codereview.chromium.org/11093080/diff/123004/chrome/renderer/extensio...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/11093080/diff/123004/chrome/renderer/extensio...
chrome/renderer/extensions/dispatcher.cc:698:
source_map_.RegisterSource("webview.experimentalAPI",
You'll need someone from extensions to review this name.  Seems fine to me, but
I don't know if they would prefer experimental.webview (like the offscreen one
above).

https://codereview.chromium.org/11093080/diff/123004/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view_experimental_api.js (right):

https://codereview.chromium.org/11093080/diff/123004/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view_experimental_api.js:38: throw
'permissionrequest decision has already been made';
This is a string shown to web developers in the exception, right?  Is there a
convention or review needed for that (e.g., punctuation, etc)?

https://codereview.chromium.org/11093080/diff/123004/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view_experimental_api.js:40:
objectNode['-internal-setPermission']('media', requestId, true);
Can we check anywhere else that we don't call this repeatedly?  Not sure I trust
the page not to bypass this check.

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js
(right):

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js:170:
// This test check that not calling e.preventDefault() and not deciding on a
Do we have a test somewhere for using preventDefault?

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js:211:
/*
Why are these commented out?

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html
(right):

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html:4:
// A guest that requests for media access.
nit: Drop the "for"

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest2.html
(right):

https://codereview.chromium.org/11093080/diff/123004/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest2.html:1:
<html>
How is this file different from the previous one, and why do we need it?  (If we
do need it, maybe the name and/or comment should clarify.)

https://codereview.chromium.org/11093080/diff/123004/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11093080/diff/123004/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:79:
OnAllowPermissionAccess)
Why is "Access" in the name?  Maybe AllowPermission would be better?

https://codereview.chromium.org/11093080/diff/123004/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:690: // TODO(lazyboy): We
should use timeouts to clear this pending requests if
typo: this pending requests

https://codereview.chromium.org/11093080/diff/123004/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:705: void
BrowserPluginGuest::OnAllowPermissionAccess(
Again, will this generalize to other types of permissions?

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

https://codereview.chromium.org/11093080/diff/123004/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.h:325: int
current_media_access_request_id_;
Are we going to need a counter and map for each type of permission, or could one
counter and map be generalized to work for all types of permissions?  (I'm
wondering how much code bloat we're expecting here.)

https://codereview.chromium.org/11093080/diff/123004/content/common/browser_p...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11093080/diff/123004/content/common/browser_p...
content/common/browser_plugin_messages.h:242: // will be granted permission only
if its embedder also has access to media.
I can't tell if this is specific to media or not.  (The first and last sentences
seem to disagree.)

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:139:
IPC_MESSAGE_HANDLER(BrowserPluginMsg_RequestMediaAccess,
nit: Alphabetize

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:569: // window.open CL, so
bubbling is supported.
Is this still planned?

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:964: void
BrowserPlugin::OnListenerCallMediaAccess(int request_id, bool allow) {
Can we find a clearer name for this?  There's no function called MediaAccess to
call.  Is it something like OnEmbedderDecidedMediaAccess?  (Granted/Denied would
be even clearer, but there's no sense in having two functions.)

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:307: void
RespondMediaAccessIfPending(int request_id, bool allow);
How does this differ from the above?  Not sure what the "pending" in the name
refers to.

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:309: static void
WeakCallbackForPersistObject(v8::Persistent<v8::Value> object,
How will these two generalize to other permission types?  One name is specific
to media and the other isn't.

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:379:
base::WeakPtrFactory<BrowserPlugin> weak_ptr_factory_;
Perhaps include a comment saying what this is used for.

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11093080/diff/123004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_bindings.cc:407: const NPVariant*
args,
What kind of protections do we have in place to ensure that this has the right
number of elements, and that args[2] isn't going to read past the end of the
array?  (This is a general question about this approach for bindings.)

[lazyboy, 2013-02-12 05:03:45]

re: generalizing to other permissions: yes Fady and I talked about this (Fady
feels strongly about generalizing too); I've generalized the IPC and v8 related
things in current iteration. But the browser/ side states are still media
specific (map) and non-trivial to generalize, I think we can improve this going
forward.

Couple comments not addressed are related to apps/extension, I will take care of
them as next step.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/renderer/e...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/renderer/e...
chrome/renderer/extensions/dispatcher.cc:698:
source_map_.RegisterSource("webview.experimentalAPI",
On 2013/02/11 22:20:56, creis wrote:
> You'll need someone from extensions to review this name.  Seems fine to me,
but
> I don't know if they would prefer experimental.webview (like the offscreen one
> above).

OK will do once we're happy with the CL.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/renderer/r...
File chrome/renderer/resources/extensions/web_view_experimental_api.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view_experimental_api.js:38: throw
'permissionrequest decision has already been made';
On 2013/02/11 22:20:56, creis wrote:
> This is a string shown to web developers in the exception, right?  Is there a
> convention or review needed for that (e.g., punctuation, etc)?


I'll ask asargent@, he recently added exception messages for app permissions I
think.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view_experimental_api.js:40:
objectNode['-internal-setPermission']('media', requestId, true);
On 2013/02/11 22:20:56, creis wrote:
> Can we check anywhere else that we don't call this repeatedly?  Not sure I
trust
> the page not to bypass this check.
Calling this > 1 times is no-op since the 'pending request' with request_id
should be gone.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js
(right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js:170:
// This test check that not calling e.preventDefault() and not deciding on a
On 2013/02/11 22:20:56, creis wrote:
> Do we have a test somewhere for using preventDefault?

I thought I wrote one, added now, thanks.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/embedder_has_permission/embedder.js:211:
/*
On 2013/02/11 22:20:56, creis wrote:
> Why are these commented out?

Incorrect export, un-commented.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html
(right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html:4:
// A guest that requests for media access.
On 2013/02/11 22:20:56, creis wrote:
> nit: Drop the "for"

Done.

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest2.html
(right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest2.html:1:
<html>
On 2013/02/11 22:20:56, creis wrote:
> How is this file different from the previous one, and why do we need it?  (If
we
> do need it, maybe the name and/or comment should clarify.)
There's something going on here I don't understand, it used to be the case that
I needed a separate guest url that was never allow()ed; allowing once and then
denying wasn't working; but I don't seem to see the behavior (now I don't
remember if it was the trybots). Removed it for now, I'll put it back if
required.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:79:
OnAllowPermissionAccess)
On 2013/02/11 22:20:56, creis wrote:
> Why is "Access" in the name?  Maybe AllowPermission would be better?

Done.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:690: // TODO(lazyboy): We
should use timeouts to clear this pending requests if
On 2013/02/11 22:20:56, creis wrote:
> typo: this pending requests

Done.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:705: void
BrowserPluginGuest::OnAllowPermissionAccess(
On 2013/02/11 22:20:56, creis wrote:
> Again, will this generalize to other types of permissions?

This one is generalized at this point and looks like we can reuse it for others.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.h:325: int
current_media_access_request_id_;
On 2013/02/11 22:20:56, creis wrote:
> Are we going to need a counter and map for each type of permission, or could
one
> counter and map be generalized to work for all types of permissions?  (I'm
> wondering how much code bloat we're expecting here.)

The counter can be one, but different types of permission I've seen so far
requires to store different callbacks/states, and some doesn't require any
(pointer lock). I'm not totally sure at this point, but we can improve it once
we have more than one such requests implemented (much like how
BrowserPluginBindings got generalized).

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/common/br...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/common/br...
content/common/browser_plugin_messages.h:242: // will be granted permission only
if its embedder also has access to media.
On 2013/02/11 22:20:56, creis wrote:
> I can't tell if this is specific to media or not.  (The first and last
sentences
> seem to disagree.)
Updated comment. this is generic now.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:139:
IPC_MESSAGE_HANDLER(BrowserPluginMsg_RequestMediaAccess,
On 2013/02/11 22:20:56, creis wrote:
> nit: Alphabetize

Done.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:569: // window.open CL, so
bubbling is supported.
On 2013/02/11 22:20:56, creis wrote:
> Is this still planned?

Yes.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:964: void
BrowserPlugin::OnListenerCallMediaAccess(int request_id, bool allow) {
On 2013/02/11 22:20:56, creis wrote:
> Can we find a clearer name for this?  There's no function called MediaAccess
to
> call.  Is it something like OnEmbedderDecidedMediaAccess?  (Granted/Denied
would
> be even clearer, but there's no sense in having two functions.)
Changed to OnEmbedderDecidedPermission since now it is generic, thanks.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:307: void
RespondMediaAccessIfPending(int request_id, bool allow);
On 2013/02/11 22:20:56, creis wrote:
> How does this differ from the above?  Not sure what the "pending" in the name
> refers to.
Hmm, this checks if the request is on the pending list and calls Respond*, not
sure how to name this properly.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:309: static void
WeakCallbackForPersistObject(v8::Persistent<v8::Value> object,
On 2013/02/11 22:20:56, creis wrote:
> How will these two generalize to other permission types?  One name is specific
> to media and the other isn't.
I'm sending type from shim, that makes it type independent now.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:379:
base::WeakPtrFactory<BrowserPlugin> weak_ptr_factory_;
On 2013/02/11 22:20:56, creis wrote:
> Perhaps include a comment saying what this is used for.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/123004/content/renderer/...
content/renderer/browser_plugin/browser_plugin_bindings.cc:407: const NPVariant*
args,
On 2013/02/11 22:20:56, creis wrote:
> What kind of protections do we have in place to ensure that this has the right
> number of elements, and that args[2] isn't going to read past the end of the
> array?  (This is a general question about this approach for bindings.)

BrowserPluginBindings::InvokeMethod checks for correct # of args before passing
it to this Invoke();

[Fady Samuel, 2013-02-12 16:13:20]

https://codereview.chromium.org/11093080/diff/113006/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11093080/diff/113006/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:50: const size_t
kNumMaxOutstandingMediaRequests = 1024;
Hmm, I feel like we should be centralizing all these constants as we're
accumulating a lot of constants throughout the BrowserPlugin code. This is fine
for now though, though.

https://codereview.chromium.org/11093080/diff/113006/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:738: void
BrowserPluginGuest::OnAllowPermission(int /*instance_id*/,
Alphabetize with other message handlers for messages from the embedder.

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:541: void
BrowserPlugin::OnRequestPermission(
Alphabetize with other message handlers.

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:547:
OnRequestMediaPermission(request_id, request_info);
How about just RequestMedia permission?

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:559:
pending_permission_request_ids_.insert(request_id);
I'm confused. Why do we have a pending_permission_request_ids_ and
pending_request_id_to_type_?

It seems like pending_request_id_to_type_ subsumes the functionality of
pending_permission_request_ids_?

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:581: // TODO(lazyboy): Fix
before submitting: use ancestor list instead similar to
You can probably copy and paste my code in the window.open CL. I'll hold off on
landing window.open until we land this patch, and I get some more work done on
instance ID allocation.

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:842:
alive_v8_permission_request_objects.find(request_id);
I feel like fundamentally, we want a map that's:

RequestID => RequestInfo

RequestInfo might be a struct with parameters:

1. Type
2. Pointer to a "V8PermissionRequestItem" or whatever we want to call it.

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:856: if
(alive_v8_permission_request_objects.find(id) ==
Early exit preferred.

if (alive_v8_permission_request_objects.find(id) !=
alive_v8_permission_request_objects.end())
  return;

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:876: void
BrowserPlugin::WeakCallbackForPersistObject(
// static

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:964: if (type ==
BrowserPlugin::INVALID) {
This can probably be a switch statement. Only two cases now, but it would make
it clear that more can be added later.

https://codereview.chromium.org/11093080/diff/113006/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:994: void
BrowserPlugin::OnEmbedderDecidedPermission(int request_id, bool allow) {
I'm not too keen on the On* method names for things that aren't message
handlers. What about PermissionResponse?

[lazyboy, 2013-02-12 18:57:51]

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:50: const size_t
kNumMaxOutstandingMediaRequests = 1024;
On 2013/02/12 16:13:20, Fady Samuel wrote:
> Hmm, I feel like we should be centralizing all these constants as we're
> accumulating a lot of constants throughout the BrowserPlugin code. This is
fine
> for now though, though.

OK. As we discussed offline, this might be tricky/ugly to generalize the
browser/ side code, but hopefully doable.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:738: void
BrowserPluginGuest::OnAllowPermission(int /*instance_id*/,
On 2013/02/12 16:13:20, Fady Samuel wrote:
> Alphabetize with other message handlers for messages from the embedder.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:541: void
BrowserPlugin::OnRequestPermission(
On 2013/02/12 16:13:20, Fady Samuel wrote:
> Alphabetize with other message handlers.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:547:
OnRequestMediaPermission(request_id, request_info);
On 2013/02/12 16:13:20, Fady Samuel wrote:
> How about just RequestMedia permission?

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:559:
pending_permission_request_ids_.insert(request_id);
On 2013/02/12 16:13:20, Fady Samuel wrote:
> I'm confused. Why do we have a pending_permission_request_ids_ and
> pending_request_id_to_type_?
> 
> It seems like pending_request_id_to_type_ subsumes the functionality of
> pending_permission_request_ids_?

Merged them, thanks.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:581: // TODO(lazyboy): Fix
before submitting: use ancestor list instead similar to
On 2013/02/12 16:13:20, Fady Samuel wrote:
> You can probably copy and paste my code in the window.open CL. I'll hold off
on
> landing window.open until we land this patch, and I get some more work done on
> instance ID allocation.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:842:
alive_v8_permission_request_objects.find(request_id);
On 2013/02/12 16:13:20, Fady Samuel wrote:
> I feel like fundamentally, we want a map that's:
> 
> RequestID => RequestInfo
> 
> RequestInfo might be a struct with parameters:
> 
> 1. Type
> 2. Pointer to a "V8PermissionRequestItem" or whatever we want to call it.

The V8_*_requests container is not parallel to the pending_permission_requests_
container. The pending_permission_requests_ container has superset of
requests/request_ids, so storing type in this map is redundant.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:856: if
(alive_v8_permission_request_objects.find(id) ==
On 2013/02/12 16:13:20, Fady Samuel wrote:
> Early exit preferred.
> 
> if (alive_v8_permission_request_objects.find(id) !=
> alive_v8_permission_request_objects.end())
>   return;

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:876: void
BrowserPlugin::WeakCallbackForPersistObject(
On 2013/02/12 16:13:20, Fady Samuel wrote:
> // static

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:964: if (type ==
BrowserPlugin::INVALID) {
On 2013/02/12 16:13:20, Fady Samuel wrote:
> This can probably be a switch statement. Only two cases now, but it would make
> it clear that more can be added later.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/113006/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:994: void
BrowserPlugin::OnEmbedderDecidedPermission(int request_id, bool allow) {
On 2013/02/12 16:13:20, Fady Samuel wrote:
> I'm not too keen on the On* method names for things that aren't message
> handlers. What about PermissionResponse? 

I see. But I think this name makes it clearer to understand think, keeping it as
is for now.

[Fady Samuel, 2013-02-12 19:13:09]

Just a few nits until Charlie gives this another look.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:541: void
BrowserPlugin::OnRequestPermission(
It looks like this should be placed after OnLoadStop.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:546: if (permission_type ==
"media")
"media" should probably be a constant.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:312: void
OnRequestPermission(int instance_id,
nit: Put in alphabetical order with the message handlers above.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:316: // Requests media access
permission from the embedder.
nit: The methods below are not message handlers. Move them above message
handlers.

[lazyboy, 2013-02-12 22:14:33]

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:541: void
BrowserPlugin::OnRequestPermission(
On 2013/02/12 19:13:09, Fady Samuel wrote:
> It looks like this should be placed after OnLoadStop.

It is.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:546: if (permission_type ==
"media")
On 2013/02/12 19:13:09, Fady Samuel wrote:
> "media" should probably be a constant.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:312: void
OnRequestPermission(int instance_id,
On 2013/02/12 19:13:09, Fady Samuel wrote:
> nit: Put in alphabetical order with the message handlers above.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/116009/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:316: // Requests media access
permission from the embedder.
On 2013/02/12 19:13:09, Fady Samuel wrote:
> nit: The methods below are not message handlers. Move them above message
> handlers.

Done.

[Charlie Reis, 2013-02-13 01:44:47]

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:750: // decision has not
been made.
I thought we were watching for when event.request was garbage collected.  Is
there a need for a timer?

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.h:162: 
nit: Remove extra line.

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:393: void
BrowserPlugin::PopulateAncestorList() {
Please have a WebKit expert (perhaps abarth) review this and how it's used.

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:121: // A request from shim to
track lifetime of a js object related to media
nit: js -> JavaScript

Also, we probably shouldn't assume that the webview shim (in chrome) is the only
user of the BrowserPlugin.  Maybe shim -> content client?

Also, is this something specific to media, or should the comment be generalized?

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:286: void
RespondPermission(PermissionRequestType type,
This name (and the one below) feels awkward to me.  Should it be
RespondToPermissionRequest?  Or even something like AllowPermission?

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:402:
base::WeakPtrFactory<BrowserPlugin> weak_ptr_factory_;
Please have someone familiar with V8 review all this logic for the MakeWeak
callback, since I'm not knowledgable here.  Perhaps abarth?

[lazyboy, 2013-02-13 04:07:47]

I've rewritten tests to mock out requesting for media devices, trybots are
expected to pass now (fingers crossed).

So far I have 4 issues that are not addressed:
a. [abarth] v8 persistent MakeWeak.
b. [abarth] check if the way to read the ancestor nodes of plugin is OK.
c. [extensions_team] check what to call the web_view_experimental_api.js file's
resource name.
d. [extensions_team] check the exception message's language since this is
developer facing

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:750: // decision has not
been made.
On 2013/02/13 01:44:47, creis wrote:
> I thought we were watching for when event.request was garbage collected.  Is
> there a need for a timer?

yes (Fady also had the same comment), this should not be required anymore,
removed.

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.h:162: 
On 2013/02/13 01:44:47, creis wrote:
> nit: Remove extra line.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:393: void
BrowserPlugin::PopulateAncestorList() {
On 2013/02/13 01:44:47, creis wrote:
> Please have a WebKit expert (perhaps abarth) review this and how it's used.
Yes, I'll do that once you're OK with the CL.

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:121: // A request from shim to
track lifetime of a js object related to media
On 2013/02/13 01:44:47, creis wrote:
> nit: js -> JavaScript
> 
> Also, we probably shouldn't assume that the webview shim (in chrome) is the
only
> user of the BrowserPlugin.  Maybe shim -> content client?
> 
> Also, is this something specific to media, or should the comment be
generalized?

Updated comment.
This is already generalized, stale comments corrected.

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:286: void
RespondPermission(PermissionRequestType type,
On 2013/02/13 01:44:47, creis wrote:
> This name (and the one below) feels awkward to me.  Should it be
> RespondToPermissionRequest?  Or even something like AllowPermission?
Renamed to
AllowPermission
and
AllowPermissionIfRequestIsPending

https://chromiumcodereview.appspot.com/11093080/diff/120008/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:402:
base::WeakPtrFactory<BrowserPlugin> weak_ptr_factory_;
On 2013/02/13 01:44:47, creis wrote:
> Please have someone familiar with V8 review all this logic for the MakeWeak
> callback, since I'm not knowledgable here.  Perhaps abarth?
Yes, my plan is to get this reviewed by abarth@ after you're happy with the CL.

[Charlie Reis, 2013-02-13 21:08:05]

Ok, LGTM with the nits below.  Please make sure http://crbug.com/175230 gets
fixed before landing this, along with the other review issues you mentioned.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
chrome/browser/extensions/web_view_browsertest.cc:32: // We do not issue the
actual media request, the fact that the request reached
nit: semicolon.  :)

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
chrome/browser/extensions/web_view_browsertest.cc:731:
ASSERT_TRUE(embedder_web_contents != NULL);
nit: Don't need the != NULL.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
chrome/browser/extensions/web_view_browsertest.cc:795: 
nit: Extra line.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html
(right):

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html:7:
window.console.log('guest starts test ...');
Why are there three?  Do you need this at all?

https://chromiumcodereview.appspot.com/11093080/diff/127001/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/127001/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:290: void
AllowPermissionIfRequestIsPending(int request_id, bool allow);
Please add comments for these three.

[lazyboy, 2013-02-13 22:17:34]

+abarth for v8 Persistent.MakeWeak changes and checking security issues with dom
traversal, specifically:

a)
browser_plugin.cc: 
BrowserPlugin::PersistRequestObject() (called from web_view_experimental_api.js
and browser_plugin_bindings.cc)
and
BrowserPlugin::WeakCallbackForPersistObject()

b)
browser_plugin.cc: 
BrowserPlugin::PopulateAncestorList()/HaseventListeners()


+mpcomplete for extensions:
We are adding API implementation for web_view.js on experimental, which is
loaded from web_view_experimental_api.js
a)
https://chromiumcodereview.appspot.com/11093080/patch/137002/127021
names it "webview.experimentalAPI", what is the naming convention here?
"experimental.webview"?
b)
We also throw js exception which has developer facing message: 
https://chromiumcodereview.appspot.com/11093080/patch/137002/127023
'permissionrequest decision has already been made'
Is that good enough or do we need to provide more (and formal) context?

Thanks.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
chrome/browser/extensions/web_view_browsertest.cc:32: // We do not issue the
actual media request, the fact that the request reached
On 2013/02/13 21:08:05, creis wrote:
> nit: semicolon.  :)

Done.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
chrome/browser/extensions/web_view_browsertest.cc:731:
ASSERT_TRUE(embedder_web_contents != NULL);
On 2013/02/13 21:08:05, creis wrote:
> nit: Don't need the != NULL.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/browser/ex...
chrome/browser/extensions/web_view_browsertest.cc:795: 
On 2013/02/13 21:08:05, creis wrote:
> nit: Extra line.
Removed.

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/test/data/...
File
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html
(right):

https://chromiumcodereview.appspot.com/11093080/diff/127001/chrome/test/data/...
chrome/test/data/extensions/platform_apps/web_view/media_access/media_access_guest.html:7:
window.console.log('guest starts test ...');
On 2013/02/13 21:08:05, creis wrote:
> Why are there three?  Do you need this at all?

Ah test function, removed.

https://chromiumcodereview.appspot.com/11093080/diff/127001/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/127001/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:290: void
AllowPermissionIfRequestIsPending(int request_id, bool allow);
On 2013/02/13 21:08:05, creis wrote:
> Please add comments for these three.

Done.

[Matt Perry, 2013-02-13 23:20:08]

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/e...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/e...
chrome/renderer/extensions/dispatcher.cc:834:
extension->HasAPIPermission(APIPermission::kExperimental)) {
We actually deprecated the "experimental" permission. Instead, we use
chrome/common/extensions/api/_permission_features.json to restrict certain APIs
to the "dev" channel. Once it's not an experiment anymore, then we release it to
the stable channel.

But after thinking about this, it seems like it won't work for your use case. We
want "webview" to have additional features on dev-channel only. There's no
official support for that, but you can use
  |Feature::GetCurrentChannel() < chrome::VersionInfo::CHANNEL_DEV|
for now. If that's true, automatically add "webview.experimental" (I'd drop the
"API").

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/r...
File chrome/renderer/resources/extensions/web_view_experimental_api.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view_experimental_api.js:41: throw
'permissionrequest decision has already been made';
throw new Error(<error string>)

also, use a full sentence and maybe a bit more context. The current error is
cryptic to me.

[lazyboy, 2013-02-14 00:09:46]

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/e...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/e...
chrome/renderer/extensions/dispatcher.cc:834:
extension->HasAPIPermission(APIPermission::kExperimental)) {
On 2013/02/13 23:20:08, Matt Perry wrote:
> We actually deprecated the "experimental" permission. Instead, we use
> chrome/common/extensions/api/_permission_features.json to restrict certain
APIs
> to the "dev" channel. Once it's not an experiment anymore, then we release it
to
> the stable channel.
> 
> But after thinking about this, it seems like it won't work for your use case.
We
> want "webview" to have additional features on dev-channel only. There's no
> official support for that, but you can use
>   |Feature::GetCurrentChannel() < chrome::VersionInfo::CHANNEL_DEV|
> for now. If that's true, automatically add "webview.experimental" (I'd drop
the
> "API").
OK thanks.
btw, I'm assuming you meant <= CHANNEL_DEV.
Done.

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/r...
File chrome/renderer/resources/extensions/web_view_experimental_api.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view_experimental_api.js:41: throw
'permissionrequest decision has already been made';
On 2013/02/13 23:20:08, Matt Perry wrote:
> throw new Error(<error string>)
> 
> also, use a full sentence and maybe a bit more context. The current error is
> cryptic to me.

Done.

[Fady Samuel, 2013-02-14 00:11:36]

Just a small nit until abarth@ reviews.

https://codereview.chromium.org/11093080/diff/138001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/11093080/diff/138001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view_experimental.js:21: 'Permission
has already been decided for this "permissionrequest" event';
nit: Period at the end of the sentence.

[lazyboy, 2013-02-14 00:13:56]

https://codereview.chromium.org/11093080/diff/138001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/11093080/diff/138001/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view_experimental.js:21: 'Permission
has already been decided for this "permissionrequest" event';
On 2013/02/14 00:11:36, Fady Samuel wrote:
> nit: Period at the end of the sentence.

Done.

[Matt Perry, 2013-02-14 00:22:46]

lgtm

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/e...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/137002/chrome/renderer/e...
chrome/renderer/extensions/dispatcher.cc:834:
extension->HasAPIPermission(APIPermission::kExperimental)) {
On 2013/02/14 00:09:46, lazyboy wrote:
> On 2013/02/13 23:20:08, Matt Perry wrote:
> > We actually deprecated the "experimental" permission. Instead, we use
> > chrome/common/extensions/api/_permission_features.json to restrict certain
> APIs
> > to the "dev" channel. Once it's not an experiment anymore, then we release
it
> to
> > the stable channel.
> > 
> > But after thinking about this, it seems like it won't work for your use
case.
> We
> > want "webview" to have additional features on dev-channel only. There's no
> > official support for that, but you can use
> >   |Feature::GetCurrentChannel() < chrome::VersionInfo::CHANNEL_DEV|
> > for now. If that's true, automatically add "webview.experimental" (I'd drop
> the
> > "API").
> OK thanks.
> btw, I'm assuming you meant <= CHANNEL_DEV.
> Done.

Woops, good catch.

[abarth-chromium, 2013-02-14 18:56:53]

Some comments below.  I'll try to give you more comments later today.

The general approach looks fine.  Please don't feel obligated to wait for me. 
We can address any details in a followup patch if you like.

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:861: return;
Why do you both DCHECK and handle this case?  It seems like you should do one or
the other but not both.

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:879: if (result.second) {
Again, you're handling the case of the DCHECK failing.  In this case, you'll
create a big memory leak.  It's better to crash and get stack traces.  Perhaps
we should use CHECK rather than DCHECK?

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:899:
persistent_object.Dispose();
We usually call Clear() too.  I'm not sure if that's needed.

An important thing to know about these weak callbacks is that they can happen
any time GC runs, which can be any time you're inside V8.  That means you need
to be prepared for re-entrancy.  In particular, OnRequestObjectGarbageCollected
looks like it modifies a map.  That means you can never enter V8 while you're
iterating that map.  Otherwise, you might trigger GC and invalidate the map.

[abarth-chromium, 2013-02-14 20:46:45]

Maybe a good approach here is to call OnRequestObjectGarbageCollected
asynchronously?  That will solve the re-entrancy problem.

[lazyboy, 2013-02-14 21:03:56]

On 2013/02/14 20:46:45, abarth wrote:
> Maybe a good approach here is to call OnRequestObjectGarbageCollected
> asynchronously?  That will solve the re-entrancy problem.

OK I will do that;
Out of curiosity, if we don't iterate the map, then would this code be
re-entrant safe? more specifically can map.find()/erase()/insert()  be thought
as atomic op in this context?

Also do you see any issue with node traversal in PopulateAncestorList()?

[abarth-chromium, 2013-02-14 21:15:50]

> Out of curiosity, if we don't iterate the map, then would this code be
> re-entrant safe? more specifically can map.find()/erase()/insert()  be thought
> as atomic op in this context?

I didn't chase down all the work you're doing inside the weak callback to see if
it was safe.  Mostly I mention it because it wasn't obvious to me when I started
writing weak callbacks and it's something that still bites us from time to time.

> Also do you see any issue with node traversal in PopulateAncestorList()?

It was a little worrying that you needed to walk the ancestor list.  I didn't
study it closely enough to understand why you needed to do that.

[lazyboy, 2013-02-14 21:47:19]

"It was a little worrying that you needed to walk the ancestor list.  I didn't
study it closely enough to understand why you needed to do that."

We want to check if plugin or any ancestors of the plugin has any listener
registered for the permissionrequest event. This is so that bubbling works, I
can separate this change out if this is risky and needs discussion; we then just
won't support bubbling with this CL.

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:861: return;
On 2013/02/14 18:56:53, abarth wrote:
> Why do you both DCHECK and handle this case?  It seems like you should do one
or
> the other but not both.
The if () {...} part is what we want, so a request to persist an object more
than once is ignored. If javascript accidentally calls this > once then we fail
on debug build so we can catch potential implementation bugs. (This is expected
to be called once). Is this a bad practice? I think I've seen code in chromium
using this pattern.

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:879: if (result.second) {
On 2013/02/14 18:56:53, abarth wrote:
> Again, you're handling the case of the DCHECK failing.  In this case, you'll
> create a big memory leak.  It's better to crash and get stack traces.  Perhaps
> we should use CHECK rather than DCHECK?

CHECK is good here.
Done.

https://codereview.chromium.org/11093080/diff/133004/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:899:
persistent_object.Dispose();
On 2013/02/14 18:56:53, abarth wrote:
> We usually call Clear() too.  I'm not sure if that's needed.
> 
> An important thing to know about these weak callbacks is that they can happen
> any time GC runs, which can be any time you're inside V8.  That means you need
> to be prepared for re-entrancy.  In particular,
OnRequestObjectGarbageCollected
> looks like it modifies a map.  That means you can never enter V8 while you're
> iterating that map.  Otherwise, you might trigger GC and invalidate the map.
Added Clear().
Made OnRequestObjectGarbaggeCollected async.

[Fady Samuel, 2013-02-14 21:54:45]

https://codereview.chromium.org/11093080/diff/131013/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/131013/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:904:
MessageLoop::current()->PostTask(
Hmm, the obligatory "What happens if you remove BrowserPlugin from the DOM?"
question. :-P Fun! Fun! Fun!

Suppose the BrowserPlugin was scheduled to be removed from the DOM but hasn't
yet been removed (BrowserPlugin::destroy() was called).

Couldn't we be scheduling the call to
BrowserPlugin::OnRequestObjectGarbageCollected after
BrowserPlugin::~BrowserPlugin?

We should probably be checking whether this BrowserPlugin is scheduled to be
deleted before scheduling more tasks.

[abarth-chromium, 2013-02-14 21:59:23]

> We want to check if plugin or any ancestors of the plugin has any listener
> registered for the permissionrequest event. This is so that bubbling works, I
> can separate this change out if this is risky and needs discussion; we then
just
> won't support bubbling with this CL.

Does that mean you're re-implementing the event dispatch algorithm?  I should
study this aspect of the change more carefully.

[lazyboy, 2013-02-14 22:06:01]

On 2013/02/14 21:59:23, abarth wrote:
> > We want to check if plugin or any ancestors of the plugin has any listener
> > registered for the permissionrequest event. This is so that bubbling works,
I
> > can separate this change out if this is risky and needs discussion; we then
> just
> > won't support bubbling with this CL.
> 
> Does that mean you're re-implementing the event dispatch algorithm?  I should
> study this aspect of the change more carefully.
No. We need to check this because if there was no event listeners registered for
permissionrequest then we immediately deny the permission request. The event
dispatch mechanism stays the same.

[lazyboy, 2013-02-14 23:04:01]

On 2013/02/14 21:54:45, Fady Samuel wrote:
>
https://codereview.chromium.org/11093080/diff/131013/content/renderer/browser...
> File content/renderer/browser_plugin/browser_plugin.cc (right):
> 
>
https://codereview.chromium.org/11093080/diff/131013/content/renderer/browser...
> content/renderer/browser_plugin/browser_plugin.cc:904:
> MessageLoop::current()->PostTask(
> Hmm, the obligatory "What happens if you remove BrowserPlugin from the DOM?"
> question. :-P Fun! Fun! Fun!
> 
> Suppose the BrowserPlugin was scheduled to be removed from the DOM but hasn't
> yet been removed (BrowserPlugin::destroy() was called).
> 
> Couldn't we be scheduling the call to
> BrowserPlugin::OnRequestObjectGarbageCollected after
> BrowserPlugin::~BrowserPlugin?
> 
We should probably be checking whether this BrowserPlugin is scheduled to be
deleted before scheduling more tasks.

As discussed and checked offline, we should be fine here since we are using weak
pointer, also added a note according to your suggestion.

[Fady Samuel, 2013-02-14 23:17:35]

On 2013/02/14 23:04:01, lazyboy wrote:
> On 2013/02/14 21:54:45, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/11093080/diff/131013/content/renderer/browser...
> > File content/renderer/browser_plugin/browser_plugin.cc (right):
> > 
> >
>
https://codereview.chromium.org/11093080/diff/131013/content/renderer/browser...
> > content/renderer/browser_plugin/browser_plugin.cc:904:
> > MessageLoop::current()->PostTask(
> > Hmm, the obligatory "What happens if you remove BrowserPlugin from the DOM?"
> > question. :-P Fun! Fun! Fun!
> > 
> > Suppose the BrowserPlugin was scheduled to be removed from the DOM but
hasn't
> > yet been removed (BrowserPlugin::destroy() was called).
> > 
> > Couldn't we be scheduling the call to
> > BrowserPlugin::OnRequestObjectGarbageCollected after
> > BrowserPlugin::~BrowserPlugin?
> > 
> We should probably be checking whether this BrowserPlugin is scheduled to be
> deleted before scheduling more tasks.
> 
> As discussed and checked offline, we should be fine here since we are using
weak
> pointer, also added a note according to your suggestion.

Very cool that PostTask automagically does this!

[lazyboy, 2013-02-19 16:21:23]

Adding Cris for reviewing browser_plugin_messages.h while abarth@ is reviewing
the other bits.

[abarth-chromium, 2013-02-19 17:44:12]

Thanks for addressing my comments.

[Cris Neckar, 2013-02-19 19:40:56]

https://codereview.chromium.org/11093080/diff/152001/content/common/browser_p...
File content/common/browser_plugin_messages.h (right):

https://codereview.chromium.org/11093080/diff/152001/content/common/browser_p...
content/common/browser_plugin_messages.h:248: std::string /* permission_type */,
It seems like this should be an enum rather than a string. Is there a particular
reason you are mapping these to string constants?

https://codereview.chromium.org/11093080/diff/152001/content/common/browser_p...
content/common/browser_plugin_messages.h:384: std::string /* permission_type */,
ditto

[lazyboy, 2013-02-19 21:38:40]

https://chromiumcodereview.appspot.com/11093080/diff/152001/content/common/br...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/152001/content/common/br...
content/common/browser_plugin_messages.h:248: std::string /* permission_type */,
On 2013/02/19 19:40:56, Cris Neckar wrote:
> It seems like this should be an enum rather than a string. Is there a
particular
> reason you are mapping these to string constants?

Changed to enum.

https://chromiumcodereview.appspot.com/11093080/diff/152001/content/common/br...
content/common/browser_plugin_messages.h:384: std::string /* permission_type */,
On 2013/02/19 19:40:56, Cris Neckar wrote:
> ditto

Done.

[Fady Samuel, 2013-02-19 21:44:33]

https://chromiumcodereview.appspot.com/11093080/diff/156001/content/common/br...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/156001/content/common/br...
content/common/browser_plugin_messages.h:35: enum BrowserPluginPermissionType {
This seems messy. Could we move this to another file? See AccessibilityMode in
view_messages as an example.

[lazyboy, 2013-02-19 22:08:47]

Added new file, PTAL.

https://chromiumcodereview.appspot.com/11093080/diff/156001/content/common/br...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/156001/content/common/br...
content/common/browser_plugin_messages.h:35: enum BrowserPluginPermissionType {
On 2013/02/19 21:44:33, Fady Samuel wrote:
> This seems messy. Could we move this to another file? See AccessibilityMode in
> view_messages as an example.

Done.

[Cris Neckar, 2013-02-20 20:10:24]

ipc changes lgtm

[abarth-chromium, 2013-02-20 20:46:43]

This seems fine.  I think you've got some memory leaks, however.

https://codereview.chromium.org/11093080/diff/158031/chrome/renderer/resource...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://codereview.chromium.org/11093080/diff/158031/chrome/renderer/resource...
chrome/renderer/resources/extensions/web_view_experimental.js:49:
objectNode['-internal-setPermission'](requestId, true);
Do you intend to call this function with |this| set to objectNode?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:564: AllowPermission(
It's confusing that you're using a function called "allow permission" to deny
the permission.

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:565:
BrowserPluginPermissionTypeMedia, request_id, false /* allow */);
It's confusing that you're using a function called "AllowPermission" to deny the
permission.

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:578:
base::Value::CreateIntegerValue(request_id);
Does this leak memory?  If not, where is this memory freed?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:594:
WebKit::WebString::fromUTF8(event_name))) {
It seems like we should create the WebString once instead of for each step in
the ancestor chain.

I think you're going to have to get rid of ancestors_ and walk the ancestor
chain directly here.

I'm not this it makes sense to check whether the ancestors have event listeners.
 That seems like a premature performance optimization.

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:868: }
Why are you DCHECKing a condition and then handling it's negation?  Perhaps we
should use CHECK rather than DCHECK?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:899: delete item_ptr;
I'm sad we need to manually manage memory like this.  Does Chromium really not
have better tools for memory management?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:997: break;
Should we have a DCHECK or a CHECK here?  How bad an error is this?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:123: // A request from content
client to track lifetime of a Javascript object
Javascript -> JavaScript

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:280: const
base::DictionaryValue& request_info);
bad indent

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:292: // object goes out of
scope in Javascript.
Javascript -> JavaScript.

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.h:410:
std::vector<WebKit::WebNode> ancestors_;
Doesn't this create reference cycles?  Normally it's not a good idea to store a
RefPtr to your ancestors.

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_bindings.cc:348: 3) {
3 ?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_bindings.cc:355: &args[0],
StringFromNPVariant(args[1]), Int32FromNPVariant(args[2]));
Doesn't &args[0] == args?

https://codereview.chromium.org/11093080/diff/158031/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin_bindings.cc:403:
browser_plugin::kMethodInternalSetPermission, 2) {
I see.  These magic numbers must be the number of parameters.

[lazyboy, 2013-02-20 22:09:35]

https://chromiumcodereview.appspot.com/11093080/diff/158031/chrome/renderer/r...
File chrome/renderer/resources/extensions/web_view_experimental.js (right):

https://chromiumcodereview.appspot.com/11093080/diff/158031/chrome/renderer/r...
chrome/renderer/resources/extensions/web_view_experimental.js:49:
objectNode['-internal-setPermission'](requestId, true);
On 2013/02/20 20:46:44, abarth wrote:
> Do you intend to call this function with |this| set to objectNode?

Did you mean to set |this| to null?
objectNode['-internal-setPermission'].call(null, requestId, true)

If so, I'm getting "NPMethod called on non-NPObject" error.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:564: AllowPermission(
On 2013/02/20 20:46:44, abarth wrote:
> It's confusing that you're using a function called "allow permission" to deny
> the permission.
changed to "DecidePermission".

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:565:
BrowserPluginPermissionTypeMedia, request_id, false /* allow */);
On 2013/02/20 20:46:44, abarth wrote:
> It's confusing that you're using a function called "AllowPermission" to deny
the
> permission.
ditto.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:578:
base::Value::CreateIntegerValue(request_id);
On 2013/02/20 20:46:44, abarth wrote:
> Does this leak memory?  If not, where is this memory freed?

Seems we have this leak in all places where we're calling TriggerEvent, let me
fix that in a separate smaller patch.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:594:
WebKit::WebString::fromUTF8(event_name))) {
On 2013/02/20 20:46:44, abarth wrote:
> It seems like we should create the WebString once instead of for each step in
> the ancestor chain.
> 
> I think you're going to have to get rid of ancestors_ and walk the ancestor
> chain directly here.
> 
> I'm not this it makes sense to check whether the ancestors have event
listeners.
>  That seems like a premature performance optimization.
Fixed WebString creation inside loop.
Removed ancestors_.

"I'm not this it makes sense to check whether the ancestors have event
listeners." ->
If we want to make this event bubble-able so any ancestor can also register for
this event; would we not have to do that?

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:868: }
On 2013/02/20 20:46:44, abarth wrote:
> Why are you DCHECKing a condition and then handling it's negation?  Perhaps we
> should use CHECK rather than DCHECK?
Changed to CHECK.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:899: delete item_ptr;
On 2013/02/20 20:46:44, abarth wrote:
> I'm sad we need to manually manage memory like this.  Does Chromium really not
> have better tools for memory management?
I'm not sure, the closest I could find was STLValueDeleter/STLElementDeleter in
stl_util.h, which doesn't fit our requirement here.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:997: break;
On 2013/02/20 20:46:44, abarth wrote:
> Should we have a DCHECK or a CHECK here?  How bad an error is this?
Added DCHECK.
This function is "supposed to" be accessible from web_view.js only, so an
incorrect permission_type is wrong, but should not be harmful; DCHECK is better
in these case, right?

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:123: // A request from content
client to track lifetime of a Javascript object
On 2013/02/20 20:46:44, abarth wrote:
> Javascript -> JavaScript

Done.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:280: const
base::DictionaryValue& request_info);
On 2013/02/20 20:46:44, abarth wrote:
> bad indent

Done.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:292: // object goes out of
scope in Javascript.
On 2013/02/20 20:46:44, abarth wrote:
> Javascript -> JavaScript.

Done.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.h:410:
std::vector<WebKit::WebNode> ancestors_;
On 2013/02/20 20:46:44, abarth wrote:
> Doesn't this create reference cycles?  Normally it's not a good idea to store
a
> RefPtr to your ancestors.
Removed ancestors_.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin_bindings.cc:355: &args[0],
StringFromNPVariant(args[1]), Int32FromNPVariant(args[2]));
On 2013/02/20 20:46:44, abarth wrote:
> Doesn't &args[0] == args?

Changed to args.

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin_bindings.cc:403:
browser_plugin::kMethodInternalSetPermission, 2) {
On 2013/02/20 20:46:44, abarth wrote:
> I see.  These magic numbers must be the number of parameters.
Yes.

[lazyboy, 2013-02-20 23:15:33]

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
content/renderer/browser_plugin/browser_plugin.cc:578:
base::Value::CreateIntegerValue(request_id);
On 2013/02/20 22:09:36, lazyboy wrote:
> On 2013/02/20 20:46:44, abarth wrote:
> > Does this leak memory?  If not, where is this memory freed?
> 
> Seems we have this leak in all places where we're calling TriggerEvent, let me
> fix that in a separate smaller patch.

I was wrong on this one, TriggerEvent() already frees up the memory by creating
DictionaryValue from |props|.

[lazyboy, 2013-02-22 17:05:04]

On 2013/02/20 23:15:33, lazyboy wrote:
>
https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
> File content/renderer/browser_plugin/browser_plugin.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11093080/diff/158031/content/renderer/...
> content/renderer/browser_plugin/browser_plugin.cc:578:
> base::Value::CreateIntegerValue(request_id);
> On 2013/02/20 22:09:36, lazyboy wrote:
> > On 2013/02/20 20:46:44, abarth wrote:
> > > Does this leak memory?  If not, where is this memory freed?
> > 
> > Seems we have this leak in all places where we're calling TriggerEvent, let
me
> > fix that in a separate smaller patch.
> 
> I was wrong on this one, TriggerEvent() already frees up the memory by
creating
> DictionaryValue from |props|.

@abarth, can you take a look again? Thanks.

[Fady Samuel, 2013-02-22 18:39:51]

https://codereview.chromium.org/11093080/diff/167002/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/167002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:891: if (plugin) {
What's the point of the if statement given the comment below?

[lazyboy, 2013-02-22 19:14:43]

https://codereview.chromium.org/11093080/diff/167002/content/renderer/browser...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11093080/diff/167002/content/renderer/browser...
content/renderer/browser_plugin/browser_plugin.cc:891: if (plugin) {
On 2013/02/22 18:39:51, Fady Samuel wrote:
> What's the point of the if statement given the comment below?
If the weak pointer is pointing to invalid BrowserPlugin* at this point, this
won't try to PostTask.
The comment below is for case where |plugin| is valid at this point but not
valid when the post task is executed.
Also from weak_ptr.h: "You should always null-test an object of this class
before using it..."

[abarth-chromium, 2013-02-22 20:49:37]

I'm sorry that I don't have time to review this patch again.

[lazyboy, 2013-02-27 21:15:30]

+ben@ for chrome/* changes, specifically:
** Presubmit Messages **
Missing OWNER reviewers for these files:
    chrome/chrome_renderer.gypi
    chrome/renderer/resources/renderer_resources.grd

I'll go ahead with this CL as the concerns have been/being addressed:
1. Disable instantiating plugin <object> directly: crrev.com/12224126 (needs to
be submitted first).
2. The two concerns in browser_plugin.cc has been addressed from abarth@'s
review: MakeWeakPtr and traversing ancestors.
Ty.

[Ben Goodger (Google), 2013-02-28 17:49:13]

lgtm

[commit-bot: I haz the power, 2013-03-01 05:59:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/11093080/180001

[commit-bot: I haz the power, 2013-03-01 05:59:25]

Presubmit check for 11093080-180001 failed and returned exit status 1.

INFO:root:Found 27 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  content/common/browser_plugin_message_enums.h

Presubmit checks took 3.6s to calculate.

[commit-bot: I haz the power, 2013-03-01 06:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/11093080/179002

[commit-bot: I haz the power, 2013-03-01 06:29:36]

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

[commit-bot: I haz the power, 2013-03-01 06:38:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/11093080/192001

[commit-bot: I haz the power, 2013-03-01 09:36:47]

Change committed as 185503