Make various gfx classes more amenable to use as compile-time constants.

Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=danakj@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/275539a60ec716bea022200fa650a409772a81bf

[Peter Kasting, 2016-06-10 00:19:19]

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

[Peter Kasting, 2016-06-10 00:19:19]

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/size_f.h#ne...
ui/gfx/geometry/size_f.h:23: : width_(width < 0 ? 0 : width), height_(height < 0
? 0 : height) {}
This behavior will only differ when one of the provided values is NaN, which
didn't seem worrisome.

[danakj, 2016-06-10 00:36:48]

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/box_f.h#new...
ui/gfx/geometry/box_f.h:72: constexpr float x() const { return origin_.x(); }
is this constexpr and const not redundant? Why is it not placed where "const" is
currently?

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/insets.h#ne...
ui/gfx/geometry/insets.h:67: void operator-=(const Insets& insets) {
Can you add a unit test for each new operator?

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/insets.h#ne...
ui/gfx/geometry/insets.h:105: lhs += rhs;
These methods could be constexpr (or at least not depend on mutating an Insets)
if you reverse the dependency.

ie. This method does:

return Insets(lhs.top + rhs.top, ...);

And operator+= can do:
*this = *this + rhs;

wdyt?

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/insets_f.h#...
ui/gfx/geometry/insets_f.h:89: lhs += rhs;
same question

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/quad_f.h#ne...
ui/gfx/geometry/quad_f.h:26: constexpr QuadF() {}
nit: = default?

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/rect.h#newc...
ui/gfx/geometry/rect.h:37: constexpr Rect() {}
nit: = default?

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/size_f.h#ne...
ui/gfx/geometry/size_f.h:23: : width_(width < 0 ? 0 : width), height_(height < 0
? 0 : height) {}
On 2016/06/10 00:19:19, Peter Kasting wrote:
> This behavior will only differ when one of the provided values is NaN, which
> didn't seem worrisome.

Hm. https://bugs.chromium.org/p/chromium/issues/detail?id=407444#c20

IIRC fmaxf > a<0?0:x > std::max(), but obviously doing it at compile time is
even better. I don't see fmaxf mentioned no that bug though either.

Would you mind watching some thread-cpu-times-per-frame perf tests when this
lands (or do a perf tryjob) to verify this is not a problem?

[vmpstr, 2016-06-10 00:46:15]

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

[vmpstr, 2016-06-10 00:46:15]

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/box_f.h#new...
ui/gfx/geometry/box_f.h:72: constexpr float x() const { return origin_.x(); }
On 2016/06/10 00:36:47, danakj wrote:
> is this constexpr and const not redundant? Why is it not placed where "const"
is
> currently?

I think in C++14, non-static constexpr member function does not have to be
implicitly const, so maybe keep it as future proofing?

[vmpstr, 2016-06-10 00:46:24]

Description was changed from

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

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

to

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

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

[vmpstr, 2016-06-10 00:46:24]

vmpstr@chromium.org changed reviewers:
- vmpstr@chromium.org

[Peter Kasting, 2016-06-10 05:42:56]

Description was changed from

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

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

to

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Peter Kasting, 2016-06-10 05:50:06]

Turns out making these constexpr, much like making DISALLOW_COPY_AND_ASSIGN use
=delete, starts exposing a bunch of unused variables around the codebase.  Joy. 
I wanted another yak shave.

Once I get all the compile fixes in I'll move them all to a separate patch, but
see replies and questions below for now.

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/box_f.h#new...
ui/gfx/geometry/box_f.h:72: constexpr float x() const { return origin_.x(); }
On 2016/06/10 00:36:47, danakj wrote:
> is this constexpr and const not redundant? Why is it not placed where "const"
is
> currently?

No, they're not redundant.  Originally, constexpr implied const as you expect
(and as I expected originally).  This was removed in N3598.  More detail is
available at http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3598.html
.

MSVC 2015 will actually warn you that you've written a constexpr-but-not-const
function and ask if that's really what you intend.

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/insets.h#ne...
ui/gfx/geometry/insets.h:67: void operator-=(const Insets& insets) {
On 2016/06/10 00:36:47, danakj wrote:
> Can you add a unit test for each new operator?

Good idea. Done.

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/insets.h#ne...
ui/gfx/geometry/insets.h:105: lhs += rhs;
On 2016/06/10 00:36:47, danakj wrote:
> These methods could be constexpr (or at least not depend on mutating an
Insets)
> if you reverse the dependency.
> 
> ie. This method does:
> 
> return Insets(lhs.top + rhs.top, ...);
> 
> And operator+= can do:
> *this = *this + rhs;
> 
> wdyt?

I don't have a strong opinion.  I took this format from
http://en.cppreference.com/w/cpp/language/operators (see the section on "Binary
arithmetic operators").  They mention something about optimizing chained
addition.  I don't know whether your proposal is equally optimal, or if we even
care.  Intuitively, it seems like if += is more common than +, the way this is
structured now might be better, but my instincts as to what the compiler can
optimize are very bad.

I guess what I'm saying is, glance at that stuff, figure out if you still think
your suggestion is better, and I'll go whichever route you prefer.

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/quad_f.h#ne...
ui/gfx/geometry/quad_f.h:26: constexpr QuadF() {}
On 2016/06/10 00:36:48, danakj wrote:
> nit: = default?

Fine with me, I have no preference.  Done in all three places.

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/size_f.h#ne...
ui/gfx/geometry/size_f.h:23: : width_(width < 0 ? 0 : width), height_(height < 0
? 0 : height) {}
On 2016/06/10 00:36:48, danakj wrote:
> On 2016/06/10 00:19:19, Peter Kasting wrote:
> > This behavior will only differ when one of the provided values is NaN, which
> > didn't seem worrisome.
> 
> Hm. https://bugs.chromium.org/p/chromium/issues/detail?id=407444#c20
> 
> IIRC fmaxf > a<0?0:x > std::max(), but obviously doing it at compile time is
> even better. I don't see fmaxf mentioned no that bug though either.
> 
> Would you mind watching some thread-cpu-times-per-frame perf tests when this
> lands (or do a perf tryjob) to verify this is not a problem?

Is there somewhere that covers how to do this/what to look at?  I've never
looked at the things you mention so I'm not sure where to be looking.

Also, it turns out some tests rely on the NaN behavior.  Specifically,
QuadF::BoundingBox() can be called with e.g. the left and right edges both at
infinity, in which case the computed rect width is NaN, and the test expects
zero width.  I thought about modifying the test to expect the empty rect (which
is what we get if NaNs get returned from BoundingBox()), but that seemed wrong. 
(Perhaps this suggests that the caller shouldn't return the empty rect when it
detects NaN?)  I thought about modifying BoundingBox() to force inf - inf to be
0 instead of NaN, but that seemed like a hack.  Finally I put an explicit
isnan() check here -- which actually works since that function is constexpr.

This makes me a bit unhappy, though.

https://codereview.chromium.org/2051343002/diff/20001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_perftest.cc (right):

https://codereview.chromium.org/2051343002/diff/20001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_perftest.cc:191: /* This block doesn't seem to do
anything, missing part of the loop body?
Dana, I think you wrote this, want to comment on what needs to be done here?

https://codereview.chromium.org/2051343002/diff/20001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_perftest.cc:203:
occlusion.GetUnoccludedContentRect(query_content_rect);
Oops, ignore this typo line

[Peter Kasting, 2016-06-10 07:03:18]

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/size_f.h#ne...
ui/gfx/geometry/size_f.h:23: : width_(width < 0 ? 0 : width), height_(height < 0
? 0 : height) {}
On 2016/06/10 05:50:06, Peter Kasting wrote:
> Finally I put an explicit
> isnan() check here -- which actually works since that function is constexpr.

...except on MSVC.

F___.

OK, going back to my other options.  I don't want to just bail on making this
constexpr, since that will also cause everything using a SizeF to not be
constexpr.  And BoxF already uses a test like this without the isnan() check. 
So I think maybe it really is right to not do that check here.

So I've mucked with MathUtil::(Map,Project)EnclosingClipRect() so that when they
see NaN coords, they force them individually to zero instead of bailing and
returning an empty rect.  This changed some of the expected test results.

I can see an argument for why this behavior is actually more sane.  I can also
see an argument for leaving these functions untouched, and instead making the
calling tests expect empty rects in more cases.  And I can see other arguments
too.

In short, I am extremely uncertain that what I'm doing is ideal, but the
existing code kinda seems wrong (or at least "not intentionally constructed")
too.

[Peter Kasting, 2016-06-10 21:40:21]

Hold off reviewing this; I'm splitting the variable removals out, and I had an
idea for how to address the NaN issue in a way that doesn't change behavior.

[Peter Kasting, 2016-06-14 18:51:59]

OK, this is ready for re-review.  The cleanup has been landed, and the changes
to the SizeF/BoxF constructors are in a separate CL, so this is just constexpr
and operators now.

[danakj, 2016-06-14 19:04:12]

LGTM

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

https://codereview.chromium.org/2051343002/diff/1/ui/gfx/geometry/insets.h#ne...
ui/gfx/geometry/insets.h:105: lhs += rhs;
On 2016/06/10 05:50:06, Peter Kasting wrote:
> On 2016/06/10 00:36:47, danakj wrote:
> > These methods could be constexpr (or at least not depend on mutating an
> Insets)
> > if you reverse the dependency.
> > 
> > ie. This method does:
> > 
> > return Insets(lhs.top + rhs.top, ...);
> > 
> > And operator+= can do:
> > *this = *this + rhs;
> > 
> > wdyt?
> 
> I don't have a strong opinion.  I took this format from
> http://en.cppreference.com/w/cpp/language/operators (see the section on
"Binary
> arithmetic operators").  They mention something about optimizing chained
> addition.  I don't know whether your proposal is equally optimal, or if we
even
> care.  Intuitively, it seems like if += is more common than +, the way this is
> structured now might be better, but my instincts as to what the compiler can
> optimize are very bad.
> 
> I guess what I'm saying is, glance at that stuff, figure out if you still
think
> your suggestion is better, and I'll go whichever route you prefer.

I see. Interesting. I guess if we don't need constexpr on these then this is
nice for chaining, which we probably don't do much. I don't have much of an
opinion I guess until we need these to be constexpr, so this is fine.

[Peter Kasting, 2016-06-14 22:57:53]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-06-14 22:57:54]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2051343002/#ps180001
(title: "Delete destructor")

[commit-bot: I haz the power, 2016-06-14 22:58:32]

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

[commit-bot: I haz the power, 2016-06-15 00:59:38]

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

[commit-bot: I haz the power, 2016-06-15 00:59:39]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Peter Kasting, 2016-06-15 01:30:17]

The CQ bit was checked by pkasting@chromium.org

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

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

[commit-bot: I haz the power, 2016-06-15 01:45:31]

Description was changed from

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=danakj@chromium.org

Committed: https://crrev.com/275539a60ec716bea022200fa650a409772a81bf
Cr-Commit-Position: refs/heads/master@{#399815}
==========

[commit-bot: I haz the power, 2016-06-15 01:45:32]

Patchset 10 (id:??) landed as
https://crrev.com/275539a60ec716bea022200fa650a409772a81bf
Cr-Commit-Position: refs/heads/master@{#399815}

[Peter Kasting, 2016-06-15 01:46:25]

Description was changed from

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=danakj@chromium.org

Committed: https://crrev.com/275539a60ec716bea022200fa650a409772a81bf
Cr-Commit-Position: refs/heads/master@{#399815}
==========

to

==========
Make various gfx classes more amenable to use as compile-time constants.

Specifically, this means marking most constructors and simple accessors as
constexpr.  It also means removing the explicit empty destructors, since that
can qualify as a nontrivial destructor and bung things up.  (If we really want
to explicitly declare these, the right way would be to use "= default".)

This also adds some missing operators to the Insets classes.  I needed one of
these, and the style guide says to overload all related operators when
overloading one.

BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=danakj@chromium.org

Committed:
https://chromium.googlesource.com/chromium/src/+/275539a60ec716bea022200fa650...
==========

[Peter Kasting, 2016-06-15 01:46:27]

Committed patchset #10 (id:180001) manually as
275539a60ec716bea022200fa650a409772a81bf (presubmit successful).