permissions: switch from explicitly passing queue controller to callbacks

permissions: switch from explicitly passing queue controller to callbacks

Explicitly passing a raw QueueController object is extremely bad design
as it leads to inflexibility plus questions about lifetimes and ownership
of the pointer.

Moreover, with the new multiple permissions work, queue controller is
disappearing so using a callback means its replacement can be dropped
in without any modifications to the delegates.

This CL depends on https://codereview.chromium.org/1343553003/

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

BUG=516626
Committed: https://crrev.com/152f7f2870e2745de968e9dc21b1ee34099b967c
Cr-Commit-Position: refs/heads/master@{#349151}

[Lalit Maganti, 2015-09-11 10:20:46]

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

[Lalit Maganti, 2015-09-11 10:20:47]

Mounir: PTAL when you can.

[mlamouri (slow - plz ping), 2015-09-15 12:56:00]

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:52: void
ProtectedMediaIdentifierInfoBarDelegate::SetPermission(
Why is that needed? Could it directly inherits from PermissionInfobarDelegate?

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.h (right):

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.h:10: #include
"base/callback.h"
Use callback_forward.h

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

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:26:
PermissionInfobarDelegate(const PermissionRequestID& id,
Do you still need this? |id| doesn't seem to be used anymore.

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:29: const
base::Callback<void(bool, bool)>& callback);
nit:  could you name the callback so readers know what is is for?
using PermissionSetCallback = base::Callback<void(bool, bool)>;

And have all the classes inheriting from PermissionInfobarDelegate uses this
name.

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

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_queue_controller.cc:116:
base::Callback<void(bool, bool)> callback =
rename to PermissionInfobarDelegate::PermissionSetCallback

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_queue_controller.cc:118:
base::Unretained(controller),
Add a comment explaining why base::Unretained() is safe here.

[Lalit Maganti, 2015-09-15 13:16:54]

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:52: void
ProtectedMediaIdentifierInfoBarDelegate::SetPermission(
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> Why is that needed? Could it directly inherits from PermissionInfobarDelegate?

IDK why but right now it does not inherit from PermissionInfobarDelegate. I
could make it inherit but I thought it would just pollute this change.

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.h (right):

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.h:10: #include
"base/callback.h"
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> Use callback_forward.h

ATM I can't because I store a callback but if I do add the subclassing of
PermissionInfobarDelegate then I could do this I guess.

[Lalit Maganti, 2015-09-15 14:19:25]

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:52: void
ProtectedMediaIdentifierInfoBarDelegate::SetPermission(
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> Why is that needed? Could it directly inherits from PermissionInfobarDelegate?

Done.

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.h (right):

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.h:10: #include
"base/callback.h"
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> Use callback_forward.h

Done.

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

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:26:
PermissionInfobarDelegate(const PermissionRequestID& id,
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> Do you still need this? |id| doesn't seem to be used anymore.

Done.

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_infobar_delegate.h:29: const
base::Callback<void(bool, bool)>& callback);
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> nit:  could you name the callback so readers know what is is for?
> using PermissionSetCallback = base::Callback<void(bool, bool)>;
> 
> And have all the classes inheriting from PermissionInfobarDelegate uses this
> name.

Done.

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

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_queue_controller.cc:116:
base::Callback<void(bool, bool)> callback =
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> rename to PermissionInfobarDelegate::PermissionSetCallback

Done.

https://codereview.chromium.org/1332293002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_queue_controller.cc:118:
base::Unretained(controller),
On 2015/09/15 12:55:59, Mounir Lamouri wrote:
> Add a comment explaining why base::Unretained() is safe here.

Done.

[Lalit Maganti, 2015-09-15 17:14:56]

Mounir: PTAL

[mlamouri (slow - plz ping), 2015-09-15 17:26:42]

lgtm

https://codereview.chromium.org/1332293002/diff/40001/chrome/browser/media/pr...
File chrome/browser/media/protected_media_identifier_infobar_delegate.cc
(right):

https://codereview.chromium.org/1332293002/diff/40001/chrome/browser/media/pr...
chrome/browser/media/protected_media_identifier_infobar_delegate.cc:8: #include
"chrome/browser/permissions/permission_request_id.h"
nit: remove this include.

[Lalit Maganti, 2015-09-15 17:34:31]

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

[Lalit Maganti, 2015-09-15 17:34:32]

jochen@: could you please take a look at all the non chrome/browser/permissions
changes?

[jochen (gone - plz use gerrit), 2015-09-16 11:20:12]

lgtm

[Lalit Maganti, 2015-09-16 16:03:19]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-16 16:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1332293002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-16 16:03:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1332293002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1332293002/80001

[commit-bot: I haz the power, 2015-09-16 17:30:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-16 17:31:26]

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

[commit-bot: I haz the power, 2015-09-23 12:57:17]

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