Extend the geometry APIs.

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=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/c47c16536c224fabbfb858c9a15b27cbb5518b12

[jeffbrown, 2015-12-10 22:22:02]

jeffbrown@google.com changed reviewers:
+ abarth@google.com, vardhan@google.com

[abarth-chromium, 2015-12-10 22:49:52]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org, jamesr@chromium.org

[abarth-chromium, 2015-12-10 22:49:53]

LGTM, but you might want jamesr to approve add this concept to this layer.

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.cc:10: static float
kIdentityMatrix[]{1.f, 0.f, 0.f, 0.f,  // comments to prevent
const ?

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/inte...
File mojo/services/geometry/interfaces/geometry.mojom (right):

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/inte...
mojo/services/geometry/interfaces/geometry.mojom:49: int32
bottom_right_radius_y;
It's very common for all of these radii to have the same value.  In SkRRect,
there's a bunch of work to take faster paths for simpler cases.  If/when we
convert this class into an SkRRect, we should make sure we're still hitting
those fast paths.

[jamesr, 2015-12-10 23:35:50]

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/logging.h:8: #include "mojo/common/logging.h"
i don't see this header in
https://github.com/domokit/mojo/tree/master/mojo/common - where does it come
from?

[vardhan, 2015-12-10 23:38:43]

On 2015/12/10 23:35:50, jamesr wrote:
>
https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
> File mojo/services/geometry/cpp/logging.h (right):
> 
>
https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
> mojo/services/geometry/cpp/logging.h:8: #include "mojo/common/logging.h"
> i don't see this header in
> https://github.com/domokit/mojo/tree/master/mojo/common - where does it come
> from?

(this CL is dependent on https://codereview.chromium.org/1519673002/, which adds
some operator<<() overloads for printing mojo maps, arrays, etc.)

[jamesr, 2015-12-10 23:49:29]

OK.  The cpp libraries in mojo/services/XXX/cpp are intended to be generally
reusable libraries (i.e. usable by consumers that aren't using the rest of our
C++ stuff).  The code in //mojo/common in practice depends on a bunch of the
rest of our code, for example //base.  While the code in
https://codereview.chromium.org/1519673002/ doesn't have a base dependency the
target it is linked into does and so it won't be possible to use for a consumer
unless they have pulled in our //base or have pulled our our build targets.

If this code depends only on the C++ standard library then maybe you could put
the logging helpers in the C++ section of the mojo SDK and then this library
could depend on that.  Alternately, you could put the logging code somewhere
other than //mojo/common such that we could enforce its separation from base and
other things.

[viettrungluu, 2015-12-10 23:54:27]

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

[viettrungluu, 2015-12-10 23:54:28]

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/logging.h:8: #include "mojo/common/logging.h"
On 2015/12/10 23:35:50, jamesr wrote:
> i don't see this header in
> https://github.com/domokit/mojo/tree/master/mojo/common - where does it come
> from?

More relevantly, none of that seems to be needed for this file (or the
corresponding .cc). All that seems to be needed is std::ostream. (This file
should just need <iosfwd>.)

[jamesr, 2015-12-10 23:55:56]

The impl needs the logging helpers, though, so the same thing applies there :)

[viettrungluu, 2015-12-11 00:15:07]

On 2015/12/10 23:55:56, jamesr wrote:
> The impl needs the logging helpers, though, so the same thing applies there :)

Not sure how ... but I guess I'm missing something.

[viettrungluu, 2015-12-11 00:15:13]

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.cc:5: #include <string>
You want <string.h> (or <cstring>) for memcpy, not <string>.

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/logging.cc:55: return os << value.matrix;
Delete.

[jeffbrown, 2015-12-11 19:40:55]

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.cc:5: #include <string>
On 2015/12/11 00:15:13, viettrungluu wrote:
> You want <string.h> (or <cstring>) for memcpy, not <string>.

Done.

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/geometry_util.cc:10: static float
kIdentityMatrix[]{1.f, 0.f, 0.f, 0.f,  // comments to prevent
On 2015/12/10 22:49:53, abarth-chromium wrote:
> const ?

Done.

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/logging.cc:55: return os << value.matrix;
On 2015/12/11 00:15:13, viettrungluu wrote:
> Delete.

Gahh... thanks.

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

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/cpp/...
mojo/services/geometry/cpp/logging.h:8: #include "mojo/common/logging.h"
On 2015/12/10 23:54:28, viettrungluu wrote:
> On 2015/12/10 23:35:50, jamesr wrote:
> > i don't see this header in
> > https://github.com/domokit/mojo/tree/master/mojo/common - where does it come
> > from?
> 
> More relevantly, none of that seems to be needed for this file (or the
> corresponding .cc). All that seems to be needed is std::ostream. (This file
> should just need <iosfwd>.)

ahh, iosfwd is a good idea

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/inte...
File mojo/services/geometry/interfaces/geometry.mojom (right):

https://codereview.chromium.org/1517853002/diff/1/mojo/services/geometry/inte...
mojo/services/geometry/interfaces/geometry.mojom:49: int32
bottom_right_radius_y;
On 2015/12/10 22:49:53, abarth-chromium wrote:
> It's very common for all of these radii to have the same value.  In SkRRect,
> there's a bunch of work to take faster paths for simpler cases.  If/when we
> convert this class into an SkRRect, we should make sure we're still hitting
> those fast paths.

Skia auto-detects the type when provided with the more generalized set of
parameters.

I was debating whether to only support simple cases in the compositor but this
wasn't any more trouble and it makes for easier interop with Flutter's layer
system.  I expect we'll revisit this sooner or later depending on how things
work out.

[jeffbrown, 2015-12-17 19:39:29]

Description was changed from

==========
Extend the geometry APIs.

Add a RRect structure for rounded rect.

Define helpers for manipulating transformation matrices.

Define output formatters for geometry datatypes.

BUG=
==========

to

==========
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=abarth@chromium.org

Committed:
https://chromium.googlesource.com/external/mojo/+/c47c16536c224fabbfb858c9a15...
==========

[jeffbrown, 2015-12-17 19:39:31]

Committed patchset #7 (id:120001) manually as
c47c16536c224fabbfb858c9a15b27cbb5518b12 (presubmit successful).