cc: Moving anti-aliasing decision to parent compositor.

cc: Moving anti-aliasing decision to parent compositor.

Changed edge antialiasing flags in TileDrawQuad to edge flags and moved to SharedQuadState.
The quad cannot tell anymore if it needs antialiasing or not. The renderer will make that decision.
Moved blending decision from GLRenderer::drawQuad to GLRenderer::draw*Quad functions.

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

[reveman, 2013-02-26 01:49:28]

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h
File cc/draw_quad.h (right):

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode75
cc/draw_quad.h:75: // Is the left edge of this tile aligned with the originating
layer's
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode79
cc/draw_quad.h:79: // Is the top edge of this tile aligned with the originating
layer's
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode83
cc/draw_quad.h:83: // Is the right edge of this tile aligned with the
originating layer's
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode89
cc/draw_quad.h:89: // Is the bottom edge of this tile aligned with the
originating layer's
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode95
cc/draw_quad.h:95: // Is any edge of this tile aligned with the originating
layer's
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode97
cc/draw_quad.h:97: bool IsEdge() const {
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode98
cc/draw_quad.h:98: return IsLeftEdge() || IsTopEdge() || IsRightEdge() ||
IsBottomEdge();
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc
File cc/gl_renderer.cc (left):

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#oldcode287
cc/gl_renderer.cc:287: flushTextureQuadCache();
I think we should keep this flushTextureQuadCache() call here and only move the
setBlendEnabled() call into draw methods.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode856
cc/gl_renderer.cc:856: 
nit: remove this whitespace change.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode865
cc/gl_renderer.cc:865: setBlendEnabled(true);
is the setBlendEnabled(quad->ShouldDrawWithBlending() || useAA) line below not
enough?

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode946
cc/gl_renderer.cc:946: // Enable blending when the quad properties require it or
if we decided
nit: remove whitespace at end of line.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode968
cc/gl_renderer.cc:968: setBlendEnabled(quad->ShouldDrawWithBlending());
nit: remove whitespace at end of line.

[ernstm, 2013-02-26 18:10:56]

Made changes requested in message #2.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h
File cc/draw_quad.h (right):

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode75
cc/draw_quad.h:75: // Is the left edge of this tile aligned with the originating
layer's
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode79
cc/draw_quad.h:79: // Is the top edge of this tile aligned with the originating
layer's
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode83
cc/draw_quad.h:83: // Is the right edge of this tile aligned with the
originating layer's
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode89
cc/draw_quad.h:89: // Is the bottom edge of this tile aligned with the
originating layer's
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode95
cc/draw_quad.h:95: // Is any edge of this tile aligned with the originating
layer's
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode97
cc/draw_quad.h:97: bool IsEdge() const {
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode97
cc/draw_quad.h:97: bool IsEdge() const {
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode98
cc/draw_quad.h:98: return IsLeftEdge() || IsTopEdge() || IsRightEdge() ||
IsBottomEdge();
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/draw_quad.h#newcode98
cc/draw_quad.h:98: return IsLeftEdge() || IsTopEdge() || IsRightEdge() ||
IsBottomEdge();
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc
File cc/gl_renderer.cc (left):

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#oldcode287
cc/gl_renderer.cc:287: flushTextureQuadCache();
On 2013/02/26 01:49:28, David Reveman wrote:
> I think we should keep this flushTextureQuadCache() call here and only move
the
> setBlendEnabled() call into draw methods.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode856
cc/gl_renderer.cc:856: 
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove this whitespace change.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode856
cc/gl_renderer.cc:856: 
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove this whitespace change.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode865
cc/gl_renderer.cc:865: setBlendEnabled(true);
The line below is enough. This one is a leftover from an earlier version.
On 2013/02/26 01:49:28, David Reveman wrote:
> is the setBlendEnabled(quad->ShouldDrawWithBlending() || useAA) line below not
> enough?

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode946
cc/gl_renderer.cc:946: // Enable blending when the quad properties require it or
if we decided
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

https://codereview.chromium.org/12328098/diff/4013/cc/gl_renderer.cc#newcode968
cc/gl_renderer.cc:968: setBlendEnabled(quad->ShouldDrawWithBlending());
On 2013/02/26 01:49:28, David Reveman wrote:
> nit: remove whitespace at end of line.

Done.

[reveman, 2013-02-26 20:17:25]

lgtm

aelias, this causes subtle change to the software renderer output as we no
longer use SkXfermode::kSrcOver_Mode just because anti-aliasing is needed.

[aelias_OOO_until_Jul13, 2013-02-26 20:26:27]

I understand Slavi rebaselined the software renderer layout tests to not expect
exactly the same output for AA, added him to check if this looks correct.

[danakj, 2013-02-26 20:26:29]

https://codereview.chromium.org/12328098/diff/4015/cc/shared_quad_state.h
File cc/shared_quad_state.h (right):

https://codereview.chromium.org/12328098/diff/4015/cc/shared_quad_state.h#new...
cc/shared_quad_state.h:23: const gfx::Size& content_bounds,
nit: pass gfx::Size by value (also gfx::Rect but you don't have to change the
others)

https://codereview.chromium.org/12328098/diff/4015/content/common/cc_messages.h
File content/common/cc_messages.h (right):

https://codereview.chromium.org/12328098/diff/4015/content/common/cc_messages...
content/common/cc_messages.h:206: IPC_STRUCT_TRAITS_MEMBER(content_bounds)
Can you add this to the unit tests in cc_messages_unittest.cc?

The edge_aa fields will need to be removed from it also.

[ernstm, 2013-02-26 22:08:05]

Updated unit tests.
Passes gfx::Size by value.

https://codereview.chromium.org/12328098/diff/4015/cc/shared_quad_state.h
File cc/shared_quad_state.h (right):

https://codereview.chromium.org/12328098/diff/4015/cc/shared_quad_state.h#new...
cc/shared_quad_state.h:23: const gfx::Size& content_bounds,
Done

https://codereview.chromium.org/12328098/diff/4015/content/common/cc_messages.h
File content/common/cc_messages.h (right):

https://codereview.chromium.org/12328098/diff/4015/content/common/cc_messages...
content/common/cc_messages.h:206: IPC_STRUCT_TRAITS_MEMBER(content_bounds)
On 2013/02/26 20:26:29, danakj wrote:
> Can you add this to the unit tests in cc_messages_unittest.cc?
> 
> The edge_aa fields will need to be removed from it also.

Done.

[danakj, 2013-02-26 22:11:20]

https://codereview.chromium.org/12328098/diff/11002/content/common/cc_message...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/12328098/diff/11002/content/common/cc_message...
content/common/cc_messages_unittest.cc:50: EXPECT_EQ(a->visible_content_rect,
b->visible_content_rect);
Thanks! Can you also compare the content_bounds in a and b here?

[ernstm, 2013-02-26 22:28:26]

Added missing comparison of SharedQuadState::content_bounds to unit test.

https://codereview.chromium.org/12328098/diff/11002/content/common/cc_message...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/12328098/diff/11002/content/common/cc_message...
content/common/cc_messages_unittest.cc:50: EXPECT_EQ(a->visible_content_rect,
b->visible_content_rect);
On 2013/02/26 22:11:21, danakj wrote:
> Thanks! Can you also compare the content_bounds in a and b here?

Done.

[piman, 2013-02-26 22:48:25]

LGTM for content/ but you'll need a security person to review this.

[ernstm, 2013-02-26 23:03:11]

Chris, can you please do the security review for this CL?

[reveman, 2013-02-28 01:34:01]

+justin

cris or justin, any chance we can get a security review for this by tomorrow?

[jschuh, 2013-02-28 02:12:02]

On 2013/02/28 01:34:01, David Reveman wrote:
> +justin
> 
> cris or justin, any chance we can get a security review for this by tomorrow?

You could increase the odds by responding with a very brief explanation of the
processes communicating (e.g renderer -> gpu) and the content of the messages
being changed.

[reveman, 2013-02-28 02:44:19]

On 2013/02/28 02:12:02, Justin Schuh wrote:
> On 2013/02/28 01:34:01, David Reveman wrote:
> > +justin
> > 
> > cris or justin, any chance we can get a security review for this by
tomorrow?
> 
> You could increase the odds by responding with a very brief explanation of the
> processes communicating (e.g renderer -> gpu) and the content of the messages
> being changed.

This is used for communication between compositor instances (renderer ->
browser). This patch replaces the 4 flags in the cc::TileDrawQuad struct with a
gfx::Size member in the cc::SharedQuadState struct.

The reason for this is that we want this property to not just apply to
TileDrawQuads but all type of DrawQuads and a gfx::Size member with the layer's
content bounds is a more appropriate way to communicate this than 4 flags in
each DrawQuad struct.

[jschuh, 2013-02-28 04:06:57]

On 2013/02/28 02:44:19, David Reveman wrote:
> On 2013/02/28 02:12:02, Justin Schuh wrote:
> > On 2013/02/28 01:34:01, David Reveman wrote:
> > > +justin
> > > 
> > > cris or justin, any chance we can get a security review for this by
> tomorrow?
> > 
> > You could increase the odds by responding with a very brief explanation of
the
> > processes communicating (e.g renderer -> gpu) and the content of the
messages
> > being changed.
> 
> This is used for communication between compositor instances (renderer ->
> browser). This patch replaces the 4 flags in the cc::TileDrawQuad struct with
a
> gfx::Size member in the cc::SharedQuadState struct.
> 
> The reason for this is that we want this property to not just apply to
> TileDrawQuads but all type of DrawQuads and a gfx::Size member with the
layer's
> content bounds is a more appropriate way to communicate this than 4 flags in
> each DrawQuad struct.

Yeah, it looked like there was no change in attack surface. So, rubberstamp lgtm
for ipc security since this is status quo.

[commit-bot: I haz the power, 2013-03-04 13:17:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/12328098/18012

[commit-bot: I haz the power, 2013-03-04 17:07:24]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-04 17:35:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/12328098/18012

[commit-bot: I haz the power, 2013-03-04 18:33:32]

Change committed as 185934