Remove Animation When "Resuming" Window in Lion

Remove Animation When "Resuming" Window in Lion

Adds signal to browser that session restore is in progress.  When Mac window shows it inhibits animation during session restore.

BUG=92984
TEST=Manual.  Enable session restore, launch Chrome, observe lack of swoosh animation on Mac.

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

[dhollowa, 2011-08-31 01:52:20]

sky -> session restore, browser
mark -> mac

Thanks!

[sky, 2011-08-31 04:25:35]

http://codereview.chromium.org/7809013/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7809013/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:259: is_session_restore_(false),
It seems like you only care about this in Show. Could we make Show take an enum
rather than persisting this state?

[dhollowa, 2011-08-31 16:06:09]

http://codereview.chromium.org/7809013/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7809013/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:259: is_session_restore_(false),
I considered this, and I agree, it really is the "right" thing to do (tm). 
However I decided against it because it is so invasive and ugly with the myriad
of other callers/sub-classers.  And there is precedent with show_state_ etc.

I've uploaded the result for evaluation.  Let me know.

On 2011/08/31 04:25:35, sky wrote:
> It seems like you only care about this in Show. Could we make Show take an
enum
> rather than persisting this state?

[Mark Mentovai, 2011-08-31 16:20:49]

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:96: NSWindowAnimationBehavior
savedAnimationBehavior = 0;
Style nit: you’re supposed to use saved_animation_behavior here because you’re
not in an Objective-C @protocol or @interface or @implementation.

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:99: if ([window()
respondsToSelector:@selector(animationBehavior)] &&
Fold these conditions into the same |if| that encloses them.

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:118: if ([window()
respondsToSelector:@selector(animationBehavior)] &&
Rather than doing this check, and maybe the one that encloses it, twice, come up
with a way to only have to do it once. Doing this kind of work twice is likely
to get out of sync.

Suggestion: add another bool (alongside saved_animation_behavior) initialized to
false, and only set it to true if you set saved_animation_behavior. Use that
bool to determine whether you’ll enter this “restore” block.

[sky, 2011-08-31 16:29:17]

Oi, I forgot about the show_state_. I leave it to you to pick the right one.

LGTM

  -Scott

[dhollowa, 2011-08-31 17:05:11]

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:96: NSWindowAnimationBehavior
savedAnimationBehavior = 0;
On 2011/08/31 16:20:49, Mark Mentovai wrote:
> Style nit: you’re supposed to use saved_animation_behavior here because you’re
> not in an Objective-C @protocol or @interface or @implementation.

Done.

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:99: if ([window()
respondsToSelector:@selector(animationBehavior)] &&
On 2011/08/31 16:20:49, Mark Mentovai wrote:
> Fold these conditions into the same |if| that encloses them.

Done.

http://codereview.chromium.org/7809013/diff/2002/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:118: if ([window()
respondsToSelector:@selector(animationBehavior)] &&
On 2011/08/31 16:20:49, Mark Mentovai wrote:
> Rather than doing this check, and maybe the one that encloses it, twice, come
up
> with a way to only have to do it once. Doing this kind of work twice is likely
> to get out of sync.
> 
> Suggestion: add another bool (alongside saved_animation_behavior) initialized
to
> false, and only set it to true if you set saved_animation_behavior. Use that
> bool to determine whether you’ll enter this “restore” block.

Done.  Nice, yes.  And I've done the same in drag-controller code which did
something similar.

[Mark Mentovai, 2011-08-31 17:58:53]

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:96: NSWindowAnimationBehavior
saved_animation_behavior = 0;
This no longer needs to be initialized to 0, since it’ll only be used if
did_save_animation_behavior is true, and in that case it will always have been
set.

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:104: did_save_animation_behavior
= true;
I would do this before changing the value, just because it flows better
conceptually.

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:123:
browser_->set_is_session_restore(false);
This is the only platform on which this happens, right?

Not so sure about that. Is there a more cross-platformy place to do this?

The parameterized form worked better for this aspect, I think.

[dhollowa, 2011-08-31 18:40:55]

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:96: NSWindowAnimationBehavior
saved_animation_behavior = 0;
On 2011/08/31 17:58:54, Mark Mentovai wrote:
> This no longer needs to be initialized to 0, since it’ll only be used if
> did_save_animation_behavior is true, and in that case it will always have been
> set.

Done.

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:104: did_save_animation_behavior
= true;
On 2011/08/31 17:58:54, Mark Mentovai wrote:
> I would do this before changing the value, just because it flows better
> conceptually.

Done.

http://codereview.chromium.org/7809013/diff/10/chrome/browser/ui/cocoa/browse...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:123:
browser_->set_is_session_restore(false);
On 2011/08/31 17:58:54, Mark Mentovai wrote:
> This is the only platform on which this happens, right?
> 
> Not so sure about that. Is there a more cross-platformy place to do this?
> 
> The parameterized form worked better for this aspect, I think.

Yes, this is Mac-only logic.  I agree though, this should be paired with the
setter in session_restore.cc.  I've done that.

For the record, I wrote it this way originally to match the usage pattern of
show_state_.  I agree the parameterized version is cleaner in this regard, it
just comes at such a high cost to other callers and sub-classers.  Also,
thinking about future work with Fullscreen, I can imagine needing this state
outside of |Show|.

[Mark Mentovai, 2011-08-31 18:47:01]

LGTM.

Nice!

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:177: bool is_session_restore() {
  bool is_session_restore() const {

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right):

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:267:
NSWindowAnimationBehavior savedAnimationBehavior = 0;
Get rid of the initializer like you did in the other file.

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:273:
didSaveAnimationBehavior = true;
Move this one up like you did in the other file.

[dhollowa, 2011-08-31 18:57:17]

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:177: bool is_session_restore() {
On 2011/08/31 18:47:01, Mark Mentovai wrote:
>   bool is_session_restore() const {

Done.

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right):

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:267:
NSWindowAnimationBehavior savedAnimationBehavior = 0;
On 2011/08/31 18:47:01, Mark Mentovai wrote:
> Get rid of the initializer like you did in the other file.

Done.

http://codereview.chromium.org/7809013/diff/4033/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:273:
didSaveAnimationBehavior = true;
On 2011/08/31 18:47:01, Mark Mentovai wrote:
> Move this one up like you did in the other file.

Done.

[Mark Mentovai, 2011-08-31 19:05:27]

LGTM

[Nico, 2011-09-01 16:28:41]

http://codereview.chromium.org/7809013/diff/9001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7809013/diff/9001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:96: NSWindowAnimationBehavior
saved_animation_behavior;
In gcc release builds (which is what the valgrind waterfall uses), this warns:

chrome/browser/ui/cocoa/browser_window_cocoa.mm:96:warning:
'saved_animation_behavior' may be used uninitialized in this function