Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(38)

Issue 179043: (Mac) Added unit tests for ClickHoldButtonCell and DelayedMenuButton. (Closed)

Created:
11 years, 3 months ago by viettrungluu
Modified:
9 years, 6 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

(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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rebased to ToT. #

Total comments: 14

Patch Set 3 : Use PlatformTest, etc. per pinkerton's review. #

Patch Set 4 : Read gtest docs; using constructors instead of SetUp() is okay. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+221 lines, -36 lines) Patch
M chrome/browser/cocoa/clickhold_button_cell.h View 1 2 2 chunks +9 lines, -8 lines 0 comments Download
M chrome/browser/cocoa/clickhold_button_cell.mm View 1 2 3 chunks +52 lines, -9 lines 0 comments Download
A chrome/browser/cocoa/clickhold_button_cell_unittest.mm View 1 2 3 1 chunk +62 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/delayedmenu_button.mm View 1 2 5 chunks +25 lines, -19 lines 0 comments Download
A chrome/browser/cocoa/delayedmenu_button_unittest.mm View 1 2 3 1 chunk +71 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
viettrungluu
11 years, 3 months ago (2009-08-31 20:08:58 UTC) #1
Avi (use Gerrit)
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 ...
11 years, 3 months ago (2009-08-31 20:33:31 UTC) #2
viettrungluu
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: ...
11 years, 3 months ago (2009-08-31 20:55:54 UTC) #3
pink (ping after 24hrs)
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 ...
11 years, 3 months ago (2009-09-01 12:44:56 UTC) #4
viettrungluu
11 years, 3 months ago (2009-09-01 15:26:10 UTC) #5
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.

Powered by Google App Engine
This is Rietveld 408576698