First set of unittest fixes. Many more to come ;-)...

First set of unittest fixes. Many more to come ;-)

TEST=run the unittests and watch them pass.
BUG=none


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

[dmac, 2009-10-24 00:23:16]

First set of unittests that aren't bubbles or browsers.

[Scott Hess - ex-Googler, 2009-10-24 02:45:43]

As I look at the TEST_VIEW stuff, I find myself wondering if it couldn't just be
implicit.  The SetUp() stuff already tests adding the view, presumably we're
displaying the view when we put it in the window, and we could easily enough add
-removeFromSuperview against all subviews of the window's contentView in
TearDown(), if we really think that kind of manipulation is warranted.  After
that, does the separate test add anything?

http://codereview.chromium.org/334016/diff/1/6
File chrome/browser/cocoa/extension_shelf_controller_unittest.mm (right):

http://codereview.chromium.org/334016/diff/1/6#newcode32
Line 32: TEST_VIEW(ExtensionShelfControllerTest, [controller_ view])
I don't like this.  Wasn't this defined to take a member variable as the second
parameter?

http://codereview.chromium.org/334016/diff/1/3
File chrome/browser/cocoa/infobar_gradient_view_unittest.mm (right):

http://codereview.chromium.org/334016/diff/1/3#newcode16
Line 16: initWithFrame:frame]);
alloc/init on second line.

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

http://codereview.chromium.org/334016/diff/1/2#newcode17
Line 17: initWithFrame:frame]);
alloc/init on second line.

[dmac, 2009-10-24 23:12:19]

On 2009/10/24 02:45:43, shess wrote:
> As I look at the TEST_VIEW stuff, I find myself wondering if it couldn't just
be
> implicit.  The SetUp() stuff already tests adding the view, presumably we're
> displaying the view when we put it in the window, and we could easily enough
add
> -removeFromSuperview against all subviews of the window's contentView in
> TearDown(), if we really think that kind of manipulation is warranted.  After
> that, does the separate test add anything?

The main reason I left it as a macro is that people may tear down structures
that their view depends on in their TearDown before they call
CocoaTest::TearDown (where the remove is actually going to occur). The main
place I thought this could occur would be if their view was in a special window
that they created (eg bubble view in a bubble window) and in their TearDown they
would've already called close on the bubble window that they created before we
got a chance to remove their view. As far as the manipulation being warranted,
there are several routines that views can override that would be tested by this
manipulation (we should possible add one for moving from window to window as
well) and it's relatively cheap and easy to do. Doesn't it fall under pedantic
testing? ;-) 

Certainly most of the view classes I modified had this (or similar) boiler plate
code copied in.

[dmac, 2009-10-24 23:12:33]

http://codereview.chromium.org/334016/diff/1/6
File chrome/browser/cocoa/extension_shelf_controller_unittest.mm (right):

http://codereview.chromium.org/334016/diff/1/6#newcode32
Line 32: TEST_VIEW(ExtensionShelfControllerTest, [controller_ view])
On 2009/10/24 02:45:43, shess wrote:
> I don't like this.  Wasn't this defined to take a member variable as the
second
> parameter?

No need for it to be a member variable. It just had to be a view. I did call it
view_member_name in the macro, but it really just has to be a view.

http://codereview.chromium.org/334016/diff/1/3
File chrome/browser/cocoa/infobar_gradient_view_unittest.mm (right):

http://codereview.chromium.org/334016/diff/1/3#newcode16
Line 16: initWithFrame:frame]);
On 2009/10/24 02:45:43, shess wrote:
> alloc/init on second line.

Done.

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

http://codereview.chromium.org/334016/diff/1/2#newcode17
Line 17: initWithFrame:frame]);
On 2009/10/24 02:45:43, shess wrote:
> alloc/init on second line.

Done.

[dmac, 2009-10-24 23:14:19]

BTW this CL is going to fail on the Mac trybot until
http://codereview.chromium.org/333017 gets submitted.

[pink (ping after 24hrs), 2009-10-26 14:46:06]

LGTM

[Mark Mentovai, 2009-10-26 14:48:37]

LG™

[Scott Hess - ex-Googler, 2009-10-26 16:58:08]

lgtm

http://codereview.chromium.org/334016/diff/1/6
File chrome/browser/cocoa/extension_shelf_controller_unittest.mm (right):

http://codereview.chromium.org/334016/diff/1/6#newcode32
Line 32: TEST_VIEW(ExtensionShelfControllerTest, [controller_ view])
On 2009/10/24 23:12:33, dmac wrote:
> On 2009/10/24 02:45:43, shess wrote:
> > I don't like this.  Wasn't this defined to take a member variable as the
> second
> > parameter?
> 
> No need for it to be a member variable. It just had to be a view. I did call
it
> view_member_name in the macro, but it really just has to be a view.

For some reason, I was recalling TEST_VIEW as building
AddRemove##view_member_name.  Probably because I was thinking you could use it
with multiple views.  But I was wrong!  Feel free to send me a review where
"view_member_name" -> "view", if you care to :-).