Make PermissionRequestManager::requests_ correspond to the active prompt

Make PermissionRequestManager::requests_ correspond to the active prompt

This patch reworks the usage of the PermissionRequest* vectors in the
PermissionRequestManager so that requests_ correponds the the active
prompt, if any. In particular, the vector remains empty while requests
arrive and is populated immediately before actually creating the prompt
UI. We also remove PermissionPrompt::IsVisible() which is no longer
needed as we can just check whether the vector is empty.

Currently AddRequest() calls immediately push to requests_ if it is
empty and otherwise queue the request. TriggerShowBubble() also already
has logic to use the queued requests if requests_ is empty. This patch
makes AddRequest() only queue requests and leaves moving them to the
requests_ vector to TriggerShowBubble().

This patch is preparation for making the vector of requests available
to the PermissionPrompt subclasses via the Delegate object, so that on
Android we won't need to store a copy of this vector, which can be
problematic when requests are cancelled.

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

[Timothy Loh, 2017-05-02 02:49:22]

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

[commit-bot: I haz the power, 2017-05-02 02:49:47]

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-02 03:18:40]

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

[commit-bot: I haz the power, 2017-05-02 03:18:41]

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_...)

[Timothy Loh, 2017-05-02 07:15:45]

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

[commit-bot: I haz the power, 2017-05-02 07:16:01]

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

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

[commit-bot: I haz the power, 2017-05-02 07:40:19]

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_...)

[Timothy Loh, 2017-05-02 07:44:12]

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

[commit-bot: I haz the power, 2017-05-02 07:44:22]

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-02 08:08:45]

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

[commit-bot: I haz the power, 2017-05-02 08:08:46]

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

[Timothy Loh, 2017-05-03 05:39:43]

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

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

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-03 06:26:34]

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

[commit-bot: I haz the power, 2017-05-03 06:26:35]

Dry run: This issue passed the CQ dry run.

[Timothy Loh, 2017-05-03 06:54:59]

Description was changed from

==========
wip

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

to

==========
Make PermissionRequestManager::requests_ correspond to the active prompt

This patch reworks the usage of the PermissionRequest* vectors in the
PermissionRequestManager so that requests_ correponds the the active
prompt, if any. We will only populate requests_ immediately before
calling Show(). We also remove PermissionPrompt::IsVisible() which is
no longer needed as we can just check whether the vector is empty.

Currently AddRequest() calls immediately push to requests_ if it is
empty and otherwise queue the request. TriggerShowBubble() also already
has logic to use the queued requests if requests_ is empty. This patch
makes AddRequest() only queue requests and leaves moving them to the
requests_ vector to TriggerShowBubble().

This patch is preparation for making the vector of requests available
to the PermissionPrompt subclasses via the Delegate object, so that on
Android we won't need to store a copy of this vector, which can be
problematic when requests are cancelled.

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

[Timothy Loh, 2017-05-03 06:55:00]

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

[Timothy Loh, 2017-05-03 08:21:17]

Description was changed from

==========
Make PermissionRequestManager::requests_ correspond to the active prompt

This patch reworks the usage of the PermissionRequest* vectors in the
PermissionRequestManager so that requests_ correponds the the active
prompt, if any. We will only populate requests_ immediately before
calling Show(). We also remove PermissionPrompt::IsVisible() which is
no longer needed as we can just check whether the vector is empty.

Currently AddRequest() calls immediately push to requests_ if it is
empty and otherwise queue the request. TriggerShowBubble() also already
has logic to use the queued requests if requests_ is empty. This patch
makes AddRequest() only queue requests and leaves moving them to the
requests_ vector to TriggerShowBubble().

This patch is preparation for making the vector of requests available
to the PermissionPrompt subclasses via the Delegate object, so that on
Android we won't need to store a copy of this vector, which can be
problematic when requests are cancelled.

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

to

==========
Make PermissionRequestManager::requests_ correspond to the active prompt

This patch reworks the usage of the PermissionRequest* vectors in the
PermissionRequestManager so that requests_ correponds the the active
prompt, if any. In particular, the vector remains empty while requests
arrive and is populated immediately before actually creating the prompt
UI. We also remove PermissionPrompt::IsVisible() which is no longer
needed as we can just check whether the vector is empty.

Currently AddRequest() calls immediately push to requests_ if it is
empty and otherwise queue the request. TriggerShowBubble() also already
has logic to use the queued requests if requests_ is empty. This patch
makes AddRequest() only queue requests and leaves moving them to the
requests_ vector to TriggerShowBubble().

This patch is preparation for making the vector of requests available
to the PermissionPrompt subclasses via the Delegate object, so that on
Android we won't need to store a copy of this vector, which can be
problematic when requests are cancelled.

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

[raymes, 2017-05-04 05:00:05]

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:256: decision =
AutoResponseType::DENY_ALL;
perhaps add
else
  NOTREACHED();

to maintain the invariant?

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:267:
LoadCompleted();
Can this just go in NavigateAndCommit?

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:792:
DISALLOW_COPY_AND_ASSIGN(PermissionContextBaseTests);
This usually goes after all member variables.

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:144:
base::UserMetricsAction("PermissionBubbleIFrameRequestQueued"));
this used to only happen if IsBubbleVisible. Should that still be the case now?

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:248: }
I think it could be better to move this into a separate function. Maybe have
ShowBubble()
TriggerShowBubble->PopRequestsAndShowBubble

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:382: return;
Why not get rid of the above 2 checks and just call IsBubbleVisible here?

[Timothy Loh, 2017-05-04 08:21:17]

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

[commit-bot: I haz the power, 2017-05-04 08:21: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-04 08:22:31]

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:256: decision =
AutoResponseType::DENY_ALL;
On 2017/05/04 05:00:04, raymes wrote:
> perhaps add
> else
>   NOTREACHED();
> 
> to maintain the invariant?

The value of response is DCHECK'd above.

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:267:
LoadCompleted();
On 2017/05/04 05:00:04, raymes wrote:
> Can this just go in NavigateAndCommit?

It's defined in RenderViewHostTestHarness so no, but I could wrap it up into a
single function (SetUpUrl or something?)

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:792:
DISALLOW_COPY_AND_ASSIGN(PermissionContextBaseTests);
On 2017/05/04 05:00:04, raymes wrote:
> This usually goes after all member variables.

Fixed

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_request_manager.cc (right):

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:144:
base::UserMetricsAction("PermissionBubbleIFrameRequestQueued"));
On 2017/05/04 05:00:04, raymes wrote:
> this used to only happen if IsBubbleVisible. Should that still be the case
now?

Logic shouldn't be changed here, it was being called both L142 and L153. It's
inconsistent with the non-frame case but at least it's more obvious now.

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:248: }
On 2017/05/04 05:00:05, raymes wrote:
> I think it could be better to move this into a separate function. Maybe have
> ShowBubble()
> TriggerShowBubble->PopRequestsAndShowBubble

Done.

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_request_manager.cc:382: return;
On 2017/05/04 05:00:04, raymes wrote:
> Why not get rid of the above 2 checks and just call IsBubbleVisible here?

Just checking IsBubbleVisible won't exit the !view case. I can leave it as a
IsBubbleVisible() call but I think it's more obvious like this.

[commit-bot: I haz the power, 2017-05-04 08:24:54]

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

[commit-bot: I haz the power, 2017-05-04 08:24:55]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Timothy Loh, 2017-05-05 03:25:49]

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

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

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-05 04:20:28]

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

[commit-bot: I haz the power, 2017-05-05 04:20:29]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-05-08 03:14:26]

lgtm with the change to the test below

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:267:
LoadCompleted();
On 2017/05/04 08:22:30, Timothy Loh wrote:
> On 2017/05/04 05:00:04, raymes wrote:
> > Can this just go in NavigateAndCommit?
> 
> It's defined in RenderViewHostTestHarness so no, but I could wrap it up into a
> single function (SetUpUrl or something?)

That sounds good. Or maybe NavigateToUrl

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

https://codereview.chromium.org/2853803002/diff/100001/chrome/browser/permiss...
chrome/browser/permissions/permission_request_manager.cc:246: // We switched
tabs away and back while a prompt was active.
Thanks, this is much clearer now!

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

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

[commit-bot: I haz the power, 2017-05-08 03:49:48]

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

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
File chrome/browser/permissions/permission_context_base_unittest.cc (right):

https://codereview.chromium.org/2853803002/diff/60001/chrome/browser/permissi...
chrome/browser/permissions/permission_context_base_unittest.cc:267:
LoadCompleted();
On 2017/05/08 03:14:26, raymes wrote:
> On 2017/05/04 08:22:30, Timothy Loh wrote:
> > On 2017/05/04 05:00:04, raymes wrote:
> > > Can this just go in NavigateAndCommit?
> > 
> > It's defined in RenderViewHostTestHarness so no, but I could wrap it up into
a
> > single function (SetUpUrl or something?)
> 
> That sounds good. Or maybe NavigateToUrl

Done (called it SetUpUrl)

[commit-bot: I haz the power, 2017-05-08 04:53:50]

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

[commit-bot: I haz the power, 2017-05-08 04:53:51]

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

[Timothy Loh, 2017-05-08 06:02:24]

The CQ bit was checked by timloh@chromium.org

[Timothy Loh, 2017-05-08 06:02:24]

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

[commit-bot: I haz the power, 2017-05-08 06:02:51]

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 06:10:02]

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

[commit-bot: I haz the power, 2017-05-08 06:10:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Timothy Loh, 2017-05-08 07:25:05]

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

[Timothy Loh, 2017-05-08 07:25:06]

+rsesek@, can you have a look or stamp for 
chrome/browser/ui/cocoa/permission_bubble/permission_bubble_cocoa.h/mm? Thanks!

[Robert Sesek, 2017-05-08 14:47:45]

lgtm

[Timothy Loh, 2017-05-09 01:06:09]

The CQ bit was checked by timloh@chromium.org

[commit-bot: I haz the power, 2017-05-09 01:18: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-09 03:43:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494291969554720,
"parent_rev": "ea032eab9bb15ad89bd38dea55f639b79c605a7b", "commit_rev":
"86d8eaf62f5dfd8c4d3d9d542d6c4a859ca595fe"}

[commit-bot: I haz the power, 2017-05-09 03:43:24]

Description was changed from

==========
Make PermissionRequestManager::requests_ correspond to the active prompt

This patch reworks the usage of the PermissionRequest* vectors in the
PermissionRequestManager so that requests_ correponds the the active
prompt, if any. In particular, the vector remains empty while requests
arrive and is populated immediately before actually creating the prompt
UI. We also remove PermissionPrompt::IsVisible() which is no longer
needed as we can just check whether the vector is empty.

Currently AddRequest() calls immediately push to requests_ if it is
empty and otherwise queue the request. TriggerShowBubble() also already
has logic to use the queued requests if requests_ is empty. This patch
makes AddRequest() only queue requests and leaves moving them to the
requests_ vector to TriggerShowBubble().

This patch is preparation for making the vector of requests available
to the PermissionPrompt subclasses via the Delegate object, so that on
Android we won't need to store a copy of this vector, which can be
problematic when requests are cancelled.

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

to

==========
Make PermissionRequestManager::requests_ correspond to the active prompt

This patch reworks the usage of the PermissionRequest* vectors in the
PermissionRequestManager so that requests_ correponds the the active
prompt, if any. In particular, the vector remains empty while requests
arrive and is populated immediately before actually creating the prompt
UI. We also remove PermissionPrompt::IsVisible() which is no longer
needed as we can just check whether the vector is empty.

Currently AddRequest() calls immediately push to requests_ if it is
empty and otherwise queue the request. TriggerShowBubble() also already
has logic to use the queued requests if requests_ is empty. This patch
makes AddRequest() only queue requests and leaves moving them to the
requests_ vector to TriggerShowBubble().

This patch is preparation for making the vector of requests available
to the PermissionPrompt subclasses via the Delegate object, so that on
Android we won't need to store a copy of this vector, which can be
problematic when requests are cancelled.

BUG=606138

Review-Url: https://codereview.chromium.org/2853803002
Cr-Commit-Position: refs/heads/master@{#470158}
Committed:
https://chromium.googlesource.com/chromium/src/+/86d8eaf62f5dfd8c4d3d9d542d6c...
==========

[commit-bot: I haz the power, 2017-05-09 03:43:27]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/86d8eaf62f5dfd8c4d3d9d542d6c...