gfx: Make conversions from Size to SizeF be explicit.

gfx: Make conversions from Size to SizeF be explicit.

This removes the implicit operator from Size to SizeF and adds an
explicit constructor instead.

There are many places in the code that want to do:

an_integer_size = ToFlooredSize(ScaleSize(an_integer_size, x));

This would make these all look like

an_integer_size = ToFlooredSize(ScaleSize(gfx::RectF(an_integer_size), x));

But that's pretty verbose. So add helpers similar to those we have
already for gfx::Rect, for when you want to scale a Size to a Size,
such as

an_integer_size = ScaleToFlooredSize(an_integer_size, x);

R=sky@chromium.org, vmpstr
BUG=342848
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ddaec91ec046e8a389d16049c0a2656a4e41a2ad
Cr-Commit-Position: refs/heads/master@{#350889}

[danakj, 2015-09-23 22:47:59]

danakj@chromium.org changed reviewers:
+ sievers@chromium.org

[danakj, 2015-09-23 22:48:00]

sky: please review ui/ ash/ and chrome/  (mostly mechanical outside of ui/gfx/)
vmpstr: please review cc/
sievers: please review content/

Thanks!

[vmpstr, 2015-09-23 23:04:25]

cc lgtm, with just a couple of nits

https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:70: gfx::SizeF viewport_size = gfx::SizeF(
Is this required?

what about 

gfx::SizeF viewport_size(...);

I would imagine that's an explicit instantiation

https://codereview.chromium.org/1357423009/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1357423009/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl_unittest.cc:3649:
gfx::SizeF(gfx::ToCeiledSize(gfx::SizeF(200, 200 / aspect_ratio)));
This is a bit weird. It almost like we want ToCeiledSizeF :P I think this is
fine though, since this is test.

https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size.h
File ui/gfx/geometry/size.h (right):

https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size.h#newc...
ui/gfx/geometry/size.h:84: // Helper methods to scale a gfx::Size to a new
gfx::Size.
These are great!

https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h
File ui/gfx/geometry/size_f.h (right):

https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h#ne...
ui/gfx/geometry/size_f.h:27: // TODO(danakj): Change these to checked_cast?
Why?

[danakj, 2015-09-23 23:08:20]

https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc
File cc/layers/viewport.cc (right):

https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc#newco...
cc/layers/viewport.cc:70: gfx::SizeF viewport_size = gfx::SizeF(
On 2015/09/23 23:04:25, vmpstr wrote:
> Is this required?
> 
> what about 
> 
> gfx::SizeF viewport_size(...);
> 
> I would imagine that's an explicit instantiation

Ya, but it's less obvious reading the code so I did this. Is ok?

https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h
File ui/gfx/geometry/size_f.h (right):

https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h#ne...
ui/gfx/geometry/size_f.h:27: // TODO(danakj): Change these to checked_cast?
On 2015/09/23 23:04:25, vmpstr wrote:
> Why?

Well, I think that putting an int that is too big into a float and losing some
of it is probably bad.

[vmpstr, 2015-09-23 23:20:13]

On 2015/09/23 23:08:20, danakj wrote:
> https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc
> File cc/layers/viewport.cc (right):
> 
>
https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc#newco...
> cc/layers/viewport.cc:70: gfx::SizeF viewport_size = gfx::SizeF(
> On 2015/09/23 23:04:25, vmpstr wrote:
> > Is this required?
> > 
> > what about 
> > 
> > gfx::SizeF viewport_size(...);
> > 
> > I would imagine that's an explicit instantiation
> 
> Ya, but it's less obvious reading the code so I did this. Is ok?

I'm not sure. This is valid even if the expression is already a SizeF..

Foo a = Foo(b); is not giving me any new information over
Foo a(b); 

I understand that you're trying to say that b isn't a Foo in this case, but
needs to be converted to a Foo.
Now if it was "auto a = Foo(b);"... <_<

Anyway, your call.


> 
> https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h
> File ui/gfx/geometry/size_f.h (right):
> 
>
https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h#ne...
> ui/gfx/geometry/size_f.h:27: // TODO(danakj): Change these to checked_cast?
> On 2015/09/23 23:04:25, vmpstr wrote:
> > Why?
> 
> Well, I think that putting an int that is too big into a float and losing some
> of it is probably bad.

I thought an int should fit into a float, but might lose precision. Does
checked_cast check for that?

[danakj, 2015-09-23 23:38:06]

On Wed, Sep 23, 2015 at 4:20 PM, <vmpstr@chromium.org> wrote:

> On 2015/09/23 23:08:20, danakj wrote:
>
>> https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc
>> File cc/layers/viewport.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1357423009/diff/1/cc/layers/viewport.cc#newco...
>
>> cc/layers/viewport.cc:70: gfx::SizeF viewport_size = gfx::SizeF(
>> On 2015/09/23 23:04:25, vmpstr wrote:
>> > Is this required?
>> >
>> > what about
>> >
>> > gfx::SizeF viewport_size(...);
>> >
>> > I would imagine that's an explicit instantiation
>>
>
> Ya, but it's less obvious reading the code so I did this. Is ok?
>>
>
> I'm not sure. This is valid even if the expression is already a SizeF..
>
> Foo a = Foo(b); is not giving me any new information over
> Foo a(b);
>
> I understand that you're trying to say that b isn't a Foo in this case, but
> needs to be converted to a Foo.
> Now if it was "auto a = Foo(b);"... <_<
>
> Anyway, your call.
>
>
>
> https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h
>> File ui/gfx/geometry/size_f.h (right):
>>
>
>
>
>
https://codereview.chromium.org/1357423009/diff/1/ui/gfx/geometry/size_f.h#ne...
>
>> ui/gfx/geometry/size_f.h:27: // TODO(danakj): Change these to
>> checked_cast?
>> On 2015/09/23 23:04:25, vmpstr wrote:
>> > Why?
>>
>
> Well, I think that putting an int that is too big into a float and losing
>> some
>> of it is probably bad.
>>
>
> I thought an int should fit into a float, but might lose precision. Does
> checked_cast check for that?
>

I thought so.. it's hard to read but..

  static RangeConstraint Check(Src value) {
    return GetRangeConstraint((value <= NarrowingRange<Dst, Src>::max()),
                              (value >= NarrowingRange<Dst, Src>::min()));
  }

It wants Src value to be <= max.

  static Dst max() {
    // The following logic avoids warnings where the max function is


    // instantiated with invalid values for a bit shift (even though


    // such a function can never be called).


    static const int shift =
        (MaxExponent<Src>::value > MaxExponent<Dst>::value &&
         SrcLimits::digits < DstLimits::digits && SrcLimits::is_iec559 &&
         DstLimits::is_integer)
            ? (DstLimits::digits - SrcLimits::digits)
            : 0;

    // We use UINTMAX_C below to avoid compiler warnings about shifting
floating

    // points. Since it's a compile time calculation, it shouldn't have any


    // performance impact.


    return DstLimits::max() - static_cast<Dst>((UINTMAX_C(1) << shift) - 1);
  }

max is std::numeric_limits<float>::max, but subtract (1 << shift) - 1.

Since src is an int, is_iec559 is false. It looks like it does what I want,
but only for float->int. For int->float the shift will be 0.

So I guess not.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-09-23 23:41:26]

On Wed, Sep 23, 2015 at 4:32 PM, Dana Jansens <danakj@chromium.org> wrote:

> I thought so.. it's hard to read but..
>
>   static RangeConstraint Check(Src value) {
>     return GetRangeConstraint((value <= NarrowingRange<Dst, Src>::max()),
>                               (value >= NarrowingRange<Dst, Src>::min()));
>   }
>
> It wants Src value to be <= max.
>
>   static Dst max() {
>     // The following logic avoids warnings where the max function is
>
>
>     // instantiated with invalid values for a bit shift (even though
>
>
>     // such a function can never be called).
>
>
>     static const int shift =
>         (MaxExponent<Src>::value > MaxExponent<Dst>::value &&
>          SrcLimits::digits < DstLimits::digits && SrcLimits::is_iec559 &&
>          DstLimits::is_integer)
>             ? (DstLimits::digits - SrcLimits::digits)
>             : 0;
>
>     // We use UINTMAX_C below to avoid compiler warnings about shifting
> floating
>
>     // points. Since it's a compile time calculation, it shouldn't have
> any
>
>     // performance impact.
>
>
>     return DstLimits::max() - static_cast<Dst>((UINTMAX_C(1) << shift) -
> 1);
>   }
>
> max is std::numeric_limits<float>::max, but subtract (1 << shift) - 1.
>
> Since src is an int, is_iec559 is false. It looks like it does what I
> want, but only for float->int. For int->float the shift will be 0.
>
> So I guess not.
>

Removed the TODO (and the one on RectF).

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-09-23 23:53:12]

LGTM

[no sievers, 2015-09-24 00:07:33]

https://codereview.chromium.org/1357423009/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/1357423009/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/page_handler.cc:401:
gfx::ScaleSize(gfx::SizeF(view->GetPhysicalBackingSize()),
maybe this should have a version of ScaleSize() for gfx::Size?

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
media/blink/skcanvas_video_renderer.cc:355: dest_rect.size() !=
gfx::SizeF(video_frame->visible_rect().size()) ||
hmm, do we really want to compare the rounded integer sizes here instead?

[danakj, 2015-09-24 00:36:04]

danakj@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[danakj, 2015-09-24 00:36:05]

+dalecurtis for media/

https://codereview.chromium.org/1357423009/diff/1/content/browser/devtools/pr...
File content/browser/devtools/protocol/page_handler.cc (right):

https://codereview.chromium.org/1357423009/diff/1/content/browser/devtools/pr...
content/browser/devtools/protocol/page_handler.cc:401:
gfx::ScaleSize(gfx::SizeF(view->GetPhysicalBackingSize()),
On 2015/09/24 00:07:33, sievers wrote:
> maybe this should have a version of ScaleSize() for gfx::Size?

I didn't want to do that because ScaleSize returns a SizeF. I wanted things that
take Size to return Size and same for SizeFs. That's why I added the
ScaleToFooSize() helpers.

Here we're turning a Size into a SizeF, so requiring that to be explicit/visible
in the code is my goal. Adding a ScaleSize(Size) would hide that again.

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
media/blink/skcanvas_video_renderer.cc:355: dest_rect.size() !=
gfx::SizeF(video_frame->visible_rect().size()) ||
On 2015/09/24 00:07:33, sievers wrote:
> hmm, do we really want to compare the rounded integer sizes here instead?

Ya I'm not sure, but this is what the code does today. Pointing out potential
wierdness is why I wanted to land this. I was going to +dalecurtis for media
afterward, but can do that now. :)

[DaleCurtis, 2015-09-24 01:40:09]

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
media/blink/skcanvas_video_renderer.cc:355: dest_rect.size() !=
gfx::SizeF(video_frame->visible_rect().size()) ||
On 2015/09/24 00:36:05, danakj wrote:
> On 2015/09/24 00:07:33, sievers wrote:
> > hmm, do we really want to compare the rounded integer sizes here instead?
> 
> Ya I'm not sure, but this is what the code does today. Pointing out potential
> wierdness is why I wanted to land this. I was going to +dalecurtis for media
> afterward, but can do that now. :)

It seems like .size() should be dropped from both sides? I'd guess that's what
the original intention was here. I.e. use the implicit == which will compare the
integer width an height.

[danakj, 2015-09-24 17:48:35]

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
File media/blink/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
media/blink/skcanvas_video_renderer.cc:355: dest_rect.size() !=
gfx::SizeF(video_frame->visible_rect().size()) ||
On 2015/09/24 01:40:09, DaleCurtis wrote:
> On 2015/09/24 00:36:05, danakj wrote:
> > On 2015/09/24 00:07:33, sievers wrote:
> > > hmm, do we really want to compare the rounded integer sizes here instead?
> > 
> > Ya I'm not sure, but this is what the code does today. Pointing out
potential
> > wierdness is why I wanted to land this. I was going to +dalecurtis for media
> > afterward, but can do that now. :)
> 
> It seems like .size() should be dropped from both sides? I'd guess that's what
> the original intention was here. I.e. use the implicit == which will compare
the
> integer width an height.

I'd like to not change behaviour in this patch, but we can do that in a
followup. That ok?

[no sievers, 2015-09-24 17:55:06]

On 2015/09/24 17:48:35, danakj wrote:
>
https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
> File media/blink/skcanvas_video_renderer.cc (right):
> 
>
https://codereview.chromium.org/1357423009/diff/1/media/blink/skcanvas_video_...
> media/blink/skcanvas_video_renderer.cc:355: dest_rect.size() !=
> gfx::SizeF(video_frame->visible_rect().size()) ||
> On 2015/09/24 01:40:09, DaleCurtis wrote:
> > On 2015/09/24 00:36:05, danakj wrote:
> > > On 2015/09/24 00:07:33, sievers wrote:
> > > > hmm, do we really want to compare the rounded integer sizes here
instead?
> > > 
> > > Ya I'm not sure, but this is what the code does today. Pointing out
> potential
> > > wierdness is why I wanted to land this. I was going to +dalecurtis for
media
> > > afterward, but can do that now. :)
> > 
> > It seems like .size() should be dropped from both sides? I'd guess that's
what
> > the original intention was here. I.e. use the implicit == which will compare
> the
> > integer width an height.
> 
> I'd like to not change behaviour in this patch, but we can do that in a
> followup. That ok?

sounds good. lgtm

[danakj, 2015-09-24 17:58:55]

danakj@chromium.org changed reviewers:
+ mek@chromium.org

[danakj, 2015-09-24 17:58:56]

+mek for extensions

[Marijn Kruisselbrink, 2015-09-24 18:02:00]

extensions lgtm

[danakj, 2015-09-24 18:10:02]

media followup: https://codereview.chromium.org/1369563002

[DaleCurtis, 2015-09-24 19:32:17]

media/ lgtm

[danakj, 2015-09-25 00:39:32]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-25 00:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
sky@chromium.org, sievers@chromium.org, mek@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1357423009/#ps110083
(title: "sizefconvert-gfx: .")

[commit-bot: I haz the power, 2015-09-25 00:41:19]

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

[danakj, 2015-09-25 00:42:30]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-25 00:42:31]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
mek@chromium.org, sky@chromium.org, dalecurtis@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1357423009/#ps210001
(title: "sizefconvert-gfx: macmoremac")

[commit-bot: I haz the power, 2015-09-25 00:43:10]

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

[commit-bot: I haz the power, 2015-09-25 01:17:20]

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

[commit-bot: I haz the power, 2015-09-25 01:17:21]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[danakj, 2015-09-25 17:38:27]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-25 17:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
mek@chromium.org, sky@chromium.org, dalecurtis@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1357423009/#ps230001
(title: "sizefconvert-gfx: .")

[commit-bot: I haz the power, 2015-09-25 17:39:40]

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

[commit-bot: I haz the power, 2015-09-25 19:38:59]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-09-25 19:40:05]

Patchset 13 (id:??) landed as
https://crrev.com/ddaec91ec046e8a389d16049c0a2656a4e41a2ad
Cr-Commit-Position: refs/heads/master@{#350889}