Add detection for rotation and flip overlay transforms

Add detection for rotation and flip overlay transforms

Rotations and flips were listed in the enum, but not being allowed by
OverlayCandidate::GetOverlayTransform.

BUG=

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

[halliwell, 2015-05-09 00:30:00]

halliwell@chromium.org changed reviewers:
+ achaulk@chromium.org, alexst@chromium.org, danakj@chromium.org

[danakj, 2015-05-11 16:58:04]

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

[danakj, 2015-05-11 16:58:04]

+vollick for math, is there a more efficient way to tell if it's a flip/rotate
than transforming points?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
can you TODO pointing to a bug to make this constant public (or some public API)
on SkMatrix44 if we're going to depend on it, and follow up with that change in
skia?

I actually don't see this constant in SkMatrix44 anywhere, can you point me to
it?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:23: NONE,
usually NONE would go first so it's 0

[Ian Vollick, 2015-05-12 01:24:15]

math lgtm.

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
On 2015/05/11 at 16:58:04, danakj wrote:
> can you TODO pointing to a bug to make this constant public (or some public
API) on SkMatrix44 if we're going to depend on it, and follow up with that
change in skia?
> 
> I actually don't see this constant in SkMatrix44 anywhere, can you point me to
it?

I believe that this from transform.cc (which also cites this constant), but the
comment is now bogus -- the constant no longer exists in skia. Perhaps we could
expose it on Transform and get rid of this comment and the one in transform.cc?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:69: transform.TransformPoint(&y_axis);
Regarding Dana's question about whether we could do something better than
transforming points, we sorta could. We could avoid nuking the translation
components and construct a Point3F out of the first two columns of the matrix
instead, but I don't think the perf hit of doing it this way is significant and
this seems a bit easier to understand.

That said, I would add a comment explaining why you zero out the translation
bits.

[halliwell, 2015-05-12 01:57:09]

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
On 2015/05/12 01:24:15, vollick wrote:
> On 2015/05/11 at 16:58:04, danakj wrote:
> > can you TODO pointing to a bug to make this constant public (or some public
> API) on SkMatrix44 if we're going to depend on it, and follow up with that
> change in skia?
> > 
> > I actually don't see this constant in SkMatrix44 anywhere, can you point me
to
> it?
> 
> I believe that this from transform.cc (which also cites this constant), but
the
> comment is now bogus -- the constant no longer exists in skia. Perhaps we
could
> expose it on Transform and get rid of this comment and the one in
transform.cc?

Correct, I just copied this comment from Transform without actually looking at
SkMatrix44 :)

I guess I could expose it on Transform, although when I sat down to write a
comment for that, I see that Transform uses it somewhat patchily and not
consistently with all such comparisons:
* Some functions accept a tolerance parameter (e.g.
IsApproximatelyIdentityOrTranslation)
* A number of tests fall through to SkMatrix logic, which is sometimes exact
(isTranslate), for determinants checks that 1/det is finite
* Transform::isBackFaceVisible and Transform::Preserves2dAxisAlignment are the
only places that use kEpsilon, the former uses it on a determinant and the
latter on elements (note that SkMatrix44::preserves2dAxisAlignment accepts a
tolerance parameter which defaults to a different constant, SK_ScalarNearlyZero)

So I would probably favour just deleting the comment and keeping as a local
constant here.  I also think this type of error bound, while a totally arbitrary
fudge factor, is at least somewhat possible to bound by the nature of the
calculation that leads to the data.  In other words, having a general-purpose
epsilon on Transform may not be so useful compared to the ability to improve the
error bounds and method locally in this file, given it's a specific calculation
that leads to these numbers.

Let me know what you think :)

[hendrikw, 2015-05-12 02:12:28]

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

[hendrikw, 2015-05-12 02:12:29]

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
On 2015/05/12 01:24:15, vollick wrote:
> On 2015/05/11 at 16:58:04, danakj wrote:
> > can you TODO pointing to a bug to make this constant public (or some public
> API) on SkMatrix44 if we're going to depend on it, and follow up with that
> change in skia?
> > 
> > I actually don't see this constant in SkMatrix44 anywhere, can you point me
to
> it?
> 
> I believe that this from transform.cc (which also cites this constant), but
the
> comment is now bogus -- the constant no longer exists in skia. Perhaps we
could
> expose it on Transform and get rid of this comment and the one in
transform.cc?

Perhaps even replace it with std::numeric_limits<float>::epsilon()?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:53: bool flipped) {
rename to flipped_over_y_axis?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:64: }
Should probably have GetXAxis() and GetYAxis() functions in the matrix that just
returns [(0,0), (1,0), (2,0)] (row, col) and [(0,1), (1,1), (2,1)].

That way you don't need to make a matrix copy, set the translation, or transform
a point (which is misleading since you're treating it as a vector)

You'll need to multiply the y components of each axis by -1 when
flipped_over_y_axis is true (since you wouldn't have a copy of the matrix)

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:72: Axis y_to = PointToAxis(y_axis);
your PointToAxis functions ignores scale, is that ok for your use case?  e.g. if
you have a 10x scale in the x direction, you'll report back as
gfx::OVERLAY_TRANSFORM_NONE.

[halliwell, 2015-05-12 15:11:31]

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
On 2015/05/12 02:12:29, hendrikw wrote:
> On 2015/05/12 01:24:15, vollick wrote:
> > On 2015/05/11 at 16:58:04, danakj wrote:
> > > can you TODO pointing to a bug to make this constant public (or some
public
> > API) on SkMatrix44 if we're going to depend on it, and follow up with that
> > change in skia?
> > > 
> > > I actually don't see this constant in SkMatrix44 anywhere, can you point
me
> to
> > it?
> > 
> > I believe that this from transform.cc (which also cites this constant), but
> the
> > comment is now bogus -- the constant no longer exists in skia. Perhaps we
> could
> > expose it on Transform and get rid of this comment and the one in
> transform.cc?
> 
> Perhaps even replace it with std::numeric_limits<float>::epsilon()?

Definitely not that :)  numeric_limits::epsilon is the next representable number
greater than 1.  The tolerance required here is significantly greater.

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:23: NONE,
On 2015/05/11 16:58:04, danakj wrote:
> usually NONE would go first so it's 0

Done.

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:53: bool flipped) {
On 2015/05/12 02:12:29, hendrikw wrote:
> rename to flipped_over_y_axis?

I agree this isn't super clear, especially when "FLIP_HORIZONTAL" is one of the
possible outcomes of this function.  Otoh, it's named this way throughout the
quad data structures ... thoughts?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:64: }
On 2015/05/12 02:12:29, hendrikw wrote:
> Should probably have GetXAxis() and GetYAxis() functions in the matrix that
just
> returns [(0,0), (1,0), (2,0)] (row, col) and [(0,1), (1,1), (2,1)].
> 
> That way you don't need to make a matrix copy, set the translation, or
transform
> a point (which is misleading since you're treating it as a vector)
> 
> You'll need to multiply the y components of each axis by -1 when
> flipped_over_y_axis is true (since you wouldn't have a copy of the matrix)
Yep, GetXAxis/GetYAxis seem like a clear way to express this.  I put the
function here for now, not sure what the policy is for adding stuff to Transform
... existing API seems quite minimal?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:69: transform.TransformPoint(&y_axis);
On 2015/05/12 01:24:15, vollick wrote:
> Regarding Dana's question about whether we could do something better than
> transforming points, we sorta could. We could avoid nuking the translation
> components and construct a Point3F out of the first two columns of the matrix
> instead, but I don't think the perf hit of doing it this way is significant
and
> this seems a bit easier to understand.
> 
> That said, I would add a comment explaining why you zero out the translation
> bits.

Indeed, not a huge perf hit, but seems silly to be doing a full transform.  I
like hendrikw@'s GetXAxis suggestion, went with that - think it's more
self-explanatory than zeroing the translation too.

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:72: Axis y_to = PointToAxis(y_axis);
On 2015/05/12 02:12:29, hendrikw wrote:
> your PointToAxis functions ignores scale, is that ok for your use case?  e.g.
if
> you have a 10x scale in the x direction, you'll report back as
> gfx::OVERLAY_TRANSFORM_NONE.

Correct.  TRANSFORM_NONE doesn't mean 'no transform', it just means 'no rotation
or flip'.  Translation and (positive) scale are handled separately in the
hardware overlay APIs.

[hendrikw, 2015-05-12 16:12:39]

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
On 2015/05/12 15:11:31, halliwell wrote:
> On 2015/05/12 02:12:29, hendrikw wrote:
> > On 2015/05/12 01:24:15, vollick wrote:
> > > On 2015/05/11 at 16:58:04, danakj wrote:
> > > > can you TODO pointing to a bug to make this constant public (or some
> public
> > > API) on SkMatrix44 if we're going to depend on it, and follow up with that
> > > change in skia?
> > > > 
> > > > I actually don't see this constant in SkMatrix44 anywhere, can you point
> me
> > to
> > > it?
> > > 
> > > I believe that this from transform.cc (which also cites this constant),
but
> > the
> > > comment is now bogus -- the constant no longer exists in skia. Perhaps we
> > could
> > > expose it on Transform and get rid of this comment and the one in
> > transform.cc?
> > 
> > Perhaps even replace it with std::numeric_limits<float>::epsilon()?
> 
> Definitely not that :)  numeric_limits::epsilon is the next representable
number
> greater than 1.  The tolerance required here is significantly greater.

If accuracy isn't important for your use case, then cool! As is, the smaller the
scale on the matrix, the larger the rotation that will be recognized as not
OVERLAY_TRANSFORM_INVALID.

Personally, I always use stl's epsilon unless I've demonstrated a case where fp
errors to creep in (starting with an arbitrarily large epsilon is akin to early
optimizing code, imo).

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:53: bool flipped) {
On 2015/05/12 15:11:31, halliwell wrote:
> On 2015/05/12 02:12:29, hendrikw wrote:
> > rename to flipped_over_y_axis?
> 
> I agree this isn't super clear, especially when "FLIP_HORIZONTAL" is one of
the
> possible outcomes of this function.  Otoh, it's named this way throughout the
> quad data structures ... thoughts?

I don't know, I would probably start the rename here, it's a bit clearer.

[danakj, 2015-05-12 16:54:32]

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:53: bool flipped) {
On 2015/05/12 16:12:39, hendrikw wrote:
> On 2015/05/12 15:11:31, halliwell wrote:
> > On 2015/05/12 02:12:29, hendrikw wrote:
> > > rename to flipped_over_y_axis?
> > 
> > I agree this isn't super clear, especially when "FLIP_HORIZONTAL" is one of
> the
> > possible outcomes of this function.  Otoh, it's named this way throughout
the
> > quad data structures ... thoughts?
> 
> I don't know, I would probably start the rename here, it's a bit clearer.

+1 Feel free to improve the name elsewhere

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:64: }
On 2015/05/12 15:11:31, halliwell wrote:
> On 2015/05/12 02:12:29, hendrikw wrote:
> > Should probably have GetXAxis() and GetYAxis() functions in the matrix that
> just
> > returns [(0,0), (1,0), (2,0)] (row, col) and [(0,1), (1,1), (2,1)].
> > 
> > That way you don't need to make a matrix copy, set the translation, or
> transform
> > a point (which is misleading since you're treating it as a vector)
> > 
> > You'll need to multiply the y components of each axis by -1 when
> > flipped_over_y_axis is true (since you wouldn't have a copy of the matrix)
> Yep, GetXAxis/GetYAxis seem like a clear way to express this.  I put the
> function here for now, not sure what the policy is for adding stuff to
Transform
> ... existing API seems quite minimal?

You could put it in cc/base/math_util if cc is the only place trying to do this.

[halliwell, 2015-05-12 22:22:20]

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate.cc
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:15: // Taken from SkMatrix44.
On 2015/05/12 16:12:39, hendrikw wrote:
> On 2015/05/12 15:11:31, halliwell wrote:
> > On 2015/05/12 02:12:29, hendrikw wrote:
> > > On 2015/05/12 01:24:15, vollick wrote:
> > > > On 2015/05/11 at 16:58:04, danakj wrote:
> > > > > can you TODO pointing to a bug to make this constant public (or some
> > public
> > > > API) on SkMatrix44 if we're going to depend on it, and follow up with
that
> > > > change in skia?
> > > > > 
> > > > > I actually don't see this constant in SkMatrix44 anywhere, can you
point
> > me
> > > to
> > > > it?
> > > > 
> > > > I believe that this from transform.cc (which also cites this constant),
> but
> > > the
> > > > comment is now bogus -- the constant no longer exists in skia. Perhaps
we
> > > could
> > > > expose it on Transform and get rid of this comment and the one in
> > > transform.cc?
> > > 
> > > Perhaps even replace it with std::numeric_limits<float>::epsilon()?
> > 
> > Definitely not that :)  numeric_limits::epsilon is the next representable
> number
> > greater than 1.  The tolerance required here is significantly greater.
> 
> If accuracy isn't important for your use case, then cool! As is, the smaller
the
> scale on the matrix, the larger the rotation that will be recognized as not
> OVERLAY_TRANSFORM_INVALID.
> 
> Personally, I always use stl's epsilon unless I've demonstrated a case where
fp
> errors to creep in (starting with an arbitrarily large epsilon is akin to
early
> optimizing code, imo).

Well ... this number is actually smaller than std::epsilon, not larger ... but
yes it's completely arbitrary and I am not going to defend it :)

Actually I think both may be too large.  Already computing cos(90) in floats is
about 4x this epsilon, and getting close to std::epsilon.

I've been trying to think what would make a logical number, not just an
arbitrary fudge factor.  One thought is that if |4000*x| < 1 for each x in the
comparisons below, then a fullscreen quad on a 4k display with this transform
would be within a pixel of being axis-aligned.  If that logic is correct, then
an error bound of 1/8000 might be defensible (within half a pixel everywhere on
a 4k display), and alleviate my concern about the other values discussed being
too large.

Thoughts?

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:53: bool flipped) {
On 2015/05/12 16:54:32, danakj wrote:
> On 2015/05/12 16:12:39, hendrikw wrote:
> > On 2015/05/12 15:11:31, halliwell wrote:
> > > On 2015/05/12 02:12:29, hendrikw wrote:
> > > > rename to flipped_over_y_axis?
> > > 
> > > I agree this isn't super clear, especially when "FLIP_HORIZONTAL" is one
of
> > the
> > > possible outcomes of this function.  Otoh, it's named this way throughout
> the
> > > quad data structures ... thoughts?
> > 
> > I don't know, I would probably start the rename here, it's a bit clearer.
> 
> +1 Feel free to improve the name elsewhere

Ok, renamed here and in TextureDrawQuad.

https://codereview.chromium.org/1135813005/diff/1/cc/output/overlay_candidate...
cc/output/overlay_candidate.cc:64: }
On 2015/05/12 16:54:32, danakj wrote:
> On 2015/05/12 15:11:31, halliwell wrote:
> > On 2015/05/12 02:12:29, hendrikw wrote:
> > > Should probably have GetXAxis() and GetYAxis() functions in the matrix
that
> > just
> > > returns [(0,0), (1,0), (2,0)] (row, col) and [(0,1), (1,1), (2,1)].
> > > 
> > > That way you don't need to make a matrix copy, set the translation, or
> > transform
> > > a point (which is misleading since you're treating it as a vector)
> > > 
> > > You'll need to multiply the y components of each axis by -1 when
> > > flipped_over_y_axis is true (since you wouldn't have a copy of the matrix)
> > Yep, GetXAxis/GetYAxis seem like a clear way to express this.  I put the
> > function here for now, not sure what the policy is for adding stuff to
> Transform
> > ... existing API seems quite minimal?
> 
> You could put it in cc/base/math_util if cc is the only place trying to do
this.

Moved to cc::MathUtil.

[hendrikw, 2015-05-12 22:42:37]

> Well ... this number is actually smaller than std::epsilon, not larger ... but
> yes it's completely arbitrary and I am not going to defend it :)

Oh yeah :)

> Actually I think both may be too large.  Already computing cos(90) in floats
is
> about 4x this epsilon, and getting close to std::epsilon.
> 
> I've been trying to think what would make a logical number, not just an
> arbitrary fudge factor.  One thought is that if |4000*x| < 1 for each x in the
> comparisons below, then a fullscreen quad on a 4k display with this transform
> would be within a pixel of being axis-aligned.  If that logic is correct, then
> an error bound of 1/8000 might be defensible (within half a pixel everywhere
on
> a 4k display), and alleviate my concern about the other values discussed being
> too large.
> 
> Thoughts?
> 

Yeah, that's an interesting way to look at it.  If you're ok with this type of
accuracy, maybe it doesn't matter.  I'd still use epsilon until a problem occurs
(too bad MathUtils::NearlyZero isn't properly templated), but whatever you
choose to do is probably cool.

[danakj, 2015-05-12 23:32:15]

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.cc#ne...
cc/base/math_util.cc:872: return gfx::Vector3dF(transform.matrix().get(0, 0),
getFloat() for all of these

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.h
File cc/base/math_util.h (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.h#new...
cc/base/math_util.h:245: // Returns vector that x axis (1,0,0) transforms to
under given transform
nit: period

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.h#new...
cc/base/math_util.h:248: // Returns vector that y axis (0,1,0) transforms to
under given transform
dittonit

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:566: rect.set_width(rect.width() / 2);
why the width/2 ?

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:586: rect.set_height(rect.height() / 2);
why height/2 ?

[halliwell, 2015-05-13 00:49:37]

On 2015/05/12 22:42:37, hendrikw wrote:
> > Well ... this number is actually smaller than std::epsilon, not larger ...
but
> > yes it's completely arbitrary and I am not going to defend it :)
> 
> Oh yeah :)
> 
> > Actually I think both may be too large.  Already computing cos(90) in floats
> is
> > about 4x this epsilon, and getting close to std::epsilon.
> > 
> > I've been trying to think what would make a logical number, not just an
> > arbitrary fudge factor.  One thought is that if |4000*x| < 1 for each x in
the
> > comparisons below, then a fullscreen quad on a 4k display with this
transform
> > would be within a pixel of being axis-aligned.  If that logic is correct,
then
> > an error bound of 1/8000 might be defensible (within half a pixel everywhere
> on
> > a 4k display), and alleviate my concern about the other values discussed
being
> > too large.
> > 
> > Thoughts?
> > 
> 
> Yeah, that's an interesting way to look at it.  If you're ok with this type of
> accuracy, maybe it doesn't matter.  I'd still use epsilon until a problem
occurs
> (too bad MathUtils::NearlyZero isn't properly templated), but whatever you
> choose to do is probably cool.

Moved to std::epsilon in latest patch set.  It's still pretty arbitrary, but the
previous value seemed dangerously small ... we'll certainly have to see how this
works out in practice.

[halliwell, 2015-05-13 00:49:50]

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.cc
File cc/base/math_util.cc (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.cc#ne...
cc/base/math_util.cc:872: return gfx::Vector3dF(transform.matrix().get(0, 0),
On 2015/05/12 23:32:15, danakj wrote:
> getFloat() for all of these

Done.

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.h
File cc/base/math_util.h (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.h#new...
cc/base/math_util.h:245: // Returns vector that x axis (1,0,0) transforms to
under given transform
On 2015/05/12 23:32:15, danakj wrote:
> nit: period

Done.

https://codereview.chromium.org/1135813005/diff/40001/cc/base/math_util.h#new...
cc/base/math_util.h:248: // Returns vector that y axis (0,1,0) transforms to
under given transform
On 2015/05/12 23:32:15, danakj wrote:
> dittonit

Done.

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:566: rect.set_width(rect.width() / 2);
On 2015/05/12 23:32:15, danakj wrote:
> why the width/2 ?

Following the same pattern as existing tests (you can see the same thing in
AllowPositiveScaleTransform). 
The way these tests are set up, it always asserts (in the validator's
CheckOverlaySupport) that the final rect is equal to kOverlayRect.  So if the
test has x-axis scale of 2, as this one does, it has to halve the input quad
width.

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:586: rect.set_height(rect.height() / 2);
On 2015/05/12 23:32:15, danakj wrote:
> why height/2 ?

Same reason as previous test.  In this case, y scale is 2.

[halliwell, 2015-05-13 15:56:10]

halliwell@chromium.org changed reviewers:
+ wfh@chromium.org

[halliwell, 2015-05-13 15:56:58]

+ @wfh for cc_messages change

[Will Harris, 2015-05-13 16:00:29]

content/common/cc_messages.h lgtm

[danakj, 2015-05-13 16:29:26]

LGTM

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
File cc/output/overlay_unittest.cc (right):

https://codereview.chromium.org/1135813005/diff/40001/cc/output/overlay_unitt...
cc/output/overlay_unittest.cc:566: rect.set_width(rect.width() / 2);
On 2015/05/13 00:49:50, halliwell wrote:
> On 2015/05/12 23:32:15, danakj wrote:
> > why the width/2 ?
> 
> Following the same pattern as existing tests (you can see the same thing in
> AllowPositiveScaleTransform). 
> The way these tests are set up, it always asserts (in the validator's
> CheckOverlaySupport) that the final rect is equal to kOverlayRect.  So if the
> test has x-axis scale of 2, as this one does, it has to halve the input quad
> width.

Ah, missed the scale2 down there.

[halliwell, 2015-05-13 17:07:10]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2015-05-13 17:07:10]

The patchset sent to the CQ was uploaded after l-g-t-m from vollick@chromium.org
Link to the patchset: https://codereview.chromium.org/1135813005/#ps60001
(title: "Nits, epsilon change, get->getFloat")

[commit-bot: I haz the power, 2015-05-13 17:09:03]

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

[commit-bot: I haz the power, 2015-05-13 17:27:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 17:27:40]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[halliwell, 2015-05-13 18:25:19]

halliwell@chromium.org changed reviewers:
+ sky@chromium.org

[halliwell, 2015-05-13 18:26:07]

On 2015/05/13 17:27:40, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

@sky - ptal at changes under mojo/ and components/surfaces

[sky, 2015-05-13 19:41:27]

LGTM

[halliwell, 2015-05-13 20:34:15]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2015-05-13 20:34:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, danakj@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1135813005/#ps80001
(title: "Fix mojo surfaces unit test, chase flipped rename to TextureQuadState")

[commit-bot: I haz the power, 2015-05-13 20:35:26]

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

[commit-bot: I haz the power, 2015-05-13 21:58:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-13 21:59:52]

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