Remove PermissionInfoBarDelegate from desktop builds.

Remove PermissionInfoBarDelegate from desktop builds.

This CL compiles PermissionInfoBarDelegate only on Android. It also
refactors the delegate subclasses to move their Create methods to
PermissionInfoBarDelegate, simplifying the PermissionsQueueController.

This refactor simplifies the work necessary to allow infobars to be
switched out for modal permission prompt dialogs by centralising the
delegate creation logic.

BUG=658125
Committed: https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275
Cr-Commit-Position: refs/heads/master@{#427202}

[dominickn, 2016-10-21 06:02:52]

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

[commit-bot: I haz the power, 2016-10-21 06:03:13]

Dry run: 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-10-21 07:17:53]

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

[commit-bot: I haz the power, 2016-10-21 07:17:54]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-23 22:17:47]

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

[dominickn, 2016-10-23 22:17:48]

Hey raymes, WDYT? This consolidates the PermissionInfoBarDelegate creation,
which will make adding a modal prompt easier (one callsite). I think lshang's
eventual plan is to remove the subclasses of PermissionInfoBarDelegate, so this
is a step on that path too.

If you're okay I'll TBR a chrome/browser reviewer for everything else.

[raymes, 2016-10-24 01:00:36]

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

[raymes, 2016-10-24 01:00:36]

It's not exactly clear how this will make what you're trying to do easier, but
it seems nicer to me anyway :) Do you have followup CL sketches to see where
you're headed?

Liu's working in this area - so I'll let her lg first but otherwise it looks
good.

https://codereview.chromium.org/2443463003/diff/1/chrome/browser/media/protec...
File chrome/browser/media/protected_media_identifier_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2443463003/diff/1/chrome/browser/media/protec...
chrome/browser/media/protected_media_identifier_infobar_delegate_android.h:22:
protected:
nit: can this be private like the others?

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

https://codereview.chromium.org/2443463003/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_queue_controller.cc:127: type_ ==
content::PermissionType::PROTECTED_MEDIA_IDENTIFIER);
nit: I'm not sure this is necessary as we should hit a NOTREACHED in
PermissionInfoBarDelegate::Create

[dominickn, 2016-10-24 01:39:38]

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

[commit-bot: I haz the power, 2016-10-24 01:39:43]

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

[dominickn, 2016-10-24 01:42:00]

Thanks! I discussed this with lshang@ last week, so she should be across the
planned changes.

This assists the modal permissions dialog by centralising the Create logic in
PermissionInfoBarDelegate. The subclasses don't do anything special there; and
the method currently returns the created infobar (which has already been added
to the Android infobar stack). For modals, we need a separate Create method
since modals can't be stacked and we don't want them to create an infobar (for
now, while we support both, modals will just reuse the InfoBarDelegate).

https://codereview.chromium.org/2443463003/diff/1/chrome/browser/media/protec...
File chrome/browser/media/protected_media_identifier_infobar_delegate_android.h
(right):

https://codereview.chromium.org/2443463003/diff/1/chrome/browser/media/protec...
chrome/browser/media/protected_media_identifier_infobar_delegate_android.h:22:
protected:
On 2016/10/24 01:00:36, raymes wrote:
> nit: can this be private like the others?

Done.

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

https://codereview.chromium.org/2443463003/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_queue_controller.cc:127: type_ ==
content::PermissionType::PROTECTED_MEDIA_IDENTIFIER);
On 2016/10/24 01:00:36, raymes wrote:
> nit: I'm not sure this is necessary as we should hit a NOTREACHED in
> PermissionInfoBarDelegate::Create

Done.

[commit-bot: I haz the power, 2016-10-24 04:04:53]

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

[commit-bot: I haz the power, 2016-10-24 04:04:54]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-10-24 06:16:20]

Assuming lshang doesn't have any concerns, lgtm

[lshang, 2016-10-24 07:10:55]

lgtm

https://codereview.chromium.org/2443463003/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2443463003/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.cc:78: }
Since you're here, could you also move the method GetMessageText() up here,
between ShouldShowPersistenceToggle() and the constructor? so that they're of
the same order as declaration.

[dominickn, 2016-10-24 08:32:54]

Thanks!

+thestig: chrome/browser/* rubber stamp. This CL moves identical logic from
PermissionInfoBarDelegate's subclasses into PermissionInfoBarDelegate, in
preparation for introducing a modal permission dialog experiment and the
eventual deletion of the subclasses.

https://codereview.chromium.org/2443463003/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_infobar_delegate.cc (right):

https://codereview.chromium.org/2443463003/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.cc:78: }
On 2016/10/24 07:10:55, lshang wrote:
> Since you're here, could you also move the method GetMessageText() up here,
> between ShouldShowPersistenceToggle() and the constructor? so that they're of
> the same order as declaration.

Done.

[dominickn, 2016-10-24 08:33:27]

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

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

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

[dominickn, 2016-10-24 08:35:48]

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

[dominickn, 2016-10-24 08:35:49]

Actually +thestig for chrome/browser/* rubber stamp.

This CL moves identical logic from
PermissionInfoBarDelegate's subclasses into PermissionInfoBarDelegate, in
preparation for introducing a modal permission dialog experiment and the
eventual deletion of the subclasses.

[commit-bot: I haz the power, 2016-10-24 09:43:35]

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

[commit-bot: I haz the power, 2016-10-24 09:43:36]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-10-24 20:36:44]

lgtm

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:36: static
infobars::InfoBar* Create(content::PermissionType type,
Any chance we can change this to return a std::unique_ptr while we are here?

[dominickn, 2016-10-24 23:01:20]

Thanks!

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:36: static
infobars::InfoBar* Create(content::PermissionType type,
On 2016/10/24 20:36:44, Lei Zhang wrote:
> Any chance we can change this to return a std::unique_ptr while we are here?

I don't think that's safe; the InfoBarService owns the infobar and its lifetime.
See infobars::Infobar::CloseSoon, which is called from
InfoBarService::RemoveInfoBar, which is called from PermissionQueueController.
CloseSoon deletes this.

[Lei Zhang, 2016-10-24 23:04:21]

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:36: static
infobars::InfoBar* Create(content::PermissionType type,
On 2016/10/24 23:01:20, dominickn wrote:
> On 2016/10/24 20:36:44, Lei Zhang wrote:
> > Any chance we can change this to return a std::unique_ptr while we are here?
> 
> I don't think that's safe; the InfoBarService owns the infobar and its
lifetime.
> See infobars::Infobar::CloseSoon, which is called from
> InfoBarService::RemoveInfoBar, which is called from PermissionQueueController.
> CloseSoon deletes this.

Can you add a comment to explain this? Oftentimes a Create() method passes
ownership to the caller and it is not obvious that's not happening here.

[dominickn, 2016-10-24 23:22:45]

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

[commit-bot: I haz the power, 2016-10-24 23:23:42]

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

[dominickn, 2016-10-24 23:26:04]

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
File chrome/browser/permissions/permission_infobar_delegate.h (right):

https://codereview.chromium.org/2443463003/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:36: static
infobars::InfoBar* Create(content::PermissionType type,
On 2016/10/24 23:04:21, Lei Zhang wrote:
> On 2016/10/24 23:01:20, dominickn wrote:
> > On 2016/10/24 20:36:44, Lei Zhang wrote:
> > > Any chance we can change this to return a std::unique_ptr while we are
here?
> > 
> > I don't think that's safe; the InfoBarService owns the infobar and its
> lifetime.
> > See infobars::Infobar::CloseSoon, which is called from
> > InfoBarService::RemoveInfoBar, which is called from
PermissionQueueController.
> > CloseSoon deletes this.
> 
> Can you add a comment to explain this? Oftentimes a Create() method passes
> ownership to the caller and it is not obvious that's not happening here.

Done.

[commit-bot: I haz the power, 2016-10-25 00:54:41]

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

[commit-bot: I haz the power, 2016-10-25 00:54:42]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-10-25 00:58:55]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-10-25 00:58:55]

The patchset sent to the CQ was uploaded after l-g-t-m from lshang@chromium.org,
raymes@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2443463003/#ps60001
(title: "Add comment explaining lifetime")

[commit-bot: I haz the power, 2016-10-25 00:59:34]

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-10-25 01:05:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-25 01:08:48]

Description was changed from

==========
Remove PermissionInfoBarDelegate from desktop builds.

This CL compiles PermissionInfoBarDelegate only on Android. It also
refactors the delegate subclasses to move their Create methods to
PermissionInfoBarDelegate, simplifying the PermissionsQueueController.

This refactor simplifies the work necessary to allow infobars to be
switched out for modal permission prompt dialogs by centralising the
delegate creation logic.

BUG=658125
==========

to

==========
Remove PermissionInfoBarDelegate from desktop builds.

This CL compiles PermissionInfoBarDelegate only on Android. It also
refactors the delegate subclasses to move their Create methods to
PermissionInfoBarDelegate, simplifying the PermissionsQueueController.

This refactor simplifies the work necessary to allow infobars to be
switched out for modal permission prompt dialogs by centralising the
delegate creation logic.

BUG=658125

Committed: https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275
Cr-Commit-Position: refs/heads/master@{#427202}
==========

[commit-bot: I haz the power, 2016-10-25 01:08:49]

Patchset 4 (id:??) landed as
https://crrev.com/67e6efd22bbef2baea0ac32fb603659f2ee93275
Cr-Commit-Position: refs/heads/master@{#427202}