Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749513011:
Stabilize empty rect handling in EnclosingRect. (Closed)
|
Created:
3 years, 9 months ago by Peter Mayo Modified:
3 years, 8 months ago Reviewers:
danakj CC:
chromium-reviews, cc-bugs_chromium.org, sky Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionStabilize empty rect handling in EnclosingRect.
We want empty Rects to have empty enclosing rects, per
https://chromiumcodereview.appspot.com/15233003, but we would
prefer this to not bounce around based on floating point calculations
accumulating some epsilon sized errors.
roundOut in SKIA does not have either the former or the new behavior,
so we are drifting apart from an underlying expectation, and should be
careful this doesn't cause future inconsistencies between paths,
like mainthread/ impl thread.
BUG=None
R=danakj
CC=sky
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2749513011
Cr-Commit-Position: refs/heads/master@{#460794}
Committed: https://chromium.googlesource.com/chromium/src/+/faa28efcb640abddbf5f80a75724bad3d6e20a31
Patch Set 1 #
Total comments: 1
Patch Set 2 : Make less sensitive #Patch Set 3 : Keep size positive or zero. #Patch Set 4 : Use size in Enclosing computation #
Total comments: 2
Patch Set 5 : Drop limits #Patch Set 6 : Add ARM code branch #
Total comments: 5
Patch Set 7 : Fix edges better #
Total comments: 2
Patch Set 8 : Remove logicals, add tests, fix inconsistency, unwrap internals #
Total comments: 1
Patch Set 9 : typo #Patch Set 10 : Rebase to ToT and fix comment typo. #
Total comments: 13
Patch Set 11 : SizeFPrivate, prettier unittests. #Patch Set 12 : Move kTrivial inside the class #
Total comments: 5
Patch Set 13 : Convert to FRIEND_TEST_ALL_PREFIXES #Patch Set 14 : Change constnat naming convention, add assertion that nearly trivial isn't trivial #Patch Set 15 : Rebased #
Total comments: 4
Patch Set 16 : Nits #
Dependent Patchsets: Messages
Total messages: 53 (25 generated)
Here is the change related to keeping empty rects empty on 'Enclosing'. One of the key observations is that we are going to integer rects, where fractions are not meaningful, so neither should tiny squares. Some other potential threshold values are 1/16 - where a square of that size cannot alter the shade by one common level, or 1/256 where even a wide rectangle with one dimension of that can't. float:epsilon seems too small, it's easy to accumulate errors bigger than that in 4*4 matrix math.
On 2017/03/17 16:13:10, Peter Mayo wrote: > Here is the change related to keeping empty rects empty on 'Enclosing'. > > One of the key observations is that we are going to integer rects, where > fractions are not meaningful, so neither should tiny squares. So I think this is actually not right. The idea of enclosing rect is that we do care about partial pixels. We either want to fully include or exclude them though. We don't want floating point error to cause empty to become non-empty, that is the only thing. But that actually should be a property of gfx::RectF not of ToEnclosingRect, cuz gfx::Rect::IsEmpty should be accurate too. So maybe we should be doing something there. > Some other > potential threshold values are 1/16 - where a square of that size cannot alter > the shade by one common level, or 1/256 where even a wide rectangle with one > dimension of that can't. > > float:epsilon seems too small, it's easy to accumulate errors bigger than that > in 4*4 matrix math.
Description was changed from ========== Stabilize empty rect handling in EnclosingRect. We want empty Rects to have empty enclosing rects, per https://chromiumcodereview.appspot.com/15233003, but we would prefer this to not bounce around based on floating point calculations accumulating some epsilon sized errors. roundOut in SKIA does not have either the former or the new behavior, so we are drifting apart from an underlying expectation, and should be careful this doesn't cause future inconsistencies between paths, like mainthread/ impl thread. BUG=None R=danakj CC=sky ========== to ========== Stabilize empty rect handling in EnclosingRect. We want empty Rects to have empty enclosing rects, per https://chromiumcodereview.appspot.com/15233003, but we would prefer this to not bounce around based on floating point calculations accumulating some epsilon sized errors. roundOut in SKIA does not have either the former or the new behavior, so we are drifting apart from an underlying expectation, and should be careful this doesn't cause future inconsistencies between paths, like mainthread/ impl thread. BUG=None R=danakj CC=sky CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2749513011/diff/1/ui/gfx/geometry/rect_conver... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2749513011/diff/1/ui/gfx/geometry/rect_conver... ui/gfx/geometry/rect_conversions.cc:17: static const float kEmptyDimension = 0.1f; That is too big - it causes unittests to fail. (occlusion tests) .09 is empirically enough to make the tests pass. Interesting. https://codereview.chromium.org/2749513011/diff/80001/ui/gfx/geometry/rect_co... File ui/gfx/geometry/rect_conversions.cc (left): https://codereview.chromium.org/2749513011/diff/80001/ui/gfx/geometry/rect_co... ui/gfx/geometry/rect_conversions.cc:94: git cl format did this. https://codereview.chromium.org/2749513011/diff/80001/ui/gfx/geometry/size_f.cc File ui/gfx/geometry/size_f.cc (right): https://codereview.chromium.org/2749513011/diff/80001/ui/gfx/geometry/size_f.... ui/gfx/geometry/size_f.cc:5: #include <limits> remove
On 2017/03/17 16:46:13, danakj wrote: > On 2017/03/17 16:13:10, Peter Mayo wrote: > > Here is the change related to keeping empty rects empty on 'Enclosing'. > > > > One of the key observations is that we are going to integer rects, where > > fractions are not meaningful, so neither should tiny squares. > > So I think this is actually not right. The idea of enclosing rect is that we do > care about partial pixels. We either want to fully include or exclude them > though. We don't want floating point error to cause empty to become non-empty, > that is the only thing. But that actually should be a property of gfx::RectF not > of ToEnclosingRect, cuz gfx::Rect::IsEmpty should be accurate too. So maybe we > should be doing something there. Moved the comparison into Size, inside of RectF > > > Some other > > potential threshold values are 1/16 - where a square of that size cannot alter > > the shade by one common level, or 1/256 where even a wide rectangle with one > > dimension of that can't. > > > > float:epsilon seems too small, it's easy to accumulate errors bigger than that > > in 4*4 matrix math.
On 2017/03/17 16:46:13, danakj wrote: > On 2017/03/17 16:13:10, Peter Mayo wrote: > > Here is the change related to keeping empty rects empty on 'Enclosing'. > > > > One of the key observations is that we are going to integer rects, where > > fractions are not meaningful, so neither should tiny squares. > > So I think this is actually not right. The idea of enclosing rect is that we do > care about partial pixels. We either want to fully include or exclude them > though. We don't want floating point error to cause empty to become non-empty, > that is the only thing. But that actually should be a property of gfx::RectF not > of ToEnclosingRect, cuz gfx::Rect::IsEmpty should be accurate too. So maybe we > should be doing something there. Moved the comparison into Size, inside of RectF > > > Some other > > potential threshold values are 1/16 - where a square of that size cannot alter > > the shade by one common level, or 1/256 where even a wide rectangle with one > > dimension of that can't. > > > > float:epsilon seems too small, it's easy to accumulate errors bigger than that > > in 4*4 matrix math.
https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > kTrivial; } How would height be < kTrivial? This is now just checking if height() == kTrivial which seems strange. These don't seem needed once you're clamping to 0 at the setters?
https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > kTrivial; } On 2017/03/21 16:16:36, danakj wrote: > How would height be < kTrivial? This is now just checking if height() == > kTrivial which seems strange. These don't seem needed once you're clamping to 0 > at the setters? The best thing about these is that they make it clear that the decision about whether a Size/Width/Height is empty/trivial belongs here, rather than distributed all over the codebase. Clamping them in the setters keeps us from further accumulating small parts of errors, and causing scaling to stay sticky at empty. I guess I've lived in too many projects where being robust to someone adding a new method that modifies an internal (for example, by passing it by reference to a skia transform method). Regarding the point that the test is asymmetric for == kTrivial, I would propose changing the setters. I'm also open to not clamping in the setters. My least favorite option is removing the boolean tests and letting clients implicitly test against zero.
https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > kTrivial; } On 2017/03/21 16:45:57, Peter Mayo wrote: > On 2017/03/21 16:16:36, danakj wrote: > > How would height be < kTrivial? This is now just checking if height() == > > kTrivial which seems strange. These don't seem needed once you're clamping to > 0 > > at the setters? > > The best thing about these is that they make it clear that the decision about > whether a Size/Width/Height is empty/trivial belongs here, rather than > distributed all over the codebase. > > Clamping them in the setters keeps us from further accumulating small parts of > errors, and causing scaling to stay sticky at empty. > > I guess I've lived in too many projects where being robust to someone adding a > new method that modifies an internal (for example, by passing it by reference to > a skia transform method). > > Regarding the point that the test is asymmetric for == kTrivial, I would propose > changing the setters. > > I'm also open to not clamping in the setters. My least favorite option is > removing the boolean tests and letting clients implicitly test against zero. I think I'm okay with clamping at the setters. But I'm just questioning why you'd want HasHeight/HasWidth then. It seems like IsEmpty() does what you want then.
On 2017/03/21 17:42:05, danakj wrote: > https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f.h > File ui/gfx/geometry/size_f.h (right): > > https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f... > ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > > kTrivial; } > On 2017/03/21 16:45:57, Peter Mayo wrote: > > On 2017/03/21 16:16:36, danakj wrote: > > > How would height be < kTrivial? This is now just checking if height() == > > > kTrivial which seems strange. These don't seem needed once you're clamping > to > > 0 > > > at the setters? > > > > The best thing about these is that they make it clear that the decision about > > whether a Size/Width/Height is empty/trivial belongs here, rather than > > distributed all over the codebase. > > > > Clamping them in the setters keeps us from further accumulating small parts of > > errors, and causing scaling to stay sticky at empty. > > > > I guess I've lived in too many projects where being robust to someone adding a > > new method that modifies an internal (for example, by passing it by reference > to > > a skia transform method). > > > > Regarding the point that the test is asymmetric for == kTrivial, I would > propose > > changing the setters. > > > > I'm also open to not clamping in the setters. My least favorite option is > > removing the boolean tests and letting clients implicitly test against zero. > > I think I'm okay with clamping at the setters. But I'm just questioning why > you'd want HasHeight/HasWidth then. It seems like IsEmpty() does what you want > then. IsEmpty does both directions, and we used to preserve widths with no height, and vice-versa. So I kept it. Can that behavior go away? The unittests assert it doesn't.
On Tue, Mar 21, 2017 at 1:55 PM, <petermayo@chromium.org> wrote: > On 2017/03/21 17:42:05, danakj wrote: > > > https://codereview.chromium.org/2749513011/diff/120001/ui/ > gfx/geometry/size_f.h > > File ui/gfx/geometry/size_f.h (right): > > > > > https://codereview.chromium.org/2749513011/diff/120001/ui/ > gfx/geometry/size_f.h#newcode49 > > ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > > > kTrivial; } > > On 2017/03/21 16:45:57, Peter Mayo wrote: > > > On 2017/03/21 16:16:36, danakj wrote: > > > > How would height be < kTrivial? This is now just checking if > height() == > > > > kTrivial which seems strange. These don't seem needed once you're > clamping > > to > > > 0 > > > > at the setters? > > > > > > The best thing about these is that they make it clear that the decision > about > > > whether a Size/Width/Height is empty/trivial belongs here, rather than > > > distributed all over the codebase. > > > > > > Clamping them in the setters keeps us from further accumulating small > parts > of > > > errors, and causing scaling to stay sticky at empty. > > > > > > I guess I've lived in too many projects where being robust to someone > adding > a > > > new method that modifies an internal (for example, by passing it by > reference > > to > > > a skia transform method). > > > > > > Regarding the point that the test is asymmetric for == kTrivial, I > would > > propose > > > changing the setters. > > > > > > I'm also open to not clamping in the setters. My least favorite option > is > > > removing the boolean tests and letting clients implicitly test against > zero. > > > > I think I'm okay with clamping at the setters. But I'm just questioning > why > > you'd want HasHeight/HasWidth then. It seems like IsEmpty() does what > you want > > then. > IsEmpty does both directions, and we used to preserve widths with no > height, and > vice-versa. So I kept it. > > Can that behavior go away? The unittests assert it doesn't. > I see. HasHeight() would be { return height() > 0; } though. But you can just do that yourself. I don't see why we need them still? We should preserve width but no height, and vice versa. Code depends on that. If not in cc, then in blink which we want to use these. > > https://codereview.chromium.org/2749513011/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/21 18:22:15, danakj wrote:
> On Tue, Mar 21, 2017 at 1:55 PM, <mailto:petermayo@chromium.org> wrote:
>
...
> > IsEmpty does both directions, and we used to preserve widths with no
> > height, and
> > vice-versa. So I kept it.
> >
> > Can that behavior go away? The unittests assert it doesn't.
> >
>
> I see. HasHeight() would be { return height() > 0; } though. But you can
> just do that yourself. I don't see why we need them still? We should
> preserve width but no height, and vice versa. Code depends on that. If not
> in cc, then in blink which we want to use these.
>
>
I think the decision as to whether HasHeight() is { return height() > 0; } or {
return height() > kTrivial; } belongs in one place, rather than spread through
the calling code as the magic numbers, even between, 0. or epsilon. e.g. if
someone were to decide clamping is bad, removing it should all be in size_f.cc
[h] not rect.cc and friends/distant relations.
I'm cool with inlining it in the header for the optimizer to work it's magic on
Today's flavor.
Cool, so, I'd just prefer to not add APIs to gfx::SizeF that don't seem to be needed. You can check width() == 0 instead of HasWidth(). Let's make this patch the minimal change needed, which is to clamp and test for that? https://codereview.chromium.org/2749513011/diff/140001/ui/gfx/geometry/size_f.cc File ui/gfx/geometry/size_f.cc (right): https://codereview.chromium.org/2749513011/diff/140001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.cc:20: set_width(width() <= other.width() ? width() : other.width()); These change doesn't seem needed. Both width() and other.width() will be valid widths since they are coming from SizeF.
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Copied some previous comments into the in-line style for reviewing changes, and making sure they're addressed. Dry run of CQ looking good. https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > kTrivial; } On 2017/03/21 17:42:05, danakj wrote: > On 2017/03/21 16:45:57, Peter Mayo wrote: > > On 2017/03/21 16:16:36, danakj wrote: > > > How would height be < kTrivial? This is now just checking if height() == > > > kTrivial which seems strange. These don't seem needed once you're clamping > to > > 0 > > > at the setters? > > > > The best thing about these is that they make it clear that the decision about > > whether a Size/Width/Height is empty/trivial belongs here, rather than > > distributed all over the codebase. > > > > Clamping them in the setters keeps us from further accumulating small parts of > > errors, and causing scaling to stay sticky at empty. > > > > I guess I've lived in too many projects where being robust to someone adding a > > new method that modifies an internal (for example, by passing it by reference > to > > a skia transform method). > > > > Regarding the point that the test is asymmetric for == kTrivial, I would > propose > > changing the setters. > > > > I'm also open to not clamping in the setters. My least favorite option is > > removing the boolean tests and letting clients implicitly test against zero. > > I think I'm okay with clamping at the setters. But I'm just questioning why > you'd want HasHeight/HasWidth then. It seems like IsEmpty() does what you want > then. > > IsEmpty does both directions, and we used to preserve widths with no > > height, and > > vice-versa. So I kept it. > > > > Can that behavior go away? The unittests assert it doesn't. > > > > I see. HasHeight() would be { return height() > 0; } though. But you can > just do that yourself. I don't see why we need them still? We should > preserve width but no height, and vice versa. Code depends on that. If not > in cc, then in blink which we want to use these. > > I think the decision as to whether HasHeight() is { return height() > 0; } or { return height() > kTrivial; } belongs in one place, rather than spread through the calling code as the magic numbers, even between, 0. or epsilon. e.g. if someone were to decide clamping is bad, removing it should all be in size_f.cc [h] not rect.cc and friends/distant relations. I'm cool with inlining it in the header for the optimizer to work it's magic on Today's flavor. https://codereview.chromium.org/2749513011/diff/120001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:49: bool HasHeight() const { return height() > kTrivial; } Dana Wrote: > Cool, so, I'd just prefer to not add APIs to gfx::SizeF that don't seem to be > needed. You can check width() == 0 instead of HasWidth(). Let's make this patch > the minimal change needed, which is to clamp and test for that? OK, removed. https://codereview.chromium.org/2749513011/diff/140001/ui/gfx/geometry/size_f.cc File ui/gfx/geometry/size_f.cc (right): https://codereview.chromium.org/2749513011/diff/140001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.cc:20: set_width(width() <= other.width() ? width() : other.width()); On 2017/03/23 16:10:42, danakj wrote: > These change doesn't seem needed. Both width() and other.width() will be valid > widths since they are coming from SizeF. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2749513011/diff/180001/ui/gfx/geometry/size_u... File ui/gfx/geometry/size_unittest.cc (right): https://codereview.chromium.org/2749513011/diff/180001/ui/gfx/geometry/size_u... ui/gfx/geometry/size_unittest.cc:164: // The using the setter. Then
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/rect_c... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:20: int width = rect.size().width() I think you can revert the changes in this file? Why dropping the std::max()? That should be a separate CL if that's intentional. https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:17: static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon(); I don't think a kTrivial should be part of the gfx:: namespace, it's not a gfx concept, it's a detail of gfx::SizeF. You could make it public in gfx::SizeF, but I think I'd prefer if you made it private and either redefine it or use other small constants in unit tests. (You need to define storage in the .cc file for static variables even if they are constexpr) https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:34: void set_width(float width) { width_ = width > kTrivial ? width : 0.f; } When I wrote the fmaxf I tried >?: vs std::max vs fmaxf and found fmaxf was the best on binary size. Can you keep it and just swap the 0 with kTrivial? Changing it probably means you get to explore the binary size on android before/after which sounds unfun and easier to just leave it alone. https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_u... File ui/gfx/geometry/size_unittest.cc (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_u... ui/gfx/geometry/size_unittest.cc:228: SizeF right, bottom; Not sure that |right| and |bottom| express these, or |left| and |top|. I'd maybe suggest with_constructed_width, with_constructed_height, with_set_width, with_set_height or something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/rect_c... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:20: int width = rect.size().width() On 2017/03/23 20:12:06, danakj wrote: > I think you can revert the changes in this file? > > Why dropping the std::max()? That should be a separate CL if that's intentional. std::max with 0 is irrelevant once Size_F can't be smaller than zero. size.width() >=0 -> min_x <= max_x -> CeiledInt(max_x-min_x) >= 0 So, I think it should be part of this CL, since all the stuff less than kTrivial -> 0.0 If it shouldn't, we should have some unit tests to demonstrate the edge case. https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:17: static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon(); On 2017/03/23 20:12:06, danakj wrote: > I don't think a kTrivial should be part of the gfx:: namespace, it's not a gfx > concept, it's a detail of gfx::SizeF. You could make it public in gfx::SizeF, > but I think I'd prefer if you made it private and either redefine it or use > other small constants in unit tests. > > (You need to define storage in the .cc file for static variables even if they > are constexpr) I was trying to get the compilers to be able to inline the constant when (or if) they inline the code. In order to be used in part of a constexpr function (like it is) kTrivial then also has to be a visible constexpr when the declaration (and definition) are read. I agree it doesn't need gfx:: visibility, but when I put it inside the class, each instance got a copy, which is bad. As a compromise, what would you think about a well named namespace like SizeFPrivate? Forcing all of the constructors of size to never inline might be costly considering how widely used they are. Should I measure this on a few platforms to make a more informed decision? https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:34: void set_width(float width) { width_ = width > kTrivial ? width : 0.f; } On 2017/03/23 20:12:06, danakj wrote: > When I wrote the fmaxf I tried >?: vs std::max vs fmaxf and found fmaxf was the > best on binary size. Can you keep it and just swap the 0 with kTrivial? Changing > it probably means you get to explore the binary size on android before/after > which sounds unfun and easier to just leave it alone. Ack - I'll put it back. https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_u... File ui/gfx/geometry/size_unittest.cc (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_u... ui/gfx/geometry/size_unittest.cc:228: SizeF right, bottom; On 2017/03/23 20:12:06, danakj wrote: > Not sure that |right| and |bottom| express these, or |left| and |top|. I'd maybe > suggest with_constructed_width, with_constructed_height, with_set_width, > with_set_height or something? I have a better way I'll propose with the next edits.
https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:34: void set_width(float width) { width_ = width > kTrivial ? width : 0.f; } On 2017/03/23 23:58:41, Peter Mayo wrote: > On 2017/03/23 20:12:06, danakj wrote: > > When I wrote the fmaxf I tried >?: vs std::max vs fmaxf and found fmaxf was > the > > best on binary size. Can you keep it and just swap the 0 with kTrivial? > Changing > > it probably means you get to explore the binary size on android before/after > > which sounds unfun and easier to just leave it alone. > > Ack - I'll put it back. Ick - um, no, we want to clamp to 0, not kTrivial.
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/rect_c... File ui/gfx/geometry/rect_conversions.cc (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/rect_c... ui/gfx/geometry/rect_conversions.cc:20: int width = rect.size().width() On 2017/03/23 23:58:41, Peter Mayo wrote: > On 2017/03/23 20:12:06, danakj wrote: > > I think you can revert the changes in this file? > > > > Why dropping the std::max()? That should be a separate CL if that's > intentional. > > std::max with 0 is irrelevant once Size_F can't be smaller than zero. > > size.width() >=0 -> min_x <= max_x -> CeiledInt(max_x-min_x) >= 0 > > So, I think it should be part of this CL, since all the stuff less than kTrivial > -> 0.0 > > If it shouldn't, we should have some unit tests to demonstrate the edge case. Ah, this was clamping but RectF already does so it's redundant. Okay. rect.width() is a simpler way to write rect.size().width() tho, can you leave that? same with rect.height() https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:17: static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon(); On 2017/03/23 23:58:41, Peter Mayo wrote: > On 2017/03/23 20:12:06, danakj wrote: > > I don't think a kTrivial should be part of the gfx:: namespace, it's not a gfx > > concept, it's a detail of gfx::SizeF. You could make it public in gfx::SizeF, > > but I think I'd prefer if you made it private and either redefine it or use > > other small constants in unit tests. > > > > (You need to define storage in the .cc file for static variables even if they > > are constexpr) > > I was trying to get the compilers to be able to inline the constant when (or if) > they inline the code. In order to be used in part of a constexpr function (like > it is) kTrivial then also has to be a visible constexpr when the declaration > (and definition) are read. > > I agree it doesn't need gfx:: visibility, but when I put it inside the class, > each instance got a copy, which is bad. Sorry, I meant inside the class as a private static. There would still only be a single instance. > As a compromise, what would you think > about a well named namespace like SizeFPrivate? > > Forcing all of the constructors of size to never inline might be costly > considering how widely used they are. Should I measure this on a few platforms > to make a more informed decision? https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:34: void set_width(float width) { width_ = width > kTrivial ? width : 0.f; } On 2017/03/24 01:17:45, Peter Mayo wrote: > On 2017/03/23 23:58:41, Peter Mayo wrote: > > On 2017/03/23 20:12:06, danakj wrote: > > > When I wrote the fmaxf I tried >?: vs std::max vs fmaxf and found fmaxf was > > the > > > best on binary size. Can you keep it and just swap the 0 with kTrivial? > > Changing > > > it probably means you get to explore the binary size on android before/after > > > which sounds unfun and easier to just leave it alone. > > > > Ack - I'll put it back. > > Ick - um, no, we want to clamp to 0, not kTrivial. Oh, I see. The comparison but output are different now. Good point.
https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/220001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:17: static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon(); On 2017/03/24 14:55:35, danakj wrote: > On 2017/03/23 23:58:41, Peter Mayo wrote: > > On 2017/03/23 20:12:06, danakj wrote: > > > I don't think a kTrivial should be part of the gfx:: namespace, it's not a > gfx > > > concept, it's a detail of gfx::SizeF. You could make it public in > gfx::SizeF, > > > but I think I'd prefer if you made it private and either redefine it or use > > > other small constants in unit tests. > > > > > > (You need to define storage in the .cc file for static variables even if > they > > > are constexpr) > > > > I was trying to get the compilers to be able to inline the constant when (or > if) > > they inline the code. In order to be used in part of a constexpr function > (like > > it is) kTrivial then also has to be a visible constexpr when the declaration > > (and definition) are read. > > > > I agree it doesn't need gfx:: visibility, but when I put it inside the class, > > each instance got a copy, which is bad. > > Sorry, I meant inside the class as a private static. There would still only be a > single instance. > > > As a compromise, what would you think > > about a well named namespace like SizeFPrivate? > > > > Forcing all of the constructors of size to never inline might be costly > > considering how widely used they are. Should I measure this on a few > platforms > > to make a more informed decision? > I don't think you addressed this part. If kTrivial is a static const it is not a constexpr, and so we have to ditch constexpr from the SizeF constructor.
On Fri, Mar 24, 2017 at 5:05 PM, <petermayo@chromium.org> wrote: > > https://codereview.chromium.org/2749513011/diff/220001/ui/ > gfx/geometry/size_f.h > File ui/gfx/geometry/size_f.h (right): > > https://codereview.chromium.org/2749513011/diff/220001/ui/ > gfx/geometry/size_f.h#newcode17 > ui/gfx/geometry/size_f.h:17: static constexpr float kTrivial = 8.f * > std::numeric_limits<float>::epsilon(); > On 2017/03/24 14:55:35, danakj wrote: > > On 2017/03/23 23:58:41, Peter Mayo wrote: > > > On 2017/03/23 20:12:06, danakj wrote: > > > > I don't think a kTrivial should be part of the gfx:: namespace, > it's not a > > gfx > > > > concept, it's a detail of gfx::SizeF. You could make it public in > > gfx::SizeF, > > > > but I think I'd prefer if you made it private and either redefine > it or use > > > > other small constants in unit tests. > > > > > > > > (You need to define storage in the .cc file for static variables > even if > > they > > > > are constexpr) > > > > > > I was trying to get the compilers to be able to inline the constant > when (or > > if) > > > they inline the code. In order to be used in part of a constexpr > function > > (like > > > it is) kTrivial then also has to be a visible constexpr when the > declaration > > > (and definition) are read. > > > > > > I agree it doesn't need gfx:: visibility, but when I put it inside > the class, > > > each instance got a copy, which is bad. > > > > Sorry, I meant inside the class as a private static. There would still > only be a > > single instance. > > > > > As a compromise, what would you think > > > about a well named namespace like SizeFPrivate? > > > > > > Forcing all of the constructors of size to never inline might be > costly > > > considering how widely used they are. Should I measure this on a > few > > platforms > > > to make a more informed decision? > > > > I don't think you addressed this part. If kTrivial is a static const it > is not a constexpr, and so we have to ditch constexpr from the SizeF > constructor. > Oh, can you not static constexpr? I am assuming that works.. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/24 21:24:12, danakj wrote: > On Fri, Mar 24, 2017 at 5:05 PM, <mailto:petermayo@chromium.org> wrote: > > > > > https://codereview.chromium.org/2749513011/diff/220001/ui/ > > gfx/geometry/size_f.h > > File ui/gfx/geometry/size_f.h (right): > > > > https://codereview.chromium.org/2749513011/diff/220001/ui/ > > gfx/geometry/size_f.h#newcode17 > > ui/gfx/geometry/size_f.h:17: static constexpr float kTrivial = 8.f * > > std::numeric_limits<float>::epsilon(); > > On 2017/03/24 14:55:35, danakj wrote: > > > On 2017/03/23 23:58:41, Peter Mayo wrote: > > > > On 2017/03/23 20:12:06, danakj wrote: > > > > > I don't think a kTrivial should be part of the gfx:: namespace, > > it's not a > > > gfx > > > > > concept, it's a detail of gfx::SizeF. You could make it public in > > > gfx::SizeF, > > > > > but I think I'd prefer if you made it private and either redefine > > it or use > > > > > other small constants in unit tests. > > > > > > > > > > (You need to define storage in the .cc file for static variables > > even if > > > they > > > > > are constexpr) > > > > > > > > I was trying to get the compilers to be able to inline the constant > > when (or > > > if) > > > > they inline the code. In order to be used in part of a constexpr > > function > > > (like > > > > it is) kTrivial then also has to be a visible constexpr when the > > declaration > > > > (and definition) are read. > > > > > > > > I agree it doesn't need gfx:: visibility, but when I put it inside > > the class, > > > > each instance got a copy, which is bad. > > > > > > Sorry, I meant inside the class as a private static. There would still > > only be a > > > single instance. > > > > > > > As a compromise, what would you think > > > > about a well named namespace like SizeFPrivate? > > > > > > > > Forcing all of the constructors of size to never inline might be > > costly > > > > considering how widely used they are. Should I measure this on a > > few > > > platforms > > > > to make a more informed decision? > > > > > > > I don't think you addressed this part. If kTrivial is a static const it > > is not a constexpr, and so we have to ditch constexpr from the SizeF > > constructor. > > > > Oh, can you not static constexpr? I am assuming that works.. > Apparently the debugger I was using is slightly misleading: (rr) p resized $1 = {width_ = 0, height_ = 0, static kTrivial = 9.53674316e-07} (rr) p sizeof(resized) $2 = 8 (rr) p sizeof(resized.width_) $3 = 4 Updated.
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:63: friend float SizeFPrivateMatch(); // Test access to the kTrivial constant. make friends the first thing in the private block and whitespace below. you can use FRIEND_TEST to friend access to the constant https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest_prod.h... https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_u... File ui/gfx/geometry/size_unittest.cc (right): https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_u... ui/gfx/geometry/size_unittest.cc:160: static float myTrivial = SizeFPrivateMatch() / 2.f; these names are all (old) blink style not chromium style "my" is not descriptive, maybe smaller_than_trivial?
Done. https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:63: friend float SizeFPrivateMatch(); // Test access to the kTrivial constant. On 2017/03/28 18:00:20, danakj wrote: > make friends the first thing in the private block and whitespace below. you can > use FRIEND_TEST to friend access to the constant > https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest_prod.h... Done. https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_u... File ui/gfx/geometry/size_unittest.cc (right): https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_u... ui/gfx/geometry/size_unittest.cc:160: static float myTrivial = SizeFPrivateMatch() / 2.f; On 2017/03/28 18:00:20, danakj wrote: > these names are all (old) blink style not chromium style > > "my" is not descriptive, maybe smaller_than_trivial? Done. https://codereview.chromium.org/2749513011/diff/260001/ui/gfx/geometry/size_u... ui/gfx/geometry/size_unittest.cc:217: test.SetSize(nearlyTrivial, nearlyTrivial); Added an expectation here that validates nearly_trivial isn't clamped, to make sure we're testing what we expect - that scaling it to trivial clamps.
Rebased & ready to go?
The CQ bit was checked by petermayo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM w/ nits https://codereview.chromium.org/2749513011/diff/320001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/320001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:51: bool IsEmpty() const { return !height_ || !width_; } this doesn't seem like a related change anymore, I would revert this, it doesn't need to be part of the revision history https://codereview.chromium.org/2749513011/diff/320001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:70: static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon(); nit: normally statics and methods come before primate members, and put a whitespace between each block of them
https://codereview.chromium.org/2749513011/diff/320001/ui/gfx/geometry/size_f.h File ui/gfx/geometry/size_f.h (right): https://codereview.chromium.org/2749513011/diff/320001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:51: bool IsEmpty() const { return !height_ || !width_; } On 2017/03/29 19:10:52, danakj wrote: > this doesn't seem like a related change anymore, I would revert this, it doesn't > need to be part of the revision history Done. https://codereview.chromium.org/2749513011/diff/320001/ui/gfx/geometry/size_f... ui/gfx/geometry/size_f.h:70: static constexpr float kTrivial = 8.f * std::numeric_limits<float>::epsilon(); On 2017/03/29 19:10:52, danakj wrote: > nit: normally statics and methods come before primate members, and put a > whitespace between each block of them Done.
The CQ bit was checked by petermayo@chromium.org
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/2749513011/#ps340001 (title: "Nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1490886921046420,
"parent_rev": "cc4863010c8c9b706274a1eeed803d5a3f8250ca", "commit_rev":
"faa28efcb640abddbf5f80a75724bad3d6e20a31"}
Message was sent while issue was closed.
Description was changed from ========== Stabilize empty rect handling in EnclosingRect. We want empty Rects to have empty enclosing rects, per https://chromiumcodereview.appspot.com/15233003, but we would prefer this to not bounce around based on floating point calculations accumulating some epsilon sized errors. roundOut in SKIA does not have either the former or the new behavior, so we are drifting apart from an underlying expectation, and should be careful this doesn't cause future inconsistencies between paths, like mainthread/ impl thread. BUG=None R=danakj CC=sky CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Stabilize empty rect handling in EnclosingRect. We want empty Rects to have empty enclosing rects, per https://chromiumcodereview.appspot.com/15233003, but we would prefer this to not bounce around based on floating point calculations accumulating some epsilon sized errors. roundOut in SKIA does not have either the former or the new behavior, so we are drifting apart from an underlying expectation, and should be careful this doesn't cause future inconsistencies between paths, like mainthread/ impl thread. BUG=None R=danakj CC=sky CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2749513011 Cr-Commit-Position: refs/heads/master@{#460794} Committed: https://chromium.googlesource.com/chromium/src/+/faa28efcb640abddbf5f80a75724... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:340001) as https://chromium.googlesource.com/chromium/src/+/faa28efcb640abddbf5f80a75724... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
