Mac: improve apparent z-order problems when switching Spaces.

Mac: improve apparent z-order problems when switching Spaces.

Remove the status bubble as child window when hidden, rather than just setting
its opacity to 0. I'm not sure about the effects, if any, of doing this rather
than destroying the window completely. This doesn't eliminate z-order problems
with Spaces, but should improve it considerably.

This may also ameliorate the problems with moving windows between Spaces using
Expose.

BUG=28107, 24956
TEST=Create a bunch of windows; get the status bubble to appear in the active (key) window and then get it to disappear; switch to another spaces and back; make sure the active window is on top; repeat.

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

[viettrungluu, 2009-11-27 18:47:09]

See my comments on the related bugs. Admittedly, my confidence that this patch
will always work properly is kind of low.

[Nico, 2009-11-30 02:04:28]

Merging this in makes my simple spaces repro stop working. I think this should
go into M4.

A few questions first, though.

http://codereview.chromium.org/434120/diff/1/2
File chrome/browser/cocoa/chrome_event_processing_window.mm (right):

http://codereview.chromium.org/434120/diff/1/2#newcode72
chrome/browser/cocoa/chrome_event_processing_window.mm:72: DCHECK_EQ([event
window], self);
woo!

http://codereview.chromium.org/434120/diff/1/3
File chrome/browser/cocoa/status_bubble_mac.h (right):

http://codereview.chromium.org/434120/diff/1/3#newcode75
chrome/browser/cocoa/status_bubble_mac.h:75: bool is_attached() { return
([window_ parentWindow] != nil); }
drop surrounding ()?

http://codereview.chromium.org/434120/diff/1/4
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/434120/diff/1/4#newcode344
chrome/browser/cocoa/status_bubble_mac.mm:344: // If the parent window is
offscreen when the child is added, the child will
Is this comment still true? Looks like the visibility of |parent_| is no longer
checked.

http://codereview.chromium.org/434120/diff/1/4#newcode380
chrome/browser/cocoa/status_bubble_mac.mm:380: DCHECK(state_ != kBubbleHidden ||
!is_attached());
    DCHECK(state_ == kBubbleHidden ^ is_attached());

?

[viettrungluu, 2009-11-30 08:43:18]

Thanks, Nico.

I'm a little ambivalent about pushing this into 249, though it would improve
interaction with Spaces.

(I've played with things a bit more, but they don't seem to help. I wonder if
things would be better if we set the opacity of the view instead of the window.)

http://codereview.chromium.org/434120/diff/1/2
File chrome/browser/cocoa/chrome_event_processing_window.mm (right):

http://codereview.chromium.org/434120/diff/1/2#newcode72
chrome/browser/cocoa/chrome_event_processing_window.mm:72: DCHECK_EQ([event
window], self);
On 2009/11/30 02:04:28, Nico wrote:
> woo!

I'm not sure how to reply to that! (Obviously, this is not needed for this
change, but I've seen failure here when playing with Spaces, so the extra
information from DCHECK_EQ() when/if it fails again would be good.)

http://codereview.chromium.org/434120/diff/1/3
File chrome/browser/cocoa/status_bubble_mac.h (right):

http://codereview.chromium.org/434120/diff/1/3#newcode75
chrome/browser/cocoa/status_bubble_mac.h:75: bool is_attached() { return
([window_ parentWindow] != nil); }
On 2009/11/30 02:04:28, Nico wrote:
> drop surrounding ()?

I'm of the school that uses parens when a comparison's truth value is used as a
value (assigned or return). (I kind of hate "a = b == c;" though it's not as bad
as "a == b == c" or "a = b == c == d;", both of which are pure evil.)

I'll drop the parens if someone (possibly you) insists.

http://codereview.chromium.org/434120/diff/1/4
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/434120/diff/1/4#newcode344
chrome/browser/cocoa/status_bubble_mac.mm:344: // If the parent window is
offscreen when the child is added, the child will
On 2009/11/30 02:04:28, Nico wrote:
> Is this comment still true? Looks like the visibility of |parent_| is no
longer
> checked.

Comment changed.

http://codereview.chromium.org/434120/diff/1/4#newcode380
chrome/browser/cocoa/status_bubble_mac.mm:380: DCHECK(state_ != kBubbleHidden ||
!is_attached());
On 2009/11/30 02:04:28, Nico wrote:
>     DCHECK(state_ == kBubbleHidden ^ is_attached());
> 
> ?

Sure. Though my version gains an extra pair of parens since I find it morally
repugnant (okay, I exaggerate ...) that the bitwise logical operators have lower
precedence than the comparison operators. (Extra commentary (a.k.a. rant): This
was obviously a mistake on the part of Ritchie, especially since the C
comparison operators merely return a not-well-defined nonzero value in the case
of truth, thereby making bitwise operations on the result nonsensical. C++ has
made it useful, if not morally correct. Of course, it was also a mistake of
Ritchie not to endow C with a logical XOR operator. For some reason, of all the
features the designers of C++ saw fit to add, this never made the list.)

[pink (ping after 24hrs), 2009-11-30 14:20:52]

lgtm. i think this is safe enough for 249.

http://codereview.chromium.org/434120/diff/2002/3005
File chrome/browser/cocoa/status_bubble_mac.h (right):

http://codereview.chromium.org/434120/diff/2002/3005#newcode75
chrome/browser/cocoa/status_bubble_mac.h:75: bool is_attached() { return
([window_ parentWindow] != nil); }
remove the parens. return is not a function.

http://codereview.chromium.org/434120/diff/2002/3006
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/434120/diff/2002/3006#newcode351
chrome/browser/cocoa/status_bubble_mac.mm:351: if (is_attached())
should you DCHECK(is_attached()) here or can this also be called multiple times?
Is a comment like the one above warranted?

[Nico, 2009-11-30 15:22:11]

LG

[viettrungluu, 2009-11-30 15:48:00]

Thanks.

I'm biased against this CL since it took me a surprising amount of trouble to
get it working right (or at least not failing DCHECK()'s). I guess the final
result isn't so bad. If nothing blows up, I'll merge it into 249 later today.

http://codereview.chromium.org/434120/diff/2002/3005
File chrome/browser/cocoa/status_bubble_mac.h (right):

http://codereview.chromium.org/434120/diff/2002/3005#newcode75
chrome/browser/cocoa/status_bubble_mac.h:75: bool is_attached() { return
([window_ parentWindow] != nil); }
On 2009/11/30 14:20:52, pink wrote:
> remove the parens. return is not a function.

Of course not -- that's why there's a space. ;-) The parens indicate that the
comparison's truth value is being used (see what I said to Nico). But since I'm
outnumbered, I've changed it.

http://codereview.chromium.org/434120/diff/2002/3006
File chrome/browser/cocoa/status_bubble_mac.mm (right):

http://codereview.chromium.org/434120/diff/2002/3006#newcode351
chrome/browser/cocoa/status_bubble_mac.mm:351: if (is_attached())
On 2009/11/30 14:20:52, pink wrote:
> should you DCHECK(is_attached()) here or can this also be called multiple
times?

No. It's definitely called multiple times when the window is destroyed.
Moreover, it should probably be safe to call the public interface Hide()
multiple times (the interface doesn't specify this, but there's no interface to
check the state).

> Is a comment like the one above warranted?

Comment added.