Misc. cleanup

Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the IconLabelBubbleView list and reorder the code to match.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has "bubble" in the name.
* Factor code into some helpers in preparation for adding more uses/complexity in the future.
* Functional change: Make the leading padding match the trailing padding for MD in all cases, not just when there's no image. This looks better to me, is simpler, and will be necessary for an upcoming change.
* Add missing const to unittest code.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance between objects instead of at the edge of an object.

BUG=586423
TEST=none

Committed: https://crrev.com/031b547b8763c3026b53dea120ccf3d4fa176492
Cr-Commit-Position: refs/heads/master@{#383917}

[Peter Kasting, 2016-03-25 09:26:11]

Description was changed from

==========
Cleanup

BUG=586423
TEST=none
==========

to

==========
Misc. cleanup:

For ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list.
* Factor some code into a new helper, 
MinimumWidthForImageWithBackgroundShown(), in preparation for using this in a
second place in a future CL.
* Add comments.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

For IconLabelBubbleView:

* Clarify comments.
* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor some code into a new helper, GetInternalSpacing(), in preparation for
making it more complicated in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.

For layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

[Peter Kasting, 2016-03-25 09:26:11]

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

[Peter Kasting, 2016-03-25 09:27:39]

Description was changed from

==========
Misc. cleanup:

For ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list.
* Factor some code into a new helper, 
MinimumWidthForImageWithBackgroundShown(), in preparation for using this in a
second place in a future CL.
* Add comments.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

For IconLabelBubbleView:

* Clarify comments.
* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor some code into a new helper, GetInternalSpacing(), in preparation for
making it more complicated in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.

For layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

to

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list.
* Factor some code into a new helper, 
MinimumWidthForImageWithBackgroundShown(), in preparation for using this in a
second place in a future CL.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor some code into a new helper, GetInternalSpacing(), in preparation for
making it more complicated in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

[Peter Kasting, 2016-03-26 05:08:33]

Description was changed from

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list.
* Factor some code into a new helper, 
MinimumWidthForImageWithBackgroundShown(), in preparation for using this in a
second place in a future CL.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor some code into a new helper, GetInternalSpacing(), in preparation for
making it more complicated in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

to

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list and reorder the code to match.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor code into some helpers in preparation for adding more uses/complexity
in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.
* Add missing const to unittest code.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

[varkha, 2016-03-30 03:03:42]

Not sure if I am missing something or you left the spacing on purpose.

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right):

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:86: // Called
when the user clicks the view; this freezes or cancels the animation
s/cancels/snaps to target?

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:230: label_width
+ MinimumWidthForImageWithBackgroundShown();
Don't you need to include GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_SPACING)
a.k.a. GetInternalSpacing() when both the label and icon are shown?

[Peter Kasting, 2016-03-30 03:56:54]

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right):

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:86: // Called
when the user clicks the view; this freezes or cancels the animation
On 2016/03/30 03:03:42, varkha wrote:
> s/cancels/snaps to target?

Done.

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1834933002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:230: label_width
+ MinimumWidthForImageWithBackgroundShown();
On 2016/03/30 03:03:42, varkha wrote:
> Don't you need to include GetLayoutConstant(ICON_LABEL_VIEW_INTERNAL_SPACING)
> a.k.a. GetInternalSpacing() when both the label and icon are shown?

Yes.  Not sure how I didn't notice this.

[varkha, 2016-03-30 05:09:22]

lgtm.

[Peter Kasting, 2016-03-30 05:16:05]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2016-03-30 05:16:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1834933002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1834933002/80001

[commit-bot: I haz the power, 2016-03-30 06:06:15]

Description was changed from

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list and reorder the code to match.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor code into some helpers in preparation for adding more uses/complexity
in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.
* Add missing const to unittest code.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

to

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list and reorder the code to match.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor code into some helpers in preparation for adding more uses/complexity
in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.
* Add missing const to unittest code.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

[commit-bot: I haz the power, 2016-03-30 06:06:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-30 06:07:47]

Description was changed from

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list and reorder the code to match.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor code into some helpers in preparation for adding more uses/complexity
in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.
* Add missing const to unittest code.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none
==========

to

==========
Misc. cleanup:

All:

* Add/update/clarify comments.

ContentSettingImageView:

* This doesn't directly subclass views::View.  Move those overrides into the
IconLabelBubbleView list and reorder the code to match.
* Nuke IsBubbleShowing() and just inline its trivial code into its lone caller.

IconLabelBubbleView:

* Rename GetBubbleOutPadding() to GetOuterPadding(), the class already has
"bubble" in the name.
* Factor code into some helpers in preparation for adding more uses/complexity
in the future.
* Functional change: Make the leading padding match the trailing padding for MD
in all cases, not just when there's no image. This looks better to me, is
simpler, and will be necessary for an upcoming change.
* Add missing const to unittest code.

layout_constants.*:

* Rename "padding" to "spacing" in a place where we're referring to the distance
between objects instead of at the edge of an object.

BUG=586423
TEST=none

Committed: https://crrev.com/031b547b8763c3026b53dea120ccf3d4fa176492
Cr-Commit-Position: refs/heads/master@{#383917}
==========

[commit-bot: I haz the power, 2016-03-30 06:07:48]

Patchset 5 (id:??) landed as
https://crrev.com/031b547b8763c3026b53dea120ccf3d4fa176492
Cr-Commit-Position: refs/heads/master@{#383917}