cc: Add point-based UV coordinate on TextureLayer

cc: Add point-based UV coordinate on TextureLayer

Previously, the uv coordinates was using gfx::RectF as a struct.
This was limitating because it could not handle negative width
and heights. Soring them as 2 gfx::PointF for the Top-Left and
Bottom-Right points aleviate that restriction.
The current implementation of the phone UI rely on this feature.

BUG=168927
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177288

[Jerome, 2013-01-10 17:29:22]

Removed limitation on the UV coordinates for the TextureLayer (using negative
sized rects). This is used for the Tabswitcher.

[piman, 2013-01-10 17:52:06]

ui/compositor LGTM. Note, you probably have non-compiling or failing tests with
the change, you'll want to fix them.

https://codereview.chromium.org/11783094/diff/1/ui/gfx/size_base_impl.h
File ui/gfx/size_base_impl.h (right):

https://codereview.chromium.org/11783094/diff/1/ui/gfx/size_base_impl.h#newco...
ui/gfx/size_base_impl.h:19: // TODO(aelias): Remove these ifdefs for Android.
See http://crbug.com/168927
nit: remove TODO

[danakj, 2013-01-10 18:12:26]

i like the idea, tho not the abbreviations.

enne@ can you review this please?

https://codereview.chromium.org/11783094/diff/1/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/1/cc/video_layer_impl.cc#newcod...
cc/video_layer_impl.cc:227: gfx::PointF uvtl(0, 0);
uv_top_left

please dont use cryptic abbreviations lile this

https://codereview.chromium.org/11783094/diff/1/cc/video_layer_impl.cc#newcod...
cc/video_layer_impl.cc:228: gfx::PointF uvbr(texWidthScale, texHeightScale);
being a bit unfamiliar with this stuff, i wonder if a point (position) and
vector2d (scale) would make sense instead of keeping the rect paradigm.

[Jerome, 2013-01-10 19:21:55]

https://codereview.chromium.org/11783094/diff/1/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/1/cc/video_layer_impl.cc#newcod...
cc/video_layer_impl.cc:227: gfx::PointF uvtl(0, 0);
On 2013/01/10 18:12:26, danakj wrote:
> uv_top_left
> 
> please dont use cryptic abbreviations lile this

Done.

https://codereview.chromium.org/11783094/diff/1/cc/video_layer_impl.cc#newcod...
cc/video_layer_impl.cc:228: gfx::PointF uvbr(texWidthScale, texHeightScale);
Usually UV coordinates are expressed on a per vertex basis. So using points
feels actually more natural to to that than using origin+size.

https://codereview.chromium.org/11783094/diff/1/ui/gfx/size_base_impl.h
File ui/gfx/size_base_impl.h (right):

https://codereview.chromium.org/11783094/diff/1/ui/gfx/size_base_impl.h#newco...
ui/gfx/size_base_impl.h:19: // TODO(aelias): Remove these ifdefs for Android.
See http://crbug.com/168927
On 2013/01/10 17:52:06, piman wrote:
> nit: remove TODO

Done.

[danakj, 2013-01-11 02:21:51]

https://codereview.chromium.org/11783094/diff/5001/cc/nine_patch_layer_impl_u...
File cc/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/nine_patch_layer_impl_u...
cc/nine_patch_layer_impl_unittest.cc:87: gfx::RectF
texRect(texQuad->uv_top_left, uvSize);
you can use this from rect_f.h

UI_EXPORT RectF BoundingRect(const PointF& p1, const PointF& p2);

https://codereview.chromium.org/11783094/diff/5001/cc/nine_patch_layer_impl_u...
cc/nine_patch_layer_impl_unittest.cc:142: gfx::RectF
texRect(texQuad->uv_top_left, uvSize);
same here

https://codereview.chromium.org/11783094/diff/5001/cc/software_renderer.cc
File cc/software_renderer.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/software_renderer.cc#ne...
cc/software_renderer.cc:261: gfx::PointF uv_top_left =
gfx::ScalePoint(quad->uv_top_left, bitmap->width(), bitmap->height());
you could use BoundingRect here and keep the rest mostly as it was

https://codereview.chromium.org/11783094/diff/5001/cc/test/render_pass_test_c...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/test/render_pass_test_c...
cc/test/render_pass_test_common.cc:105: gfx::PointF(0, 0),
0.f and 1.f

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:94: uv_bottom_right = gfx::PointF();
should this be PointF(1.f, 1.f)

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:99: float u_scale = (uv_bottom_right.x() -
uv_top_left.x()) / rectF.width();
I might have messed this up, or you might see a more clean/more semantically
descriptive way to do this, but generally it's nice if we can write the math in
terms of points/vectors instead of breaking it down into components. So I was
thinking something like:

gfx::PointF old_top_left = uv_top_left;
gfx::PointF old_bottom_left = uv_bottom_left;

gfx::Vector2dF clipped_rect_offset = clippedRect.origin() - rectF.origin();

uv_top_left = old_top_left +
    gfx::ScaleVector2dF(
        old_bottom_right - old_top_left,
        clipped_rect_offset.x() / rectF.width(),
        clipped_rect_offset.y() / rectF.height());
uv_bottom_right = uv_top_left +
    gfx::ScaleVector2dF(
        old_bottom_right - old_top_left,
        clippedRect.width() / rectF.width(),
        clippedRect.height() / rectF.height());

WDYT?

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.h
File cc/texture_draw_quad.h (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.h#new...
cc/texture_draw_quad.h:24: const gfx::PointF& uv_top_left,
please pass PointF by value instead of by reference since it contains <= 2
floats

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.cc
File cc/texture_layer.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.cc#newcode40
cc/texture_layer.cc:40: , m_uvTopLeft(0, 0)
can you make these 0.f and 1.f while you're here?

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.h
File cc/texture_layer.h (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.h#newcode43
cc/texture_layer.h:43: void setUV(const gfx::PointF& topLeft, const gfx::PointF&
bottomRight);
pass by value please

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.cc#n...
cc/texture_layer_impl.cc:21: , m_uvTopLeft(0, 0)
0.f and 1.f please

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.h
File cc/texture_layer_impl.h (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.h#ne...
cc/texture_layer_impl.h:36: void setUVTopLeft(const gfx::PointF& topLeft) {
m_uvTopLeft = topLeft; }
pass by value please

https://codereview.chromium.org/11783094/diff/5001/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/video_layer_impl.cc#new...
cc/video_layer_impl.cc:227: gfx::PointF uvTopLeft(0, 0);
(0.f, 0.f) or () please

https://codereview.chromium.org/11783094/diff/5001/cc/video_layer_impl.cc#new...
cc/video_layer_impl.cc:239: gfx::PointF uvTopLeft(0, 0);
same

[Jerome, 2013-01-11 18:00:01]

https://codereview.chromium.org/11783094/diff/5001/cc/nine_patch_layer_impl_u...
File cc/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/nine_patch_layer_impl_u...
cc/nine_patch_layer_impl_unittest.cc:87: gfx::RectF
texRect(texQuad->uv_top_left, uvSize);
On 2013/01/11 02:21:51, danakj wrote:
> you can use this from rect_f.h
> 
> UI_EXPORT RectF BoundingRect(const PointF& p1, const PointF& p2);

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/nine_patch_layer_impl_u...
cc/nine_patch_layer_impl_unittest.cc:142: gfx::RectF
texRect(texQuad->uv_top_left, uvSize);
On 2013/01/11 02:21:51, danakj wrote:
> same here

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/software_renderer.cc
File cc/software_renderer.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/software_renderer.cc#ne...
cc/software_renderer.cc:261: gfx::PointF uv_top_left =
gfx::ScalePoint(quad->uv_top_left, bitmap->width(), bitmap->height());
On 2013/01/11 02:21:51, danakj wrote:
> you could use BoundingRect here and keep the rest mostly as it was

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/test/render_pass_test_c...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/test/render_pass_test_c...
cc/test/render_pass_test_common.cc:105: gfx::PointF(0, 0),
On 2013/01/11 02:21:51, danakj wrote:
> 0.f and 1.f

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:94: uv_bottom_right = gfx::PointF();
To keep the previous behavior uv_bottom_right should be 0,0 too.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:99: float u_scale = (uv_bottom_right.x() -
uv_top_left.x()) / rectF.width();
LGTM.
Any reason why there is no 
void Vector2dF::Scale(const Vector2dF& other)? That would allow to keep almost
everything vectors.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.h
File cc/texture_draw_quad.h (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.h#new...
cc/texture_draw_quad.h:24: const gfx::PointF& uv_top_left,
On 2013/01/11 02:21:51, danakj wrote:
> please pass PointF by value instead of by reference since it contains <= 2
> floats

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.cc
File cc/texture_layer.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.cc#newcode40
cc/texture_layer.cc:40: , m_uvTopLeft(0, 0)
On 2013/01/11 02:21:51, danakj wrote:
> can you make these 0.f and 1.f while you're here?

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.h
File cc/texture_layer.h (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer.h#newcode43
cc/texture_layer.h:43: void setUV(const gfx::PointF& topLeft, const gfx::PointF&
bottomRight);
On 2013/01/11 02:21:51, danakj wrote:
> pass by value please

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.cc
File cc/texture_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.cc#n...
cc/texture_layer_impl.cc:21: , m_uvTopLeft(0, 0)
On 2013/01/11 02:21:51, danakj wrote:
> 0.f and 1.f please

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.h
File cc/texture_layer_impl.h (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_layer_impl.h#ne...
cc/texture_layer_impl.h:36: void setUVTopLeft(const gfx::PointF& topLeft) {
m_uvTopLeft = topLeft; }
On 2013/01/11 02:21:51, danakj wrote:
> pass by value please

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/video_layer_impl.cc#new...
cc/video_layer_impl.cc:227: gfx::PointF uvTopLeft(0, 0);
On 2013/01/11 02:21:51, danakj wrote:
> (0.f, 0.f) or () please

Done.

https://codereview.chromium.org/11783094/diff/5001/cc/video_layer_impl.cc#new...
cc/video_layer_impl.cc:239: gfx::PointF uvTopLeft(0, 0);
On 2013/01/11 02:21:51, danakj wrote:
> same

Done.

[danakj, 2013-01-11 20:00:12]

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11783094/diff/5001/cc/texture_draw_quad.cc#ne...
cc/texture_draw_quad.cc:99: float u_scale = (uv_bottom_right.x() -
uv_top_left.x()) / rectF.width();
On 2013/01/11 18:00:01, Jerome wrote:
> LGTM.
> Any reason why there is no 
> void Vector2dF::Scale(const Vector2dF& other)? That would allow to keep almost
> everything vectors.

Ya, I thought about this too.. Ill file a bug

[Jerome, 2013-01-14 17:23:13]

ping :)

[enne (OOO), 2013-01-14 17:58:39]

https://codereview.chromium.org/11783094/diff/15001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11783094/diff/15001/cc/texture_draw_quad.cc#n...
cc/texture_draw_quad.cc:99: /*
...

https://codereview.chromium.org/11783094/diff/15001/cc/texture_draw_quad.cc#n...
cc/texture_draw_quad.cc:109: gfx::Vector2dF uv_scale(uv_bottom_right -
uv_top_left);
Are there unit tests for this texture clipping and uv recalculation? Can you add
one where there's a negative width and height?

[Jerome, 2013-01-14 23:47:07]

Added unittests.

https://codereview.chromium.org/11783094/diff/15001/cc/texture_draw_quad.cc
File cc/texture_draw_quad.cc (right):

https://codereview.chromium.org/11783094/diff/15001/cc/texture_draw_quad.cc#n...
cc/texture_draw_quad.cc:99: /*
Whoops...

https://codereview.chromium.org/11783094/diff/15001/cc/texture_draw_quad.cc#n...
cc/texture_draw_quad.cc:109: gfx::Vector2dF uv_scale(uv_bottom_right -
uv_top_left);
On 2013/01/14 17:58:40, enne wrote:
> Are there unit tests for this texture clipping and uv recalculation? Can you
add
> one where there's a negative width and height?

Done.

[danakj, 2013-01-15 01:48:42]

I'm quite happy with this CL modulo nits, but I'd like to be convinced of the
expected values in the tests. Can you please add some nice comments to explain
the math for how those values are the correct expected ones?

Thanks!

https://codereview.chromium.org/11783094/diff/20001/cc/draw_quad_unittest.cc
File cc/draw_quad_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:409: gfx::PointF uvTopLeftClipped(0.26f, 0.3f);
Can you add a comment here to justify these expected values? The more verbose
the better :)

https://codereview.chromium.org/11783094/diff/20001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:426: gfx::PointF uvTopLeftClipped(0.68f, 0.6f);
same here

https://codereview.chromium.org/11783094/diff/20001/cc/heads_up_display_layer...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/heads_up_display_layer...
cc/heads_up_display_layer_impl.cc:100: gfx::PointF uv_top_left(0, 0);
nit: 0.f

https://codereview.chromium.org/11783094/diff/20001/cc/heads_up_display_layer...
cc/heads_up_display_layer_impl.cc:101: gfx::PointF uv_bottom_right(1, 1);
nit: 1.f

https://codereview.chromium.org/11783094/diff/20001/cc/nine_patch_layer_impl_...
File cc/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/nine_patch_layer_impl_...
cc/nine_patch_layer_impl_unittest.cc:85: gfx::RectF texRect =
BoundingRect(texQuad->uv_top_left, texQuad->uv_bottom_right);
nit: gfx::BoundingRect

https://codereview.chromium.org/11783094/diff/20001/cc/nine_patch_layer_impl_...
cc/nine_patch_layer_impl_unittest.cc:138: gfx::RectF texRect =
BoundingRect(texQuad->uv_top_left, texQuad->uv_bottom_right);
nit: gfx::BoundingRect

https://codereview.chromium.org/11783094/diff/20001/cc/scrollbar_layer_impl.cc
File cc/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/scrollbar_layer_impl.c...
cc/scrollbar_layer_impl.cc:105: gfx::PointF uvTopLeft(0, 0);
nit: 0.f

https://codereview.chromium.org/11783094/diff/20001/cc/scrollbar_layer_impl.c...
cc/scrollbar_layer_impl.cc:106: gfx::PointF uvBottomRight(1, 1);
nit: 1.f

https://codereview.chromium.org/11783094/diff/20001/cc/software_renderer.cc
File cc/software_renderer.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/software_renderer.cc#n...
cc/software_renderer.cc:261: gfx::RectF uvRect =
gfx::ScaleRect(BoundingRect(quad->uv_top_left, quad->uv_bottom_right),
nit: gfx::BoundingRect

https://codereview.chromium.org/11783094/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/11783094/diff/20001/ui/compositor/layer.cc#ne...
ui/compositor/layer.cc:794: gfx::PointF uv_top_left(0, 0);
nit: 0.f

[Jerome, 2013-01-15 17:58:11]

All addressed.

https://codereview.chromium.org/11783094/diff/20001/cc/draw_quad_unittest.cc
File cc/draw_quad_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:409: gfx::PointF uvTopLeftClipped(0.26f, 0.3f);
On 2013/01/15 01:48:42, danakj wrote:
> Can you add a comment here to justify these expected values? The more verbose
> the better :)

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:426: gfx::PointF uvTopLeftClipped(0.68f, 0.6f);
On 2013/01/15 01:48:42, danakj wrote:
> same here

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/heads_up_display_layer...
File cc/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/heads_up_display_layer...
cc/heads_up_display_layer_impl.cc:100: gfx::PointF uv_top_left(0, 0);
On 2013/01/15 01:48:42, danakj wrote:
> nit: 0.f

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/heads_up_display_layer...
cc/heads_up_display_layer_impl.cc:101: gfx::PointF uv_bottom_right(1, 1);
On 2013/01/15 01:48:42, danakj wrote:
> nit: 1.f

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/nine_patch_layer_impl_...
File cc/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/nine_patch_layer_impl_...
cc/nine_patch_layer_impl_unittest.cc:85: gfx::RectF texRect =
BoundingRect(texQuad->uv_top_left, texQuad->uv_bottom_right);
On 2013/01/15 01:48:42, danakj wrote:
> nit: gfx::BoundingRect

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/nine_patch_layer_impl_...
cc/nine_patch_layer_impl_unittest.cc:138: gfx::RectF texRect =
BoundingRect(texQuad->uv_top_left, texQuad->uv_bottom_right);
On 2013/01/15 01:48:42, danakj wrote:
> nit: gfx::BoundingRect

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/scrollbar_layer_impl.cc
File cc/scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/scrollbar_layer_impl.c...
cc/scrollbar_layer_impl.cc:105: gfx::PointF uvTopLeft(0, 0);
On 2013/01/15 01:48:42, danakj wrote:
> nit: 0.f

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/scrollbar_layer_impl.c...
cc/scrollbar_layer_impl.cc:106: gfx::PointF uvBottomRight(1, 1);
On 2013/01/15 01:48:42, danakj wrote:
> nit: 1.f

Done.

https://codereview.chromium.org/11783094/diff/20001/cc/software_renderer.cc
File cc/software_renderer.cc (right):

https://codereview.chromium.org/11783094/diff/20001/cc/software_renderer.cc#n...
cc/software_renderer.cc:261: gfx::RectF uvRect =
gfx::ScaleRect(BoundingRect(quad->uv_top_left, quad->uv_bottom_right),
On 2013/01/15 01:48:42, danakj wrote:
> nit: gfx::BoundingRect

Done.

https://codereview.chromium.org/11783094/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/11783094/diff/20001/ui/compositor/layer.cc#ne...
ui/compositor/layer.cc:794: gfx::PointF uv_top_left(0, 0);
On 2013/01/15 01:48:42, danakj wrote:
> nit: 0.f

Done.

[danakj, 2013-01-15 22:39:09]

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc
File cc/draw_quad_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:398: // Original quad is at (30, 40) size 50*60
nit: comments are complete sentences including period and capitalization.

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:400: // after transform the quad is at (40, 60) size
50*60
nit: same thing

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:413: // UV value for 'B' vertex
nit: complete sentences for all these please.

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:426: gfx::PointF uvTopLeftClipped(0.26f, 0.3f);
I was thinking more about describing these expected values, rather than the
inputs - though the inputs will be very helpful in understanding the description
of these expected outputs. Like:

// This is A - B + D^2
// 0.5 - 0.6 + 0.2^2 = 0.26

[Jerome, 2013-01-15 23:14:34]

Added math to the unittest comments to explain the expected values.

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc
File cc/draw_quad_unittest.cc (right):

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:398: // Original quad is at (30, 40) size 50*60
On 2013/01/15 22:39:09, danakj wrote:
> nit: comments are complete sentences including period and capitalization.

Done.

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:400: // after transform the quad is at (40, 60) size
50*60
On 2013/01/15 22:39:09, danakj wrote:
> nit: same thing

Done.

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:413: // UV value for 'B' vertex
On 2013/01/15 22:39:09, danakj wrote:
> nit: complete sentences for all these please.

Done.

https://codereview.chromium.org/11783094/diff/28001/cc/draw_quad_unittest.cc#...
cc/draw_quad_unittest.cc:426: gfx::PointF uvTopLeftClipped(0.26f, 0.3f);
On 2013/01/15 22:39:09, danakj wrote:
> I was thinking more about describing these expected values, rather than the
> inputs - though the inputs will be very helpful in understanding the
description
> of these expected outputs. Like:
> 
> // This is A - B + D^2
> // 0.5 - 0.6 + 0.2^2 = 0.26

Done.

[danakj, 2013-01-15 23:21:36]

Thanks, LGTM

[commit-bot: I haz the power, 2013-01-16 18:45:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jscholler@chromium.org/11783094/33001

[commit-bot: I haz the power, 2013-01-16 18:46:16]

Presubmit check for 11783094-33001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit/compositor_bindings
    content/common/cc_messages.h

Presubmit checks took 5.9s to calculate.

[Jerome, 2013-01-16 18:52:51]

enne: I need a lgtm on
- webkit/compositor_bindings/

jschuh: I need an IPC security review
- content/common/cc_messages.h

Thanks

[jschuh, 2013-01-16 18:55:45]

ipc security lgtm

[enne (OOO), 2013-01-16 18:56:49]

lgtm

[commit-bot: I haz the power, 2013-01-16 19:08:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jscholler@chromium.org/11783094/33001

[commit-bot: I haz the power, 2013-01-16 20:08:17]

Retried try job too often on linux_clang for step(s) compile

[commit-bot: I haz the power, 2013-01-16 22:14:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jscholler@chromium.org/11783094/21023

[commit-bot: I haz the power, 2013-01-17 00:39:44]

Change committed as 177288