Add float version of gfx::Range

Add float version of gfx::Range

Adds gfx::RangeF, the float version of gfx::Range. Range unit tests are updated to test both classes.

BUG=
R=rsesek,msw

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

[ckocagil, 2015-01-27 00:49:44]

ckocagil@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2015-01-27 17:51:46]

This looks good, but it needs tests.

I'm also wondering if it'd be better to implement this by turning Range into
TRange<ValueType> to support both int and float, since the implementations are
largely the same.

[ckocagil, 2015-01-30 18:21:46]

Templatified and updated unit tests. PTAL.

[Robert Sesek, 2015-01-30 20:47:02]

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h
File ui/gfx/range/range.h (left):

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#old...
ui/gfx/range/range.h:29: // A Range contains two integer values that represent a
numeric range, like the
This comment was lost; please restore it.

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h
File ui/gfx/range/range.h (right):

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:39: GFX_EXPORT TRangeBase<size_t> InvalidRange<size_t>();
Why do these need to be GFX_EXPORT if they're internal:: and not used outside of
//gfx ?

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:143: class Range : public TRangeBase<size_t> {
This class should be GFX_EXPORT.

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:162: GFX_EXPORT Range(const CHARRANGE& range, LONG
total_length = -1);
Why is this ctor GFX_EXPORT but none of the others are?

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:172: GFX_EXPORT CHARRANGE ToCHARRANGE() const;
Same, why is this GFX_EXPORT?

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:177: class RangeF : public TRangeBase<float> {
Same, GFX_EXPORT.

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range_unitt...
File ui/gfx/range/range_unittest.cc (right):

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range_unitt...
ui/gfx/range/range_unittest.cc:225: TEST(RangeTest, EmptyInit) {
I'd recommend using
https://code.google.com/p/googletest/wiki/V1_7_AdvancedGuide#Typed_Tests
instead. It'll make the test output more clear.

[ckocagil, 2015-01-31 02:44:39]

Comments addressed, PTAL

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h
File ui/gfx/range/range.h (right):

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:39: GFX_EXPORT TRangeBase<size_t> InvalidRange<size_t>();
On 2015/01/30 20:47:02, Robert Sesek wrote:
> Why do these need to be GFX_EXPORT if they're internal:: and not used outside
of
> //gfx ?

Done.

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:143: class Range : public TRangeBase<size_t> {
On 2015/01/30 20:47:02, Robert Sesek wrote:
> This class should be GFX_EXPORT.

Done. (ditto RangeF)

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range.h#new...
ui/gfx/range/range.h:162: GFX_EXPORT Range(const CHARRANGE& range, LONG
total_length = -1);
On 2015/01/30 20:47:02, Robert Sesek wrote:
> Why is this ctor GFX_EXPORT but none of the others are?

Removed, the class is itself GFX_EXPORT now. Same for the GFX_EXPORT comments
below.

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range_unitt...
File ui/gfx/range/range_unittest.cc (right):

https://codereview.chromium.org/876873003/diff/40001/ui/gfx/range/range_unitt...
ui/gfx/range/range_unittest.cc:225: TEST(RangeTest, EmptyInit) {
On 2015/01/30 20:47:02, Robert Sesek wrote:
> I'd recommend using
> https://code.google.com/p/googletest/wiki/V1_7_AdvancedGuide#Typed_Tests
> instead. It'll make the test output more clear.

Done. I didn't know that existed!

[Robert Sesek, 2015-02-02 21:42:11]

Last few comments :-)

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h
File ui/gfx/range/range.h (right):

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
The copyright year should actually remain the same when modifying files.

https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8p4JKV76kig/dis...

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:65: return internal::InvalidRange<T>();
Don't need internal:: since this is in the namespace.

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:95: return start_ == other.start() && end_ == other.end();
Use other.start_ and other.end_ for consistency.

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:157: Range(const internal::TRangeBase<size_t>& other);
Do you have to use the internal name here, or can |const Range& other| work?

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:195: RangeF(const internal::TRangeBase<float>& other);
Same.

[ckocagil, 2015-02-03 00:51:05]

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h
File ui/gfx/range/range.h (right):

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/02/02 21:42:11, Robert Sesek wrote:
> The copyright year should actually remain the same when modifying files.
> 
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8p4JKV76kig/dis...

Done.

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:65: return internal::InvalidRange<T>();
On 2015/02/02 21:42:11, Robert Sesek wrote:
> Don't need internal:: since this is in the namespace.

In this case we actually need it since TRangeBase::InvalidRange and
internal::InvalidRange have the same name. For other cases below (RangeLength,
etc.) I removed internal::.

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:95: return start_ == other.start() && end_ == other.end();
On 2015/02/02 21:42:11, Robert Sesek wrote:
> Use other.start_ and other.end_ for consistency.

Done.

https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range.h#ne...
ui/gfx/range/range.h:157: Range(const internal::TRangeBase<size_t>& other);
On 2015/02/02 21:42:11, Robert Sesek wrote:
> Do you have to use the internal name here, or can |const Range& other| work?

We need to construct a Range from a TRangeBase here:
https://codereview.chromium.org/876873003/diff/140001/ui/gfx/range/range_win....

[Robert Sesek, 2015-02-03 01:12:25]

LGTM

[Robert Sesek, 2015-02-03 01:14:57]

But please update the CL description to be more accurate w.r.t this change now
does.

[ckocagil, 2015-02-03 01:22:13]

The CQ bit was checked by ckocagil@chromium.org

[ckocagil, 2015-02-03 01:22:53]

The CQ bit was unchecked by ckocagil@chromium.org

[ckocagil, 2015-02-03 01:23:40]

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

[ckocagil, 2015-02-03 01:27:36]

+msw: very simple RTHB review.

On 2015/02/03 01:14:57, Robert Sesek wrote:
> But please update the CL description to be more accurate w.r.t this change now
> does.

Done.

[msw, 2015-02-03 20:30:17]

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

[msw, 2015-02-03 20:30:18]

Dana recently removed templatization of gfx::Rect[F] (and maybe other classes?)
for inlining performance and binary sizes. So you may want to follow that
pattern instead. See
https://chromium.googlesource.com/chromium/src/+/a5cc021b6ee19af6a931db1042ec...

Also, split the gfx::Range changes out from the RenderText changes.

[ckocagil, 2015-02-10 12:31:06]

Dana: Any thoughts regarding perf of our use of templates in range.h? We're
exporting classes (Range and RangeF) that derive from a template instantiation.

[danakj, 2015-02-10 17:44:00]

On Tue, Feb 10, 2015 at 4:31 AM, <ckocagil@chromium.org> wrote:

> Dana: Any thoughts regarding perf of our use of templates in range.h? We're
> exporting classes (Range and RangeF) that derive from a template
> instantiation.
>

Im not a fan of templating int/float for geometry classes. You'd have to
measure to see if there's any perf difference, but I think not templating
those classes was a large readability improvement too.



> https://codereview.chromium.org/876873003/
>

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

[ckocagil, 2015-02-12 16:06:46]

Done. Reverted to the non-template implementation.

On 2015/02/03 20:30:18, msw wrote:
> Also, split the gfx::Range changes out from the RenderText changes.

I don't understand. Which changes?

[Robert Sesek, 2015-02-12 16:52:37]

LGTM

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc
File ui/gfx/range/range_f.cc (right):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc...
ui/gfx/range/range_f.cc:37: return !base::IsNaN(start_) && !base::IsNaN(end_);
Use start() and end() like the rest of the implementation does.

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h
File ui/gfx/range/range_f.h (right):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h#...
ui/gfx/range/range_f.h:16: // A floating version of Range. RangeF is made of a
start and end position; when
floating -> float

[msw, 2015-02-12 21:01:05]

On 2015/02/12 16:06:46, ckocagil wrote:
> On 2015/02/03 20:30:18, msw wrote:
> > Also, split the gfx::Range changes out from the RenderText changes.
> 
> I don't understand. Which changes?

I meant converting the RTHB typedef to gfx::RangeF, but you can leave that in
this CL, it's not so bad.

+CC mukai for heads up, to address the render_text.h LineSegment::width TODO
after this CL lands.

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc
File ui/gfx/range/range_f.cc (right):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc...
ui/gfx/range/range_f.cc:37: return !base::IsNaN(start_) && !base::IsNaN(end_);
On 2015/02/12 16:52:37, Robert Sesek wrote:
> Use start() and end() like the rest of the implementation does.

Can this just "return *this != InvalidRange();" like Range?

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h
File ui/gfx/range/range_f.h (right):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h#...
ui/gfx/range/range_f.h:33: // The range is valid if neither field is NaN.
nit: Duplicate the comment from Range: "// Checks if the range is valid through
comparision to InvalidRange()." (maybe correct the spelling of "comparison" both
here and there)

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
File ui/gfx/range/range_unittest.cc (left):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
ui/gfx/range/range_unittest.cc:58: EXPECT_EQ(0U, r.length());
nit: please retain the length==0, start==end, reversed==false checks. The
semantics of an invalid RangeF should match those of an invalid Range.

[ckocagil, 2015-02-13 18:15:16]

New patchsets have been uploaded after l-g-t-m from rsesek@chromium.org

[ckocagil, 2015-02-13 18:47:47]

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc
File ui/gfx/range/range_f.cc (right):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc...
ui/gfx/range/range_f.cc:37: return !base::IsNaN(start_) && !base::IsNaN(end_);
On 2015/02/12 21:01:04, msw wrote:
> On 2015/02/12 16:52:37, Robert Sesek wrote:
> > Use start() and end() like the rest of the implementation does.
> 
> Can this just "return *this != InvalidRange();" like Range?

No. NaN isn't equal to any other value, including NaN.

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.cc...
ui/gfx/range/range_f.cc:37: return !base::IsNaN(start_) && !base::IsNaN(end_);
On 2015/02/12 16:52:37, Robert Sesek wrote:
> Use start() and end() like the rest of the implementation does.

Done.

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h
File ui/gfx/range/range_f.h (right):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h#...
ui/gfx/range/range_f.h:16: // A floating version of Range. RangeF is made of a
start and end position; when
On 2015/02/12 16:52:37, Robert Sesek wrote:
> floating -> float

Done.

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_f.h#...
ui/gfx/range/range_f.h:33: // The range is valid if neither field is NaN.
On 2015/02/12 21:01:04, msw wrote:
> nit: Duplicate the comment from Range: "// Checks if the range is valid
through
> comparision to InvalidRange()." (maybe correct the spelling of "comparison"
both
> here and there)

We can't compare to InvalidRange() so I improved the wording here and corrected
the spelling of "comparison" in range.h.

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
File ui/gfx/range/range_unittest.cc (left):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
ui/gfx/range/range_unittest.cc:58: EXPECT_EQ(0U, r.length());
On 2015/02/12 21:01:04, msw wrote:
> nit: please retain the length==0, start==end, reversed==false checks. The
> semantics of an invalid RangeF should match those of an invalid Range.

start==end isn't possible since NaN isn't equal to NaN. Others done.

[msw, 2015-02-13 19:29:31]

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
File ui/gfx/range/range_unittest.cc (left):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
ui/gfx/range/range_unittest.cc:58: EXPECT_EQ(0U, r.length());
On 2015/02/13 18:47:47, ckocagil wrote:
> On 2015/02/12 21:01:04, msw wrote:
> > nit: please retain the length==0, start==end, reversed==false checks. The
> > semantics of an invalid RangeF should match those of an invalid Range.
> 
> start==end isn't possible since NaN isn't equal to NaN. Others done.

Does this mean you can't test against an invalid range?
  gfx::RangeF my_range = gfx::RangeF::InvalidRange();
  if (my_range != gfx::RangeF::InvalidRange())
    printf("I assume |my_range| is valid, but it's not!!!");
I think that's a common check that this class should support.
Modify operator==() and add test cases here for == and !=.

What other semantic differences are caused by using NaN?
(please check&test Max(), Min(), and EqualsIgnoringDirection())
Did you consider using std::numeric_limits<float>::max() to parallel Range's
std::numeric_limits<size_t>::max())?

[ckocagil, 2015-02-13 20:05:50]

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
File ui/gfx/range/range_unittest.cc (left):

https://codereview.chromium.org/876873003/diff/180001/ui/gfx/range/range_unit...
ui/gfx/range/range_unittest.cc:58: EXPECT_EQ(0U, r.length());
On 2015/02/13 19:29:31, msw wrote:
> Did you consider using std::numeric_limits<float>::max() to parallel Range's
> std::numeric_limits<size_t>::max())?

Good call. Done.

- Restored "start==end" case
- Added GetMax(), GetMin(), operator==, EqualsIgnoringDirection() cases.
- IsValid() uses "*this != InvalidRange()"
- Removed IsValid() special cases due to NaN's incomparability.
- Updated comments

[msw, 2015-02-13 20:14:13]

Nice! lgtm

[ckocagil, 2015-02-18 17:08:01]

rsesek: Can you take another look please?

[Robert Sesek, 2015-02-18 17:21:23]

lgtm

[ckocagil, 2015-02-18 17:22:02]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2015-02-18 17:22:55]

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

[commit-bot: I haz the power, 2015-02-18 18:15:34]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-18 18:16:25]

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