Total guess fix for the bug....

Total guess fixes for the bug.

Add DCHECKs for main thread in all the places we use animation contexts.
Added a stack based class that takes an animate flag to enable/disable animation operations (grouping, durations) to clean up the code that does optional animation.
This might help the crashes in the bug because before we created animation context groups when we didn't need them and those empty groups could be tripping up something in the system frameworks (I some something similar in nib loading on 10.6).

BUG=26979
TEST=like we have a repeat fail case...

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

[Mark Mentovai, 2009-11-11 19:22:03]

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

http://codereview.chromium.org/390014/diff/1/2#newcode707
Line 707: [[NSAnimationContext currentContext]
Do we need to worry about this one too?

http://codereview.chromium.org/390014/diff/1/2#newcode720
Line 720: if (animate) [NSAnimationContext endGrouping];
Was there actually anything happening inside this grouping other than more
groupings?

[Mark Mentovai, 2009-11-11 19:22:45]

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

http://codereview.chromium.org/390014/diff/1/2#newcode707
Line 707: [[NSAnimationContext currentContext]
Do we need to worry about this one too?

http://codereview.chromium.org/390014/diff/1/2#newcode720
Line 720: if (animate) [NSAnimationContext endGrouping];
Was there actually anything happening inside this grouping other than
more groupings?

http://codereview.chromium.org/390014

[TVL, 2009-11-11 19:28:19]

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

http://codereview.chromium.org/390014/diff/1/2#newcode707
Line 707: [[NSAnimationContext currentContext]
On 2009/11/11 19:22:03, Mark Mentovai wrote:
> Do we need to worry about this one too?

yes and no? line 703 right above is visible && animate, so it is already
covered.

http://codereview.chromium.org/390014/diff/1/2#newcode720
Line 720: if (animate) [NSAnimationContext endGrouping];
On 2009/11/11 19:22:03, Mark Mentovai wrote:
> Was there actually anything happening inside this grouping other than more
> groupings?

yes, lines 650 and 655 might push a frame size change via the animator.

[TVL, 2009-11-11 19:28:38]

Reviewers: Mark Mentovai,


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

http://codereview.chromium.org/390014/diff/1/2#newcode707
Line 707: [[NSAnimationContext currentContext]
On 2009/11/11 19:22:03, Mark Mentovai wrote:
> Do we need to worry about this one too?

yes and no? line 703 right above is visible && animate, so it is already
covered.

http://codereview.chromium.org/390014/diff/1/2#newcode720
Line 720: if (animate) [NSAnimationContext endGrouping];
On 2009/11/11 19:22:03, Mark Mentovai wrote:
> Was there actually anything happening inside this grouping other than
more
> groupings?

yes, lines 650 and 655 might push a frame size change via the animator.

Description:
Total guess fix for the bug.

On 10.6 with nib loading, I found Apple loading objects into a hash map that
uses NSObject definition of equality before the objects were fully loaded  
from
Nib.  My subclass caused a major choke here because before they were filled  
in,
they all seemed to be equal.  I'm wonder is NSAnimationContext has a like  
issue,
our code can create a bunch of groups (beginGrouping/endGrouping) that we  
never
actually put anything into.  Maybe once some of those go in, all hell breaks
loose.  This prevents those from happening.

BUG=26979
TEST=like we have a repeat fail case...

Please review this at http://codereview.chromium.org/390014

SVN Base: svn://svn.chromium.org/chrome/trunk/src/

Affected files:
   M     chrome/browser/cocoa/tab_strip_controller.mm


Index: chrome/browser/cocoa/tab_strip_controller.mm
===================================================================
--- chrome/browser/cocoa/tab_strip_controller.mm	(revision 31674)
+++ chrome/browser/cocoa/tab_strip_controller.mm	(working copy)
@@ -546,8 +546,10 @@
    const float kMinSelectedTabWidth = [TabController minSelectedTabWidth];

    NSRect enclosingRect = NSZeroRect;
-  [NSAnimationContext beginGrouping];
-  [[NSAnimationContext currentContext] gtm_setDuration:kAnimationDuration];
+  if (animate) {
+    [NSAnimationContext beginGrouping];
+    [[NSAnimationContext currentContext]  
gtm_setDuration:kAnimationDuration];
+  }

    // Update the current subviews and their z-order if requested.
    if (doUpdate)
@@ -603,14 +605,14 @@
      if (isPlaceholder) {
        // Move the current tab to the correct location instantly.
        // We need a duration or else it doesn't cancel an inflight  
animation.
-      [NSAnimationContext beginGrouping];
+      if (animate) [NSAnimationContext beginGrouping];
        [[NSAnimationContext currentContext]  
setDuration:kMinimumTimeInterval];
        tabFrame.origin.x = placeholderFrame_.origin.x;
        // TODO(alcor): reenable this
        //tabFrame.size.height += 10.0 * placeholderStretchiness_;
        id target = animate ? [[tab view] animator] : [tab view];
        [target setFrame:tabFrame];
-      [NSAnimationContext endGrouping];
+      if (animate) [NSAnimationContext endGrouping];

        // Store the frame by identifier to aviod redundant calls to  
animator.
        NSValue* identifier = [NSValue valueWithPointer:[tab view]];
@@ -715,7 +717,7 @@
      }
    }

-  [NSAnimationContext endGrouping];
+  if (animate) [NSAnimationContext endGrouping];
    [dragBlockingView_ setFrame:enclosingRect];

    // Mark that we've successfully completed layout of at least one tab.

[Mark Mentovai, 2009-11-11 20:38:45]

lgtm

http://codereview.chromium.org/390014/diff/3/6
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/390014/diff/3/6#newcode557
Line 557: [self regenerateSubviewList];
Fix indentation while you're in here.

http://codereview.chromium.org/390014/diff/3/6#newcode609
Line 609: if (animate) [NSAnimationContext beginGrouping];
Is it helpful for this to be on the same line?

This whole block checks animate three times now.  We can at least get that down
to two, by setting |target| here inside this first conditional.

http://codereview.chromium.org/390014/diff/3/6#newcode721
Line 721: if (animate) [NSAnimationContext endGrouping];
Again with the same-line-ness.

[pink (ping after 24hrs), 2009-11-11 21:00:43]

I don't really like this as it obscures the begin/ends inside indenting and
makes it easier to forget to end as code changes.

I also really don't think this will fix anything, so it seems like a stab in the
dark with little benefit. We can try it and see if it fixes things, but I have
low confidence and if it doesn't I'd want to see it reverted.

[TVL, 2009-11-11 21:06:08]

On 2009/11/11 21:00:43, pink wrote:
> I don't really like this as it obscures the begin/ends inside indenting and
> makes it easier to forget to end as code changes.
> 
> I also really don't think this will fix anything, so it seems like a stab in
the
> dark with little benefit. We can try it and see if it fixes things, but I have
> low confidence and if it doesn't I'd want to see it reverted.

Not disagreeing, but if we aren't animating, we shouldn't be triggering these,
the docs point out the stack does a copy of the previous layer each time you add
one.  So we're doing work for no benefit, and we know we're crashing in code
related to the animations contexts.  It would be easy to make a stack based
class to that does the begin/end based on a passed in bool for animate, if that
would make things cleaner.

[pink (ping after 24hrs), 2009-11-11 21:16:00]

Yes, that's a good idea. I'd prefer a stack-based class that takes an animate
param.

[TVL, 2009-11-11 21:45:42]

updated.  could add a helper for the min animiation duration thing if we want.

[Mark Mentovai, 2009-11-11 21:55:17]

lg

http://codereview.chromium.org/390014/diff/3009/5010
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/390014/diff/3009/5010#newcode60
Line 60: ScopedNSAnimationContextGroup(bool animate)
“You want me to be…explicit?”

http://codereview.chromium.org/390014/diff/3009/5010#newcode66
Line 66: ~ScopedNSAnimationContextGroup() {
Blank before.

http://codereview.chromium.org/390014/diff/3009/5010#newcode71
Line 71: void SetCurrentContextDuration(NSTimeInterval duration) {
Blank before.

http://codereview.chromium.org/390014/diff/3009/5010#newcode76
Line 76: private:
Blank before.

[pink (ping after 24hrs), 2009-11-11 21:59:59]

lgtm

http://codereview.chromium.org/390014/diff/3009/5010
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/390014/diff/3009/5010#newcode637
Line 637: [[NSAnimationContext currentContext]
setDuration:kMinimumTimeInterval];
shouldn't this use localAnimationGroup? maybe add a "SetMinimumDuration()" that
doesn't go through gtm for steve-shift-slowdown?

http://codereview.chromium.org/390014/diff/3009/5010#newcode734
Line 734: [[NSAnimationContext currentContext]
same as above?

[TVL, 2009-11-11 22:03:44]

http://codereview.chromium.org/390014/diff/3009/5010
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/390014/diff/3009/5010#newcode637
Line 637: [[NSAnimationContext currentContext]
setDuration:kMinimumTimeInterval];
On 2009/11/11 22:00:00, pink wrote:
> shouldn't this use localAnimationGroup? maybe add a "SetMinimumDuration()"
that
> doesn't go through gtm for steve-shift-slowdown?

So the more I read those, the more I wasn't sure.  The comment on
kMinimumTimeInterval implies we're trying to stop the currently running
interval, but the docs on NSAnimationContext seem to imply it is on the current
group.  So I wasn't sure if it is really doing what we wanted.  Is there a good
way to check?  if we do want in on the local group (and to ignore the animate
flag because we want to clear something) we could add ClearCurrentAnimation()
that does this magic?

[pink (ping after 24hrs), 2009-11-11 22:15:52]

I think we do want to respect the animation flag here and do it on the
local group. What it's saying (if i understand it) is "we're going to
animate this view to a different location, do it very very quickly".
It's different than "canceling" the animation, which is done by
specifically setting animations to NULL in the animation list for an
object.

I think.

On Wed, Nov 11, 2009 at 5:03 PM,  <thomasvl@chromium.org> wrote:
>
> http://codereview.chromium.org/390014/diff/3009/5010
> File chrome/browser/cocoa/tab_strip_controller.mm (right):
>
> http://codereview.chromium.org/390014/diff/3009/5010#newcode637
> Line 637: [[NSAnimationContext currentContext]
> setDuration:kMinimumTimeInterval];
> On 2009/11/11 22:00:00, pink wrote:
>>
>> shouldn't this use localAnimationGroup? maybe add a
>
> "SetMinimumDuration()" that
>>
>> doesn't go through gtm for steve-shift-slowdown?
>
> So the more I read those, the more I wasn't sure. =A0The comment on
> kMinimumTimeInterval implies we're trying to stop the currently running
> interval, but the docs on NSAnimationContext seem to imply it is on the
> current group. =A0So I wasn't sure if it is really doing what we wanted.
> Is there a good way to check? =A0if we do want in on the local group (and
> to ignore the animate flag because we want to clear something) we could
> add ClearCurrentAnimation() that does this magic?
>
> http://codereview.chromium.org/390014
>



--=20
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[pink (ping after 24hrs), 2009-11-11 22:23:49]

LGTM

http://codereview.chromium.org/390014/diff/4009/4012
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/390014/diff/4009/4012#newcode82
Line 82: // passed to +[NSAnimationContext setDuration:] to stop an in-progress
-[NSAnimationContext setDuration:]. It's not a class method.

[Mark Mentovai, 2009-11-11 22:26:10]

http://codereview.chromium.org/390014/diff/4009/4012
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/390014/diff/4009/4012#newcode83
Line 83: // animation as quickly as possible.
Should this just do

void SetCurrentContextShortestDuration() {
  const NSTimeInterval kMinimumTimeInterval =
      std::numeric_limits<NSTimeInterval>::min();
  SetCurrentContextDuration(kMinimumTimeInterval);
}

for simplicity and reuse and stuff?

http://codereview.chromium.org/390014/diff/4009/4012#newcode89
Line 89: private:
BLANK BEFORE.