[Mac] Cleans up the StatusBubbleMac code.

[Mac] Cleans up the StatusBubbleMac code.

This CL reduces unnecessary work in StatusBubbleMac and replaces it with some
DCHECKs to verify invariants.  (The invariant here is that we set
kWindowSizeDeterminedLater once, when state is set to kBubbleHidden, and the
size stays that way until the bubble transitions to a different state.  If that
invariant holds, then we don't need to constantly re-set the size for a hidden
bubble.)

BUG=None
TEST=No visible impact.  Status bubble should continue to show, expand, and hide properly.

Committed: https://crrev.com/b62daf588fe52c461c8b2c09771dbb32fbfdbab6
Cr-Commit-Position: refs/heads/master@{#318928}

[rohitrao (ping after 24h), 2015-03-03 03:31:27]

rohitrao@chromium.org changed reviewers:
+ erikchen@chromium.org

[rohitrao (ping after 24h), 2015-03-03 03:31:28]

Here's a CL to reduce (what I think is) unnecessary work and replace it with
some DCHECKs to verify invariants.  (The invariant here is that we set
kWindowSizeDeterminedLater once, when state is set to kBubbleHidden, and the
size stays that way until the bubble transitions to a different state.  If that
invariant holds, then we don't need to constantly re-set the size for a hidden
bubble.)

I also added a comment at the top of the file with a bunch of info that I've
scavenged from various bugs.  Some of the status bubble's behavior looks odd
until you realize that it's fixing specific Spaces oddities.  Let me know if you
think any of that comment is incorrect.

[rohitrao (ping after 24h), 2015-03-03 12:58:42]

DCHECKs are firing in a few browsertests.  Looking at those now.

[rohitrao (ping after 24h), 2015-03-03 15:54:00]

Ready for a review now.  Browser tests were crashing because I was not properly
setting the window size when reparenting windows.  I've added a unit test to try
and cover this case.

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/status_bubble_mac.mm:479: // set it to
kWindowSizeDeterminedLater?
I think we can probably get away with always using kWindowSizeDeterminedLater
here.  Even if the bubble is currently showing, it's almost certainly got the
wrong size for the new window, so it won't matter if it's (1, 1) or some other
larger size.  In either case, we still have the call to UpdateSizeAndPosition()
after reparenting.

I made the most conservative change I could, but I think we could simplify this
further if we wanted.

[erikchen, 2015-03-03 18:19:50]

lgtm

Compared to the existing code, your CL is a strict improvement. The comments are
especially helpful.

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/status_bubble_mac.mm:482: frame.size =
ui::kWindowSizeDeterminedLater.size;
"""
NSRect frame = ui::kWindowSizeDeterminedLater.size;
if (state_ != kBubbleHidden)
  frame = CalculateWindowFrame(/*expand=*/false);
"""

saves a call to CalculateWindowFrame.

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/status_bubble_mac.mm:510: is_expanded_ = false;
is_expanded_ gets set to false in several locations:

SetURL()
Hide()
and now here.

This is confusing. There's almost certainly some overlap of work, and it's
unclear to me when exactly is_expanded_ is supposed to be set to false.

[rohitrao (ping after 24h), 2015-03-03 18:53:51]

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/status_bubble_mac.mm (right):

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/status_bubble_mac.mm:482: frame.size =
ui::kWindowSizeDeterminedLater.size;
On 2015/03/03 at 18:19:50, erikchen wrote:
> """
> NSRect frame = ui::kWindowSizeDeterminedLater.size;
> if (state_ != kBubbleHidden)
>   frame = CalculateWindowFrame(/*expand=*/false);
> """
> 
> saves a call to CalculateWindowFrame.

Done.

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/status_bubble_mac.mm:510: is_expanded_ = false;
On 2015/03/03 at 18:19:50, erikchen wrote:
> is_expanded_ gets set to false in several locations:
> 
> SetURL()
> Hide()
> and now here.
> 
> This is confusing. There's almost certainly some overlap of work, and it's
unclear to me when exactly is_expanded_ is supposed to be set to false.

Agreed.  I don't fully understand when it needs to be set.  I moved this call
from SetURL() to here because it didn't seem to belong in SetURL().  I suspect
that this call is enough on its own, but not positive.

https://codereview.chromium.org/968263005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/status_bubble_mac.mm:510: is_expanded_ = false;
On 2015/03/03 at 18:19:50, erikchen wrote:
> is_expanded_ gets set to false in several locations:
> 
> SetURL()
> Hide()
> and now here.
> 
> This is confusing. There's almost certainly some overlap of work, and it's
unclear to me when exactly is_expanded_ is supposed to be set to false.

Agreed.  I don't fully understand when it needs to be set.  I moved this call
from SetURL() to here because it didn't seem to belong in SetURL().  I suspect
that this call is enough on its own, but not positive.

[rohitrao (ping after 24h), 2015-03-03 18:54:01]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2015-03-03 18:54:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/968263005/#ps40001 (title:
"Review.")

[commit-bot: I haz the power, 2015-03-03 18:55:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/968263005/40001

[commit-bot: I haz the power, 2015-03-03 19:29:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-03 19:30:41]

Patchset 3 (id:??) landed as
https://crrev.com/b62daf588fe52c461c8b2c09771dbb32fbfdbab6
Cr-Commit-Position: refs/heads/master@{#318928}