Make GroupedPermissionInfoBarDelegate's methods call through PermissionPromptAndroid

Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate
and adds Accept() and Cancel() methods to the delegate, which asks
PermissionPromptAndroid to handle accept or deny actions. then defers them to
PermissionRequestManager, which will store UMA metrics, store settings of each
requests, and remove requests in its request list.

BUG=606138
Committed: https://crrev.com/6dcb3141c8e832d80a4f83e8a4e235319908e57e
Cr-Commit-Position: refs/heads/master@{#429154}

[lshang, 2016-10-24 09:14:42]

Description was changed from

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid


Merge branch 'add_permission_prompt_android' into
set_permission_of_permission_prompt_android


add owner


rebase


some change

BUG=
==========

to

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl(which is its owner), and PermissionPromptImpl will call
into PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

This CL adds PermissionPromptAndroid as GroupedPermissionInfobar's owner and
changed ProcessButton() to go through PermissionPromptAndroid to handle accept
or deny actions.

BUG=606138
==========

[lshang, 2016-10-25 04:16:09]

Patchset #2 (id:20001) has been deleted

[lshang, 2016-10-25 04:23:28]

Description was changed from

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl(which is its owner), and PermissionPromptImpl will call
into PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

This CL adds PermissionPromptAndroid as GroupedPermissionInfobar's owner and
changed ProcessButton() to go through PermissionPromptAndroid to handle accept
or deny actions.

BUG=606138
==========

to

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds a pointer of PermissionPromptAndroid to GroupedPermissionInfobar
and changes ProcessButton() to go through PermissionPromptAndroid to handle
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, call callback methods
of each requests, and remove requests in its request list.

BUG=606138
==========

[lshang, 2016-10-25 04:23:45]

Description was changed from

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds a pointer of PermissionPromptAndroid to GroupedPermissionInfobar
and changes ProcessButton() to go through PermissionPromptAndroid to handle
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, call callback methods
of each requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds a pointer of PermissionPromptAndroid to GroupedPermissionInfobar 
and changes ProcessButton() to go through PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, call callback methods
of each requests, and remove requests in its request list.

BUG=606138
==========

[lshang, 2016-10-25 04:30:05]

Description was changed from

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds a pointer of PermissionPromptAndroid to GroupedPermissionInfobar 
and changes ProcessButton() to go through PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, call callback methods
of each requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds a pointer of PermissionPromptAndroid to GroupedPermissionInfobar 
and changes ProcessButton() to go through PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

[lshang, 2016-10-25 04:30:38]

lshang@chromium.org changed reviewers:
+ dominickn@chromium.org

[lshang, 2016-10-25 04:30:38]

Dom, PTAL thanks!

This is a subsequent CL of https://codereview.chromium.org/2315563002/. This
makes 'Allow' and 'Block' buttons work in the infobar and return to
PermissionRequestManager.

[lshang, 2016-10-26 04:45:20]

Dom, I've updated the patch. PTAL thanks!

[lshang, 2016-10-26 04:47:44]

Description was changed from

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds a pointer of PermissionPromptAndroid to GroupedPermissionInfobar 
and changes ProcessButton() to go through PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and changes ProcessButton() in GroupedPermissionInfobar to go through delegate's
Accept() and Cancel() methods, which asks PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

[dominickn, 2016-10-26 05:01:45]

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:93:
permission_prompt_(permission_prompt) {}
Is it ever valid to create a GroupedPermissionInfoBarDelegate with a nullptr
PermissionPromptAndroid? If not, maybe DCHECK(permission_prompt) here and remove
the null checks in Accept() and Cancel()?

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:75:
PermissionPromptAndroid* permission_prompt_;
Nit: add a comment to say the PermissionPromptAndroid's lifetime matches that of
this object as it controls the lifetime of the infobar which we are a delegate
to.

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right):

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:45:
DCHECK((action == InfoBarAndroid::ACTION_OK) ||
Do you actually need this change? ConfirmInfoBar::ProcessButton does exactly
this, so it should work if you don't change GroupedPermissionInfoBar at all
(it's call the overridden delegate method).

[lshang, 2016-10-26 05:42:04]

Patchset #5 (id:100001) has been deleted

[lshang, 2016-10-26 05:43:23]

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:93:
permission_prompt_(permission_prompt) {}
On 2016/10/26 05:01:45, dominickn wrote:
> Is it ever valid to create a GroupedPermissionInfoBarDelegate with a nullptr
> PermissionPromptAndroid? If not, maybe DCHECK(permission_prompt) here and
remove
> the null checks in Accept() and Cancel()?

Done. DCHECK added. The delegate should not be created if
PermissionPropmtAndroid is null.

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/permissi...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:75:
PermissionPromptAndroid* permission_prompt_;
On 2016/10/26 05:01:45, dominickn wrote:
> Nit: add a comment to say the PermissionPromptAndroid's lifetime matches that
of
> this object as it controls the lifetime of the infobar which we are a delegate
> to.

Done.

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/grouped_permission_infobar.cc (right):

https://codereview.chromium.org/2440403002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/grouped_permission_infobar.cc:45:
DCHECK((action == InfoBarAndroid::ACTION_OK) ||
On 2016/10/26 05:01:45, dominickn wrote:
> Do you actually need this change? ConfirmInfoBar::ProcessButton does exactly
> this, so it should work if you don't change GroupedPermissionInfoBar at all
> (it's call the overridden delegate method).

Done.

[dominickn, 2016-10-26 05:57:54]

lgtm - also you should change the CL description to be something like

"Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid"

[lshang, 2016-10-26 06:00:35]

Description was changed from

==========
Make ProcessButton() of GroupedPermissionInfobar to go through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and changes ProcessButton() in GroupedPermissionInfobar to go through delegate's
Accept() and Cancel() methods, which asks PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and changes ProcessButton() in GroupedPermissionInfobar to go through delegate's
Accept() and Cancel() methods, which asks PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

[lshang, 2016-10-26 06:00:57]

On 2016/10/26 05:57:54, dominickn wrote:
> lgtm - also you should change the CL description to be something like
> 
> "Make GroupedPermissionInfoBarDelegate's methods call through
> PermissionPromptAndroid"

Yeah you're right. Thanks for reminding!

[lshang, 2016-10-26 06:02:47]

Description was changed from

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and changes ProcessButton() in GroupedPermissionInfobar to go through delegate's
Accept() and Cancel() methods, which asks PermissionPromptAndroid to handle 
accept or deny actions. PermissionPromptAndroid defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and adds Accept() and Cancel() methods to the delegate, which asks 
PermissionPromptAndroid to handle accept or deny actions. then defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

[lshang, 2016-10-26 06:04:18]

lshang@chromium.org changed reviewers:
+ raymes@chromium.org

[lshang, 2016-10-26 06:04:18]

Raymes, PTAL thanks! This CL makes accept/cancel action to go back to
PermissionRequestManager so that the result is properly handled.

[raymes, 2016-10-27 01:32:33]

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:30:
base::WrapUnique(new GroupedPermissionInfoBarDelegate(
nit: can this be MakeUnique too?

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:76: //
controls the lifetime of the infobar which we are a delegate to.
Hmm - it's not immediately clear to me that the PermissionPromptAndroid's
lifetime will outlast the InfoBar's lifetime, since the InfoBar is actually
managed by the InfoBarService. For example, even calling RemoveInfobar won't
necessarily cause the Infobar to be deleted synchronously. I wonder if we need
to hold a WeakPtr here? I'm not certain though - it's not simple to reason about
at least. Do you or Dom have any insights?

[lshang, 2016-10-28 03:18:48]

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc:30:
base::WrapUnique(new GroupedPermissionInfoBarDelegate(
On 2016/10/27 01:32:33, raymes wrote:
> nit: can this be MakeUnique too?

No, because the constructor of GroupedPermissionInfoBarDelegate is private.

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/grouped_permission_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2440403002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/grouped_permission_infobar_delegate_android.h:76: //
controls the lifetime of the infobar which we are a delegate to.
On 2016/10/27 01:32:33, raymes wrote:
> Hmm - it's not immediately clear to me that the PermissionPromptAndroid's
> lifetime will outlast the InfoBar's lifetime, since the InfoBar is actually
> managed by the InfoBarService. For example, even calling RemoveInfobar won't
> necessarily cause the Infobar to be deleted synchronously. I wonder if we need
> to hold a WeakPtr here? I'm not certain though - it's not simple to reason
about
> at least. Do you or Dom have any insights?

I think your point makes sense, that's a tricky one. PermissionRequestManager
will delete PermissionPrompt after calls to hide bubble, but I'm not sure if
user could still interact with bubble's buttons during this time, or the bubble
close animation got delayed for some reason. 

Use WeakPtr should be safer. Or we could also dynamically get PermissionPrompt
from PermissionRequestManager to see if it has been destroyed or not.

[lshang, 2016-10-31 08:21:46]

Patchset #6 (id:140001) has been deleted

[lshang, 2016-10-31 08:24:40]

raymes, I've updated the patch. As we discussed, add Set() and Reset() to avoid
calling destroyed permission_prompt pointer in infobar delegate.

[raymes, 2016-11-01 00:07:46]

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:41:
infobar_delegate->SetPermissionPrompt(this);
Now that I look at it, I think passing this pointer in the constructor as you
had before is slightly better, because it means SetPermissionPrompt won't
accidentally be called multiple times.

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:55:
infobar_->delegate()->AsGroupedPermissionInfoBarDelegate();
I don't think we need to add this function, I think we can just static_cast the
infobar delegate to the GroupedPermissionInfobarDelegate because we know it will
be one of those.

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:56:
infobar_delegate->ResetPermissionPrompt();
Maybe this should just go in the destructor of this class? In terms of lifetimes
it should be safe to call as long as this class is alive, so it might make it
slightly clearer what's going on. You could also name the function
PermissionPromptDestroyed

[lshang, 2016-11-01 04:32:50]

Patchset #7 (id:180001) has been deleted

[lshang, 2016-11-01 04:33:42]

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:41:
infobar_delegate->SetPermissionPrompt(this);
On 2016/11/01 00:07:46, raymes wrote:
> Now that I look at it, I think passing this pointer in the constructor as you
> had before is slightly better, because it means SetPermissionPrompt won't
> accidentally be called multiple times.

Done.

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:55:
infobar_->delegate()->AsGroupedPermissionInfoBarDelegate();
On 2016/11/01 00:07:46, raymes wrote:
> I don't think we need to add this function, I think we can just static_cast
the
> infobar delegate to the GroupedPermissionInfobarDelegate because we know it
will
> be one of those.

Refer to other infobars, the convention seems to use a As***InfoBarDelegate() to
do the
downcasting(https://cs.chromium.org/chromium/src/components/infobars/core/infobar_delegate.h?l=213).


I don't feel strongly about it here, since we already use static_cast in
GroupedPermissionInfoBar::GetDelegate(), so proooobably it's ok?

Done.

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:56:
infobar_delegate->ResetPermissionPrompt();
On 2016/11/01 00:07:46, raymes wrote:
> Maybe this should just go in the destructor of this class? In terms of
lifetimes
> it should be safe to call as long as this class is alive, so it might make it
> slightly clearer what's going on. You could also name the function
> PermissionPromptDestroyed

Done.

[raymes, 2016-11-01 05:29:16]

lgtm

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2440403002/diff/160001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:55:
infobar_->delegate()->AsGroupedPermissionInfoBarDelegate();
On 2016/11/01 04:33:42, lshang wrote:
> On 2016/11/01 00:07:46, raymes wrote:
> > I don't think we need to add this function, I think we can just static_cast
> the
> > infobar delegate to the GroupedPermissionInfobarDelegate because we know it
> will
> > be one of those.
> 
> Refer to other infobars, the convention seems to use a As***InfoBarDelegate()
to
> do the
>
downcasting(https://cs.chromium.org/chromium/src/components/infobars/core/infobar_delegate.h?l=213).
> 
> 
> I don't feel strongly about it here, since we already use static_cast in
> GroupedPermissionInfoBar::GetDelegate(), so proooobably it's ok?
> 
> Done.

If you know the type ahead of time (which I think in these cases we do, right?)
then a static cast is better because it avoids the virtual function call.

https://codereview.chromium.org/2440403002/diff/200001/chrome/browser/permiss...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2440403002/diff/200001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:24:
static_cast<GroupedPermissionInfoBarDelegate*>(infobar_->delegate());
Could infobar_ be an GroupedPermissionInfoBar? Then you could just call
infobar_->GetDelegate() here?

[lshang, 2016-11-01 22:58:55]

Thanks Raymes!

https://codereview.chromium.org/2440403002/diff/200001/chrome/browser/permiss...
File chrome/browser/permissions/permission_prompt_android.cc (right):

https://codereview.chromium.org/2440403002/diff/200001/chrome/browser/permiss...
chrome/browser/permissions/permission_prompt_android.cc:24:
static_cast<GroupedPermissionInfoBarDelegate*>(infobar_->delegate());
On 2016/11/01 05:29:15, raymes wrote:
> Could infobar_ be an GroupedPermissionInfoBar? Then you could just call
> infobar_->GetDelegate() here? 

GetDelegate() is a private method of infobar, so I think static cast
InfobarDelegate is better here.

[lshang, 2016-11-01 22:59:00]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-11-01 22:59:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2440403002/#ps200001
(title: "address")

[commit-bot: I haz the power, 2016-11-01 22:59:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 23:35:32]

Description was changed from

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and adds Accept() and Cancel() methods to the delegate, which asks 
PermissionPromptAndroid to handle accept or deny actions. then defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and adds Accept() and Cancel() methods to the delegate, which asks 
PermissionPromptAndroid to handle accept or deny actions. then defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

[commit-bot: I haz the power, 2016-11-01 23:35:33]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-11-01 23:38:28]

Description was changed from

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to 
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate 
and adds Accept() and Cancel() methods to the delegate, which asks 
PermissionPromptAndroid to handle accept or deny actions. then defers them to 
PermissionRequestManager, which will store UMA metrics, store settings of each 
requests, and remove requests in its request list.

BUG=606138
==========

to

==========
Make GroupedPermissionInfoBarDelegate's methods call through
PermissionPromptAndroid

On desktop, when 'Allow' or 'Block' button gets clicked, it will go through
PermissionPromptImpl, and PermissionPromptImpl defers it to
PermissionRequestManager(also a Delegate) to handle permission accept or
deny.

On Android, for GroupedPermissionInfoBars, 'Allow' and 'Block' button click
gets handled in ProcessButton(), which will calls into InfoBarDelegates, and
currently in ConfirmInfobarDelegate they just do nothing and return true.

This CL adds PermissionPromptAndroid* to GroupedPermissionInfobarDelegate
and adds Accept() and Cancel() methods to the delegate, which asks
PermissionPromptAndroid to handle accept or deny actions. then defers them to
PermissionRequestManager, which will store UMA metrics, store settings of each
requests, and remove requests in its request list.

BUG=606138

Committed: https://crrev.com/6dcb3141c8e832d80a4f83e8a4e235319908e57e
Cr-Commit-Position: refs/heads/master@{#429154}
==========

[commit-bot: I haz the power, 2016-11-01 23:38:31]

Patchset 7 (id:??) landed as
https://crrev.com/6dcb3141c8e832d80a4f83e8a4e235319908e57e
Cr-Commit-Position: refs/heads/master@{#429154}