Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846733003:
UI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier (Closed)
|
Created:
4 years, 8 months ago by loyso (OOO) Modified:
4 years, 8 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, kinuko+watch, cc-bugs_chromium.org, Eric Willigers, jbroman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier
blink::UnitBezier is a superset for gfx::CubicBezier.
(see previous preparations: http://crrev.com/1819783002)
1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h.
2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier.
3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc
BUG=577016
Committed: https://crrev.com/5ca4480095fe6660018edc298fdc922dc1b6a9e5
Cr-Commit-Position: refs/heads/master@{#385090}
Patch Set 1 #Patch Set 2 : Try to fix windows MSVC linking. #Patch Set 3 : Do not use public static member variable. #Patch Set 4 : Rename it. #
Total comments: 9
Patch Set 5 : Move method from header to .cc #Patch Set 6 : Rename members. Get rid of GetDefaultEpsilon. #
Total comments: 8
Patch Set 7 : Slope must clamp input x to [0, 1] range. #Patch Set 8 : Add one more TODO for Range. #Messages
Total messages: 39 (11 generated)
Description was changed from ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. gfx::CubicBezier becomes a superset for blink::UnitBezier after that. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc Previous preparations: crrev.com/1819783002 BUG=577016 ========== to ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. gfx::CubicBezier becomes a superset for blink::UnitBezier after that. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc Previous preparations: crrev.com/1819783002 BUG=577016 ==========
loyso@chromium.org changed reviewers: + alancutter@chromium.org, vollick@chromium.org
Description was changed from ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. gfx::CubicBezier becomes a superset for blink::UnitBezier after that. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc Previous preparations: crrev.com/1819783002 BUG=577016 ========== to ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. gfx::CubicBezier becomes a superset for blink::UnitBezier after that. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc Previous preparations: http://crrev.com/1819783002 BUG=577016 ==========
Description was changed from ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. gfx::CubicBezier becomes a superset for blink::UnitBezier after that. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc Previous preparations: http://crrev.com/1819783002 BUG=577016 ========== to ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ==========
Description was changed from ========== Blink Platform Animation: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ========== to ========== UI gfx geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ==========
Description was changed from ========== UI gfx geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ========== to ========== UI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ==========
danakj@chromium.org changed reviewers: + danakj@chromium.org
Why not just use the gfx type directly everywhere then?
On 2016/03/31 18:59:44, danakj wrote: > Why not just use the gfx type directly everywhere then? Current implementation lacks some API features. Basically, I update the gfx::CubicBezier implementation so it can be used everywhere.
On Thu, Mar 31, 2016 at 3:53 PM, <loyso@chromium.org> wrote: > On 2016/03/31 18:59:44, danakj wrote: > > Why not just use the gfx type directly everywhere then? > Current implementation lacks some API features. Basically, I update the > gfx::CubicBezier implementation so it can be used everywhere. > Right, but I mean after you did that, why wrap it? -- 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 Thu, Mar 31, 2016 at 3:53 PM, <loyso@chromium.org> wrote: > On 2016/03/31 18:59:44, danakj wrote: > > Why not just use the gfx type directly everywhere then? > Current implementation lacks some API features. Basically, I update the > gfx::CubicBezier implementation so it can be used everywhere. > Right, but I mean after you did that, why wrap it? -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2016/03/31 23:08:20, danakj wrote: > On Thu, Mar 31, 2016 at 3:53 PM, <mailto:loyso@chromium.org> wrote: > > > On 2016/03/31 18:59:44, danakj wrote: > > > Why not just use the gfx type directly everywhere then? > > Current implementation lacks some API features. Basically, I update the > > gfx::CubicBezier implementation so it can be used everywhere. > Right, but I mean after you did that, why wrap it? This as a convenient 'view' at gfx::CubicBezier for Blink people. We can unwrap it at any time. To be specific, alancutter@ asked me to preserve Blink API if possible. In general, we want to keep dependencies well scoped (we maintain some DEPS in blink/platform on per-folder basis) to understand them. Consider this as an extra level of indirection to decouple Blink from the rest of Chromium. I understand that this is linked to architectural discussions like this: https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/4iau... Overall, this is our vision within the Blink Onion Soup project, as I understand it.
On Thu, Mar 31, 2016 at 5:49 PM, <loyso@chromium.org> wrote: > On 2016/03/31 23:08:20, danakj wrote: > > On Thu, Mar 31, 2016 at 3:53 PM, <mailto:loyso@chromium.org> wrote: > > > > > On 2016/03/31 18:59:44, danakj wrote: > > > > Why not just use the gfx type directly everywhere then? > > > Current implementation lacks some API features. Basically, I update the > > > gfx::CubicBezier implementation so it can be used everywhere. > > Right, but I mean after you did that, why wrap it? > This as a convenient 'view' at gfx::CubicBezier for Blink people. We can > unwrap > it at any time. > To be specific, alancutter@ asked me to preserve Blink API if possible. > > In general, we want to keep dependencies well scoped (we maintain some > DEPS in > blink/platform on per-folder basis) to understand them. > Consider this as an extra level of indirection to decouple Blink from the > rest > of Chromium. > > I understand that this is linked to architectural discussions like this: > > https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/4iau... > Overall, this is our vision within the Blink Onion Soup project, as I > understand > it. > In that case, it turned out elliot didn't want us doing weird string stuff in core or modules, and to keep that in platform, but was otherwise fine with using GLES2Interface directly everywhere in blink. I'd like to see us use all the gfx geometry stuff through blink, there's no need to maintain two int rects, float rects, regions, etc. This falls into the same scope. I'm not trying to stop you from making these the same I think that's great. But I don't think we actually want to have to maintain wrappers. At the verrry worst, I think you should introduce a header in platform/ or wtf/ that #includes the gfx header, and does a type alias from gfx::UnitBezier to a blink::UnitBezier. That is, if people are against adding ui/gfx/geometry/cubic_bezier.h to core/DEPS. Then you'd core/ use the blink:: type name and platform/ use the gfx:: type name. Elliot do you have any thoughts here? -- 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 Thu, Mar 31, 2016 at 5:49 PM, <loyso@chromium.org> wrote: > On 2016/03/31 23:08:20, danakj wrote: > > On Thu, Mar 31, 2016 at 3:53 PM, <mailto:loyso@chromium.org> wrote: > > > > > On 2016/03/31 18:59:44, danakj wrote: > > > > Why not just use the gfx type directly everywhere then? > > > Current implementation lacks some API features. Basically, I update the > > > gfx::CubicBezier implementation so it can be used everywhere. > > Right, but I mean after you did that, why wrap it? > This as a convenient 'view' at gfx::CubicBezier for Blink people. We can > unwrap > it at any time. > To be specific, alancutter@ asked me to preserve Blink API if possible. > > In general, we want to keep dependencies well scoped (we maintain some > DEPS in > blink/platform on per-folder basis) to understand them. > Consider this as an extra level of indirection to decouple Blink from the > rest > of Chromium. > > I understand that this is linked to architectural discussions like this: > > https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/4iau... > Overall, this is our vision within the Blink Onion Soup project, as I > understand > it. > In that case, it turned out elliot didn't want us doing weird string stuff in core or modules, and to keep that in platform, but was otherwise fine with using GLES2Interface directly everywhere in blink. I'd like to see us use all the gfx geometry stuff through blink, there's no need to maintain two int rects, float rects, regions, etc. This falls into the same scope. I'm not trying to stop you from making these the same I think that's great. But I don't think we actually want to have to maintain wrappers. At the verrry worst, I think you should introduce a header in platform/ or wtf/ that #includes the gfx header, and does a type alias from gfx::UnitBezier to a blink::UnitBezier. That is, if people are against adding ui/gfx/geometry/cubic_bezier.h to core/DEPS. Then you'd core/ use the blink:: type name and platform/ use the gfx:: type name. Elliot do you have any thoughts here? -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I think long term we'd like to share the implementations with ui/gfx. The rule about wrappers was to force people to think about what they're exposing and how it interacts with WTF::Vector, String, and Map. For example if this thing takes a std::vector, and the primary user has data in blink in a WTF::Vector, we're paying constant conversion costs and should probably rethink the API. Similarly the ToString() methods on all the gfx types are std::string, but in blink we'd use String/StringBuilder for this. I've currently been using the type alias trick as a way to assert that "You look at the API, and yes it makes sense in blink to use directly." and you considered if a wrapper, or a WTF friendly impl in blink would be a better option. I had been more strict about forcing API wrappers like this at first, especially for things that have large or complex API where blink really doesn't need to call all the methods (ex. some CC things), but for gfx types we might want to just white list one at a time, or alias the types as we asses the API surface. (fwiw the wrappers are also nice when the API is used in hundreds of files all over and the base or gfx one has a subtly different API. Churning the entire codebase is often not worth it, or can introduce bugs, over just removing the code duplication and having a WTF or Platform adaptor.) danakj@ What do you think for this case?
> I think long term we'd like to share the implementations with ui/gfx. The rule > about wrappers was to force people to think about what they're exposing and how > it interacts with WTF::Vector, String, and Map. For example if this thing takes > a std::vector, and the primary user has data in blink in a WTF::Vector, we're > paying constant conversion costs and should probably rethink the API. Similarly > the ToString() methods on all the gfx types are std::string, but in blink we'd > use String/StringBuilder for this. For geometry classes, ToString() should literally not appear in code that we ship. It is for debugging and for the implementation of operator<< used in tests for EXPECT_EQ and such. So I don't think it's worth worrying about here. Maybe we use it somewhere for tracing output. > I've currently been using the type alias trick as a way to assert that "You look > at the API, and yes it makes sense in blink to use directly." and you considered > if a wrapper, or a WTF friendly impl in blink would be a better option. That makes sense. Adding a header at a time to the DEPS files would also be a reasonable way to signal this, I think. > I had been more strict about forcing API wrappers like this at first, especially > for things that have large or complex API where blink really doesn't need to > call all the methods (ex. some CC things), but for gfx types we might want to > just white list one at a time, or alias the types as we asses the API surface. Yeah. I think we should be looking to whitelist one at a time, and replace users with the new one, then move on to the next, essentially. I wouldn't want to do the whole directory at once, let's look at them one at a time. This also helps us be more consistent by converting uses over one at a time instead of having a huge mess of both for a long time. > (fwiw the wrappers are also nice when the API is used in hundreds of files all > over and the base or gfx one has a subtly different API. Churning the entire > codebase is often not worth it, or can introduce bugs, over just removing the > code duplication and having a WTF or Platform adaptor.) If we're just building a wrapper to call thru to the gfx class, I don't think bugs or behaviour changes can come into play any differently by using the gfx type directly. That appears to be the case here. So I think a wrapper is strictly worse. For other types, we should determine if there is behaviour differences, and then I guess consider what to do for those on a case-by-case basis. Maybe a wrapper function local to the place that depends on the behaviour, maybe a wrapper class to help transition, or something. I think of cc::Region as a good example of this, it has slightly different behaviour than SkRegion. I'm not sure if it matches core's Region, and we need to have a look at it. Re: churn.. I tend to optimize for the simplest solution and best code for future readers/writers over optimizing against changing things. I think eliminating layers has a lot of value, so I don't worry about it causing churn. > danakj@ What do you think for this case? So ya in this case, I'd like to see us DEPS +ui/gfx/geometry/cubic_bezier.h in WebKit/ or equivalent and replace uses of UnitBezier. https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... File ui/gfx/geometry/cubic_bezier.h (right): https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:39: double SolveCurveX(double x, double epsilon) const { Put this in the .cc file. https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:40: DCHECK(x >= 0.0); DCHECK_GE https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:111: void Range(double* min, double* max) const { Just make 2 getters, minimum() and maximum()?
> So ya in this case, I'd like to see us DEPS +ui/gfx/geometry/cubic_bezier.h in WebKit/ or equivalent and replace uses of UnitBezier. Can I do it (to 'unwrap') in a separate CL? It will change call sites, which is too much. This CL is about changing the implementation. https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... File ui/gfx/geometry/cubic_bezier.h (right): https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:111: void Range(double* min, double* max) const { On 2016/04/01 01:35:58, danakj wrote: > Just make 2 getters, minimum() and maximum()? It would affect the call sites. Can we change the API in a separate CL?
Yes please do it in steps. -- 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.
Yes please do it in steps. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... File ui/gfx/geometry/cubic_bezier.h (right): https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:39: double SolveCurveX(double x, double epsilon) const { On 2016/04/01 01:35:58, danakj wrote: > Put this in the .cc file. Done. https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:40: DCHECK(x >= 0.0); On 2016/04/01 01:35:58, danakj wrote: > DCHECK_GE Done.
LGTM % vollick https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... File ui/gfx/geometry/cubic_bezier.h (right): https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:111: void Range(double* min, double* max) const { On 2016/04/01 02:58:03, loyso wrote: > On 2016/04/01 01:35:58, danakj wrote: > > Just make 2 getters, minimum() and maximum()? > It would affect the call sites. Can we change the API in a separate CL? Sure yep. Put a TODO? https://codereview.chromium.org/1846733003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/animation/UnitBezier.h (right): https://codereview.chromium.org/1846733003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/animation/UnitBezier.h:13: struct UnitBezier { TODO to replace with the gfx type? https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... File ui/gfx/geometry/cubic_bezier_unittest.cc (right): https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... ui/gfx/geometry/cubic_bezier_unittest.cc:39: CubicBezier basicUse(0.5, 1.0, 0.5, 1.0); basic_use https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... ui/gfx/geometry/cubic_bezier_unittest.cc:181: CubicBezier atEdgeOfRange(0.5, 1.0, 0.5, 1.0); at_edge_of_range https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... ui/gfx/geometry/cubic_bezier_unittest.cc:185: CubicBezier largeEpsilon(0.5, 1.0, 0.5, 1.0); large_epsilon
On 2016/04/01 19:26:18, danakj wrote: > LGTM % vollick > > https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... > File ui/gfx/geometry/cubic_bezier.h (right): > > https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... > ui/gfx/geometry/cubic_bezier.h:111: void Range(double* min, double* max) const { > On 2016/04/01 02:58:03, loyso wrote: > > On 2016/04/01 01:35:58, danakj wrote: > > > Just make 2 getters, minimum() and maximum()? > > It would affect the call sites. Can we change the API in a separate CL? > > Sure yep. Put a TODO? > > https://codereview.chromium.org/1846733003/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/animation/UnitBezier.h (right): > > https://codereview.chromium.org/1846733003/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/animation/UnitBezier.h:13: struct UnitBezier > { > TODO to replace with the gfx type? > > https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... > File ui/gfx/geometry/cubic_bezier_unittest.cc (right): > > https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... > ui/gfx/geometry/cubic_bezier_unittest.cc:39: CubicBezier basicUse(0.5, 1.0, 0.5, > 1.0); > basic_use > > https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... > ui/gfx/geometry/cubic_bezier_unittest.cc:181: CubicBezier atEdgeOfRange(0.5, > 1.0, 0.5, 1.0); > at_edge_of_range > > https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... > ui/gfx/geometry/cubic_bezier_unittest.cc:185: CubicBezier largeEpsilon(0.5, 1.0, > 0.5, 1.0); > large_epsilon This lgtm, too, though I agree with Dana about using ui/gfx directly (though, of course, in a follow-up).
https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... File ui/gfx/geometry/cubic_bezier.h (right): https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:111: void Range(double* min, double* max) const { On 2016/04/01 19:26:17, danakj wrote: > On 2016/04/01 02:58:03, loyso wrote: > > On 2016/04/01 01:35:58, danakj wrote: > > > Just make 2 getters, minimum() and maximum()? > > It would affect the call sites. Can we change the API in a separate CL? > > Sure yep. Put a TODO? Or, make the blink wrapper use the min/max accessors, with the TODO over there.
https://codereview.chromium.org/1846733003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/animation/UnitBezier.h (right): https://codereview.chromium.org/1846733003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/animation/UnitBezier.h:13: struct UnitBezier { On 2016/04/01 19:26:17, danakj wrote: > TODO to replace with the gfx type? Done. https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... File ui/gfx/geometry/cubic_bezier_unittest.cc (right): https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... ui/gfx/geometry/cubic_bezier_unittest.cc:39: CubicBezier basicUse(0.5, 1.0, 0.5, 1.0); On 2016/04/01 19:26:17, danakj wrote: > basic_use Done. https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... ui/gfx/geometry/cubic_bezier_unittest.cc:181: CubicBezier atEdgeOfRange(0.5, 1.0, 0.5, 1.0); On 2016/04/01 19:26:18, danakj wrote: > at_edge_of_range Done. https://codereview.chromium.org/1846733003/diff/100001/ui/gfx/geometry/cubic_... ui/gfx/geometry/cubic_bezier_unittest.cc:185: CubicBezier largeEpsilon(0.5, 1.0, 0.5, 1.0); On 2016/04/01 19:26:17, danakj wrote: > large_epsilon Done.
Should we move ClampToRange from cc to gfx/geometry?
https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... File ui/gfx/geometry/cubic_bezier.h (right): https://codereview.chromium.org/1846733003/diff/60001/ui/gfx/geometry/cubic_b... ui/gfx/geometry/cubic_bezier.h:111: void Range(double* min, double* max) const { On 2016/04/01 19:54:33, danakj wrote: > On 2016/04/01 19:26:17, danakj wrote: > > On 2016/04/01 02:58:03, loyso wrote: > > > On 2016/04/01 01:35:58, danakj wrote: > > > > Just make 2 getters, minimum() and maximum()? > > > It would affect the call sites. Can we change the API in a separate CL? > > > > Sure yep. Put a TODO? > > Or, make the blink wrapper use the min/max accessors, with the TODO over there. Done. It's CC-only for now.
On Sun, Apr 3, 2016 at 7:39 PM, <loyso@chromium.org> wrote: > Should we move ClampToRange from cc to gfx/geometry? > Is it needed outside of cc? -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Sun, Apr 3, 2016 at 7:39 PM, <loyso@chromium.org> wrote: > Should we move ClampToRange from cc to gfx/geometry? > Is it needed outside of cc? -- 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 2016/04/04 20:04:19, danakj wrote: > Is it needed outside of cc? Well, I need it in this CL. x = std::min(std::max(x, 0.0), 1.0); I suspect, it is needed in other places on levels which are lower than CC. It would be nice to have some Clamp function at the base level, IMHO.
On 2016/04/05 00:07:16, loyso wrote:
> On 2016/04/04 20:04:19, danakj wrote:
> > Is it needed outside of cc?
> Well, I need it in this CL.
> x = std::min(std::max(x, 0.0), 1.0);
> I suspect, it is needed in other places on levels which are lower than CC.
> It would be nice to have some Clamp function at the base level, IMHO.
Ah I see. There's 2 callers in cc now, and one in
content/browser/gpu/compositor_util.cc, not a huge hit. There are a whole lot of
"min(max(" or "max(min(" calls throughout chrome, but I'm not sure writing a
global helper for this is really the right thing to do, people would continue to
just write min(max()) probably since those are very well known.
I think I'd lean toward deleting it from MathUtil over moving that to gfx/. You
can write an anonymous ClampToRange() in the cubic_bezier.cc for now if you
don't want to write min(max()) directly?
On 2016/04/05 00:37:19, danakj wrote:
> Ah I see. There's 2 callers in cc now, and one in
> content/browser/gpu/compositor_util.cc, not a huge hit. There are a whole lot
of
> "min(max(" or "max(min(" calls throughout chrome, but I'm not sure writing a
> global helper for this is really the right thing to do, people would continue
to
> just write min(max()) probably since those are very well known.
>
> I think I'd lean toward deleting it from MathUtil over moving that to gfx/.
You
> can write an anonymous ClampToRange() in the cubic_bezier.cc for now if you
> don't want to write min(max()) directly?
min(max is ok in this CL.
Would be nice to unify it in chromium and make a PSA so people can start using
it.
The CQ bit was checked by loyso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1846733003/#ps140001 (title: "Add one more TODO for Range.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846733003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846733003/140001
Message was sent while issue was closed.
Description was changed from ========== UI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ========== to ========== UI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== UI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 ========== to ========== UI GFX Geometry: Make UnitBezier a wrapper for gfx::CubicBezier blink::UnitBezier is a superset for gfx::CubicBezier. (see previous preparations: http://crrev.com/1819783002) 1) Move UnitBezier.cpp/h implementation to cubic_bezier.cc/h. 2) Re-implement blink::UnitBezier as a thin inline wrapper for gfx::CubicBezier. 3) Merge missing UnitBezier unit tests into cubic_bezier_unittest.cc BUG=577016 Committed: https://crrev.com/5ca4480095fe6660018edc298fdc922dc1b6a9e5 Cr-Commit-Position: refs/heads/master@{#385090} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5ca4480095fe6660018edc298fdc922dc1b6a9e5 Cr-Commit-Position: refs/heads/master@{#385090} |
