Basic test class that is going to replace CocoaTestHelper and CocoaNoWindowTe...

Basic test class that is going to replace CocoaTestHelper and CocoaNoWindowTestHelper.
Is responsible for bootstrapping cocoa and verifying that all windows are
closed down correctly between tests. Also includes a macro for doing some
very repetitive view testing that we had multiple copies of. I included a single
changed over test so that you can see the difference.

BUG=none
TEST=none


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

[dmac, 2009-10-23 06:42:51]

Here's the initial testing base class to look at. I've got all the tests working
on my machine now based on this, and it reduces our code and increases our code
coverage. Win. Once we get this in, I can start submitting the modified tests.

[Mark Mentovai, 2009-10-23 14:10:53]

I think this is generally an improvement.

http://codereview.chromium.org/327010/diff/8001/8002
File chrome/browser/cocoa/cocoa_test_helper.h (right):

http://codereview.chromium.org/327010/diff/8001/8002#newcode70
Line 70: virtual void TearDown();
Is it important to have a TearDown or can we just do this stuff in the
destructor?

Mike and I have been down this road before, and we found that using the
destructor was mostly more handy than TearDown because the destructor runs after
the destructors of a subclass' members.

http://codereview.chromium.org/327010/diff/8001/8002#newcode82
Line 82: std::vector<NSWindow*>* ApplicationWindows();
Seems like this ought to be static.

http://codereview.chromium.org/327010/diff/8001/8002#newcode83
Line 83: bool called_tear_down_;
Blank line between functions and data.  I read this quickly at first and thought
that ApplicationWindows was data because there was no separation.

http://codereview.chromium.org/327010/diff/8001/8002#newcode122
Line 122: // TODO(dmaclach): remove
Remove when?  Is there a cleanup task associated with this, or is it just "as
soon as all the callers are gone?"

http://codereview.chromium.org/327010/diff/8001/8003
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/8001/8003#newcode69
Line 69: // Bootstrap Cocoa. It's very unhappy without this.
Tell me about it.  :)

http://codereview.chromium.org/327010/diff/8001/8003#newcode77
Line 77: // Recycle the pool to clean up any stuff that was put on the
A blank line before a nice big comment block like this helps you find the top
more easily.

http://codereview.chromium.org/327010/diff/8001/8003#newcode93
Line 93: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
ScopedNSAutoreleasePool?  It's OK to introduce a new scope for your pool.

http://codereview.chromium.org/327010/diff/8001/8003#newcode94
Line 94: [NSApp updateWindows];
Hmm, does this need to be before nextEventMatchingMask:... and sendEvent:?  I
think it usually follows sendEvent:.

http://codereview.chromium.org/327010/diff/8001/8003#newcode132
Line 132: // in this list may not be valid NSWindows anytime after this returns.
You
nit: any time

http://codereview.chromium.org/327010/diff/8001/8004
File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):

http://codereview.chromium.org/327010/diff/8001/8004#newcode10
Line 10: class SadTabViewTest : public CocoaTest {
Formatting nit:  don't indent anything within a namespace.  Also, on the next
line, public should have the weird one-space indent.

[dmac, 2009-10-23 15:00:38]

On 2009/10/23 14:10:53, Mark Mentovai wrote:
> I think this is generally an improvement.
> 
> http://codereview.chromium.org/327010/diff/8001/8002
> File chrome/browser/cocoa/cocoa_test_helper.h (right):
> 
> http://codereview.chromium.org/327010/diff/8001/8002#newcode70
> Line 70: virtual void TearDown();
> Is it important to have a TearDown or can we just do this stuff in the
> destructor?
> 
> Mike and I have been down this road before, and we found that using the
> destructor was mostly more handy than TearDown because the destructor runs
after
> the destructors of a subclass' members.

The reason it can't be done in the destructor is that structures that the
windows may be depending on may have been destroyed when the subclass members
were destructed. Normally the structures that the windows depend on are part of
the windows' NSWindowController and their destruction is done in a controlled
fashion. When we are injecting structures in as part of unit tests we need to be
sure to have the windows destroyed before anything else. Hopefully this makes
sense.

> http://codereview.chromium.org/327010/diff/8001/8002#newcode82
> Line 82: std::vector<NSWindow*>* ApplicationWindows();
> Seems like this ought to be static.

Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8002#newcode83
> Line 83: bool called_tear_down_;
> Blank line between functions and data.  I read this quickly at first and
thought
> that ApplicationWindows was data because there was no separation.
Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8002#newcode122
> Line 122: // TODO(dmaclach): remove
> Remove when?  Is there a cleanup task associated with this, or is it just "as
> soon as all the callers are gone?"
Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8003
> File chrome/browser/cocoa/cocoa_test_helper.mm (right):
> 
> http://codereview.chromium.org/327010/diff/8001/8003#newcode69
> Line 69: // Bootstrap Cocoa. It's very unhappy without this.
> Tell me about it.  :)
> 
> http://codereview.chromium.org/327010/diff/8001/8003#newcode77
> Line 77: // Recycle the pool to clean up any stuff that was put on the
> A blank line before a nice big comment block like this helps you find the top
> more easily.
Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8003#newcode93
> Line 93: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> ScopedNSAutoreleasePool?  It's OK to introduce a new scope for your pool.
Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8003#newcode94
> Line 94: [NSApp updateWindows];
> Hmm, does this need to be before nextEventMatchingMask:... and sendEvent:?  I
> think it usually follows sendEvent:.
Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8003#newcode132
> Line 132: // in this list may not be valid NSWindows anytime after this
returns.
> You
> nit: any time
Fixed 
> 
> http://codereview.chromium.org/327010/diff/8001/8004
> File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):
> 
> http://codereview.chromium.org/327010/diff/8001/8004#newcode10
> Line 10: class SadTabViewTest : public CocoaTest {
> Formatting nit:  don't indent anything within a namespace.  Also, on the next
> line, public should have the weird one-space indent.

Fixed

[pink (ping after 24hrs), 2009-10-23 16:01:38]

overall LGTM with some nits.

http://codereview.chromium.org/327010/diff/9001/10001
File chrome/browser/cocoa/cocoa_test_helper.h (right):

http://codereview.chromium.org/327010/diff/9001/10001#newcode87
Line 87: CocoaTestHelperWindow* test_window_;
is this a strong or weak ref? should be commented as such.

http://codereview.chromium.org/327010/diff/9001/10001#newcode95
Line 95: #define TEST_VIEW(test_fixture, view_member_name) \
yay!

http://codereview.chromium.org/327010/diff/9001/10002
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/9001/10002#newcode54
Line 54: initial_windows_.reset(ApplicationWindows());
this seems like a crutch for other "poorly written" tests that come before this
one. Should we ultimately strive to remove this? A TODO perhaps?

http://codereview.chromium.org/327010/diff/9001/10002#newcode134
Line 134: // in this list may no longer be valid NSWindows any time after this
returns.
shouldn't this comment be in the header, or at file-level here, rather than
being within the function impl? It seems like something the caller should know
about.

http://codereview.chromium.org/327010/diff/9001/10003
File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):

http://codereview.chromium.org/327010/diff/9001/10003#newcode20
Line 20: SadTabView* view_;
this should be marked as weak. Man this is gonna be a hard habit to break. We
need some docs.

[dmac, 2009-10-23 16:16:41]

On 2009/10/23 16:01:38, pink wrote:
> overall LGTM with some nits.
> 
> http://codereview.chromium.org/327010/diff/9001/10001
> File chrome/browser/cocoa/cocoa_test_helper.h (right):
> 
> http://codereview.chromium.org/327010/diff/9001/10001#newcode87
> Line 87: CocoaTestHelperWindow* test_window_;
> is this a strong or weak ref? should be commented as such.

I thought we only needed to comment when weak? Fixed.

> http://codereview.chromium.org/327010/diff/9001/10001#newcode95
> Line 95: #define TEST_VIEW(test_fixture, view_member_name) \
> yay!

Cheering for a macro? I'm headed to hell...I hear the skiing's good right now
;-)
 
> http://codereview.chromium.org/327010/diff/9001/10002
> File chrome/browser/cocoa/cocoa_test_helper.mm (right):
> 
> http://codereview.chromium.org/327010/diff/9001/10002#newcode54
> Line 54: initial_windows_.reset(ApplicationWindows());
> this seems like a crutch for other "poorly written" tests that come before
this
> one. Should we ultimately strive to remove this? A TODO perhaps?

It's not a crutch. It's just so that earlier tests that fail don't interfere
with your tests by reporting you having errors due to them not having cleaned
up. It was more intended to keep the actual window failures local to the test
that caused them, but I can see how it would mask people who completely forgot
to inherit from CocoaTest and didn't clean up after themselves.

What do people think? Should I just remove that bit? it would greatly simplify
the comparison code.
 
> http://codereview.chromium.org/327010/diff/9001/10002#newcode134
> Line 134: // in this list may no longer be valid NSWindows any time after this
> returns.
> shouldn't this comment be in the header, or at file-level here, rather than
> being within the function impl? It seems like something the caller should know
> about.

Fixed. Although it is a private method.
> 
> http://codereview.chromium.org/327010/diff/9001/10003
> File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):
> 
> http://codereview.chromium.org/327010/diff/9001/10003#newcode20
> Line 20: SadTabView* view_;
> this should be marked as weak. Man this is gonna be a hard habit to break. We
> need some docs.

Fixed. It is weak, but is owned by the view hierarchy. That's better than having
two strong references to it. As it was we didn't have clear ownership.

[Scott Hess - ex-Googler, 2009-10-23 16:50:47]

On 2009/10/23 15:00:38, dmac wrote:
> On 2009/10/23 14:10:53, Mark Mentovai wrote:
> > I think this is generally an improvement.
> > 
> > http://codereview.chromium.org/327010/diff/8001/8002
> > File chrome/browser/cocoa/cocoa_test_helper.h (right):
> > 
> > http://codereview.chromium.org/327010/diff/8001/8002#newcode70
> > Line 70: virtual void TearDown();
> > Is it important to have a TearDown or can we just do this stuff in the
> > destructor?
> > 
> > Mike and I have been down this road before, and we found that using the
> > destructor was mostly more handy than TearDown because the destructor runs
> after
> > the destructors of a subclass' members.
> 
> The reason it can't be done in the destructor is that structures that the
> windows may be depending on may have been destroyed when the subclass members
> were destructed. Normally the structures that the windows depend on are part
of
> the windows' NSWindowController and their destruction is done in a controlled
> fashion. When we are injecting structures in as part of unit tests we need to
be
> sure to have the windows destroyed before anything else. Hopefully this makes
> sense.

I'm still anti-TearDown(), as I think it makes the common case more brittle (if
someone forgets to call TearDown, we lose a valuable EXPECT, but they aren't
allowed to forget to call ~CocoaTest()).  The tests that involve
NSViewController know who they are, they could call CleanUpMyController() in
their destructor.  Another possibility would be a helper class to own
NSViewController test instances so that it is destructed in the right order
(like CocoaTestHelper).

Hmm, or CocoaTest::Autorelease(), which collects objects to release at the end
of the test.  Subclasses call that when they construct their NSViewController
instances.

[Scott Hess - ex-Googler, 2009-10-23 16:53:32]

One general comment - my similar CL separated "initialize Cocoa and make sure we
don't leak windows" from "here's a convenience window".  I did this so that you
could write tests which don't use windows at all and fail expectations if
something was unexpectedly creating windows.  Say you wanted to test font
handling or something.

http://codereview.chromium.org/327010/diff/9001/10003
File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):

http://codereview.chromium.org/327010/diff/9001/10003#newcode20
Line 20: SadTabView* view_;
On 2009/10/23 16:01:38, pink wrote:
> this should be marked as weak. Man this is gonna be a hard habit to break. We
> need some docs.

Dave, you say this is weak because we don't want two strong refs ... why not? 
It could have a dozen strong refs at one point.  If the refcount doesn't unwind
properly, that's a bug.  If the view is removed from the window and something
breaks because of this remaining reference, that's probably a bug, too.

Beyond that, if you can push TearDown() into ~CocoaTest(), then this reference
will be gone before ~CocoaTest() is called.

http://codereview.chromium.org/327010/diff/6005/6006
File chrome/browser/cocoa/cocoa_test_helper.h (right):

http://codereview.chromium.org/327010/diff/6005/6006#newcode101
Line 101: #define TEST_VIEW(test_fixture, view_member_name) \
Could you just define an instance function RunTheStandardViewTests(NSView*), and
then everyone can define their own trivial TEST_F()?  It's not a huge loss in
terms of lines, but ... macros are evil.  Except when they're deliciously evil.

http://codereview.chromium.org/327010/diff/6005/6006#newcode113
Line 113: // any tests based on them.
I think you should move the CocoaTest stuff to cocoa_test.h/mm, and plan to nuke
cocoa_test_helper.h/mm entirely.  I think the only reason it's a "helper" is
because you add it to some other class, so no reason to keep that sense around
long-term.

http://codereview.chromium.org/327010/diff/6005/6007
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/6005/6007#newcode29
Line 29: [self setPretendIsKeyWindow:YES];
Might be appropriate to reverse these two statements so that the view becomes
first-responder in a window which thinks it is front and center.  And then
reverse the -clear* sense to that things unwind the same way.

Unless there was a reason for this ordering, in which case a comment is in order
:-).

[Scott Hess - ex-Googler, 2009-10-23 16:54:56]

Also

PLUS ONE MILLION FOR PEDANTIC TESTING!


On Fri, Oct 23, 2009 at 9:53 AM,  <shess@chromium.org> wrote:
> One general comment - my similar CL separated "initialize Cocoa and make
> sure we
> don't leak windows" from "here's a convenience window". =A0I did this so =
that
> you
> could write tests which don't use windows at all and fail expectations if
> something was unexpectedly creating windows. =A0Say you wanted to test fo=
nt
> handling or something.
>
>
> http://codereview.chromium.org/327010/diff/9001/10003
> File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):
>
> http://codereview.chromium.org/327010/diff/9001/10003#newcode20
> Line 20: SadTabView* view_;
> On 2009/10/23 16:01:38, pink wrote:
>>
>> this should be marked as weak. Man this is gonna be a hard habit to
>
> break. We
>>
>> need some docs.
>
> Dave, you say this is weak because we don't want two strong refs ... why
> not? =A0It could have a dozen strong refs at one point. =A0If the refcoun=
t
> doesn't unwind properly, that's a bug. =A0If the view is removed from the
> window and something breaks because of this remaining reference, that's
> probably a bug, too.
>
> Beyond that, if you can push TearDown() into ~CocoaTest(), then this
> reference will be gone before ~CocoaTest() is called.
>
> http://codereview.chromium.org/327010/diff/6005/6006
> File chrome/browser/cocoa/cocoa_test_helper.h (right):
>
> http://codereview.chromium.org/327010/diff/6005/6006#newcode101
> Line 101: #define TEST_VIEW(test_fixture, view_member_name) \
> Could you just define an instance function
> RunTheStandardViewTests(NSView*), and then everyone can define their own
> trivial TEST_F()? =A0It's not a huge loss in terms of lines, but ...
> macros are evil. =A0Except when they're deliciously evil.
>
> http://codereview.chromium.org/327010/diff/6005/6006#newcode113
> Line 113: // any tests based on them.
> I think you should move the CocoaTest stuff to cocoa_test.h/mm, and plan
> to nuke cocoa_test_helper.h/mm entirely. =A0I think the only reason it's =
a
> "helper" is because you add it to some other class, so no reason to keep
> that sense around long-term.
>
> http://codereview.chromium.org/327010/diff/6005/6007
> File chrome/browser/cocoa/cocoa_test_helper.mm (right):
>
> http://codereview.chromium.org/327010/diff/6005/6007#newcode29
> Line 29: [self setPretendIsKeyWindow:YES];
> Might be appropriate to reverse these two statements so that the view
> becomes first-responder in a window which thinks it is front and center.
> =A0And then reverse the -clear* sense to that things unwind the same way.
>
> Unless there was a reason for this ordering, in which case a comment is
> in order :-).
>
> http://codereview.chromium.org/327010
>

[dmac, 2009-10-23 17:09:24]

On 2009/10/23 16:50:47, shess wrote:
> On 2009/10/23 15:00:38, dmac wrote:
> > On 2009/10/23 14:10:53, Mark Mentovai wrote:
> > > I think this is generally an improvement.
> > > 
> > > http://codereview.chromium.org/327010/diff/8001/8002
> > > File chrome/browser/cocoa/cocoa_test_helper.h (right):
> > > 
> > > http://codereview.chromium.org/327010/diff/8001/8002#newcode70
> > > Line 70: virtual void TearDown();
> > > Is it important to have a TearDown or can we just do this stuff in the
> > > destructor?
> > > 
> > > Mike and I have been down this road before, and we found that using the
> > > destructor was mostly more handy than TearDown because the destructor runs
> > after
> > > the destructors of a subclass' members.
> > 
> > The reason it can't be done in the destructor is that structures that the
> > windows may be depending on may have been destroyed when the subclass
members
> > were destructed. Normally the structures that the windows depend on are part
> of
> > the windows' NSWindowController and their destruction is done in a
controlled
> > fashion. When we are injecting structures in as part of unit tests we need
to
> be
> > sure to have the windows destroyed before anything else. Hopefully this
makes
> > sense.
> 
> I'm still anti-TearDown(), as I think it makes the common case more brittle
(if
> someone forgets to call TearDown, we lose a valuable EXPECT, but they aren't
> allowed to forget to call ~CocoaTest()).  The tests that involve
> NSViewController know who they are, they could call CleanUpMyController() in
> their destructor.  Another possibility would be a helper class to own
> NSViewController test instances so that it is destructed in the right order
> (like CocoaTestHelper).

They actually aren't allowed to forget to call TearDown. Check out the code and
you'll see the bool flag I threw in there to verify that TearDown gets called.
I'm actually anti the helper classes because it gets easy to accidentally have
unintentional destructor ordering dependencies that could fall apart anytime one
tries to do a basic class refactoring.

> 
> Hmm, or CocoaTest::Autorelease(), which collects objects to release at the end
> of the test.  Subclasses call that when they construct their NSViewController
> instances.

Some things not only need an autorelease, but require the event loop to be spun
to fully dealloc themselves. NSWindows are the big example (and their
NSTextViews).

[dmac, 2009-10-23 17:22:12]

On 2009/10/23 16:53:32, shess wrote:
> One general comment - my similar CL separated "initialize Cocoa and make sure
we
> don't leak windows" from "here's a convenience window".  I did this so that
you
> could write tests which don't use windows at all and fail expectations if
> something was unexpectedly creating windows.  Say you wanted to test font
> handling or something.

My test window is created lazily, so you must explicitly call it to create a
window as part of your test, so I don't think this is an issue.

> 
> http://codereview.chromium.org/327010/diff/9001/10003
> File chrome/browser/cocoa/sad_tab_view_unittest.mm (right):
> 
> http://codereview.chromium.org/327010/diff/9001/10003#newcode20
> Line 20: SadTabView* view_;
> On 2009/10/23 16:01:38, pink wrote:
> > this should be marked as weak. Man this is gonna be a hard habit to break.
We
> > need some docs.
> 
> Dave, you say this is weak because we don't want two strong refs ... why not? 
> It could have a dozen strong refs at one point.  If the refcount doesn't
unwind
> properly, that's a bug.  If the view is removed from the window and something
> breaks because of this remaining reference, that's probably a bug, too.

The main reason I want to keep this weak is to find places where we are
unintentionally depending on a spare reference to be around (such as the
removeSubview case without intentionally retaining it ahead of time). That's
also why I wrote the macro to handle the removeSubview case because with all the
duplication there were places people were doing it wrong and I wanted to
centralize it.

> Beyond that, if you can push TearDown() into ~CocoaTest(), then this reference
> will be gone before ~CocoaTest() is called.

See my note to Mark earlier about why we can't do this. Things the windows (and
their views and controllers etc) depend on can't be deleted before we kill off
the window.

> 
> http://codereview.chromium.org/327010/diff/6005/6006
> File chrome/browser/cocoa/cocoa_test_helper.h (right):
> 
> http://codereview.chromium.org/327010/diff/6005/6006#newcode101
> Line 101: #define TEST_VIEW(test_fixture, view_member_name) \
> Could you just define an instance function RunTheStandardViewTests(NSView*),
and
> then everyone can define their own trivial TEST_F()?  It's not a huge loss in
> terms of lines, but ... macros are evil.  Except when they're deliciously
evil.

We could, but wouldn't this just mean more duplicated code? I mean we are
already calling TEST_F (which is a macro), I'm just substituting one macro for
another.
 
> http://codereview.chromium.org/327010/diff/6005/6006#newcode113
> Line 113: // any tests based on them.
> I think you should move the CocoaTest stuff to cocoa_test.h/mm, and plan to
nuke
> cocoa_test_helper.h/mm entirely.  I think the only reason it's a "helper" is
> because you add it to some other class, so no reason to keep that sense around
> long-term.

Will do in a later CL.

> 
> http://codereview.chromium.org/327010/diff/6005/6007
> File chrome/browser/cocoa/cocoa_test_helper.mm (right):
> 
> http://codereview.chromium.org/327010/diff/6005/6007#newcode29
> Line 29: [self setPretendIsKeyWindow:YES];
> Might be appropriate to reverse these two statements so that the view becomes
> first-responder in a window which thinks it is front and center.  And then
> reverse the -clear* sense to that things unwind the same way.
> 
> Unless there was a reason for this ordering, in which case a comment is in
order
> :-).

Fixed. New version up.

[Scott Hess - ex-Googler, 2009-10-23 17:23:05]

On 2009/10/23 17:09:24, dmac wrote:
> On 2009/10/23 16:50:47, shess wrote:
> They actually aren't allowed to forget to call TearDown. Check out the code
and
> you'll see the bool flag I threw in there to verify that TearDown gets called.
> I'm actually anti the helper classes because it gets easy to accidentally have
> unintentional destructor ordering dependencies that could fall apart anytime
one
> tries to do a basic class refactoring.

Darn your thinking ahead!  You've convinced me, if we can't otherwise wedge it
into the destructor without horrible hoops.

> > Hmm, or CocoaTest::Autorelease(), which collects objects to release at the
end
> > of the test.  Subclasses call that when they construct their
NSViewController
> > instances.
> 
> Some things not only need an autorelease, but require the event loop to be
spun
> to fully dealloc themselves. NSWindows are the big example (and their
> NSTextViews).

Hmm, maybe Autorelease() wasn't the right name.  This isn't -autorelease.  I
meant "Register this object to be sent -release at the end of the test".  Let's
call is ScottoRelease() for purposes of argument, then the implementation would
look something like:

   scoped_nsobject<NSArray> scotto_release_pool_;

  void CocoaTest::ScottoRelease(NSObject* anObject) {
    [scotto_release_pool_ addObject:anObject];  // retains anObject, of course.
  }

This keeps a reference on the object until after CocoaTest::~CocoaTest() has
completed.  The subclass can use a scoped_nsobject<> just fine, so long as it
calls ScottoRelease() on it.

I think.

-scott

[dmac, 2009-10-23 17:31:44]

On 2009/10/23 17:23:05, shess wrote:
> On 2009/10/23 17:09:24, dmac wrote:
> > On 2009/10/23 16:50:47, shess wrote:
> > They actually aren't allowed to forget to call TearDown. Check out the code
> and
> > you'll see the bool flag I threw in there to verify that TearDown gets
called.
> > I'm actually anti the helper classes because it gets easy to accidentally
have
> > unintentional destructor ordering dependencies that could fall apart anytime
> one
> > tries to do a basic class refactoring.
> 
> Darn your thinking ahead!  You've convinced me, if we can't otherwise wedge it
> into the destructor without horrible hoops.

I spent a week at it (amongst lots of other things ;-) ) and every time I
thought I had a viable solution something else would fall apart. This is the
cleanest design I can come up with. You should've seen some of the beautiful
template classes I had for a while with nice little callbacks and everything.
Callbacks are a frickin' nightmare in C++. It's also the reason that I didn't
submit this in little bits and pieces until I had the whole thing working as I
wanted to be sure I covered every case that we had before I ran into troubles
and had to start backpedaling.

> 
> > > Hmm, or CocoaTest::Autorelease(), which collects objects to release at the
> end
> > > of the test.  Subclasses call that when they construct their
> NSViewController
> > > instances.
> > 
> > Some things not only need an autorelease, but require the event loop to be
> spun
> > to fully dealloc themselves. NSWindows are the big example (and their
> > NSTextViews).
> 
> Hmm, maybe Autorelease() wasn't the right name.  This isn't -autorelease.  I
> meant "Register this object to be sent -release at the end of the test". 
Let's
> call is ScottoRelease() for purposes of argument, then the implementation
would
> look something like:
> 
>    scoped_nsobject<NSArray> scotto_release_pool_;
> 
>   void CocoaTest::ScottoRelease(NSObject* anObject) {
>     [scotto_release_pool_ addObject:anObject];  // retains anObject, of
course.
>   }
> 
> This keeps a reference on the object until after CocoaTest::~CocoaTest() has
> completed.  The subclass can use a scoped_nsobject<> just fine, so long as it
> calls ScottoRelease() on it.
> 
> I think.

Most of our worries here are mainly due to the contrived nature of unittests
where we are injecting mock objects, or trying to set up environments for our
classes to run in. I see where you are coming from with ScottoRelease, but
there's a couple of issues:

a) not everything wants release. NSWindowControllers want close to be called on
their windows for example.
b) We still run into the issue where we need to be sure windows are cleaned up
before the things injected in them. At least doing it in TearDown vs the d'tor
gives us a very hard clean separation that's easy to document.
c) See my other note for the reason I'm keeping the view intentionally a weak
reference.

Cheers,
Dave

[dmac, 2009-10-23 17:51:06]

Pink gave me a conditional LGTM. Is everybody happy with this? I would like to
get it in so I can start rolling other CLs at you before I lose the east coast
contingent in 3 hours or so.

[Mark Mentovai, 2009-10-23 17:57:21]

http://www.nvbdi.org/images/approved_rubber_stamp.jpg

[Scott Hess - ex-Googler, 2009-10-23 18:43:49]

LGTM, minor nits.

The vector<>* thing feels wrong, though, unless there's a reason.

http://codereview.chromium.org/327010/diff/6010/11009
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/6010/11009#newcode76
Line 76: [test_window_ close];
Suggest test_window_ = nil here.  Just in case.

http://codereview.chromium.org/327010/diff/6010/11009#newcode89
Line 89: std::vector<NSWindow*> windows_left;
Move to point of first use.

http://codereview.chromium.org/327010/diff/6010/11009#newcode122
Line 122: for (size_t i=0; i<windows_left.size(); i++) {
spacing around = and <.  And ++i.

http://codereview.chromium.org/327010/diff/6010/11009#newcode132
Line 132: std::vector<NSWindow*>* CocoaTest::ApplicationWindows() {
Why not return a vector<> rather than a vector<>*, here?  I had to look really
closely at windowsWaiting and loop to figure out why you even bothered to have
them (I think windowsWaiting is needed not to leak?).

[dmac, 2009-10-23 19:04:07]

http://codereview.chromium.org/327010/diff/6010/11009
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/6010/11009#newcode76
Line 76: [test_window_ close];
On 2009/10/23 18:43:49, shess wrote:
> Suggest test_window_ = nil here.  Just in case.

Fixed

http://codereview.chromium.org/327010/diff/6010/11009#newcode89
Line 89: std::vector<NSWindow*> windows_left;
On 2009/10/23 18:43:49, shess wrote:
> Move to point of first use.

Done.

http://codereview.chromium.org/327010/diff/6010/11009#newcode122
Line 122: for (size_t i=0; i<windows_left.size(); i++) {
On 2009/10/23 18:43:49, shess wrote:
> spacing around = and <.  And ++i.

Done.

http://codereview.chromium.org/327010/diff/6010/11009#newcode132
Line 132: std::vector<NSWindow*>* CocoaTest::ApplicationWindows() {
On 2009/10/23 18:43:49, shess wrote:
> Why not return a vector<> rather than a vector<>*, here?  I had to look really
> closely at windowsWaiting and loop to figure out why you even bothered to have
> them (I think windowsWaiting is needed not to leak?).

because I was being dumb ;-)  Good catch. In a previous revision I was using
NSArrays and changed them over to vectors, but didn't change from pointers down
to straight assignments.

[Scott Hess - ex-Googler, 2009-10-23 19:19:48]

Sticky LGTM, even w/out my suggestions.

http://codereview.chromium.org/327010/diff/7005/1007
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/7005/1007#newcode91
Line 91: std::vector<NSWindow*> windows_left;
I really hate that I'm doing this, because I really want this checked in, but
...

windows_left should be windowsLeft, or startDate should be start_date, or the
local variables should otherwise be consistently named.

http://codereview.chromium.org/327010/diff/7005/1007#newcode95
Line 95: while (loop) {
And as long as I brought one thing up, could this now just be:
  while (ApplicationWindows().size() > 0)
or even just:
  while (1)
???

[dmac, 2009-10-23 19:35:21]

http://codereview.chromium.org/327010/diff/7005/1007
File chrome/browser/cocoa/cocoa_test_helper.mm (right):

http://codereview.chromium.org/327010/diff/7005/1007#newcode91
Line 91: std::vector<NSWindow*> windows_left;
On 2009/10/23 19:19:48, shess wrote:
> I really hate that I'm doing this, because I really want this checked in, but
> ...
> 
> windows_left should be windowsLeft, or startDate should be start_date, or the
> local variables should otherwise be consistently named.

Good catch. Fixed throughout. Also it pointed out to me the spare windows_left
that hadn't been removed when I moved it into the loop.

http://codereview.chromium.org/327010/diff/7005/1007#newcode95
Line 95: while (loop) {
On 2009/10/23 19:19:48, shess wrote:
> And as long as I brought one thing up, could this now just be:
>   while (ApplicationWindows().size() > 0)
> or even just:
>   while (1)
> ???

I did this intentionally so that we wouldn't hit the event loop if there were no
windows to close. I didn't want anything that may happen in the event loop to
affect the test state. Also I separated it out so that people were aware that I
wasn't depending on windowsWaiting to change, that the only way out of the loop
was through the two breaks below.

I did however change it from BOOL to bool.