Vertically align text on empty bookmarks bar

Just allow the labels on the instructions view to vertically align themselves

BUG=566124
Committed: https://crrev.com/488e6af77baaffa471598c05574326a18d7098c4
Cr-Commit-Position: refs/heads/master@{#400184}

[kylix_rd, 2016-06-13 18:18:42]

Description was changed from

==========
Just allow the labels placed on the instructions view to vertically align
themselves

R=sky@chromium.org
BUG=
==========

to

==========
Just allow the labels on the instructions view to vertically align themselves

BUG=566124
==========

[kylix_rd, 2016-06-13 18:30:03]

The issue here is that the BookmarkBarInstructionsView tries to align the text
on the baseline for each of the label controls it embeds. The "Apps" LabelButton
just allows the text to be rendered vertically centered by setting the height of
the Label to match the LabelButton. By changing the BookmarkBarInstructionsView
to do the same thing, the text will align.

GetBaseline() isn't consistently implemented and in some cases such as the Label
control, it returns incorrect information. The actual baseline offset for a
Label isn't calculated until the text is actually rendered by
RenderTextHarfBuzz. Because of this, using GetBaseline() to align controls only
works when all the Label controls share a common parent.

[kylix_rd, 2016-06-13 18:44:34]

kylixrd@chromium.org changed reviewers:
+ pkasting@google.com

[kylix_rd, 2016-06-13 18:45:45]

Added pkasting to reviewers. sky@ is out until the 16th.

[Peter Kasting, 2016-06-13 20:48:56]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-06-13 20:49:01]

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:68: int
baseline = view->GetBaseline();
Does this code need changing too?  Seems like it should stay in sync with
Layout().

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:90:
view->SetBounds(x, 0, view_width, height());
How does this look in the attached bookmark bar?

[kylix_rd, 2016-06-13 20:59:43]

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:68: int
baseline = view->GetBaseline();
On 2016/06/13 20:49:00, Peter Kasting wrote:
> Does this code need changing too?  Seems like it should stay in sync with
> Layout().

This code is merely for determining the preferred size of the control, which
will be the smallest bounding box around the text. In this case, the baseline
will be the proper offset from a 0 origin.

In the control's current incarnation, there are, at most, two child controls,
both of which will use the same font size. Even though this code is trying to
align all the controls on their baseline, in practice, this code seems to not
actually do anything.

I was reticent to remove it without getting more feedback.

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:90:
view->SetBounds(x, 0, view_width, height());
On 2016/06/13 20:49:00, Peter Kasting wrote:
> How does this look in the attached bookmark bar?

I'll admit that I'm not sure. How is attached vs. detached bookmarks bar
enabled/disabled?

I would imagine it shouldn't look any different because all this code does is
mimic what the LabelButton does with its embedded Label and LinkLabel controls.
The LabelButton::Layout() code sets the label_ bounds to be the full height of
the control itself.

[Peter Kasting, 2016-06-13 21:06:06]

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:68: int
baseline = view->GetBaseline();
On 2016/06/13 20:59:43, kylix_rd wrote:
> On 2016/06/13 20:49:00, Peter Kasting wrote:
> > Does this code need changing too?  Seems like it should stay in sync with
> > Layout().
> 
> This code is merely for determining the preferred size of the control, which
> will be the smallest bounding box around the text. In this case, the baseline
> will be the proper offset from a 0 origin.
> 
> In the control's current incarnation, there are, at most, two child controls,
> both of which will use the same font size. Even though this code is trying to
> align all the controls on their baseline, in practice, this code seems to not
> actually do anything.
> 
> I was reticent to remove it without getting more feedback.

I'd simplify it as much as possible.  Seems like you can skip the ascent +
descent stuff and just ask the font for its height directly since there's no
alignment to take into account.

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:90:
view->SetBounds(x, 0, view_width, height());
On 2016/06/13 20:59:43, kylix_rd wrote:
> On 2016/06/13 20:49:00, Peter Kasting wrote:
> > How does this look in the attached bookmark bar?
> 
> I'll admit that I'm not sure. How is attached vs. detached bookmarks bar
> enabled/disabled?

Toggle with ctrl-shift-b.

[kylix_rd, 2016-06-13 21:17:20]

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:68: int
baseline = view->GetBaseline();
On 2016/06/13 21:06:06, Peter Kasting wrote:
> On 2016/06/13 20:59:43, kylix_rd wrote:
> > On 2016/06/13 20:49:00, Peter Kasting wrote:
> > > Does this code need changing too?  Seems like it should stay in sync with
> > > Layout().
> > 
> > This code is merely for determining the preferred size of the control, which
> > will be the smallest bounding box around the text. In this case, the
baseline
> > will be the proper offset from a 0 origin.
> > 
> > In the control's current incarnation, there are, at most, two child
controls,
> > both of which will use the same font size. Even though this code is trying
to
> > align all the controls on their baseline, in practice, this code seems to
not
> > actually do anything.
> > 
> > I was reticent to remove it without getting more feedback.
> 
> I'd simplify it as much as possible.  Seems like you can skip the ascent +
> descent stuff and just ask the font for its height directly since there's no
> alignment to take into account.

Done.

https://codereview.chromium.org/2065653002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:90:
view->SetBounds(x, 0, view_width, height());
On 2016/06/13 21:06:06, Peter Kasting wrote:
> On 2016/06/13 20:59:43, kylix_rd wrote:
> > On 2016/06/13 20:49:00, Peter Kasting wrote:
> > > How does this look in the attached bookmark bar?
> > 
> > I'll admit that I'm not sure. How is attached vs. detached bookmarks bar
> > enabled/disabled?
> 
> Toggle with ctrl-shift-b.

Ok, the alignment remains correct.

[Peter Kasting, 2016-06-13 21:53:58]

LGTM

[kylix_rd, 2016-06-13 21:55:51]

On 2016/06/13 21:53:58, Peter Kasting wrote:
> LGTM

Should I wait for sky@ or TBR= it?

[sky, 2016-06-16 15:43:02]

LGTM - Peter is an owner of this directory too, so you didn't need to wait for
me or TBR.

[kylix_rd, 2016-06-16 15:55:11]

The CQ bit was checked by kylixrd@chromium.org

[commit-bot: I haz the power, 2016-06-16 15:55:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2065653002/20001

[commit-bot: I haz the power, 2016-06-16 17:18:59]

Description was changed from

==========
Just allow the labels on the instructions view to vertically align themselves

BUG=566124
==========

to

==========
Just allow the labels on the instructions view to vertically align themselves

BUG=566124
==========

[commit-bot: I haz the power, 2016-06-16 17:19:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-16 17:19:07]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 17:21:19]

Description was changed from

==========
Just allow the labels on the instructions view to vertically align themselves

BUG=566124
==========

to

==========
Just allow the labels on the instructions view to vertically align themselves

BUG=566124

Committed: https://crrev.com/488e6af77baaffa471598c05574326a18d7098c4
Cr-Commit-Position: refs/heads/master@{#400184}
==========

[commit-bot: I haz the power, 2016-06-16 17:21:21]

Patchset 2 (id:??) landed as
https://crrev.com/488e6af77baaffa471598c05574326a18d7098c4
Cr-Commit-Position: refs/heads/master@{#400184}