Issue 1537633005: Extend the geometry APIs.

Issue 1537633005: Extend the geometry APIs. (Closed)

Created:
5 years ago by jeffbrown

Modified:
4 years, 11 months ago

Reviewers:
abarth, abarth-chromium, viettrungluu, jamesr

CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org

Base URL:
git@github.com:domokit/mojo.git@moz-10

Target Ref:
refs/heads/master

Project:
mojo

Visibility:
Public.

More Reviews

Description

Extend the geometry APIs. Add a RRect structure for rounded rect. Define helpers for manipulating transformation matrices. Define output formatters for geometry datatypes. BUG= R=viettrungluu@chromium.org, abarth, abarth@chromium.org, jamesr, viettrungluu Review URL: https://codereview.chromium.org/1517853002 . Committed: https://chromium.googlesource.com/external/mojo/+/55f4d308e5e8352bc5f41242bd8b19db431bc774

Patch Set 2 : fix include order #

Patch Set 3 : use formatting.h #

Patch Set 5 : rebase #

Created: 4 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+353 lines, -67 lines)			Patch
M	mojo/dart/packages/mojo_services/lib/mojo/geometry.mojom.dart	View	1 2	18 chunks	+223 lines, -66 lines	0 comments	Download
M	mojo/services/geometry/cpp/BUILD.gn	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
A	mojo/services/geometry/cpp/formatting.h	View	1 2 3	1 chunk	+21 lines, -0 lines	0 comments	Download
A	mojo/services/geometry/cpp/formatting.cc	View	1 2	1 chunk	+58 lines, -0 lines	0 comments	Download
M	mojo/services/geometry/cpp/geometry_util.h	View		2 chunks	+4 lines, -1 line	0 comments	Download
A	mojo/services/geometry/cpp/geometry_util.cc	View	1	1 chunk	+29 lines, -0 lines	0 comments	Download
M	mojo/services/geometry/interfaces/geometry.mojom	View		1 chunk	+15 lines, -0 lines	0 comments	Download

Depends on Patchset:

Issue 1532993002 Patch 60001

Dependent Patchsets:

Issue 1552963002 Patch 40001

Issue 1552963002 Patch 60001

Messages

Total messages: 7 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

viettrungluu

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/BUILD.gn File mojo/services/geometry/cpp/BUILD.gn (right): https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/BUILD.gn#newcode10 mojo/services/geometry/cpp/BUILD.gn:10: public_configs = [ "../../public/build/config:mojo_services" ] See mojo/services/public/build/config/BUILD.gn. Without the ...

5 years ago (2015-12-17 23:05:29 UTC) #2

jeffbrown

5 years ago (2015-12-18 01:41:22 UTC) #3

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
File mojo/services/geometry/cpp/BUILD.gn (right):

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/BUILD.gn:10: public_configs = [
"../../public/build/config:mojo_services" ]
On 2015/12/17 23:05:29, viettrungluu wrote:
> See mojo/services/public/build/config/BUILD.gn.
> 
> Without the relative include(s?), I guess you don't need this. Maybe we should
> get rid of mojo/services/public/build/config/BUILD.gn entirely?

Yeah, the relative includes are really annoying since they affect all downstream
code too.  Would be nice to get rid of that.

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
File mojo/services/geometry/cpp/geometry_util.h (right):

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.h:8: #include
"mojo/services/geometry/interfaces/geometry.mojom.h"
On 2015/12/17 23:05:29, viettrungluu wrote:
> Hmmm. So, not having "mojo/services/" before was "correct" according to some
> philosophy (which we haven't adhered to very strictly).
> 
> I wonder if we should stick to that philosophy, or eliminate it. (See my
comment
> in the BUILD.gn file.)

This change was probably unintentional (since this code used to live elsewhere
before it was moved here) but shows the downsides of those relative includes.

I think in general it's easier to work with the source tree if we either say
that all includes should be global or all should be relative (and have the build
system introduce the necessary include paths as part of declaring dependencies
on other modules).

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.h:37: void
SetIdentityTransform(Transform* transform);
On 2015/12/17 23:05:29, viettrungluu wrote:
> I wonder if it wouldn't be more natural for these to return a |Transform|
> instead. (I don't feel strongly about this at all.)

I considered that but this ended up being more general.  Works well for
stack-allocated objects too.  I also considered making "SetTranslationTransform"
into something more like a post-concatenation operation but decided to leave
that for another day.

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
File mojo/services/geometry/cpp/logging.cc (right):

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/logging.cc:7: #include
"mojo/services/geometry/cpp/logging.h"
On 2015/12/17 23:05:29, viettrungluu wrote:
> nit: The implementation file should include its header first.
> 
> (This is in the style guide, and is to try to ensure that headers include
> everything they need.)

Done.

viettrungluu

Thanks, lgtm. (I guess you previously had Adam's lg also.) https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/geometry_util.h File mojo/services/geometry/cpp/geometry_util.h (right): https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/geometry_util.h#newcode8 ...

5 years ago (2015-12-18 02:05:36 UTC) #4

Thanks, lgtm. (I guess you previously had Adam's lg also.)

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
File mojo/services/geometry/cpp/geometry_util.h (right):

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.h:8: #include
"mojo/services/geometry/interfaces/geometry.mojom.h"
On 2015/12/18 01:41:22, jeffbrown wrote:
> On 2015/12/17 23:05:29, viettrungluu wrote:
> > Hmmm. So, not having "mojo/services/" before was "correct" according to some
> > philosophy (which we haven't adhered to very strictly).
> > 
> > I wonder if we should stick to that philosophy, or eliminate it. (See my
> comment
> > in the BUILD.gn file.)
> 
> This change was probably unintentional (since this code used to live elsewhere
> before it was moved here) but shows the downsides of those relative includes.
> 
> I think in general it's easier to work with the source tree if we either say
> that all includes should be global or all should be relative (and have the
build
> system introduce the necessary include paths as part of declaring dependencies
> on other modules).

I think it's definitely easier for us. The original rationale was (something
like) that consumers may want to put the code somewhere else.

Probably, for now (especially since we're not consistent about this and no one
puts the code anywhere else) it's easier to go with absolute paths.

When we get around to a packaging system for C++ libraries, etc., we can worry
about this....

https://codereview.chromium.org/1537633005/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.h:37: void
SetIdentityTransform(Transform* transform);
On 2015/12/18 01:41:22, jeffbrown wrote:
> On 2015/12/17 23:05:29, viettrungluu wrote:
> > I wonder if it wouldn't be more natural for these to return a |Transform|
> > instead. (I don't feel strongly about this at all.)
> 
> I considered that but this ended up being more general.  Works well for
> stack-allocated objects too.  I also considered making
"SetTranslationTransform"
> into something more like a post-concatenation operation but decided to leave
> that for another day.

OK.

jeffbrown

4 years, 11 months ago (2016-01-26 23:50:45 UTC) #7

Message was sent while issue was closed.

Committed patchset #5 (id:80001) manually as
55f4d308e5e8352bc5f41242bd8b19db431bc774 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages