A BookmarkBarFolderButtonCell should have the favicon aligned left when no title is provided

A BookmarkBarFolderButtonCell should have the favicon aligned left when no title is provided

This change has been made at the parent class level, where the rendering
of the BookmarkButtonCell is implemented. It aligns the favicon with
NSImageLeft if there is no title.

A test has also been added to check for the differences in image positioning between these two classes.

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

[KushalP, 2011-12-04 00:23:59]

This is a nicer fix than adding a space to the title text! :D

Please can someone send this through the trybots when they get a chance? Thanks.

Here's the command:

trychange.py -R codereview.chromium.org/8792001 --email kushi.p@gmail.com --name
issue_8792001 --root src -p 1

[KushalP, 2011-12-04 00:26:09]

If there's a smart way to unit test this, please let me know.

[Ilya Sherman, 2011-12-05 23:53:57]

On 2011/12/04 00:23:59, KushalP wrote:
> This is a nicer fix than adding a space to the title text! :D

Aye :)

> Please can someone send this through the trybots when they get a chance?
Thanks.

I don't think there's a need for trybots for this change.  I'm assuming you've
compiled and verified the code change locally.  Beyond that, we can just let the
full set of tests be run by the commit queue when the change is ready to be
committed.  There's only so many trybots to go around, so I recommend trying to
use them only if you can't compile and test locally (or you're worried that your
change might behave differently on different OSes, etc.), or aren't planning to
submit via the commit queue.

On 2011/12/04 00:26:09, KushalP wrote:
> If there's a smart way to unit test this, please let me know.

You should be able to do write something along the lines of existing tests
bookmark_button_cell_unittest.mm.  You could create two bookmarks, one with a
title and one without a title, and verify that their imagePosition methods
return the same value when within a folder, but different values when directly
on the bar.  (It would be even better if you could just test the image frame's
left coordinate directly, but I'm not sure whether there's an easy way to do
that.)

http://codereview.chromium.org/8792001/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

http://codereview.chromium.org/8792001/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:120: withString:@" "];
nit: Hmm, this isn't relevant to this CL so much as one of your previous ones;
but are these replacements still necessary now that we've updated the model?

http://codereview.chromium.org/8792001/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:122: // default, Cocoa
leaves extra space in an attempt to display an empty title.
nit: This comment should be moved into the "else" case, and updated to clarify
that we only do this for bookmarks directly on the bookmarks bar, but not for
those in folders.

http://codereview.chromium.org/8792001/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:128: // the favicon
aligned to the left.
nit: "folder button cell" reads as slightly confusing to me in this comment. 
Perhaps "Left-align icons for bookmarks within folders, regardless of whether
there is a title."  (With a matching comment in the "else" case describing what
happens for bookmarks directly in the bar.)

[KushalP, 2011-12-13 17:39:06]

http://codereview.chromium.org/8792001/diff/1/chrome/browser/ui/cocoa/bookmar...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

http://codereview.chromium.org/8792001/diff/1/chrome/browser/ui/cocoa/bookmar...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:120: withString:@" "];
On 2011/12/05 23:53:58, Ilya Sherman wrote:
> nit: Hmm, this isn't relevant to this CL so much as one of your previous ones;
> but are these replacements still necessary now that we've updated the model?

I'll have a look and see what happens when I remove it. It's assigned
post-addition to the model so I'd assume that it's safe to remove.

[KushalP, 2011-12-13 20:27:08]

Updated with new patch set fixing nits and adding a unit test.

[Ilya Sherman, 2011-12-13 22:35:24]

LG with nits :)


> On 2011/12/04 00:26:09, KushalP wrote:
> > If there's a smart way to unit test this, please let me know.
> 
> You should be able to do write something along the lines of existing tests
> bookmark_button_cell_unittest.mm.  You could create two bookmarks, one with a
> title and one without a title, and verify that their imagePosition methods
> return the same value when within a folder, but different values when directly
> on the bar.  (It would be even better if you could just test the image frame's
> left coordinate directly, but I'm not sure whether there's an easy way to do
> that.)

Hmm, I wonder if you could use NSCell's imageRectForBounds: method to compare
the left coordinates directly.  That would make for a slightly less
fragile/implementation-dependent test.

http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (left):

http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:120: withString:@" "];
nit: For the sake of clarity when scanning the revision history later on, please
make this change in a separate CL.

http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:126: // bookmarks bar
(directly) and not in a folder.
nit: This comment is a little confusing, because it initially sounds like it
will be saying "Only show the icon when foo, otherwise don't show the icon",
whereas what it's really saying is "Show just the icon, no title, when foo." 
It's also a little confusing because there's nothing other than the icon to show
if there isn't at title -- setting the image position to NSImageOnly really just
affects the spacing of the icon.  I think the old comment conveyed this pretty
well, so I'd tweak it to something like:

// For untitled bookmarks visible directly in the bookmarks bar, squeeze
// things tight by displaying only the image. By default, Cocoa leaves extra
// space in an attempt to display an empty title.

[KushalP, 2011-12-13 22:51:01]

On 2011/12/13 22:35:24, Ilya Sherman wrote:
> Hmm, I wonder if you could use NSCell's imageRectForBounds: method to compare
> the left coordinates directly.  That would make for a slightly less
> fragile/implementation-dependent test.

Do you mean comparing the NSRect that the method returns when I provide it with
an NSRect? If so, should I keep the current NSImage position checks that I have
currently?

[KushalP, 2011-12-13 22:51:31]

http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (left):

http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:120: withString:@" "];
On 2011/12/13 22:35:24, Ilya Sherman wrote:
> nit: For the sake of clarity when scanning the revision history later on,
please
> make this change in a separate CL.

As in a completely different commit to the repo? Sorry if this is a dumb
question I just get confused as CL is a perforce term.

[Ilya Sherman, 2011-12-13 22:55:39]

On 2011/12/13 22:51:31, KushalP wrote:
>
http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
> File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (left):
> 
>
http://codereview.chromium.org/8792001/diff/6001/chrome/browser/ui/cocoa/book...
> chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:120: withString:@"
"];
> On 2011/12/13 22:35:24, Ilya Sherman wrote:
> > nit: For the sake of clarity when scanning the revision history later on,
> please
> > make this change in a separate CL.
> 
> As in a completely different commit to the repo? Sorry if this is a dumb
> question I just get confused as CL is a perforce term.

Yep, a completely different commit.  We've kind of co-opted CL from perforce,
for better or worse ;)

[Ilya Sherman, 2011-12-13 22:57:16]

On 2011/12/13 22:51:01, KushalP wrote:
> On 2011/12/13 22:35:24, Ilya Sherman wrote:
> > Hmm, I wonder if you could use NSCell's imageRectForBounds: method to
compare
> > the left coordinates directly.  That would make for a slightly less
> > fragile/implementation-dependent test.
> 
> Do you mean comparing the NSRect that the method returns when I provide it
with
> an NSRect? If so, should I keep the current NSImage position checks that I
have
> currently?

Specifically, comparing the NSRect's left x-coordinate.  If you get that
working, there's no need for the NSImage position checks -- those are more of an
implementation detail (though they work ok for testing if the coordinate
approach does not work out.)

[Ilya Sherman, 2011-12-14 00:27:39]

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:59:
EXPECT_EQ(67, ([cell imageRectForBounds:rect]).origin.y);
nit: No need to test the y co-ordinate.

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:60:
EXPECT_EQ(67, ([cell imageRectForBounds:rect]).origin.x);
Rather than testing that the x-coordinate is 55 or 67, please test that the
x-coordinate is different depending on whether a title is set for |cell|; and
that it is the same regardless of whether a title is set for |folder_cell|.

[KushalP, 2011-12-14 00:31:04]

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:60:
EXPECT_EQ(67, ([cell imageRectForBounds:rect]).origin.x);
On 2011/12/14 00:27:39, Ilya Sherman wrote:
> Rather than testing that the x-coordinate is 55 or 67, please test that the
> x-coordinate is different depending on whether a title is set for |cell|; and
> that it is the same regardless of whether a title is set for |folder_cell|.

So something like:

EXPECT_NE(67, ([cell imageRectForBounds:rect]).origin.x);

?

[Ilya Sherman, 2011-12-14 00:33:05]

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:60:
EXPECT_EQ(67, ([cell imageRectForBounds:rect]).origin.x);
On 2011/12/14 00:31:04, KushalP wrote:
> On 2011/12/14 00:27:39, Ilya Sherman wrote:
> > Rather than testing that the x-coordinate is 55 or 67, please test that the
> > x-coordinate is different depending on whether a title is set for |cell|;
and
> > that it is the same regardless of whether a title is set for |folder_cell|.
> 
> So something like:
> 
> EXPECT_NE(67, ([cell imageRectForBounds:rect]).origin.x);
> 
> ?

Something like:

[cell setBookmarkCellText:@"" image:image];
int x_without_title = [cell imageRectForBounds:rect]).origin.x;
[cell setBookmarkCellText:@"test" image:image];
int x_with_title = [cell imageRectForBounds:rect]).origin.x;
EXPECT_NE(x_without_title, x_with_title);

(Except "int" is probably the wrong integer type for the coordinate.)

[Ilya Sherman, 2011-12-14 00:36:42]

On 2011/12/14 00:33:05, Ilya Sherman wrote:
>
http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
> File
> chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
> (right):
> 
>
http://codereview.chromium.org/8792001/diff/14001/chrome/browser/ui/cocoa/boo...
>
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:60:
> EXPECT_EQ(67, ([cell imageRectForBounds:rect]).origin.x);
> On 2011/12/14 00:31:04, KushalP wrote:
> > On 2011/12/14 00:27:39, Ilya Sherman wrote:
> > > Rather than testing that the x-coordinate is 55 or 67, please test that
the
> > > x-coordinate is different depending on whether a title is set for |cell|;
> and
> > > that it is the same regardless of whether a title is set for
|folder_cell|.
> > 
> > So something like:
> > 
> > EXPECT_NE(67, ([cell imageRectForBounds:rect]).origin.x);
> > 
> > ?
> 
> Something like:
> 
> [cell setBookmarkCellText:@"" image:image];
> int x_without_title = [cell imageRectForBounds:rect]).origin.x;
> [cell setBookmarkCellText:@"test" image:image];
> int x_with_title = [cell imageRectForBounds:rect]).origin.x;
> EXPECT_NE(x_without_title, x_with_title);
> 
> (Except "int" is probably the wrong integer type for the coordinate.)

(We might also want to compare the cell widths, using NSCell's cellSize:, since
in the one case we set the position to NSImageOnly to make things more compact.)

[KushalP, 2011-12-14 01:01:13]

Updated with with and x-coord tests.

[KushalP, 2011-12-14 01:31:51]

On 2011/12/14 01:01:13, KushalP wrote:
> Updated with with and x-coord tests.

*width

[Ilya Sherman, 2011-12-14 21:38:07]

http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:61:
EXPECT_EQ(NSImageOnly, [cell imagePosition]);
nit: Please remove these statements, as they are now primarily testing an
implementation detail that the other EXPECT_* statements are testing at a more
abstract level.

http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:68:
EXPECT_NE(cell_x_without_title, cell_x_with_title);
nit: EXPECT_LT rather than EXPECT_NE

http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:69:
EXPECT_EQ(cell_width_without_title, cell_width_with_title);
This currently tests that the bookmark icons within the cells are sized
identically.  I was instead suggesting that we test the *cell* widths, including
both the image and the title (if any).  The cell widths should be different --
we should be able to write EXPECT_LT(cell_width_without_title,
cell_width_with_title).

[KushalP, 2011-12-14 22:10:05]

http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:69:
EXPECT_EQ(cell_width_without_title, cell_width_with_title);
On 2011/12/14 21:38:07, Ilya Sherman wrote:
> This currently tests that the bookmark icons within the cells are sized
> identically.  I was instead suggesting that we test the *cell* widths,
including
> both the image and the title (if any).  The cell widths should be different --
> we should be able to write EXPECT_LT(cell_width_without_title,
> cell_width_with_title).

I've just looked through the NSCell and NSButton Apple docs and neither of them
provide an obvious way to get the width of the cell.

Do you mean adding the widths provided for the text and image rects and then
adding them?

[Ilya Sherman, 2011-12-14 22:48:58]

On 2011/12/14 22:10:05, KushalP wrote:
>
http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
> File
> chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
> (right):
> 
>
http://codereview.chromium.org/8792001/diff/9005/chrome/browser/ui/cocoa/book...
>
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:69:
> EXPECT_EQ(cell_width_without_title, cell_width_with_title);
> On 2011/12/14 21:38:07, Ilya Sherman wrote:
> > This currently tests that the bookmark icons within the cells are sized
> > identically.  I was instead suggesting that we test the *cell* widths,
> including
> > both the image and the title (if any).  The cell widths should be different
--
> > we should be able to write EXPECT_LT(cell_width_without_title,
> > cell_width_with_title).
> 
> I've just looked through the NSCell and NSButton Apple docs and neither of
them
> provide an obvious way to get the width of the cell.

Does NSCell's cellSize: method not let you compute the cell's width?

[Ryan Sleevi, 2011-12-15 00:45:54]

http://codereview.chromium.org/8792001/diff/21002/chrome/browser/ui/cocoa/boo...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/21002/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:31:
EXPECT_TRUE(view.get());
Drive by: Should these be ASSERT_TRUEs instead? 

The difference is that ASSERT_TRUE will (gracefully) fail the test if the
assertion fails - which is useful for checking for NULL pointers before you
dereference them (which would crash the test and abort running more)

[KushalP, 2011-12-15 01:29:26]

http://codereview.chromium.org/8792001/diff/21002/chrome/browser/ui/cocoa/boo...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

http://codereview.chromium.org/8792001/diff/21002/chrome/browser/ui/cocoa/boo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:31:
EXPECT_TRUE(view.get());
On 2011/12/15 00:45:54, Ryan Sleevi wrote:
> Drive by: Should these be ASSERT_TRUEs instead? 
> 
> The difference is that ASSERT_TRUE will (gracefully) fail the test if the
> assertion fails - which is useful for checking for NULL pointers before you
> dereference them (which would crash the test and abort running more)

Yes they should. Updating now.

[Ilya Sherman, 2011-12-15 01:49:29]

LGTM, thanks :)

[commit-bot: I haz the power, 2011-12-15 01:49:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/8792001/26001

[commit-bot: I haz the power, 2011-12-15 03:08:33]

Change committed as 114588