The close button for bubbles should be an ImageButton, not a LabelButton.

The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also eliminates usage of a non-const ref, and simplifies some code
by refactoring into temps.

BUG=686962
Review-Url: https://codereview.chromium.org/2667903002
Cr-Commit-Position: refs/heads/master@{#447337}
Committed: https://chromium.googlesource.com/chromium/src/+/5c8c2e7b510f494dbde1a63f4fda119701a727db

[Peter Kasting, 2017-01-31 03:36:03]

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

[Peter Kasting, 2017-01-31 03:36:48]

Description was changed from

==========
The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also simplifies some code by refactoring into temps.

BUG=686962
==========

to

==========
The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also eliminates usage of a non-const ref, and simplifies some code
by refactoring into temps.

BUG=686962
==========

[Peter Kasting, 2017-01-31 03:37:03]

Description was changed from

==========
The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also eliminates usage of a non-const ref, and simplifies some code
by refactoring into temps.

BUG=686962
==========

to

==========
The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also eliminates usage of a non-const ref, and simplifies some code
by refactoring into temps.

BUG=686962
==========

[Peter Kasting, 2017-01-31 03:37:08]

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

[commit-bot: I haz the power, 2017-01-31 03:37:23]

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-01-31 05:33:10]

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

[commit-bot: I haz the power, 2017-01-31 05:33:11]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-31 17:53:16]

lgtm. Back in the day, I wanted to remove the ImageButon type, it's just a
LabelButton without a label... now that doesn't sound as brilliant.

https://codereview.chromium.org/2667903002/diff/1/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2667903002/diff/1/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view.cc:325: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
q: did you remember to test MD and non-MD?

[Peter Kasting, 2017-01-31 21:31:01]

https://codereview.chromium.org/2667903002/diff/1/ui/views/bubble/bubble_fram...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/2667903002/diff/1/ui/views/bubble/bubble_fram...
ui/views/bubble/bubble_frame_view.cc:325: if
(ui::MaterialDesignController::IsSecondaryUiMaterial()) {
On 2017/01/31 17:53:16, msw wrote:
> q: did you remember to test MD and non-MD?

The Harmony close button is a VectorIconButton instead, so it's not actually
affected by this change.

[Peter Kasting, 2017-01-31 21:31:05]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:31:50]

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-01-31 21:39:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485898265162020, "parent_rev":
"0da7a38852b601f5964d44a374f86ea0b30b6ef0", "commit_rev":
"5c8c2e7b510f494dbde1a63f4fda119701a727db"}

[commit-bot: I haz the power, 2017-01-31 21:40:15]

Description was changed from

==========
The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also eliminates usage of a non-const ref, and simplifies some code
by refactoring into temps.

BUG=686962
==========

to

==========
The close button for bubbles should be an ImageButton, not a LabelButton.

This button never has a label; it's purely an image.  Making it a LabelButton
means its minimum height is the height of the label font, even if no label is
visible.  Accordingly, the button was not actually sized to its image (14x14),
but to 14x16 (at least on my system -- maybe font metrics vary).

I can't see any other effect of this.  This button is not focusable, so there's
no taborder focus rect that would change.  I would be surprised if anyone
notices the difference.  I only noticed while trying to figure out how the
positioning algorithms here work in preparation for simplifying them for
Harmony.

This change also eliminates usage of a non-const ref, and simplifies some code
by refactoring into temps.

BUG=686962

Review-Url: https://codereview.chromium.org/2667903002
Cr-Commit-Position: refs/heads/master@{#447337}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c8c2e7b510f494dbde1a63f4fda...
==========

[commit-bot: I haz the power, 2017-01-31 21:40:16]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/5c8c2e7b510f494dbde1a63f4fda...