Set maximum size for HoverHighlightView right icon.

Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251
Committed: https://crrev.com/297d221658fa33897e7e0af309df1575057d0c09
Cr-Commit-Position: refs/heads/master@{#407284}

[jdufault, 2016-07-20 23:36:20]

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

[commit-bot: I haz the power, 2016-07-20 23:36:42]

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, 2016-07-21 00:20:45]

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

[commit-bot: I haz the power, 2016-07-21 00:20:46]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-07-21 19:47:37]

Description was changed from

==========
Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251
==========

to

==========
Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251
==========

[jdufault, 2016-07-21 19:47:37]

jdufault@chromium.org changed reviewers:
+ stevenjb@chromium.org

[jdufault, 2016-07-21 19:47:48]

Steven, PTAL. Thanks!

[stevenjb, 2016-07-21 20:32:05]

https://codereview.chromium.org/2165133002/diff/1/ash/common/system/tray/hove...
File ash/common/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2165133002/diff/1/ash/common/system/tray/hove...
ash/common/system/tray/hover_highlight_view.cc:57:
gfx::Size(kTrayPopupDetailsIconWidth, kTrayPopupDetailsIconWidth));
Is this not a problem for other FixedSizedImageView instances? (i.e., should we
fix it there instead?)

[jdufault, 2016-07-22 21:44:59]

https://codereview.chromium.org/2165133002/diff/1/ash/common/system/tray/hove...
File ash/common/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2165133002/diff/1/ash/common/system/tray/hove...
ash/common/system/tray/hover_highlight_view.cc:57:
gfx::Size(kTrayPopupDetailsIconWidth, kTrayPopupDetailsIconWidth));
On 2016/07/21 20:32:05, stevenjb wrote:
> Is this not a problem for other FixedSizedImageView instances? (i.e., should
we
> fix it there instead?)

From my current understanding, FixedSizedImageView is fixed-size w.r.t. the size
of the layout space / view. Confusingly, this is unrelated to the size of the
rendered image (though the rendered image will get clipped to the maximum view
size).

A lot of the other users of FixedSizedImageView want the layout size to be
larger than the rendered image size, so blindly calling SetImageSize() in the
FixedSizedImageView constructor distorts lots of icons.

I think the right solution is to have FixedSizedImageView set the rendered image
size as well, and convert the existing users to use insets instead of having a
different layout size/image size. I believe this would also eliminate the CENTER
alignment calls.

If we did that, I think it'd be reasonable to eliminate FixedSizeImageView
entirely.

All of this assumes that we actually want the image size and not the layout size
to be fixed. If we want the image size to remain dynamic, then I think this CL
is probably okay as is.

[stevenjb, 2016-07-22 21:50:40]

lgtm

https://codereview.chromium.org/2165133002/diff/1/ash/common/system/tray/hove...
File ash/common/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2165133002/diff/1/ash/common/system/tray/hove...
ash/common/system/tray/hover_highlight_view.cc:57:
gfx::Size(kTrayPopupDetailsIconWidth, kTrayPopupDetailsIconWidth));
On 2016/07/22 21:44:58, jdufault wrote:
> On 2016/07/21 20:32:05, stevenjb wrote:
> > Is this not a problem for other FixedSizedImageView instances? (i.e., should
> we
> > fix it there instead?)
> 
> From my current understanding, FixedSizedImageView is fixed-size w.r.t. the
size
> of the layout space / view. Confusingly, this is unrelated to the size of the
> rendered image (though the rendered image will get clipped to the maximum view
> size).
> 
> A lot of the other users of FixedSizedImageView want the layout size to be
> larger than the rendered image size, so blindly calling SetImageSize() in the
> FixedSizedImageView constructor distorts lots of icons.
> 
> I think the right solution is to have FixedSizedImageView set the rendered
image
> size as well, and convert the existing users to use insets instead of having a
> different layout size/image size. I believe this would also eliminate the
CENTER
> alignment calls.
> 
> If we did that, I think it'd be reasonable to eliminate FixedSizeImageView
> entirely.
> 
> All of this assumes that we actually want the image size and not the layout
size
> to be fixed. If we want the image size to remain dynamic, then I think this CL
> is probably okay as is.

Fair enough, I just wanted to make sure we had thougt it through, sounds like it
has been :)

[jdufault, 2016-07-22 21:54:40]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2016-07-22 21:55:01]

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, 2016-07-22 22:40:00]

Description was changed from

==========
Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251
==========

to

==========
Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251
==========

[commit-bot: I haz the power, 2016-07-22 22:40:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-22 22:41:23]

Description was changed from

==========
Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251
==========

to

==========
Set maximum size for HoverHighlightView right icon.

Previously, the image would grow larger than kTrayPopupDetailsIconWidth.

BUG=625251

Committed: https://crrev.com/297d221658fa33897e7e0af309df1575057d0c09
Cr-Commit-Position: refs/heads/master@{#407284}
==========

[commit-bot: I haz the power, 2016-07-22 22:41:24]

Patchset 1 (id:??) landed as
https://crrev.com/297d221658fa33897e7e0af309df1575057d0c09
Cr-Commit-Position: refs/heads/master@{#407284}