(Mac) Added unit tests for ClickHoldButtonCell and DelayedMenuButton.

(Mac) Added unit tests for ClickHoldButtonCell and DelayedMenuButton.

Also added some comments and cleaned up some code.

BUG=20693
TEST=Run unit tests. Also make sure back/forward buttons (and menus) \
     work correctly.

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

[Avi (use Gerrit), 2009-08-31 20:33:31]

http://codereview.chromium.org/179043/diff/1/4
File chrome/browser/cocoa/clickhold_button_cell_unittest.mm (right):

http://codereview.chromium.org/179043/diff/1/4#newcode34
Line 34: EXPECT_FALSE([view_ superview]);
This test leaves the state of the test different than when it began. That can
cause dependencies on the order of the running of the tests, which is bad.

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

http://codereview.chromium.org/179043/diff/1/6#newcode35
Line 35: EXPECT_FALSE([button_ superview]);
Same issue: don't change state in a test unless you put it back.

[viettrungluu, 2009-08-31 20:55:54]

http://codereview.chromium.org/179043/diff/1/4
File chrome/browser/cocoa/clickhold_button_cell_unittest.mm (right):

http://codereview.chromium.org/179043/diff/1/4#newcode34
Line 34: EXPECT_FALSE([view_ superview]);
According to <http://code.google.com/p/googletest/wiki/GoogleTestPrimer> under
"Test Fixtures: ...", a new |ClickHoldButtonCellTest| (i.e., "test fixture") is
instantiated for each |TEST_F()|. So this should be okay, shouldn't it? (This
test is actually copied verbatim from elsewhere; we have many copies of this
exact test.)

[pink (ping after 24hrs), 2009-09-01 12:44:56]

LGTM with suggested fixes. You're correct about each TEST_F re-instantiating the
test object.

http://codereview.chromium.org/179043/diff/9/1007
File chrome/browser/cocoa/clickhold_button_cell.h (right):

http://codereview.chromium.org/179043/diff/9/1007#newcode36
Line 36: // Activate (click-hold) immediately on drag? Default: YES.
Do we want it to activate immediately or only after the mouse has travelled a
certain amount?

http://codereview.chromium.org/179043/diff/9/1008
File chrome/browser/cocoa/clickhold_button_cell.mm (right):

http://codereview.chromium.org/179043/diff/9/1008#newcode19
Line 19: 
no need for extra spaces around this

http://codereview.chromium.org/179043/diff/9/1009
File chrome/browser/cocoa/clickhold_button_cell_unittest.mm (right):

http://codereview.chromium.org/179043/diff/9/1009#newcode14
Line 14: class ClickHoldButtonCellTest : public testing::Test {
PlatformTest to get an autorelease pool

http://codereview.chromium.org/179043/diff/9/1010
File chrome/browser/cocoa/delayedmenu_button.mm (right):

http://codereview.chromium.org/179043/diff/9/1010#newcode78
Line 78: if (id cell = [self cell]) {
i'd break this out into multiple lines (checking for nil separately). It's a
little easier to read.

http://codereview.chromium.org/179043/diff/9/1011
File chrome/browser/cocoa/delayedmenu_button_unittest.mm (right):

http://codereview.chromium.org/179043/diff/9/1011#newcode15
Line 15: class DelayedMenuButtonTest : public testing::Test {
PlatformTest

http://codereview.chromium.org/179043/diff/9/1011#newcode46
Line 46: NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];
use a scoped_nsobject

http://codereview.chromium.org/179043/diff/9/1011#newcode56
Line 56: EXPECT_FALSE([button_ menu] == nil);
this makes more sense as EXPECT_TRUE([button_ menu]). No double-negatives.

[viettrungluu, 2009-09-01 15:26:10]

http://codereview.chromium.org/179043/diff/9/1007
File chrome/browser/cocoa/clickhold_button_cell.h (right):

http://codereview.chromium.org/179043/diff/9/1007#newcode36
Line 36: // Activate (click-hold) immediately on drag? Default: YES.
On 2009/09/01 12:44:57, pink wrote:
> Do we want it to activate immediately or only after the mouse has travelled a
> certain amount?

Documentation error; fixed. YES means activate on a big-enough drag. NO means
wait for timeout.

http://codereview.chromium.org/179043/diff/9/1008
File chrome/browser/cocoa/clickhold_button_cell.mm (right):

http://codereview.chromium.org/179043/diff/9/1008#newcode19
Line 19: 
On 2009/09/01 12:44:57, pink wrote:
> no need for extra spaces around this

Done.

http://codereview.chromium.org/179043/diff/9/1009
File chrome/browser/cocoa/clickhold_button_cell_unittest.mm (right):

http://codereview.chromium.org/179043/diff/9/1009#newcode14
Line 14: class ClickHoldButtonCellTest : public testing::Test {
On 2009/09/01 12:44:57, pink wrote:
> PlatformTest to get an autorelease pool

Done.

http://codereview.chromium.org/179043/diff/9/1010
File chrome/browser/cocoa/delayedmenu_button.mm (right):

http://codereview.chromium.org/179043/diff/9/1010#newcode78
Line 78: if (id cell = [self cell]) {
On 2009/09/01 12:44:57, pink wrote:
> i'd break this out into multiple lines (checking for nil separately). It's a
> little easier to read.

Done.

http://codereview.chromium.org/179043/diff/9/1011
File chrome/browser/cocoa/delayedmenu_button_unittest.mm (right):

http://codereview.chromium.org/179043/diff/9/1011#newcode15
Line 15: class DelayedMenuButtonTest : public testing::Test {
On 2009/09/01 12:44:57, pink wrote:
> PlatformTest

Done.

http://codereview.chromium.org/179043/diff/9/1011#newcode46
Line 46: NSMenu* menu = [[NSMenu alloc] initWithTitle:@""];
On 2009/09/01 12:44:57, pink wrote:
> use a scoped_nsobject

Done.

http://codereview.chromium.org/179043/diff/9/1011#newcode56
Line 56: EXPECT_FALSE([button_ menu] == nil);
On 2009/09/01 12:44:57, pink wrote:
> this makes more sense as EXPECT_TRUE([button_ menu]). No double-negatives.

Done.