[Mac] Bookmark button cell updates

[Mac] Bookmark button cell updates

This is extracted from the bookmark bar refactor.

Changes:
- Changing a cell's node now updates the title
- A new class method for calculating a cell's size without
instantiating one.

BUG=648560
Review-Url: https://codereview.chromium.org/2794123002
Cr-Commit-Position: refs/heads/master@{#462577}
Committed: https://chromium.googlesource.com/chromium/src/+/e4e3e7e4169e07176f674d94bbcc99f2392e927d

[lgrey, 2017-04-03 21:59:40]

The CQ bit was checked by lgrey@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 22:00:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 22:30:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 22:30:21]

Dry run: This issue passed the CQ dry run.

[lgrey, 2017-04-04 15:00:45]

lgrey@chromium.org changed reviewers:
+ ellyjones@chromium.org

[lgrey, 2017-04-04 15:00:46]

PTAL :)

[Elly Fong-Jones, 2017-04-04 15:04:36]

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:39: const CGFloat
kKernAmount = 0.2;
it seems weird that this has to be specified manually - is there a systemwide
default we should use?

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm:242:
EXPECT_CGFLOAT_EQ([bookmark_cell cellSize].width, width_for_node_a);
what is this actually testing?

[lgrey, 2017-04-04 15:12:27]

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:39: const CGFloat
kKernAmount = 0.2;
On 2017/04/04 15:04:36, Elly Fong-Jones wrote:
> it seems weird that this has to be specified manually - is there a systemwide
> default we should use?

I wondered what was going on here too. My understanding is:
|titleTextAttributes| (which is an instance method that previously had this as a
magic number) is used
to create an attributed string, which is what ends up getting drawn. To put it
another way, this value
determines the kerning, vs. just being used for size lookup.

Re: a systemwide default, my assumption is that bookmark buttons having a
specific value is
intentional. Is it worth checking in with UX?

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm:242:
EXPECT_CGFLOAT_EQ([bookmark_cell cellSize].width, width_for_node_a);
On 2017/04/04 15:04:36, Elly Fong-Jones wrote:
> what is this actually testing?

|width_for_node_a| comes from the class method, |cellSize| is from the
instantiated cell. Basically: is the width we said it would be the width it
ended up being?

[Elly Fong-Jones, 2017-04-05 11:26:37]

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:39: const CGFloat
kKernAmount = 0.2;
On 2017/04/04 15:12:27, lgrey wrote:
> On 2017/04/04 15:04:36, Elly Fong-Jones wrote:
> > it seems weird that this has to be specified manually - is there a
systemwide
> > default we should use?
> 
> I wondered what was going on here too. My understanding is:
> |titleTextAttributes| (which is an instance method that previously had this as
a
> magic number) is used
> to create an attributed string, which is what ends up getting drawn. To put it
> another way, this value
> determines the kerning, vs. just being used for size lookup.
> 
> Re: a systemwide default, my assumption is that bookmark buttons having a
> specific value is
> intentional. Is it worth checking in with UX?

Well, what's the systemwide default, and how do they look with the systemwide
default vs the current value? We can check with UX, but they will ask us for
side-by-side screenshots.

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm:242:
EXPECT_CGFLOAT_EQ([bookmark_cell cellSize].width, width_for_node_a);
On 2017/04/04 15:12:27, lgrey wrote:
> On 2017/04/04 15:04:36, Elly Fong-Jones wrote:
> > what is this actually testing?
> 
> |width_for_node_a| comes from the class method, |cellSize| is from the
> instantiated cell. Basically: is the width we said it would be the width it
> ended up being?

Got it - can you add a comment to that effect?

[lgrey, 2017-04-05 15:21:56]

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:39: const CGFloat
kKernAmount = 0.2;
On 2017/04/05 11:26:37, Elly Fong-Jones wrote:
> On 2017/04/04 15:12:27, lgrey wrote:
> > On 2017/04/04 15:04:36, Elly Fong-Jones wrote:
> > > it seems weird that this has to be specified manually - is there a
> systemwide
> > > default we should use?
> > 
> > I wondered what was going on here too. My understanding is:
> > |titleTextAttributes| (which is an instance method that previously had this
as
> a
> > magic number) is used
> > to create an attributed string, which is what ends up getting drawn. To put
it
> > another way, this value
> > determines the kerning, vs. just being used for size lookup.
> > 
> > Re: a systemwide default, my assumption is that bookmark buttons having a
> > specific value is
> > intentional. Is it worth checking in with UX?
> 
> Well, what's the systemwide default, and how do they look with the systemwide
> default vs the current value? We can check with UX, but they will ask us for
> side-by-side screenshots.

The docs say the default it "kerning is disabled", by which I'm guessing they
mean "we use the font's default kern tables without further adjustment"

Kern = 0.2 (current behavior) https://screenshot.googleplex.com/WXXF3iC6SoT
Kerning unspecified https://screenshot.googleplex.com/4qBEEmG8mn0

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm (right):

https://codereview.chromium.org/2794123002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell_unittest.mm:242:
EXPECT_CGFLOAT_EQ([bookmark_cell cellSize].width, width_for_node_a);
On 2017/04/05 11:26:37, Elly Fong-Jones wrote:
> On 2017/04/04 15:12:27, lgrey wrote:
> > On 2017/04/04 15:04:36, Elly Fong-Jones wrote:
> > > what is this actually testing?
> > 
> > |width_for_node_a| comes from the class method, |cellSize| is from the
> > instantiated cell. Basically: is the width we said it would be the width it
> > ended up being?
> 
> Got it - can you add a comment to that effect?

Done.

[Elly Fong-Jones, 2017-04-06 14:37:14]

lgtm

[lgrey, 2017-04-06 17:46:36]

The CQ bit was checked by lgrey@chromium.org

[commit-bot: I haz the power, 2017-04-06 17:47:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 19:18:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491500796387470,
"parent_rev": "882f01ad7ceb6c3c82a8bfa8dcf4982abb7d63d4", "commit_rev":
"e4e3e7e4169e07176f674d94bbcc99f2392e927d"}

[commit-bot: I haz the power, 2017-04-06 19:19:24]

Description was changed from

==========
[Mac] Bookmark button cell updates

This is extracted from the bookmark bar refactor.

Changes:
- Changing a cell's node now updates the title
- A new class method for calculating a cell's size without
instantiating one.

BUG=648560
==========

to

==========
[Mac] Bookmark button cell updates

This is extracted from the bookmark bar refactor.

Changes:
- Changing a cell's node now updates the title
- A new class method for calculating a cell's size without
instantiating one.

BUG=648560

Review-Url: https://codereview.chromium.org/2794123002
Cr-Commit-Position: refs/heads/master@{#462577}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4e3e7e4169e07176f674d94bbcc...
==========

[commit-bot: I haz the power, 2017-04-06 19:19:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e4e3e7e4169e07176f674d94bbcc...