Clean up the PermissionInfoBarDelegate hierarchy.

Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
Committed: https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de
Cr-Commit-Position: refs/heads/master@{#412754}

[dominickn, 2016-08-16 06:12:42]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 06:12:53]

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

[dominickn, 2016-08-16 06:21:27]

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

[dominickn, 2016-08-16 06:21:28]

Hey raymes, WDYT?

[commit-bot: I haz the power, 2016-08-16 06:51:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 06:51:51]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-17 03:12:12]

Description was changed from

==========
Reorganise the PermissionInfoBarDelegate hierarchy.

This CL cleans up PermissionInfoBarDelegate. The changes include:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Splits PermissionInfoBarDelegate into a base class which
   is subclassed by SinglePermissionInfoBarDelegate (which subsumes most
   of the functionality formerly in PermissionInfoBarDelegate) and
   GroupedInfoBarDelegate.
 - Changes all existing subclasses of PermissionInfoBarDelegate to
   subclass SinglePermissionInfoBarDelegate.
 - Removes redundant requesting_frame_/requesting_origin_ members.

BUG=638076
==========

to

==========
Reorganise the PermissionInfoBarDelegate hierarchy.

This CL cleans up PermissionInfoBarDelegate. The changes include:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Splits PermissionInfoBarDelegate into a base class which
   is subclassed by SinglePermissionInfoBarDelegate (which subsumes most
   of the functionality formerly in PermissionInfoBarDelegate) and
   GroupedInfoBarDelegate.
 - Changes all existing subclasses of PermissionInfoBarDelegate to
   subclass SinglePermissionInfoBarDelegate.
 - Removes redundant requesting_frame_/requesting_origin_ members.

BUG=638076
==========

[raymes, 2016-08-17 03:20:42]

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

[raymes, 2016-08-17 03:20:43]

+tsergeant - could you ptal too (I think you know this code a little better than
me)

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_infobar_delegate.h:17: // Base class for
permission infobars.
nit: fill 80chars or new paragraph

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_infobar_delegate.h:24: virtual
ContentSettingsType GetContentSettingType(int index) const = 0;
It seems like we only ever need the first content setting when accessing it
through this interface? Why not just keep the old  API which has the non-virtual
getter and the member?

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/single_permission_infobar_delegate.h (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/single_permission_infobar_delegate.h:19: // that the
accept/deny buttons grant/deny the relevant permission.
nit: fill 80 chars or new paragraph

[dominickn, 2016-08-17 03:29:38]

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_infobar_delegate.h:17: // Base class for
permission infobars.
On 2016/08/17 03:20:43, raymes wrote:
> nit: fill 80chars or new paragraph

Done.

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_infobar_delegate.h:24: virtual
ContentSettingsType GetContentSettingType(int index) const = 0;
On 2016/08/17 03:20:43, raymes wrote:
> It seems like we only ever need the first content setting when accessing it
> through this interface? Why not just keep the old  API which has the
non-virtual
> getter and the member?

The GroupedPermissionInfobarDelegate uses the index access, but the old API
doesn't make much sense for Grouped. I concluded that having the virtual API
with DCHECKs was the nicest way to implement this method (we want it on
PermissionInfoBarDelegate because it's accessed via PermissionInfoBar).

If we want to keep the old API we'll need to either change some of the
type-casting (it doesn't make sense to get a single content setting back from
the GroupedInfoBarDelegate) and move it to SinglePermissionInfoBarDelegate.

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/single_permission_infobar_delegate.h (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/single_permission_infobar_delegate.h:19: // that the
accept/deny buttons grant/deny the relevant permission.
On 2016/08/17 03:20:43, raymes wrote:
> nit: fill 80 chars or new paragraph

Done.

[raymes, 2016-08-17 03:36:40]

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/single_permission_infobar_delegate.cc (left):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/single_permission_infobar_delegate.cc:86:
PermissionUmaUtil::PermissionPromptAcceptedWithPersistenceToggle(
We're not calling this from anywhere anymore?

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/single_permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/single_permission_infobar_delegate.cc:69:
PermissionAction decision) {
nit: indentation. git cl format should catch this?

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/single_permission_infobar_delegate.h (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/single_permission_infobar_delegate.h:30:
base::string16 GetMessageText() const override;
I think this can be private? It's public in the base class but I don't think it
needs to be public here?

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/single_permission_infobar_delegate.h:40: virtual int
GetMessageResourceId() const = 0;
I think this can be private?

[raymes, 2016-08-17 03:44:41]

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2250053002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_infobar_delegate.h:24: virtual
ContentSettingsType GetContentSettingType(int index) const = 0;
On 2016/08/17 03:29:38, dominickn wrote:
> On 2016/08/17 03:20:43, raymes wrote:
> > It seems like we only ever need the first content setting when accessing it
> > through this interface? Why not just keep the old  API which has the
> non-virtual
> > getter and the member?
> 
> The GroupedPermissionInfobarDelegate uses the index access, but the old API
> doesn't make much sense for Grouped. I concluded that having the virtual API
> with DCHECKs was the nicest way to implement this method (we want it on
> PermissionInfoBarDelegate because it's accessed via PermissionInfoBar).
> 
> If we want to keep the old API we'll need to either change some of the
> type-casting (it doesn't make sense to get a single content setting back from
> the GroupedInfoBarDelegate) and move it to SinglePermissionInfoBarDelegate.

Hmm it's unclear whether the one callsite which currently uses this works
properly for grouped infobars? There are also other similar functions in the
grouped infobar delegate.

[dominickn, 2016-08-17 05:28:35]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 05:28:54]

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

[dominickn, 2016-08-17 05:44:07]

Description was changed from

==========
Reorganise the PermissionInfoBarDelegate hierarchy.

This CL cleans up PermissionInfoBarDelegate. The changes include:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Splits PermissionInfoBarDelegate into a base class which
   is subclassed by SinglePermissionInfoBarDelegate (which subsumes most
   of the functionality formerly in PermissionInfoBarDelegate) and
   GroupedInfoBarDelegate.
 - Changes all existing subclasses of PermissionInfoBarDelegate to
   subclass SinglePermissionInfoBarDelegate.
 - Removes redundant requesting_frame_/requesting_origin_ members.

BUG=638076
==========

to

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
==========

[dominickn, 2016-08-17 05:45:45]

Revamped based on comments. No longer making the grouped infobar delegate
inherit; I will implement that separately. PTAL, thanks.

[commit-bot: I haz the power, 2016-08-17 06:12:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 06:12:53]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-17 09:58:44]

Patchset #3 (id:40001) has been deleted

[dominickn, 2016-08-17 10:03:57]

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

[dominickn, 2016-08-17 10:04:52]

Description was changed from

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
==========

to

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
==========

[dominickn, 2016-08-17 10:16:31]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 10:16:49]

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

[dominickn, 2016-08-17 10:24:26]

Patchset #4 (id:80001) has been deleted

[dominickn, 2016-08-17 10:27:07]

+pkasting: PTAL at components/ and chrome/ui. This is a cleanup CL where one
change is renaming PermissionInfobarDelegate to PermissionInfoBarDelegate.

[commit-bot: I haz the power, 2016-08-17 10:33:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 10:33:25]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-08-17 23:02:23]

LGTM

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

https://codereview.chromium.org/2250053002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:45: // geolocation.
Nit: This comment merely restates the code; remove it.

https://codereview.chromium.org/2250053002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:48: return false;
Nit: Simpler:

  return (permission_type_ == content::PermissionType::GEOLOCATION) &&
         PermissionUtil::ShouldShowPersistenceToggle();

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

https://codereview.chromium.org/2250053002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.h:56: virtual int
GetMessageResourceId() const = 0;
Nit: Don't place this in the middle of the ConfirmInfoBarDelegate overrides. 
Put it below that list.  (Then move the declaration and definition in
ProtectedMediaIdentifierInfoBarDelegateAndroid to the bottom of its list of
overrides as well, to match this order.)

[dominickn, 2016-08-17 23:29:28]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2016-08-17 23:29:42]

Thanks!

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

https://codereview.chromium.org/2250053002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:45: // geolocation.
On 2016/08/17 23:02:22, Peter Kasting wrote:
> Nit: This comment merely restates the code; remove it.

Done.

https://codereview.chromium.org/2250053002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:48: return false;
On 2016/08/17 23:02:22, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return (permission_type_ == content::PermissionType::GEOLOCATION) &&
>          PermissionUtil::ShouldShowPersistenceToggle();

Done.

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

https://codereview.chromium.org/2250053002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.h:56: virtual int
GetMessageResourceId() const = 0;
On 2016/08/17 23:02:22, Peter Kasting wrote:
> Nit: Don't place this in the middle of the ConfirmInfoBarDelegate overrides. 
> Put it below that list.  (Then move the declaration and definition in
> ProtectedMediaIdentifierInfoBarDelegateAndroid to the bottom of its list of
> overrides as well, to match this order.)

Done.

[commit-bot: I haz the power, 2016-08-17 23:29:52]

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

[raymes, 2016-08-18 00:58:43]

Thanks this feels simpler.

lgtm

https://codereview.chromium.org/2250053002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_delegate.cc (left):

https://codereview.chromium.org/2250053002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:19: permission_type_);
:( I don't think this was in the last CL when I reviewed it last. It means that
it's not really as nice to have the thing as a member of the permission context.
I'd suggest moving changes like this to followup CLs to make them clear in the
future.

[dominickn, 2016-08-18 01:06:10]

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

[dominickn, 2016-08-18 01:06:11]

Thanks!

+thestig: RS for API/rename/cleanup changes in chrome/browser

https://codereview.chromium.org/2250053002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_delegate.cc (left):

https://codereview.chromium.org/2250053002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:19: permission_type_);
On 2016/08/18 00:58:43, raymes wrote:
> :( I don't think this was in the last CL when I reviewed it last. It means
that
> it's not really as nice to have the thing as a member of the permission
context.
> I'd suggest moving changes like this to followup CLs to make them clear in the
> future.

It's a bit sad that the ignore metrics are recorded in this way. My eventual
plan is to move the RecordIgnore call into PermissionUmaUtil::PermissionIgnored
(and make it static in the process). That will help centralise the recording
more.

[commit-bot: I haz the power, 2016-08-18 01:10:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 01:10:53]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-08-18 01:17:45]

https://codereview.chromium.org/2250053002/diff/120001/chrome/browser/permiss...
File chrome/browser/permissions/permission_infobar_delegate.cc (left):

https://codereview.chromium.org/2250053002/diff/120001/chrome/browser/permiss...
chrome/browser/permissions/permission_infobar_delegate.cc:19: permission_type_);
On 2016/08/18 01:06:11, dominickn wrote:
> On 2016/08/18 00:58:43, raymes wrote:
> > :( I don't think this was in the last CL when I reviewed it last. It means
> that
> > it's not really as nice to have the thing as a member of the permission
> context.
> > I'd suggest moving changes like this to followup CLs to make them clear in
the
> > future.
> 
> It's a bit sad that the ignore metrics are recorded in this way. My eventual
> plan is to move the RecordIgnore call into
PermissionUmaUtil::PermissionIgnored
> (and make it static in the process). That will help centralise the recording
> more.

Thanks - yeah the code in that last CL didn't evolve in the most helpful way for
us both. Originally there were global data members in the CL so it made sense to
have a global object, then those went away and so it made less sense to have the
thing as a global object and then that object needed to be used outside
PermissionContext base so having it as a member object hinders other uses. Arg!

[Lei Zhang, 2016-08-18 05:55:45]

lgtm

[dominickn, 2016-08-18 06:00:13]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-08-18 06:00:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2250053002/#ps120001
(title: "Address comments")

[commit-bot: I haz the power, 2016-08-18 06:00:29]

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-08-18 06:04:24]

Description was changed from

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
==========

to

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
==========

[commit-bot: I haz the power, 2016-08-18 06:04:26]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-08-18 06:06:18]

Description was changed from

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076
==========

to

==========
Clean up the PermissionInfoBarDelegate hierarchy.

This CL:

 - Renames PermissionInfobarDelegate to PermissionInfoBarDelegate
   (this is the only delegate class which doesn't follow the InfoBar
   naming convention).
 - Removes redundant requesting_frame_/requesting_origin_ members
   in PermissionInfoBarDelegate subclasses
 - Moves ShouldShowPersistenceToggle's feature query into
   PermissionUtil in preparation for the desktop and grouped infobar
   implementation
 - Moves ignore count incrementing after ignore action UMA, to be
   consistent with dismiss count UMA in PermissionContextBase.

BUG=638076

Committed: https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de
Cr-Commit-Position: refs/heads/master@{#412754}
==========

[commit-bot: I haz the power, 2016-08-18 06:06:20]

Patchset 5 (id:??) landed as
https://crrev.com/e0e576e7a078735d310ff0f66ccca2fbec6373de
Cr-Commit-Position: refs/heads/master@{#412754}