Add basic coalescing behavior to the permission bubble manager.

Add basic coalescing behavior to the permission bubble manager.

The manager will now wait 400ms after it receives both a request and its view
before showing a bubble.  This allows permissions that are requested
quickly in sequence to be shown together, in only one bubble.  Permission
requests that occur when there is already a bubble visible are queued and not
shown until the current bubble disappears.

This simple wait period is probably simpler than the final design, but is a
good stepping stone toward a better algorithm.

BUG=None

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

[leng, 2014-02-07 00:48:01]

WDYT?

[Greg Billock, 2014-02-07 20:14:24]

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:14: // This is
how many ms to wait to see if there's another permission request
add a blank line around namespace lines

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:158: requests_ =
queued_requests_;
Need to reset the accept_state_ here, too. (Set them all to true -- I defaulted
to false above which is wrong. I have a change in the CQ to fix that.)

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager.h (right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.h:60: virtual void
WebContentsDestroyed(
Lets add

DocumentOnLoadCompletedInMainFrame
DocumentLoadedInFrame

to this list. Then we can trigger on those signals and reset the timer as well.
That'll let us move faster on page load and will give us a trigger for unit
tests as well. Then we can figure out exactly what the relationship will be
between the timer and the load signals as we go.

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:17:
const int kCoalesceTestingTimeMs = 25;
I think typically we'd want this to be 0 for the existing tests, and just do a
RunLoop RunUntilIdle in WaitForCoalescing.

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:114:
void WaitForCoalescing() {
I think if we can touch the WebcontentsObserver method we can get rid of this.

[leng, 2014-02-08 00:11:18]

PTAL, thanks!

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:14: // This is
how many ms to wait to see if there's another permission request
On 2014/02/07 20:14:24, Greg Billock wrote:
> add a blank line around namespace lines

Done.

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:158: requests_ =
queued_requests_;
On 2014/02/07 20:14:24, Greg Billock wrote:
> Need to reset the accept_state_ here, too. (Set them all to true -- I
defaulted
> to false above which is wrong. I have a change in the CQ to fix that.)

Done.
I also changed the variable name to be plural, matching |requests_|.

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager.h (right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.h:60: virtual void
WebContentsDestroyed(
On 2014/02/07 20:14:24, Greg Billock wrote:
> Lets add
> 
> DocumentOnLoadCompletedInMainFrame
> DocumentLoadedInFrame
> 
> to this list. Then we can trigger on those signals and reset the timer as
well.
> That'll let us move faster on page load and will give us a trigger for unit
> tests as well. Then we can figure out exactly what the relationship will be
> between the timer and the load signals as we go.

I tested those two functions, along with DidFinishLoad, and found that the
AddRequest call happened after all of them.  So for now, I suggest we leave only
DidFinishLoad, and if we find requests being made before it is called, we can
add the others.

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc
(right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:17:
const int kCoalesceTestingTimeMs = 25;
On 2014/02/07 20:14:24, Greg Billock wrote:
> I think typically we'd want this to be 0 for the existing tests, and just do a
> RunLoop RunUntilIdle in WaitForCoalescing.

Good idea.  Done.

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc:114:
void WaitForCoalescing() {
On 2014/02/07 20:14:24, Greg Billock wrote:
> I think if we can touch the WebcontentsObserver method we can get rid of this.

I've set up DidFinishLoad to only extend the timer, as I found the requests
being added after everything else had completed.  So I'll leave the timer
mechanism in place, rather than calling a WebContentsObserver method.

[Greg Billock, 2014-02-11 16:26:50]

lgtm

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager.h (right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.h:60: virtual void
WebContentsDestroyed(
On 2014/02/08 00:11:18, leng wrote:
> On 2014/02/07 20:14:24, Greg Billock wrote:
> > Lets add
> > 
> > DocumentOnLoadCompletedInMainFrame
> > DocumentLoadedInFrame
> > 
> > to this list. Then we can trigger on those signals and reset the timer as
> well.
> > That'll let us move faster on page load and will give us a trigger for unit
> > tests as well. Then we can figure out exactly what the relationship will be
> > between the timer and the load signals as we go.
> 
> I tested those two functions, along with DidFinishLoad, and found that the
> AddRequest call happened after all of them.  So for now, I suggest we leave
only
> DidFinishLoad, and if we find requests being made before it is called, we can
> add the others.

Sounds good. Did you try with iframes? That might trigger A but not B or
something... But perhaps just mark that a TODO and we'll pick it up later.

https://codereview.chromium.org/150103013/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/150103013/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:46:
accept_states_.push_back(false);
I think this'll need a merge -- should be 'true'

https://codereview.chromium.org/150103013/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:171:
accept_states_.resize(requests_.size(), true);
LG. I'd move this right under 'requests_ = queued_requests_;' though.

[leng, 2014-02-11 16:46:51]

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
File chrome/browser/ui/website_settings/permission_bubble_manager.h (right):

https://codereview.chromium.org/150103013/diff/1/chrome/browser/ui/website_se...
chrome/browser/ui/website_settings/permission_bubble_manager.h:60: virtual void
WebContentsDestroyed(
On 2014/02/11 16:26:50, Greg Billock wrote:
> On 2014/02/08 00:11:18, leng wrote:
> > On 2014/02/07 20:14:24, Greg Billock wrote:
> > > Lets add
> > > 
> > > DocumentOnLoadCompletedInMainFrame
> > > DocumentLoadedInFrame
> > > 
> > > to this list. Then we can trigger on those signals and reset the timer as
> > well.
> > > That'll let us move faster on page load and will give us a trigger for
unit
> > > tests as well. Then we can figure out exactly what the relationship will
be
> > > between the timer and the load signals as we go.
> > 
> > I tested those two functions, along with DidFinishLoad, and found that the
> > AddRequest call happened after all of them.  So for now, I suggest we leave
> only
> > DidFinishLoad, and if we find requests being made before it is called, we
can
> > add the others.
> 
> Sounds good. Did you try with iframes? That might trigger A but not B or
> something... But perhaps just mark that a TODO and we'll pick it up later.

I did not try with iframes.  Now you know how much I know about page loading. 
:)
I added a TODO, and will look into that.

https://codereview.chromium.org/150103013/diff/120001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/150103013/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:46:
accept_states_.push_back(false);
On 2014/02/11 16:26:50, Greg Billock wrote:
> I think this'll need a merge -- should be 'true'

Done.

https://codereview.chromium.org/150103013/diff/120001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:171:
accept_states_.resize(requests_.size(), true);
On 2014/02/11 16:26:50, Greg Billock wrote:
> LG. I'd move this right under 'requests_ = queued_requests_;' though.

Done.

[leng, 2014-02-11 17:26:54]

+markusheintz for OWNERS

[markusheintz_, 2014-02-12 16:56:06]

On 2014/02/11 17:26:54, leng wrote:
> +markusheintz for OWNERS

Owners LGTM

[markusheintz_, 2014-02-12 17:00:05]

Please make sure that you try this with a site like maps. Where clicking on the
"get my location" icon/button triggers a permissions prompt. The perceived time
for the permission bubble to show up should not be bad.

I have never been a fan of using a timer, but I also don't have a better idea.

[markusheintz_, 2014-02-12 17:20:07]

Sorry two nits I missed earlier.

LGTM still holds after fixing the indentation. The second is nit is optional.

https://codereview.chromium.org/150103013/diff/350001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/150103013/diff/350001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:24:
switches::kEnablePermissionsBubbles);
nit: please us the correct indentation. (indent +2)

https://codereview.chromium.org/150103013/diff/350001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:36: for (di =
queued_requests_.begin(); di != queued_requests_.end(); di++) {
Could we use a better name than "di" for the iterator e.g.
"bubble_requests_iter" or 
"requests_iterator" or
"permission_bubble_requests" or
"permission_bubble_requests_iter"

[leng, 2014-02-12 23:09:09]

I'll be sure to play with this on Maps!

https://codereview.chromium.org/150103013/diff/350001/chrome/browser/ui/websi...
File chrome/browser/ui/website_settings/permission_bubble_manager.cc (right):

https://codereview.chromium.org/150103013/diff/350001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:24:
switches::kEnablePermissionsBubbles);
On 2014/02/12 17:20:07, markusheintz_ wrote:
> nit: please us the correct indentation. (indent +2)

Done.

https://codereview.chromium.org/150103013/diff/350001/chrome/browser/ui/websi...
chrome/browser/ui/website_settings/permission_bubble_manager.cc:36: for (di =
queued_requests_.begin(); di != queued_requests_.end(); di++) {
On 2014/02/12 17:20:07, markusheintz_ wrote:
> Could we use a better name than "di" for the iterator e.g.
> "bubble_requests_iter" or 
> "requests_iterator" or
> "permission_bubble_requests" or
> "permission_bubble_requests_iter"

Done.

[leng, 2014-02-12 23:09:21]

The CQ bit was checked by leng@chromium.org

[commit-bot: I haz the power, 2014-02-12 23:11:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leng@chromium.org/150103013/430001

[commit-bot: I haz the power, 2014-02-13 01:41:04]

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

[commit-bot: I haz the power, 2014-02-13 01:41:05]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[leng, 2014-02-13 01:43:01]

The CQ bit was checked by leng@chromium.org

[commit-bot: I haz the power, 2014-02-13 01:45:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leng@chromium.org/150103013/430001

[commit-bot: I haz the power, 2014-02-13 03:09:27]

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

[commit-bot: I haz the power, 2014-02-13 03:09:28]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[leng, 2014-02-13 18:00:23]

The CQ bit was checked by leng@chromium.org

[commit-bot: I haz the power, 2014-02-13 18:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leng@chromium.org/150103013/430001

[commit-bot: I haz the power, 2014-02-13 18:03:34]

Change committed as 251079