Allow duplicate infobar requests

Allow duplicate infobar requests

There can be duplicate infobar requests.
For example, the same EME video might issue multiple key requests, and that will cause several duplicate infobar requests.
We need to remember these requests, and once the first request's permission is set, send notification for the other requests.
When OnPermissionSet(), it is always called on the request has has_infobar() is true.
As a result, we can remove those duplicate requests that doesn't have infobar.

BUG=373641
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276630

[qinmin, 2014-06-10 21:22:05]

PTAL

[xhwang, 2014-06-10 21:48:39]

lgtm % nits

https://codereview.chromium.org/322203003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/322203003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:195: if
(i->IsForPair(requesting_frame, embedder)) {
nit: See below about using a normal for loop. If you do that, you can return
early here:

if (!foo)
  continue;

bar...

https://codereview.chromium.org/322203003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:219: }
This for loop is crazy and can be improved. Since we already have
requests_to_notify and infobars_to_remove, why can't we just have a
infobar_requests_to_remove? Then we don't need to do erase(i) in the loop and
can just use a normal for loop.

[qinmin, 2014-06-10 23:16:00]

Simplified the CL, PTAL again. 
When OnPermissionSet(), we are always called on the request that has_infobar()
is true.
So we can remove those duplicate ones that doesn't have infobars.


On 2014/06/10 21:48:39, xhwang wrote:
> lgtm % nits
> 
>
https://codereview.chromium.org/322203003/diff/60001/chrome/browser/content_s...
> File chrome/browser/content_settings/permission_queue_controller.cc (right):
> 
>
https://codereview.chromium.org/322203003/diff/60001/chrome/browser/content_s...
> chrome/browser/content_settings/permission_queue_controller.cc:195: if
> (i->IsForPair(requesting_frame, embedder)) {
> nit: See below about using a normal for loop. If you do that, you can return
> early here:
> 
> if (!foo)
>   continue;
> 
> bar...
> 
>
https://codereview.chromium.org/322203003/diff/60001/chrome/browser/content_s...
> chrome/browser/content_settings/permission_queue_controller.cc:219: }
> This for loop is crazy and can be improved. Since we already have
> requests_to_notify and infobars_to_remove, why can't we just have a
> infobar_requests_to_remove? Then we don't need to do erase(i) in the loop and
> can just use a normal for loop.

[qinmin, 2014-06-10 23:20:10]

+bauerb for OWNER of chrome/browser/content_settings/

[xhwang, 2014-06-10 23:27:06]

lgtm

But still, this loop can be simplified a lot.

[qinmin, 2014-06-11 00:09:10]

On 2014/06/10 23:27:06, xhwang wrote:
> lgtm
> 
> But still, this loop can be simplified a lot.

Done, simplified the for loop

[xhwang, 2014-06-11 00:17:40]

nit

https://codereview.chromium.org/322203003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/322203003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:197: continue;
You can have here:

if (!i->has_infobar()) {
  pending_requests_to_remove.push_back(i);
  continue;
}

[qinmin, 2014-06-11 00:42:35]

https://codereview.chromium.org/322203003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/322203003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/permission_queue_controller.cc:197: continue;
On 2014/06/11 00:17:40, xhwang wrote:
> You can have here:
> 
> if (!i->has_infobar()) {
>   pending_requests_to_remove.push_back(i);
>   continue;
> }

Done.

[Bernhard Bauer, 2014-06-11 08:43:26]

lgtm

[qinmin, 2014-06-11 16:23:11]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-06-11 16:25:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/322203003/120001

[commit-bot: I haz the power, 2014-06-11 21:00:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:18:41]

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

[commit-bot: I haz the power, 2014-06-11 21:18:43]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[qinmin, 2014-06-11 21:35:06]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:37:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/322203003/120001

[commit-bot: I haz the power, 2014-06-11 21:53:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 22:25:04]

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

[commit-bot: I haz the power, 2014-06-11 22:25:05]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[qinmin, 2014-06-11 22:37:03]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:39:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/322203003/120001

[commit-bot: I haz the power, 2014-06-11 22:43:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 22:46:34]

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

[commit-bot: I haz the power, 2014-06-11 22:46:35]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[qinmin, 2014-06-12 01:27:53]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-06-12 01:28:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/322203003/120001

[commit-bot: I haz the power, 2014-06-12 11:40:07]

Change committed as 276630