Set AllowOverlappingViews for history overlay

Set AllowOverlappingViews for history overlay

In r204580 I changed AllowOverlappingViews to only be set when
necessary. This caused the history overlay to no longer be visible.

Fix was to also check the hisotry overlay state when setting
AllowOverlappingViews.

BUG=236460, 248169
TEST=Navigate to http://www.webkit.org/blog-files/3d-transforms/poster-circle.html
Then navigate to google.com. Swipe back to the first page. Swipe
forward to the second page. Without my patch the back and forward
arrows are not visible. With my patch the arrows are visible.
NOTRY=true

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

[Robert Sesek, 2013-06-10 20:30:35]

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_controller.h:351: -
(void)onHistoryOverlayShown;
This needs comments. As do the two methods above it. Comments are non-optional
for public members.

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Decla...

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_controller.mm:1996:
++historyOverlayCount_;
Why do we need to count them?

[sail, 2013-06-10 20:56:01]

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_controller.h:351: -
(void)onHistoryOverlayShown;
On 2013/06/10 20:30:35, rsesek wrote:
> This needs comments. As do the two methods above it. Comments are non-optional
> for public members.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Decla...

Done.

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_controller.mm:1996:
++historyOverlayCount_;
On 2013/06/10 20:30:35, rsesek wrote:
> Why do we need to count them?

The window controller instance is referenced by multiple RWHVs. In practice
there won't be more than one though. I can make this a BOOL with a DCHECK if you
like.

[Robert Sesek, 2013-06-10 20:59:28]

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_controller.mm:1996:
++historyOverlayCount_;
On 2013/06/10 20:56:01, sail wrote:
> On 2013/06/10 20:30:35, rsesek wrote:
> > Why do we need to count them?
> 
> The window controller instance is referenced by multiple RWHVs. In practice
> there won't be more than one though. I can make this a BOOL with a DCHECK if
you
> like.

I guess it depends on what the invariant is. It seems odd to count if it only
ever goes to 1.

[sail, 2013-06-10 22:21:07]

In this patch I've moved my changes from ChromeRenderWidgetHostViewMacDelegate
to HistoryOverlayController.

With the original patch there was no way to wait for the fade animation to
complete before calling onHistoryOverlayHidden. With this patch the animation
works correctly.

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/16632009/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser_window_controller.mm:1996:
++historyOverlayCount_;
On 2013/06/10 20:59:28, rsesek wrote:
> On 2013/06/10 20:56:01, sail wrote:
> > On 2013/06/10 20:30:35, rsesek wrote:
> > > Why do we need to count them?
> > 
> > The window controller instance is referenced by multiple RWHVs. In practice
> > there won't be more than one though. I can make this a BOOL with a DCHECK if
> you
> > like.
> 
> I guess it depends on what the invariant is. It seems odd to count if it only
> ever goes to 1.

I verified that the count can be greater than 1. For example:
 - swipe to go back
 - immediately swipe to go forward
Because there's a delay before the overlay is hidden you can have two overlays
at the same time.

I believe you can also reproduce this with multiple tabs.

[Robert Sesek, 2013-06-10 23:02:12]

Cool. Is it possible at all to test this? :/

https://codereview.chromium.org/16632009/diff/5002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/16632009/diff/5002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_controller.h:167: size_t
historyOverlayCount_;
NSUInteger seems more apt.

[sail, 2013-06-11 19:20:13]

Added a browser test. Please take a look.

+avi for content/* API changes.

https://codereview.chromium.org/16632009/diff/5002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/16632009/diff/5002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_controller.h:167: size_t
historyOverlayCount_;
On 2013/06/10 23:02:12, rsesek wrote:
> NSUInteger seems more apt.

Done.

[Robert Sesek, 2013-06-11 19:23:30]

LGTM! Thanks.

[Avi (use Gerrit), 2013-06-11 19:25:07]

https://codereview.chromium.org/16632009/diff/17001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/16632009/diff/17001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1044: [self
shouldAllowOverlappingViews:inPresentationMode]);
What is this?

This is in direct contradiction with the docs of SetAllowOverlappingViews:

> After the first draw, do not change this setting.

Is this comment line wrong? Seems like this code that already exists makes that
assumption, in which case it's wrong or the comment should be fixed.

[sail, 2013-06-11 19:29:10]

https://codereview.chromium.org/16632009/diff/17001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/16632009/diff/17001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1044: [self
shouldAllowOverlappingViews:inPresentationMode]);
On 2013/06/11 19:25:07, Avi wrote:
> What is this?
> 
> This is in direct contradiction with the docs of SetAllowOverlappingViews:
> 
> > After the first draw, do not change this setting.
> 
> Is this comment line wrong? Seems like this code that already exists makes
that
> assumption, in which case it's wrong or the comment should be fixed.

Good catch. Removed that bit from the comments.

I added the code to dynamically set this in crrev.com/r204580. We'll also be
using this for CoreAnimation code path.

[Avi (use Gerrit), 2013-06-11 19:31:50]

Cool. LGTM

I am so looking forward to ripping this crap out once we get layers working.

[commit-bot: I haz the power, 2013-06-11 19:33:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/16632009/25001

[commit-bot: I haz the power, 2013-06-11 21:04:51]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-11 21:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/16632009/25001

[commit-bot: I haz the power, 2013-06-12 02:58:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-06-12 17:17:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/16632009/25001

[commit-bot: I haz the power, 2013-06-12 18:09:50]

Change committed as 205871