[Mac] Fix "Chrome windows use more 'hidden' space on Exposé/Mission Control".

[Mac] Fix "Chrome windows use more 'hidden' space on Exposé/Mission Control".

BUG=81969
TEST=Open http://www.google.com. Make the window small. Mouse over one
of the links on the bar at the top of the page. Then move the mouse up
out of the window. Now, bring up Exposé. The Chrome window should not
have extra blue space around in Exposé when highlighted.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133565

[Mark Mentovai, 2012-04-23 17:59:36]

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_bubble_mac.mm:121: if (is_attached())
Why not move this logic into Detach()?

Detach() is private and only called from within this class, and you’ve already
placed identical logic at each call site.

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_bubble_mac.mm:417: DCHECK(is_attached());
Is this DCHECK no longer correct (given the change you’re making/may make)?

[asvitkine_google, 2012-04-23 18:06:24]

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_bubble_mac.mm:121: if (is_attached())
On 2012/04/23 17:59:36, Mark Mentovai wrote:
> Why not move this logic into Detach()?
> 
> Detach() is private and only called from within this class, and you’ve already
> placed identical logic at each call site.

Done.

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_bubble_mac.mm:417: DCHECK(is_attached());
On 2012/04/23 17:59:36, Mark Mentovai wrote:
> Is this DCHECK no longer correct (given the change you’re making/may make)?

Done.

[sail, 2012-04-23 18:08:53]

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_bubble_mac.mm:451: if (state == kBubbleHidden) {
Hi Alexei. I think this change might reintroduce bug 54924.

In r77090 I changed the bubble so that it always remains attached. It seems like
attaching/reattaching causes a lot of things to mess up.

You should also check if you regress bugs 31821 and 61629.

[sail, 2012-04-23 18:10:22]

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/10185005/diff/1/chrome/browser/ui/cocoa/status...
chrome/browser/ui/cocoa/status_bubble_mac.mm:451: if (state == kBubbleHidden) {
On 2012/04/23 18:08:53, sail wrote:
> Hi Alexei. I think this change might reintroduce bug 54924.
> 
> In r77090 I changed the bubble so that it always remains attached. It seems
like
> attaching/reattaching causes a lot of things to mess up.
> 
> You should also check if you regress bugs 31821 and 61629.

Also, you may decide that regressing the above bugs is an ok trade off in
exchange for fixing the expose bug.

I don't really have an opinion on that, it's your call.

[Alexei Svitkine (slow), 2012-04-23 18:54:50]

Sailesh, thanks for the context here.

This change does appear to regress r31821, which is unacceptable.

I haven't tested r61629 yet, but I'm guessing that it probably breaks that too.

I'll have to give this some more thought...

[Alexei Svitkine (slow), 2012-04-23 19:19:24]

I've updated the CL to not detach/re-attach but to just set the frame size
without changing the frame origin.

This seems to also fix the bug without introducing the issues caused by
detaching/re-attaching.

[sail, 2012-04-23 19:28:07]

LGTM
Thanks so much for investigating this!

http://codereview.chromium.org/10185005/diff/9002/chrome/browser/ui/cocoa/sta...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/10185005/diff/9002/chrome/browser/ui/cocoa/sta...
chrome/browser/ui/cocoa/status_bubble_mac.mm:451: NSRect frame = [window_
frame];
can you add a comment explaining this (for example, using a (0,0) origin causes
this bug...

[Alexei Svitkine (slow), 2012-04-23 19:43:57]

Mark, are you okay with this too?

http://codereview.chromium.org/10185005/diff/9002/chrome/browser/ui/cocoa/sta...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/10185005/diff/9002/chrome/browser/ui/cocoa/sta...
chrome/browser/ui/cocoa/status_bubble_mac.mm:451: NSRect frame = [window_
frame];
On 2012/04/23 19:28:07, sail wrote:
> can you add a comment explaining this (for example, using a (0,0) origin
causes
> this bug...

Done.

[Avi (use Gerrit), 2012-04-23 19:48:05]

Yay!

[Mark Mentovai, 2012-04-23 21:54:38]

LGTM

[commit-bot: I haz the power, 2012-04-23 21:56:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10185005/11002

[commit-bot: I haz the power, 2012-04-23 23:39:20]

Change committed as 133565