permissions: remove PermissionQueueController and introduce PermissionInfoBarManager

permissions: remove PermissionQueueController and introduce PermissionInfoBarManager

Queue controller is a class which is almost 6 years old and its age is
starting to show. It uses all sorts of interesting hacks to tie request
lifecycles to the lifecycle of a web contents.

Moreover, coallescing permissions would be a nightmare since the request
id is compared and all sorts of strange checks are used to compare request
equality.

Instead of all the craziness, just rewrite the controller to be a version
of bubble manager for infobars. This leads to far simpler code as the
manager as a whole is tied to a web contents so we get lifecycle management
for free!

Furthermore, we also have multiple permisisons managed and coallesced
for when it is implemented.

This CL is also a part of a group of CLs:
(1) https://codereview.chromium.org/1332293002
(2) https://codereview.chromium.org/1337903002 (this)
(3) https://codereview.chromium.org/1332063003

BUG=516626

[Lalit Maganti, 2015-09-11 11:50:21]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[Lalit Maganti, 2015-09-11 11:50:21]

Another FYI change for splitting up the big commit.

[mlamouri (slow - plz ping), 2015-09-16 16:21:01]

A few comments from a first pass.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:76: if (web_contents ==
nullptr)
style: this isn't Java :) you can do if (!web_contents)

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:42: return;
style: wrap around { }

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:84: // Increment the
iterator as it will be invalidated.
nit: comment misplaced?

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:111: // Request may not
exist but that's OK.
rephrase: // No-op if the request does not exist.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:37: const
base::Callback<void(bool, ContentSetting)>& non_user_callback);
Why having these two callbacks? It makes things a bit messy.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:21: #endif
Do we expect to use this code in other platforms than OS_ANDROID?

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:8: #include
"base/callback.h"
nit: callback_forward.h?

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:167:
PermissionInfoBarManager::CreateForWebContents(web_contents);
Should that be OS_ANDROID specific?

https://codereview.chromium.org/1337903002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2082:
'browser/permissions/permission_infobar_request.h',
Should we make those Andorid specific?

https://codereview.chromium.org/1337903002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2088:
'browser/permissions/permission_infobar_manager.h',
ditto

[Lalit Maganti, 2015-09-16 17:28:38]

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:76: if (web_contents ==
nullptr)
On 2015/09/16 16:21:00, Mounir Lamouri wrote:
> style: this isn't Java :) you can do if (!web_contents)

Yeah my life has too much Java :P

Done.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:42: return;
On 2015/09/16 16:21:00, Mounir Lamouri wrote:
> style: wrap around { }

Done.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:84: // Increment the
iterator as it will be invalidated.
On 2015/09/16 16:21:00, Mounir Lamouri wrote:
> nit: comment misplaced?

Slightly yes.

Fixed.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:111: // Request may not
exist but that's OK.
On 2015/09/16 16:21:00, Mounir Lamouri wrote:
> rephrase: // No-op if the request does not exist.

Done.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:37: const
base::Callback<void(bool, ContentSetting)>& non_user_callback);
On 2015/09/16 16:21:01, Mounir Lamouri wrote:
> Why having these two callbacks? It makes things a bit messy.

It's necessary to correctly bypass the UMA and content settings stuff in
ContextBase.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:21: #endif
On 2015/09/16 16:21:01, Mounir Lamouri wrote:
> Do we expect to use this code in other platforms than OS_ANDROID?

Interesting question. I'm not sure. I assume because of bubble no. However,
bubble manager is excluded for iOS as well so not sure what needs to be done
there.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:8: #include
"base/callback.h"
On 2015/09/16 16:21:01, Mounir Lamouri wrote:
> nit: callback_forward.h?

I store callbacks so can't use forward.

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:167:
PermissionInfoBarManager::CreateForWebContents(web_contents);
On 2015/09/16 16:21:01, Mounir Lamouri wrote:
> Should that be OS_ANDROID specific?

Maybe - see previous comment.

https://codereview.chromium.org/1337903002/diff/100001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1337903002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2082:
'browser/permissions/permission_infobar_request.h',
On 2015/09/16 16:21:01, Mounir Lamouri wrote:
> Should we make those Andorid specific?

As previous.

https://codereview.chromium.org/1337903002/diff/100001/chrome/chrome_browser....
chrome/chrome_browser.gypi:2088:
'browser/permissions/permission_infobar_manager.h',
On 2015/09/16 16:21:01, Mounir Lamouri wrote:
> ditto

As previous.

[mlamouri (slow - plz ping), 2015-09-28 13:30:15]

CC: dfalcantara@ so he can follow the status of this CL given that it's required
for part 3 which he is reviewing.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:94: if
(PermissionInfoBarManager::FromWebContents(web_contents) != NULL) {
Remove |!= NULL| and do !PermissionInfoBarManager::...

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:97: }
What about removing the |return;| from the |PermissionBubbleManager::Enabled|
block and have a |else if| instead? The |return;| in the first block but not the
second sounds slightly error prone.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:171:
DCHECK(infobar_manager);
What if we are not in a Tab context? Couldn't the WebContents have no
PermissionInfoBarManager?

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:178:
base::Bind(&PermissionContextBase::NotifyPermissionSet,
It would be great to unify that with what Bubble does. Also,
::NotifyPermissionSet is a hack given what you are trying to do. We should have
a callback that simply run the callback with the current permission status.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:81:
manager->CancelInfoBarRequest(request);
That sounds odd. You don't actually use |accept|. Why?

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:21: // this should not
be set to -1.
Where is that code?

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:75:
content::BrowserThread::UI,
Given that we handle threads, it would be nice if we could have a
DCHECK_CURRENTLY_ON() unless we don't know in which thread we are expected to
be? (I assume it has to be UI.)

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:77:
base::Bind(&PermissionInfoBarManager::ShowNextQueuedRequest,
Why do we do that? Is it just to be compatible with bubbles?

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:102: if (will_show) {
You might be able to do that in one line.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:134:
ShowNextQueuedRequest();
Could we call Trigger... in both cases? Worse case, we wait for the next event
loop. It might simplify the code.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:31: void
CreateInfoBarRequest(
nit: CreateRequest()

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:34: const GURL&
requesting_frame,
nit: requesting_origin

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:35: const GURL&
embedder,
nit: embedding_origin

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:40: void
CancelInfoBarRequest(const PermissionRequestID& request);
nit: CancelRequest()

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:59: bool
is_show_queued_pending_;
I can't understand what you mean. Is this member correctly named?

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:84:
requests_[i].non_user_callback().Run(false, CONTENT_SETTING_DEFAULT);
Maybe you could create N infobars?

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:88: void
PermissionInfoBarRequest::Cancel(bool allowed) {
You do not seem to be using |allowed|.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:25: const GURL&
embedding_frame,
nit: s/frame/origin/g

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:26: const
base::Callback<void(bool)>& finished_callback);
nit: s/finished_callback/callback/

[Lalit Maganti, 2015-09-28 15:32:39]

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:94: if
(PermissionInfoBarManager::FromWebContents(web_contents) != NULL) {
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> Remove |!= NULL| and do !PermissionInfoBarManager::...

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:97: }
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> What about removing the |return;| from the |PermissionBubbleManager::Enabled|
> block and have a |else if| instead? The |return;| in the first block but not
the
> second sounds slightly error prone.

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:171:
DCHECK(infobar_manager);
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> What if we are not in a Tab context? Couldn't the WebContents have no
> PermissionInfoBarManager?

You're right this is an issue. Best course of action is to handle this in
permission manager and change IsPermissionBubbleManagerMissing ->
IsPermissionUIManagerMissing.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:178:
base::Bind(&PermissionContextBase::NotifyPermissionSet,
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> It would be great to unify that with what Bubble does. Also,
> ::NotifyPermissionSet is a hack given what you are trying to do. We should
have
> a callback that simply run the callback with the current permission status.

Changed as discussed.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base_unittest.cc:81:
manager->CancelInfoBarRequest(request);
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> That sounds odd. You don't actually use |accept|. Why?

I have no clue. Will look into it.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:21: // this should not
be set to -1.
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> Where is that code?

Mainly test code.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:75:
content::BrowserThread::UI,
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> Given that we handle threads, it would be nice if we could have a
> DCHECK_CURRENTLY_ON() unless we don't know in which thread we are expected to
> be? (I assume it has to be UI.)

Everything should be on the UI thread in this class. I only Post so that
coalescing works as expected.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:77:
base::Bind(&PermissionInfoBarManager::ShowNextQueuedRequest,
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> Why do we do that? Is it just to be compatible with bubbles?

That and if I didn't post, coalescing would not work correctly.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:102: if (will_show) {
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> You might be able to do that in one line.

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:134:
ShowNextQueuedRequest();
On 2015/09/28 13:30:14, Mounir Lamouri wrote:
> Could we call Trigger... in both cases? Worse case, we wait for the next event
> loop. It might simplify the code.

Changed as discussed.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:31: void
CreateInfoBarRequest(
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> nit: CreateRequest()

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:34: const GURL&
requesting_frame,
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> nit: requesting_origin

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:35: const GURL&
embedder,
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> nit: embedding_origin

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:40: void
CancelInfoBarRequest(const PermissionRequestID& request);
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> nit: CancelRequest()

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:59: bool
is_show_queued_pending_;
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> I can't understand what you mean. Is this member correctly named?

It means whether ShowNextQueuedRequest has been placed on the event loop. I'm
not sure what a better name would be.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:84:
requests_[i].non_user_callback().Run(false, CONTENT_SETTING_DEFAULT);
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> Maybe you could create N infobars?

Would require big changes in the way this class is structured :/

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:88: void
PermissionInfoBarRequest::Cancel(bool allowed) {
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> You do not seem to be using |allowed|.

Fixed.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:25: const GURL&
embedding_frame,
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> nit: s/frame/origin/g

Done.

https://codereview.chromium.org/1337903002/diff/450001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:26: const
base::Callback<void(bool)>& finished_callback);
On 2015/09/28 13:30:15, Mounir Lamouri wrote:
> nit: s/finished_callback/callback/

Done.

[mlamouri (slow - plz ping), 2015-09-29 14:02:18]

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:83: if
(PermissionBubbleManager::Enabled()) {
Could you do:
if (PermissionBubbleManager::Enabled() &&
PermissionBubbleManager::FromWebContents(web_contents)) {
  [...]
} else if (...) {
  [...]
}

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:22: const int
kNoCurrentRequestId = -2;
Could you have a CL that fixes that? Before or after this one. This isn't really
things we should have.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:53:
PermissionInfoBarRequest* request;
= nullptr;

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:60: // Try to trigger a
show for this infobar.
nit: why "Try"?

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:89: if
(ShouldIgnoreQueuedRequests() || is_show_queued_pending_)
Should |ShouldIgnoreQueuedRequests()| take |is_show_queued_pending_| into
account?

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:94:
content::BrowserThread::UI,
Please, add a DCHECK_CURRENTLY_ON(content::BrowserThread::UI) at the beginning
of this method so it's clear to the reader that we are in the UI thread and only
send to the event loop to delay and merge.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:120: if
(infobar_request->ShowInfobar(infobar_service, profile)) {
You could pass |web_contents| only here. You can get the InfoBarService from a
WebContents and you can get the associated BrowserContext (which is the content/
version of a Profile).

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:39: void
CancelInfoBarRequest(const PermissionRequestID& request);
Hmm, CancelRequest().

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:49: void Closing();
Add comments.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:53: void
OnInfoBarClosed();
ditto

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:56: bool
ShouldIgnoreQueuedRequests();
ditto

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:59: int
current_request_id_;
Why can't you do:
current_request_->request_id?

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:61: bool
is_show_queued_pending_;
I think my confusion is that "queued" and "pending" sound pretty much the same.
I think |is_show_pending_| or |is_show_queued_| would be better with a slight
preference for the former :)

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:137:
HostContentSettingsMapFactory::GetForProfile(profile)
Do not use HostContentSettingsMapFactory but PermissionManager which will use a
PermissionContext. That way, you will be able to know if a permission is really
granted/denied using GetPermissionStatus().

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:29: void AddPermission(
Here and below, please add comments for these methods.

[Lalit Maganti, 2015-10-01 17:05:14]

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_context_base.cc (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_context_base.cc:83: if
(PermissionBubbleManager::Enabled()) {
On 2015/09/29 14:02:17, Mounir Lamouri wrote:
> Could you do:
> if (PermissionBubbleManager::Enabled() &&
> PermissionBubbleManager::FromWebContents(web_contents)) {
>   [...]
> } else if (...) {
>   [...]
> }

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:22: const int
kNoCurrentRequestId = -2;
On 2015/09/29 14:02:17, Mounir Lamouri wrote:
> Could you have a CL that fixes that? Before or after this one. This isn't
really
> things we should have.

Will do.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:53:
PermissionInfoBarRequest* request;
On 2015/09/29 14:02:17, Mounir Lamouri wrote:
> = nullptr;

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:60: // Try to trigger a
show for this infobar.
On 2015/09/29 14:02:17, Mounir Lamouri wrote:
> nit: why "Try"?

Because it could fail due to a show being pending or an infobar already being
displayed.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:89: if
(ShouldIgnoreQueuedRequests() || is_show_queued_pending_)
On 2015/09/29 14:02:17, Mounir Lamouri wrote:
> Should |ShouldIgnoreQueuedRequests()| take |is_show_queued_pending_| into
> account?

I did think about this but it made the code in |ShowNextQueuedRequest| seem
quite fragile. If the boolean was set to false after the check then nothing
would every be shown.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:94:
content::BrowserThread::UI,
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> Please, add a DCHECK_CURRENTLY_ON(content::BrowserThread::UI) at the beginning
> of this method so it's clear to the reader that we are in the UI thread and
only
> send to the event loop to delay and merge.

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:120: if
(infobar_request->ShowInfobar(infobar_service, profile)) {
On 2015/09/29 14:02:17, Mounir Lamouri wrote:
> You could pass |web_contents| only here. You can get the InfoBarService from a
> WebContents and you can get the associated BrowserContext (which is the
content/
> version of a Profile).

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:39: void
CancelInfoBarRequest(const PermissionRequestID& request);
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> Hmm, CancelRequest().

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:49: void Closing();
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> Add comments.

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:53: void
OnInfoBarClosed();
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:56: bool
ShouldIgnoreQueuedRequests();
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:59: int
current_request_id_;
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> Why can't you do:
> current_request_->request_id?

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:61: bool
is_show_queued_pending_;
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> I think my confusion is that "queued" and "pending" sound pretty much the
same.
> I think |is_show_pending_| or |is_show_queued_| would be better with a slight
> preference for the former :)

Done.

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:137:
HostContentSettingsMapFactory::GetForProfile(profile)
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> Do not use HostContentSettingsMapFactory but PermissionManager which will use
a
> PermissionContext. That way, you will be able to know if a permission is
really
> granted/denied using GetPermissionStatus().

Hmmmm I would do this except PermissionManager takes PermissionType not
ContentSettingsType. Should I expose a method which allows ContentSettingsType
as an input?

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/530001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:29: void AddPermission(
On 2015/09/29 14:02:18, Mounir Lamouri wrote:
> Here and below, please add comments for these methods.

Done.

[mlamouri (slow - plz ping), 2015-10-02 11:49:40]

The two major remaining issues are the usage of PermissionManager in the request
and the relationship between AddPermission() and showing infobar. sgtm
otherwise.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:51:
weak_factory_.GetWeakPtr()));
It seems to me that we could use base::Unretained() here because |request| is
owned by |this|. Though, that wouldn't apply if the callback is ran in another
thread or asynchronously in general.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:51: void Closing();
If these are for tests only, please add "ForTests". We have checks that make
sure we don't call these methods in production code.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:64: // Remoes the
current request and shows a next queued request if available.
nit: typo

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:71:
scoped_ptr<PermissionInfoBarRequest> current_request_;
nit: add comment

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:73: bool
is_show_pending_;
nit: add comment

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:76:
scoped_ptr<PermissionInfoBarRequest>> queued_requests_;
What about using IDMap here instead? It would guarantee that |this| owns the
pointers instead of using a scoped_ptr.

nit: add comment (and point that the request from |current_request_| will not be
there)

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:54:
requests_.push_back(PermissionRequest(type, response_callback));
What if ::AddPermission() is called while the infobar is already showing?

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:149: embedding_origin_,
it->type(), std::string());
Could you use PermissionManager here.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:39: // Shows the
relavent infobar for this request in the specified web contents.
I think you should add comments regarding the relationship between
AddPermission() and ShowInfobar(). What happens if they are call next to each
other in different orders.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:42: // Used to manually
accept the request.
Here and below. What do you mean by "manually"?

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:68:
PermissionDecidedCallback response_callback_;
nit: rename |callback_|.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:74: // Callback function
used when single permission infobar is shown.
"Shown" for "OnPermissionSet"?

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:78: // Callback function
used when a coalesced infobar is shown.
ditto

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:83: // a user specified
ContentSetting.
nit: do not mention ContentSetting. Maybe:
"""
Removes any permissions from this request for which there is no need to prompt
the user.
"""

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:86: // Creates the
single permission infobar for the PermissionRequest|
nit: remove "|"

[Lalit Maganti, 2015-10-02 13:37:46]

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:51:
weak_factory_.GetWeakPtr()));
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> It seems to me that we could use base::Unretained() here because |request| is
> owned by |this|. Though, that wouldn't apply if the callback is ran in another
> thread or asynchronously in general.

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:51: void Closing();
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> If these are for tests only, please add "ForTests". We have checks that make
> sure we don't call these methods in production code.

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:64: // Remoes the
current request and shows a next queued request if available.
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> nit: typo

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:71:
scoped_ptr<PermissionInfoBarRequest> current_request_;
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> nit: add comment

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:73: bool
is_show_pending_;
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> nit: add comment

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:76:
scoped_ptr<PermissionInfoBarRequest>> queued_requests_;
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> What about using IDMap here instead? It would guarantee that |this| owns the
> pointers instead of using a scoped_ptr.
> 
> nit: add comment (and point that the request from |current_request_| will not
be
> there)

That would be good except I can't extract the pointer out of the map to store it
in current_request_. I've kept it as current for now.

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:54:
requests_.push_back(PermissionRequest(type, response_callback));
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> What if ::AddPermission() is called while the infobar is already showing?

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:149: embedding_origin_,
it->type(), std::string());
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> Could you use PermissionManager here.

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:39: // Shows the
relavent infobar for this request in the specified web contents.
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> I think you should add comments regarding the relationship between
> AddPermission() and ShowInfobar(). What happens if they are call next to each
> other in different orders.

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:42: // Used to manually
accept the request.
On 2015/10/02 11:49:40, Mounir Lamouri wrote:
> Here and below. What do you mean by "manually"?

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:68:
PermissionDecidedCallback response_callback_;
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> nit: rename |callback_|.

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:74: // Callback function
used when single permission infobar is shown.
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> "Shown" for "OnPermissionSet"?

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:78: // Callback function
used when a coalesced infobar is shown.
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> ditto

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:83: // a user specified
ContentSetting.
On 2015/10/02 11:49:39, Mounir Lamouri wrote:
> nit: do not mention ContentSetting. Maybe:
> """
> Removes any permissions from this request for which there is no need to prompt
> the user.
> """

Done.

https://codereview.chromium.org/1337903002/diff/630001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:86: // Creates the
single permission infobar for the PermissionRequest|
On 2015/10/02 11:49:40, Mounir Lamouri wrote:
> nit: remove "|"

Done.

[mlamouri (slow - plz ping), 2015-10-02 15:51:07]

lgtm with comments applied.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:35: const
PermissionDecidedCallback& response_callback) {
nit: rename to callback

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:101: if
(infobar_request->ShowInfobar(web_contents())) {
Could you add a comment explaining that if ShowInfobar returns false, that means
nothing was shown and we should go to the next queued request?

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:36: const
base::Callback<void(bool, ContentSetting)>& response_callback);
nit: rename to callback

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:180: const
std::vector<ContentSetting>& allowed) {
nit: allowed isn't a great named

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:189:
update_content_setting ? allowed[i] : CONTENT_SETTING_DEFAULT);
Shouldn't that be a DCHECK()? that if !update_content_setting allowed[i] is ===
CONTENT_SETTING_DEFAULTN.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:76: void
OnManualClose();
OnCancel?

[Lalit Maganti, 2015-10-02 16:06:43]

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:35: const
PermissionDecidedCallback& response_callback) {
On 2015/10/02 15:51:06, Mounir Lamouri wrote:
> nit: rename to callback

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:101: if
(infobar_request->ShowInfobar(web_contents())) {
On 2015/10/02 15:51:06, Mounir Lamouri wrote:
> Could you add a comment explaining that if ShowInfobar returns false, that
means
> nothing was shown and we should go to the next queued request?

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:36: const
base::Callback<void(bool, ContentSetting)>& response_callback);
On 2015/10/02 15:51:06, Mounir Lamouri wrote:
> nit: rename to callback

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:180: const
std::vector<ContentSetting>& allowed) {
On 2015/10/02 15:51:07, Mounir Lamouri wrote:
> nit: allowed isn't a great named

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:189:
update_content_setting ? allowed[i] : CONTENT_SETTING_DEFAULT);
On 2015/10/02 15:51:07, Mounir Lamouri wrote:
> Shouldn't that be a DCHECK()? that if !update_content_setting allowed[i] is
===
> CONTENT_SETTING_DEFAULTN.

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:76: void
OnManualClose();
On 2015/10/02 15:51:07, Mounir Lamouri wrote:
> OnCancel?

Done.

[Lalit Maganti, 2015-10-02 16:06:44]

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.cc (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:35: const
PermissionDecidedCallback& response_callback) {
On 2015/10/02 15:51:06, Mounir Lamouri wrote:
> nit: rename to callback

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.cc:101: if
(infobar_request->ShowInfobar(web_contents())) {
On 2015/10/02 15:51:06, Mounir Lamouri wrote:
> Could you add a comment explaining that if ShowInfobar returns false, that
means
> nothing was shown and we should go to the next queued request?

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_manager.h (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_manager.h:36: const
base::Callback<void(bool, ContentSetting)>& response_callback);
On 2015/10/02 15:51:06, Mounir Lamouri wrote:
> nit: rename to callback

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.cc (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:180: const
std::vector<ContentSetting>& allowed) {
On 2015/10/02 15:51:07, Mounir Lamouri wrote:
> nit: allowed isn't a great named

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.cc:189:
update_content_setting ? allowed[i] : CONTENT_SETTING_DEFAULT);
On 2015/10/02 15:51:07, Mounir Lamouri wrote:
> Shouldn't that be a DCHECK()? that if !update_content_setting allowed[i] is
===
> CONTENT_SETTING_DEFAULTN.

Done.

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_request.h (right):

https://codereview.chromium.org/1337903002/diff/710001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_request.h:76: void
OnManualClose();
On 2015/10/02 15:51:07, Mounir Lamouri wrote:
> OnCancel?

Done.

[mlamouri (slow - plz ping), 2015-10-03 11:22:02]

mlamouri@chromium.org changed reviewers:
+ jochen@chromium.org

[mlamouri (slow - plz ping), 2015-10-03 11:22:04]

+jochen@ for chrome/ changes.

[Lalit Maganti (personal), 2015-10-03 13:43:08]

On 2015/10/03 11:22:04, Mounir Lamouri wrote:
> +jochen@ for chrome/ changes.

This is failing tests so I need to fix and reupload. L-G-T-M here would end up
being of no use in that case :(

Going to take a few days off from Chrome work though so it might be a while
before you see that change :D

[Lalit Maganti (personal), 2015-10-26 21:27:50]

lalitmaganti@gmail.com changed reviewers:
+ lalitmaganti@gmail.com

[Lalit Maganti (personal), 2015-10-26 21:27:51]

Superseded by https://codereview.chromium.org/1422053004/
Can't close this so would appreciate someone else doing so :)

[Lalit Maganti (personal), 2015-10-26 21:27:54]

Superseded by https://codereview.chromium.org/1422053004/
Can't close this so would appreciate someone else doing so :)