Move requests from Show() argument to PermissionPrompt::Delegate

Move requests from Show() argument to PermissionPrompt::Delegate

This patch moves the vector of permission requests, and the initial
vector of accept states, from arguments to PermissionPrompt::Show() to
values stored on the PermissionPrompt::Delegate. Since the vector is of
raw pointers, the PermissionPrompt could hold inadvertently hold on to
pointers from Show() after they have been freed. This is currently being
done on PermissionPromptAndroid, so this patch fixes this.

BUG=606138
Review-Url: https://codereview.chromium.org/2868783002
Cr-Commit-Position: refs/heads/master@{#471211}
Committed: https://chromium.googlesource.com/chromium/src/+/ca4d3f297d0b03d83711a9d17f808e299aa6ff19

[Timothy Loh, 2017-05-08 07:21:47]

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

[commit-bot: I haz the power, 2017-05-08 07:22:18]

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, 2017-05-08 07:39:47]

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

[commit-bot: I haz the power, 2017-05-08 07:39:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Timothy Loh, 2017-05-09 01:32:20]

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

[commit-bot: I haz the power, 2017-05-09 01:35:08]

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, 2017-05-09 04:46:56]

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

[commit-bot: I haz the power, 2017-05-09 04:46:59]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-05-10 03:42:17]

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

[commit-bot: I haz the power, 2017-05-10 03:43:23]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-05-10 03:49:08]

Description was changed from

==========
wip

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

to

==========
Move requests from Show() argument to PermissionPrompt::Delegate

This patch moves the vector of permission requests, and the initial
vector of accept states, from arguments to PermissionPrompt::Show() to
values stored on the PermissionPrompt::Delegate. Since the vector is of
raw pointers, the PermissionPrompt could hold inadvertently hold on to
pointers from Show() after they have been freed. This is currently being
done on PermissionPromptAndroid, so this patch fixes this.

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

[Timothy Loh, 2017-05-10 03:49:09]

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

[Timothy Loh, 2017-05-10 03:51:35]

I think this fixes some dodginess/crashing when navigating frames which have
shown prompts, will either add a test separately or with 2829023002.

[commit-bot: I haz the power, 2017-05-10 05:37:24]

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

[commit-bot: I haz the power, 2017-05-10 05:37:25]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-05-11 01:17:55]

lg a few minor comments.

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

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_prompt_android.h:37: size_t
permission_count() const { return delegate_->Requests().size(); }
nit: we should probably move the implementation into the cc file:
size_t PermissionCount

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller.mm:285:
const std::vector<PermissionRequest*>& requests = delegate->Requests();
nit: move this below the 3 lines below?

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h
(right):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h:35:
void SetRequests(std::vector<PermissionRequest*> requests) {
nit: set_requests

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h:62:
std::vector<PermissionRequest*> requests();
nit: no ()

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/permission_prompt.h (right):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/permission_prompt.h:32: virtual const
std::vector<PermissionRequest*>& Requests() = 0;
We may want to add a comment that the objects in the vector are owned by the
delegate

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (left):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:411:
bubble_delegate_->CloseBubble();
Should this be removed?

[Timothy Loh, 2017-05-11 04:38:00]

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

[commit-bot: I haz the power, 2017-05-11 04:38:45]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-05-11 04:41:55]

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

[Timothy Loh, 2017-05-11 04:42:08]

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

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/permissi...
chrome/browser/permissions/permission_prompt_android.h:37: size_t
permission_count() const { return delegate_->Requests().size(); }
On 2017/05/11 01:17:54, raymes wrote:
> nit: we should probably move the implementation into the cc file:
> size_t PermissionCount

Done.

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/permission_bubble/permission_bubble_controller.mm:285:
const std::vector<PermissionRequest*>& requests = delegate->Requests();
On 2017/05/11 01:17:54, raymes wrote:
> nit: move this below the 3 lines below?

I re-arranged it a bit.

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h
(right):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h:35:
void SetRequests(std::vector<PermissionRequest*> requests) {
On 2017/05/11 01:17:54, raymes wrote:
> nit: set_requests

Done.

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/permission_bubble_browser_test_util.h:62:
std::vector<PermissionRequest*> requests();
On 2017/05/11 01:17:54, raymes wrote:
> nit: no ()

It's a function... anyway I reworked it a bit (inlined the function).

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
File chrome/browser/ui/permission_bubble/permission_prompt.h (right):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/permi...
chrome/browser/ui/permission_bubble/permission_prompt.h:32: virtual const
std::vector<PermissionRequest*>& Requests() = 0;
On 2017/05/11 01:17:55, raymes wrote:
> We may want to add a comment that the objects in the vector are owned by the
> delegate

Added a comment.

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (left):

https://codereview.chromium.org/2868783002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:411:
bubble_delegate_->CloseBubble();
On 2017/05/11 01:17:55, raymes wrote:
> Should this be removed?

It's dead code (Show is only called when there's no prompt showing), but I'll
leave it in for now since it's not particularly relevant.

[commit-bot: I haz the power, 2017-05-11 04:43:15]

Dry run: CQ is trying da patch.

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

[Timothy Loh, 2017-05-11 04:43:23]

timloh@chromium.org changed reviewers:
+ rsesek@chromium.org

[Timothy Loh, 2017-05-11 04:43:24]

+rsesek for chrome/browser/ui/cocoa/permission_bubble OWNERS, thanks!

[raymes, 2017-05-11 05:01:12]

lgtm

[commit-bot: I haz the power, 2017-05-11 06:20:24]

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

[commit-bot: I haz the power, 2017-05-11 06:20:25]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-05-11 14:24:03]

lgtm

[Timothy Loh, 2017-05-12 03:38:53]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2017-05-12 03:39:20]

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, 2017-05-12 03:45:00]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494560333925690,
"parent_rev": "99308ead46a0a196f4b8ba3efbb82b7a16d8d630", "commit_rev":
"ca4d3f297d0b03d83711a9d17f808e299aa6ff19"}

[commit-bot: I haz the power, 2017-05-12 03:45:14]

Description was changed from

==========
Move requests from Show() argument to PermissionPrompt::Delegate

This patch moves the vector of permission requests, and the initial
vector of accept states, from arguments to PermissionPrompt::Show() to
values stored on the PermissionPrompt::Delegate. Since the vector is of
raw pointers, the PermissionPrompt could hold inadvertently hold on to
pointers from Show() after they have been freed. This is currently being
done on PermissionPromptAndroid, so this patch fixes this.

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

to

==========
Move requests from Show() argument to PermissionPrompt::Delegate

This patch moves the vector of permission requests, and the initial
vector of accept states, from arguments to PermissionPrompt::Show() to
values stored on the PermissionPrompt::Delegate. Since the vector is of
raw pointers, the PermissionPrompt could hold inadvertently hold on to
pointers from Show() after they have been freed. This is currently being
done on PermissionPromptAndroid, so this patch fixes this.

BUG=606138

Review-Url: https://codereview.chromium.org/2868783002
Cr-Commit-Position: refs/heads/master@{#471211}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca4d3f297d0b03d83711a9d17f80...
==========

[commit-bot: I haz the power, 2017-05-12 03:45:15]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ca4d3f297d0b03d83711a9d17f80...