Ensure media permission requests are correctly cancelled when permission prompts are ignored

Ensure media permission requests are correctly cancelled when permission prompts are ignored

Currently when permission prompts are ignored, the PermissionManager
will still hold on to the callback that was passed in to
RequestPermission. |pending_requests_| is not cleaned up. This means
that the callers have no way of knowing if a request was ignored. This
CL changes the code so that |pending_requests_| is correctly cleaned up
when a prompt is ignored. This makes media permission prompts behave
correctly.

BUG=606630
Review-Url: https://codereview.chromium.org/2966963003
Cr-Commit-Position: refs/heads/master@{#484449}
Committed: https://chromium.googlesource.com/chromium/src/+/158a8c15d0f9aa1162c6a4583047b3a8c50046d6

[raymes, 2017-07-04 05:51:03]

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

[raymes, 2017-07-04 05:51:12]

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

[commit-bot: I haz the power, 2017-07-04 05:51:16]

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-07-04 06:17:15]

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

[commit-bot: I haz the power, 2017-07-04 06:17:16]

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

[raymes, 2017-07-04 23:05:59]

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

[commit-bot: I haz the power, 2017-07-04 23:06:10]

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-07-04 23:32:49]

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

[commit-bot: I haz the power, 2017-07-04 23:32:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2017-07-05 00:47:34]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-07-05 00:47:39]

The CQ bit was unchecked by raymes@chromium.org

[commit-bot: I haz the power, 2017-07-05 00:47:40]

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-07-05 00:47:42]

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

[commit-bot: I haz the power, 2017-07-05 00:47:43]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[raymes, 2017-07-05 00:47:44]

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

[commit-bot: I haz the power, 2017-07-05 00:47:50]

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-07-05 02:18:31]

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

[commit-bot: I haz the power, 2017-07-05 02:18:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[raymes, 2017-07-05 03:29:57]

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

[commit-bot: I haz the power, 2017-07-05 03:30:13]

Dry run: CQ is trying da patch.

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

[raymes, 2017-07-05 04:26:02]

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

[commit-bot: I haz the power, 2017-07-05 04:26:14]

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-07-05 05:31:52]

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

[commit-bot: I haz the power, 2017-07-05 05:31:52]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-07-05 07:32:50]

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

https://codereview.chromium.org/2966963003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:218:
permission_manager_->pending_requests_.Lookup(request_id_)) {
Is this check needed?

https://codereview.chromium.org/2966963003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:293:
DCHECK(pending_requests_.IsEmpty());
I'm not sure this works, do we need to move the logic in
~PermissionRequestManager() to PermissionRequestManager::ShutDown().

[raymes, 2017-07-06 00:05:43]

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

https://codereview.chromium.org/2966963003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:218:
permission_manager_->pending_requests_.Lookup(request_id_)) {
On 2017/07/05 07:32:49, Timothy Loh wrote:
> Is this check needed?

The request may contain multiple permissions and currently if any one them is
ignored, we remove the entire request, so when we get here, the request may have
been removed already. It didn't seem worth tracking ignores because ignore
implies that any future prompts from the same request would also be ignored.

https://codereview.chromium.org/2966963003/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_manager.cc:293:
DCHECK(pending_requests_.IsEmpty());
The PermissionManager is a profile-keyed service which should outlast
PermissionRequestManager which has the same lifetime as a WebContents. So I
think this should be fine.

[Timothy Loh, 2017-07-06 01:04:58]

lgtm

[raymes, 2017-07-06 02:48:28]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-07-06 02:48:45]

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-07-06 02:53:01]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499309308837220,
"parent_rev": "a3651d16f85cebe1ed5109de6444330414b29fdf", "commit_rev":
"158a8c15d0f9aa1162c6a4583047b3a8c50046d6"}

[commit-bot: I haz the power, 2017-07-06 02:53:17]

Description was changed from

==========
Ensure media permission requests are correctly cancelled when permission prompts
are ignored

Currently when permission prompts are ignored, the PermissionManager
will still hold on to the callback that was passed in to
RequestPermission. |pending_requests_| is not cleaned up. This means
that the callers have no way of knowing if a request was ignored. This
CL changes the code so that |pending_requests_| is correctly cleaned up
when a prompt is ignored. This makes media permission prompts behave
correctly.

BUG=606630
==========

to

==========
Ensure media permission requests are correctly cancelled when permission prompts
are ignored

Currently when permission prompts are ignored, the PermissionManager
will still hold on to the callback that was passed in to
RequestPermission. |pending_requests_| is not cleaned up. This means
that the callers have no way of knowing if a request was ignored. This
CL changes the code so that |pending_requests_| is correctly cleaned up
when a prompt is ignored. This makes media permission prompts behave
correctly.

BUG=606630

Review-Url: https://codereview.chromium.org/2966963003
Cr-Commit-Position: refs/heads/master@{#484449}
Committed:
https://chromium.googlesource.com/chromium/src/+/158a8c15d0f9aa1162c6a4583047...
==========

[commit-bot: I haz the power, 2017-07-06 02:53:19]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/158a8c15d0f9aa1162c6a4583047...