Dismiss EME infobar when WebMediaPlayer is destroyed.

Dismiss EME infobar when WebMediaPlayer is destroyed.

The EME infobar should be dismissed when it was requested
in an iframe and it is navigated away.

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

[Kibeom Kim (inactive), 2013-12-20 12:54:58]

qinmin@, ddorwin@ -- could you take a look at content/*/media/android/* ? Other
files are trivial changes so I will add reviewers later for those.

[ddorwin, 2013-12-20 23:59:51]

Lots of comments on the APIs and how they fit with the APIs in the spec.

+xhwang for more input.

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:786: return;
I'm surprised we don't already fire some event here. xhwang, WDYT?

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:793: return;
Hmm, I see how this case makes using CreateSession() for the success case
(suggested later) a problem. I still think the advantages of the suggestion are
appealing.

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:146: void
OnCancelSession(int media_keys_id, uint32 session_id);
These other *Session methods all have spec equivalents. This serves a very
different purpose. It should be something like OnCancelSessionRequest or
OnCancelCreateSession.

https://codereview.chromium.org/107413006/diff/100001/content/common/media/me...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/common/media/me...
content/common/media/media_player_messages_android.h:305:
IPC_MESSAGE_ROUTED2(MediaKeysHostMsg_CancelSession,
ditto

https://codereview.chromium.org/107413006/diff/100001/content/common/media/me...
content/common/media/media_player_messages_android.h:329:
IPC_MESSAGE_ROUTED3(MediaKeysMsg_SessionApproved,
Can we think of a better name than SessionApproved? SessionCreationAllowed?

It's weird naming - the name is positive but a parameter can make the meaning
negative. This is another reason why making this message the failure case (see
later) is appealing.

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:75:
requested_session_id_.erase(session_id);
You probably don't need this here. See last comment below.

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:95:
requested_session_id_.erase(session_id);
I can't find documentation, but I assume this is a nop if session_id is not
found. Is that right?

Better yet, we should never need to erase here (or line 75) because these should
not be called until the session has been created. You can just convert these to
a DCHECK(find() == end()).

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:99: void
ProxyMediaKeys::OnSessionApproved(uint32 session_id, bool success) {
Can we just use OnSessionCreated for success and have a OnSessionCreateDenied or
whatever for failure?
 * One or the other will always be called, which mans we can do all erasing in
those two.
 * For implementations that don't involve approval, they don't need to fire this
event. (However, they'd still need to do something to erase the session_id. That
might be a general problem we need to solve. Maybe this should be
OnSessionCreationFailed and called in all those cases. Actually, we call
OnSessionError in one of those cases - maybe we can use that in the not-allowed
case too. xhwang, WDYT?)
 * The naming works better (currently success=false turns "Approved" into not
approved).

The Release and Closed methods are somewhat arbitrary for erasing anyway.

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:65: base::hash_set<uint32>
requested_session_id_;
maybe session_ids_pending_create_ per later discussion.

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/renderer_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/renderer_media_player_manager.h:104: void
CancelSession(int media_keys_id, uint32 session_id);
ditto

[xhwang, 2013-12-21 00:40:48]

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:776: void
BrowserMediaPlayerManager::GenerateKeyIfAllowed(
This now should be renamed to CreateSessionIfAllowed()

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:786: return;
On 2013/12/20 23:59:52, ddorwin wrote:
> I'm surprised we don't already fire some event here. xhwang, WDYT?

Yeah, we should call OnSessionError() before we return, same as l.791.

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:99: void
ProxyMediaKeys::OnSessionApproved(uint32 session_id, bool success) {
On 2013/12/20 23:59:52, ddorwin wrote:
> Can we just use OnSessionCreated for success and have a OnSessionCreateDenied
or
> whatever for failure?
>  * One or the other will always be called, which mans we can do all erasing in
> those two.
>  * For implementations that don't involve approval, they don't need to fire
this
> event. (However, they'd still need to do something to erase the session_id.
That
> might be a general problem we need to solve. Maybe this should be
> OnSessionCreationFailed and called in all those cases. Actually, we call
> OnSessionError in one of those cases - maybe we can use that in the
not-allowed
> case too. xhwang, WDYT?)
>  * The naming works better (currently success=false turns "Approved" into not
> approved).
> 
> The Release and Closed methods are somewhat arbitrary for erasing anyway.

Can we have a CancelAllPendingSessionCreations(media_keys_id) function, which
cancels all pending session creations? Then we don't need requested_session_id_
at all, nor OnSessionApproved.

At the browser side, we can either keep track of pending calls of
RequestProtectedMediaIdentifierPermission, or change
context->CancelProtectedMediaIdentifierPermissionRequest() to
context->CancelAllProtectedMediaIdentifierPermissionRequests().

WDYT?

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.h:65: base::hash_set<uint32>
requested_session_id_;
Typically how big is this set? I wonder if using hash_set<int> is better than
just std::set<int> if not.

[ddorwin, 2013-12-21 00:44:56]

We should discuss the scenario this seeks to address. In the forthcoming
unprefixed API, an object (not element) will invoke the infobar. Does that
affect the lifetime issues, iframes, etc. at all?

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:99: void
ProxyMediaKeys::OnSessionApproved(uint32 session_id, bool success) {
On 2013/12/21 00:40:48, xhwang wrote:
> On 2013/12/20 23:59:52, ddorwin wrote:
> > Can we just use OnSessionCreated for success and have a
OnSessionCreateDenied
> or
> > whatever for failure?
> >  * One or the other will always be called, which mans we can do all erasing
in
> > those two.
> >  * For implementations that don't involve approval, they don't need to fire
> this
> > event. (However, they'd still need to do something to erase the session_id.
> That
> > might be a general problem we need to solve. Maybe this should be
> > OnSessionCreationFailed and called in all those cases. Actually, we call
> > OnSessionError in one of those cases - maybe we can use that in the
> not-allowed
> > case too. xhwang, WDYT?)
> >  * The naming works better (currently success=false turns "Approved" into
not
> > approved).
> > 
> > The Release and Closed methods are somewhat arbitrary for erasing anyway.
> 
> Can we have a CancelAllPendingSessionCreations(media_keys_id) function, which
> cancels all pending session creations? Then we don't need
requested_session_id_
> at all, nor OnSessionApproved.
> 
> At the browser side, we can either keep track of pending calls of
> RequestProtectedMediaIdentifierPermission, or change
> context->CancelProtectedMediaIdentifierPermissionRequest() to
> context->CancelAllProtectedMediaIdentifierPermissionRequests().
> 
> WDYT?

Good point. This class is always tied to media_keys_id_, so all we need is to
tell the browser that media_keys_id_ is being destroyed and it should be able to
cancel all requests related to it.

[Kibeom Kim (inactive), 2013-12-21 01:50:43]

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:99: void
ProxyMediaKeys::OnSessionApproved(uint32 session_id, bool success) {
On 2013/12/21 00:44:56, ddorwin wrote:
> On 2013/12/21 00:40:48, xhwang wrote:
> > On 2013/12/20 23:59:52, ddorwin wrote:
> > > Can we just use OnSessionCreated for success and have a
> OnSessionCreateDenied
> > or
> > > whatever for failure?
> > >  * One or the other will always be called, which mans we can do all
erasing
> in
> > > those two.
> > >  * For implementations that don't involve approval, they don't need to
fire
> > this
> > > event. (However, they'd still need to do something to erase the
session_id.
> > That
> > > might be a general problem we need to solve. Maybe this should be
> > > OnSessionCreationFailed and called in all those cases. Actually, we call
> > > OnSessionError in one of those cases - maybe we can use that in the
> > not-allowed
> > > case too. xhwang, WDYT?)
> > >  * The naming works better (currently success=false turns "Approved" into
> not
> > > approved).
> > > 
> > > The Release and Closed methods are somewhat arbitrary for erasing anyway.
> > 
> > Can we have a CancelAllPendingSessionCreations(media_keys_id) function,
which
> > cancels all pending session creations? Then we don't need
> requested_session_id_
> > at all, nor OnSessionApproved.
> > 
> > At the browser side, we can either keep track of pending calls of
> > RequestProtectedMediaIdentifierPermission, or change
> > context->CancelProtectedMediaIdentifierPermissionRequest() to
> > context->CancelAllProtectedMediaIdentifierPermissionRequests().
> > 
> > WDYT?
> 
> Good point. This class is always tied to media_keys_id_, so all we need is to
> tell the browser that media_keys_id_ is being destroyed and it should be able
to
> cancel all requests related to it.

So, looks like it's just a matter of which class we want to manage pending
media_keys_id & sessions mapping, and options are:

1. ProxyMediaKeys (current)
2. BrowserMediaPlayerManager (can have CancelAllPendingSessionCreations ipc
message instead of cancelling individually)
3. BrowserContext (if we also want to have
BrowserContext::CancelAllProtectedMediaIdentifierPermissionRequests() method.)
4. ProxyDecryptor

3. is my least favorite option becase I'd prefer to keep BrowserContext simpler
and we have other good options. 2. sounds more attractive than 1. , but, how
about 4.? although we'll need to send the canceling ipc message for individual
session, I don't think there are that many sessions (is this correct??), and
ProxyDecryptor already manages a session map "SessionIdMap sessions_;" so I
think that's less confusing and more consistent option. WDYT?

[xhwang, 2013-12-21 02:08:46]

On 2013/12/21 01:50:43, Kibeom Kim wrote:
>
https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
> File content/renderer/media/android/proxy_media_keys.cc (right):
> 
>
https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
> content/renderer/media/android/proxy_media_keys.cc:99: void
> ProxyMediaKeys::OnSessionApproved(uint32 session_id, bool success) {
> On 2013/12/21 00:44:56, ddorwin wrote:
> > On 2013/12/21 00:40:48, xhwang wrote:
> > > On 2013/12/20 23:59:52, ddorwin wrote:
> > > > Can we just use OnSessionCreated for success and have a
> > OnSessionCreateDenied
> > > or
> > > > whatever for failure?
> > > >  * One or the other will always be called, which mans we can do all
> erasing
> > in
> > > > those two.
> > > >  * For implementations that don't involve approval, they don't need to
> fire
> > > this
> > > > event. (However, they'd still need to do something to erase the
> session_id.
> > > That
> > > > might be a general problem we need to solve. Maybe this should be
> > > > OnSessionCreationFailed and called in all those cases. Actually, we call
> > > > OnSessionError in one of those cases - maybe we can use that in the
> > > not-allowed
> > > > case too. xhwang, WDYT?)
> > > >  * The naming works better (currently success=false turns "Approved"
into
> > not
> > > > approved).
> > > > 
> > > > The Release and Closed methods are somewhat arbitrary for erasing
anyway.
> > > 
> > > Can we have a CancelAllPendingSessionCreations(media_keys_id) function,
> which
> > > cancels all pending session creations? Then we don't need
> > requested_session_id_
> > > at all, nor OnSessionApproved.
> > > 
> > > At the browser side, we can either keep track of pending calls of
> > > RequestProtectedMediaIdentifierPermission, or change
> > > context->CancelProtectedMediaIdentifierPermissionRequest() to
> > > context->CancelAllProtectedMediaIdentifierPermissionRequests().
> > > 
> > > WDYT?
> > 
> > Good point. This class is always tied to media_keys_id_, so all we need is
to
> > tell the browser that media_keys_id_ is being destroyed and it should be
able
> to
> > cancel all requests related to it.
> 
> So, looks like it's just a matter of which class we want to manage pending
> media_keys_id & sessions mapping, and options are:
> 
> 1. ProxyMediaKeys (current)
> 2. BrowserMediaPlayerManager (can have CancelAllPendingSessionCreations ipc
> message instead of cancelling individually)
> 3. BrowserContext (if we also want to have
> BrowserContext::CancelAllProtectedMediaIdentifierPermissionRequests() method.)
> 4. ProxyDecryptor
> 
> 3. is my least favorite option becase I'd prefer to keep BrowserContext
simpler
> and we have other good options. 2. sounds more attractive than 1. , but, how
> about 4.? although we'll need to send the canceling ipc message for individual
> session, I don't think there are that many sessions (is this correct??), and
> ProxyDecryptor already manages a session map "SessionIdMap sessions_;" so I
> think that's less confusing and more consistent option. WDYT?

The info bar is a concept at the browser side. It's not obvious why the renderer
process
(1 and 4) needs to worry about canceling a CreateSession() call. I think this
canceling logic
should be as close as the request. That's why I propose 3, which is the one that
holds the
request to be cancelled. 3 would be acceptable to me (+ddorwin). But definitely
I think
the request tracking should be at the browser side.

[Kibeom Kim (inactive), 2013-12-30 12:33:04]

ptal

+OWNER markusheintz@ chrome/browser/content_settings/*

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:776: void
BrowserMediaPlayerManager::GenerateKeyIfAllowed(
On 2013/12/21 00:40:48, xhwang wrote:
> This now should be renamed to CreateSessionIfAllowed()

Done. (allowed-> permitted)

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:786: return;
On 2013/12/21 00:40:48, xhwang wrote:
> On 2013/12/20 23:59:52, ddorwin wrote:
> > I'm surprised we don't already fire some event here. xhwang, WDYT?
> 
> Yeah, we should call OnSessionError() before we return, same as l.791.

Done.

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.cc:793: return;
On 2013/12/20 23:59:52, ddorwin wrote:
> Hmm, I see how this case makes using CreateSession() for the success case
> (suggested later) a problem. I still think the advantages of the suggestion
are
> appealing.

Done.

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:146: void
OnCancelSession(int media_keys_id, uint32 session_id);
On 2013/12/20 23:59:52, ddorwin wrote:
> These other *Session methods all have spec equivalents. This serves a very
> different purpose. It should be something like OnCancelSessionRequest or
> OnCancelCreateSession.

Done.

https://codereview.chromium.org/107413006/diff/100001/content/common/media/me...
File content/common/media/media_player_messages_android.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/common/media/me...
content/common/media/media_player_messages_android.h:305:
IPC_MESSAGE_ROUTED2(MediaKeysHostMsg_CancelSession,
On 2013/12/20 23:59:52, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/107413006/diff/100001/content/common/media/me...
content/common/media/media_player_messages_android.h:329:
IPC_MESSAGE_ROUTED3(MediaKeysMsg_SessionApproved,
On 2013/12/20 23:59:52, ddorwin wrote:
> Can we think of a better name than SessionApproved? SessionCreationAllowed?
> 
> It's weird naming - the name is positive but a parameter can make the meaning
> negative. This is another reason why making this message the failure case (see
> later) is appealing.

Done. (Removed)

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:75:
requested_session_id_.erase(session_id);
On 2013/12/20 23:59:52, ddorwin wrote:
> You probably don't need this here. See last comment below.

Done.

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:95:
requested_session_id_.erase(session_id);
On 2013/12/20 23:59:52, ddorwin wrote:
> I can't find documentation, but I assume this is a nop if session_id is not
> found. Is that right?

(I just assumed the spec is equal to std::set<>::erase();. On Android, I think
the actual implementation is
third_party/android_tools/ndk/sources/cxx-stl/stlport/stlport/stl/_hashtable.c ,
but haven't verified it since now we don't need this line anymore)

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
File content/renderer/media/android/renderer_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/100001/content/renderer/media/...
content/renderer/media/android/renderer_media_player_manager.h:104: void
CancelSession(int media_keys_id, uint32 session_id);
On 2013/12/20 23:59:52, ddorwin wrote:
> ditto

Done.

[xhwang, 2013-12-30 19:00:59]

This looks much cleaner, thanks!

lgtm % comments

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:182:
PendingInfoBarRequests infobars_to_remove;
s/infobars_to_remove/info_bar_requests_to_cancel/

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:183: for
(PendingInfoBarRequests::iterator i(pending_infobar_requests_.begin());
nit: InfoBar/info_bar or Infobar/infobar. Now the function name uses InfoBar but
the variable is infobar.

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:183: for
(PendingInfoBarRequests::iterator i(pending_infobar_requests_.begin());
nit: typically we use PendingInfoBarRequests::iterator i =
pending_infobar_requests_.begin()...

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:187: <<
i->id().ToString();
Remove? Or at least not to use LOG(ERROR) and KKIM :)

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:190:
infobars_to_remove.push_back(*i);
Can we call GetInfoBarService(i->id())->RemoveInfoBar(i->infobar()) directly
here instead of pushing it to another vector?

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:191: ++i;
do we need to pending_infobar_requests_.erase(i) in this case as well?

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
File chrome/browser/content_settings/permission_request_id.h (right):

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_request_id.h:32: int group_id_;
You might want to add comments about what a bridge_id_ is and what a group_id_
is.

https://codereview.chromium.org/107413006/diff/400001/content/browser/downloa...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/107413006/diff/400001/content/browser/downloa...
content/browser/download/download_manager_impl_unittest.cc:425: callback));
indent callback?

https://codereview.chromium.org/107413006/diff/400001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/400001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:183: void
CreateSessionIfPermitted(int media_keys_id,
doc about what's happening if not |permitted|, and what's the relation between
this and the info bar.

[Kibeom Kim (inactive), 2013-12-30 21:52:23]

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:182:
PendingInfoBarRequests infobars_to_remove;
On 2013/12/30 19:00:59, xhwang wrote:
> s/infobars_to_remove/info_bar_requests_to_cancel/

Done.

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:183: for
(PendingInfoBarRequests::iterator i(pending_infobar_requests_.begin());
On 2013/12/30 19:00:59, xhwang wrote:
> nit: typically we use PendingInfoBarRequests::iterator i =
> pending_infobar_requests_.begin()...

Done. (seems InfoBar naming is largely inconsistent, e.g., infobar_service.h &
InfoBarService, anyways, at least in this file I changed everything to info_bar)

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:187: <<
i->id().ToString();
On 2013/12/30 19:00:59, xhwang wrote:
> Remove? Or at least not to use LOG(ERROR) and KKIM :)

Done. :)

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:190:
infobars_to_remove.push_back(*i);
On 2013/12/30 19:00:59, xhwang wrote:
> Can we call GetInfoBarService(i->id())->RemoveInfoBar(i->infobar()) directly
> here instead of pushing it to another vector?

I tried that first, but then, at RemoveInfoBar(..), the next pending infobar was
shown. So it's safe to erase all the pending infobars before we call
RemoveInfoBar. Then, I realized what I'm doing here is similar to 
PermissionQueueController::OnPermissionSet(...) so I followed that logic.

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:191: ++i;
On 2013/12/30 19:00:59, xhwang wrote:
> do we need to pending_infobar_requests_.erase(i) in this case as well?

When the infobar is removed, it's pending_infobar_requests_.erase(i) is called
in PermissionQueueController::Observe(...).

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
File chrome/browser/content_settings/permission_request_id.h (right):

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_request_id.h:32: int group_id_;
On 2013/12/30 19:00:59, xhwang wrote:
> You might want to add comments about what a bridge_id_ is and what a group_id_
> is.

Done.

https://codereview.chromium.org/107413006/diff/400001/content/browser/downloa...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/107413006/diff/400001/content/browser/downloa...
content/browser/download/download_manager_impl_unittest.cc:425: callback));
On 2013/12/30 19:00:59, xhwang wrote:
> indent callback?

Done.

https://codereview.chromium.org/107413006/diff/400001/content/browser/media/a...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/400001/content/browser/media/a...
content/browser/media/android/browser_media_player_manager.h:183: void
CreateSessionIfPermitted(int media_keys_id,
On 2013/12/30 19:00:59, xhwang wrote:
> doc about what's happening if not |permitted|, and what's the relation between
> this and the info bar.

Done.

[xhwang, 2013-12-30 21:59:39]

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:190:
infobars_to_remove.push_back(*i);
On 2013/12/30 21:52:24, Kibeom Kim wrote:
> On 2013/12/30 19:00:59, xhwang wrote:
> > Can we call GetInfoBarService(i->id())->RemoveInfoBar(i->infobar()) directly
> > here instead of pushing it to another vector?
> 
> I tried that first, but then, at RemoveInfoBar(..), the next pending infobar
was
> shown. So it's safe to erase all the pending infobars before we call
> RemoveInfoBar. Then, I realized what I'm doing here is similar to 
> PermissionQueueController::OnPermissionSet(...) so I followed that logic.

worth a comment?

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:191: ++i;
On 2013/12/30 21:52:24, Kibeom Kim wrote:
> On 2013/12/30 19:00:59, xhwang wrote:
> > do we need to pending_infobar_requests_.erase(i) in this case as well?
> 
> When the infobar is removed, it's pending_infobar_requests_.erase(i) is called
> in PermissionQueueController::Observe(...).

worth a comment?

[Kibeom Kim (inactive), 2013-12-30 22:10:09]

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:190:
infobars_to_remove.push_back(*i);
On 2013/12/30 21:59:39, xhwang wrote:
> On 2013/12/30 21:52:24, Kibeom Kim wrote:
> > On 2013/12/30 19:00:59, xhwang wrote:
> > > Can we call GetInfoBarService(i->id())->RemoveInfoBar(i->infobar())
directly
> > > here instead of pushing it to another vector?
> > 
> > I tried that first, but then, at RemoveInfoBar(..), the next pending infobar
> was
> > shown. So it's safe to erase all the pending infobars before we call
> > RemoveInfoBar. Then, I realized what I'm doing here is similar to 
> > PermissionQueueController::OnPermissionSet(...) so I followed that logic.
> 
> worth a comment?

Done.

https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:191: ++i;
On 2013/12/30 21:59:39, xhwang wrote:
> On 2013/12/30 21:52:24, Kibeom Kim wrote:
> > On 2013/12/30 19:00:59, xhwang wrote:
> > > do we need to pending_infobar_requests_.erase(i) in this case as well?
> > 
> > When the infobar is removed, it's pending_infobar_requests_.erase(i) is
called
> > in PermissionQueueController::Observe(...).
> 
> worth a comment?

Done.

[Kibeom Kim (inactive), 2014-01-02 10:52:39]

remaining OWNER

markusheintz@
  chrome/browser/content_settings/*

trivial changes:

jam@
  chrome/browser/android/*
  chrome/browser/geolocation/*
  chrome/browser/profiles/*
  chrome/test/base/*
  content/browser/download/download_manager_impl_unittest.cc
  content/public/browser/*
  content/public/test/*
  content/shell/browser/*

boliu@
  android_webview/*
cdn@
  content/common/media/media_player_messages_android.h

[Cris Neckar, 2014-01-02 19:50:46]

On 2014/01/02 10:52:39, Kibeom Kim wrote:
> remaining OWNER
> 
> markusheintz@
>   chrome/browser/content_settings/*
> 
> trivial changes:
> 
> jam@
>   chrome/browser/android/*
>   chrome/browser/geolocation/*
>   chrome/browser/profiles/*
>   chrome/test/base/*
>   content/browser/download/download_manager_impl_unittest.cc
>   content/public/browser/*
>   content/public/test/*
>   content/shell/browser/*
> 
> boliu@
>   android_webview/*
> cdn@
>   content/common/media/media_player_messages_android.h

LGTM on ipc message changes with one request.

[Cris Neckar, 2014-01-02 19:50:59]

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:198: int group_id) {
Please request another IPC message review from me in the CL where you implement
this.

[Kibeom Kim (inactive), 2014-01-02 22:42:27]

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:194: callback.Run(false);
NOTIMPLEMENTED();

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:198: int group_id) {
NOTIMPLEMENTED();

[jam, 2014-01-02 22:50:55]

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
chrome/browser/profiles/off_the_record_profile_impl.h:119: OVERRIDE;
ditto

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.h:96: OVERRIDE;
ditto

https://codereview.chromium.org/107413006/diff/1140001/chrome/test/base/testi...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/107413006/diff/1140001/chrome/test/base/testi...
chrome/test/base/testing_profile.h:295: OVERRIDE;
ditto

https://codereview.chromium.org/107413006/diff/1140001/content/public/test/te...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/107413006/diff/1140001/content/public/test/te...
content/public/test/test_browser_context.h:62: OVERRIDE;
ditto

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.cc:209: // Always reject requests
for testing.
hmm, content/shell is an example of a content embedder. in general, we tend to
allow things by default since we don't do UI.

if this code is used in content_browsertests, can you override the
BrowserContext implementation there? or you can have a setter on this class to
change the default value to false?

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.h:65: OVERRIDE;
nit: i believe the convention is never to start a line with OVERRIDE, so move
"int group_id" to the next line

[Kibeom Kim (inactive), 2014-01-02 23:26:36]

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:194: callback.Run(false);
On 2014/01/02 22:42:28, Kibeom Kim wrote:
> NOTIMPLEMENTED();

Done.

https://codereview.chromium.org/107413006/diff/1140001/android_webview/browse...
android_webview/browser/aw_browser_context.cc:198: int group_id) {
On 2014/01/02 22:42:28, Kibeom Kim wrote:
> NOTIMPLEMENTED();

Done.

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
chrome/browser/profiles/off_the_record_profile_impl.h:119: OVERRIDE;
On 2014/01/02 22:50:56, jam wrote:
> ditto

Done.

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl.h (right):

https://codereview.chromium.org/107413006/diff/1140001/chrome/browser/profile...
chrome/browser/profiles/profile_impl.h:96: OVERRIDE;
On 2014/01/02 22:50:56, jam wrote:
> ditto

Done.

https://codereview.chromium.org/107413006/diff/1140001/chrome/test/base/testi...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/107413006/diff/1140001/chrome/test/base/testi...
chrome/test/base/testing_profile.h:295: OVERRIDE;
On 2014/01/02 22:50:56, jam wrote:
> ditto

Done.

https://codereview.chromium.org/107413006/diff/1140001/content/public/test/te...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/107413006/diff/1140001/content/public/test/te...
content/public/test/test_browser_context.h:62: OVERRIDE;
On 2014/01/02 22:50:56, jam wrote:
> ditto

Done.

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.cc:209: // Always reject requests
for testing.
On 2014/01/02 22:50:56, jam wrote:
> hmm, content/shell is an example of a content embedder. in general, we tend to
> allow things by default since we don't do UI.
> 
> if this code is used in content_browsertests, can you override the
> BrowserContext implementation there? or you can have a setter on this class to
> change the default value to false?

(This is not used in any test, unless some tests accidentally visits a website
that ask this permission.)

but, is it ok to give a permission without asking for content shell? I saw that
ShellBrowserContext::RequestMIDISysExPermission() doesn't allow, so I assumed
we're disabling permissions by default for content shell.

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.h (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.h:65: OVERRIDE;
On 2014/01/02 22:50:56, jam wrote:
> nit: i believe the convention is never to start a line with OVERRIDE, so move
> "int group_id" to the next line

Done.

[jam, 2014-01-03 00:54:22]

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.cc:209: // Always reject requests
for testing.
On 2014/01/02 23:26:36, Kibeom Kim wrote:
> On 2014/01/02 22:50:56, jam wrote:
> > hmm, content/shell is an example of a content embedder. in general, we tend
to
> > allow things by default since we don't do UI.
> > 
> > if this code is used in content_browsertests, can you override the
> > BrowserContext implementation there? or you can have a setter on this class
to
> > change the default value to false?
> 
> (This is not used in any test, unless some tests accidentally visits a website
> that ask this permission.)
> 
> but, is it ok to give a permission without asking for content shell? 

yes, since content shell is just an example of a simple embedder of content, it
should assume the user gives permission for everything.

> I saw that
> ShellBrowserContext::RequestMIDISysExPermission() doesn't allow, so I assumed
> we're disabling permissions by default for content shell.

ah, that's not a good example. let me ping those folks

[jam, 2014-01-03 00:57:34]

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.cc:209: // Always reject requests
for testing.
On 2014/01/03 00:54:23, jam wrote:
> On 2014/01/02 23:26:36, Kibeom Kim wrote:
> > On 2014/01/02 22:50:56, jam wrote:
> > > hmm, content/shell is an example of a content embedder. in general, we
tend
> to
> > > allow things by default since we don't do UI.
> > > 
> > > if this code is used in content_browsertests, can you override the
> > > BrowserContext implementation there? or you can have a setter on this
class
> to
> > > change the default value to false?
> > 
> > (This is not used in any test, unless some tests accidentally visits a
website
> > that ask this permission.)
> > 
> > but, is it ok to give a permission without asking for content shell? 
> 
> yes, since content shell is just an example of a simple embedder of content,
it
> should assume the user gives permission for everything.
> 
> > I saw that
> > ShellBrowserContext::RequestMIDISysExPermission() doesn't allow, so I
assumed
> > we're disabling permissions by default for content shell.
> 
> ah, that's not a good example. let me ping those folks

btw i've sent out a fix in https://codereview.chromium.org/123663002/

[Kibeom Kim (inactive), 2014-01-03 00:59:16]

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
File content/shell/browser/shell_browser_context.cc (right):

https://codereview.chromium.org/107413006/diff/1140001/content/shell/browser/...
content/shell/browser/shell_browser_context.cc:209: // Always reject requests
for testing.
On 2014/01/03 00:54:23, jam wrote:
> On 2014/01/02 23:26:36, Kibeom Kim wrote:
> > On 2014/01/02 22:50:56, jam wrote:
> > > hmm, content/shell is an example of a content embedder. in general, we
tend
> to
> > > allow things by default since we don't do UI.
> > > 
> > > if this code is used in content_browsertests, can you override the
> > > BrowserContext implementation there? or you can have a setter on this
class
> to
> > > change the default value to false?
> > 
> > (This is not used in any test, unless some tests accidentally visits a
website
> > that ask this permission.)
> > 
> > but, is it ok to give a permission without asking for content shell? 
> 
> yes, since content shell is just an example of a simple embedder of content,
it
> should assume the user gives permission for everything.

Done. (already :) )

[jam, 2014-01-03 01:19:22]

lgtm

[boliu, 2014-01-03 01:27:32]

aw lgtm

[markusheintz_, 2014-01-07 12:22:44]

LGTM with nits

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_queue_controller.cc:204: ++i)
nit: I know there exist several opinions on this, but I prefer having { } around
the body of the for loop because the condition spans a cross several lines.

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
File chrome/browser/content_settings/permission_request_id.h (right):

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_request_id.h:37: int group_id_;
Would be great to add one sentence about when this is used.

[ddorwin, 2014-01-07 19:07:13]

A few nits. My issues have been addressed; I defer to xhwang on the */media/*
code.

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_queue_controller.cc:53: bool
has_infobar() const { return !!info_bar_; }
Why the change to a two-part name? See comment below.

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_queue_controller.cc:182: // If we
remove an info bar in the following loop, the next pending info bar
nit: "info bar" is inconsistent with description and used only 57 times compared
to 863 in the source.

https://codereview.chromium.org/107413006/diff/1460001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/1460001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.h:185: // The primary
use case is info bar permission callback, i.e., when info bar
nit: "info bar"

https://codereview.chromium.org/107413006/diff/1460001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.h:186: // can decide
user's intention either  from interacting with the actual info
nit: extra space after "either".

[Kibeom Kim (inactive), 2014-01-07 19:31:23]

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_queue_controller.cc:53: bool
has_infobar() const { return !!info_bar_; }
On 2014/01/07 19:07:14, ddorwin wrote:
> Why the change to a two-part name? See comment below.

(previous comments:
https://codereview.chromium.org/107413006/diff/400001/chrome/browser/content_...
)

Changed back to infobar_ and also s/PendingInfoBarRequest/PendingInfobarRequest

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_queue_controller.cc:182: // If we
remove an info bar in the following loop, the next pending info bar
On 2014/01/07 19:07:14, ddorwin wrote:
> nit: "info bar" is inconsistent with description and used only 57 times
compared
> to 863 in the source.

Done.

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_queue_controller.cc:204: ++i)
On 2014/01/07 12:22:45, markusheintz_ wrote:
> nit: I know there exist several opinions on this, but I prefer having { }
around
> the body of the for loop because the condition spans a cross several lines.

Done.

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
File chrome/browser/content_settings/permission_request_id.h (right):

https://codereview.chromium.org/107413006/diff/1460001/chrome/browser/content...
chrome/browser/content_settings/permission_request_id.h:37: int group_id_;
On 2014/01/07 12:22:45, markusheintz_ wrote:
> Would be great to add one sentence about when this is used.

Done.

https://codereview.chromium.org/107413006/diff/1460001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/107413006/diff/1460001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.h:185: // The primary
use case is info bar permission callback, i.e., when info bar
On 2014/01/07 19:07:14, ddorwin wrote:
> nit: "info bar"

Done.

https://codereview.chromium.org/107413006/diff/1460001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.h:186: // can decide
user's intention either  from interacting with the actual info
On 2014/01/07 19:07:14, ddorwin wrote:
> nit: extra space after "either".

Done.

[commit-bot: I haz the power, 2014-01-07 19:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/107413006/1700001

[commit-bot: I haz the power, 2014-01-07 21:18:29]

Retried try job too often on win_rel for step(s) content_browsertests,
mini_installer_test, nacl_integration, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-07 21:45:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/107413006/2110001

[commit-bot: I haz the power, 2014-01-08 01:15:03]

Change committed as 243458