Choose Largest DrawQuad Type No Longer Depended on Compiler Flag

Choose largest DrawQuad type, no longer dependent on compiler flag

Use template to choose between RenderPassDrawQuad and
StreamVideoDrawQuad for LargestDrawQuad.

BUG=429552
Committed: https://crrev.com/fe7b51ef3bda41dcea60e6fcda95ad07e720423b
Cr-Commit-Position: refs/heads/master@{#302833}

[weiliangc, 2014-11-03 18:02:26]

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

[weiliangc, 2014-11-03 18:05:41]

Oh templates are cool.

[danakj, 2014-11-03 18:14:21]

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:8: #include "cc/quads/render_pass_draw_quad.h"
including this includes GL headers which was breaking the build before. is this
going to be okay? Or should we make a helper function and move all this to a .cc
file instead?

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:15: // larger.
TODO to use std::conditional when we can do that :)

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:16: template <bool RenderPassDrawQuadLarger>
render_pass_draw_quad_is_larger

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:18: typedef RenderPassDrawQuad Type;
using Type = RenderPassDrawQuad

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:23: typedef StreamVideoDrawQuad Type;
using Type = StreamVideoDrawQuad

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:26: typedef
LargerQuadOfEitherRenderPassOrStreamVideo<(
using kLargestDrawQuad = ...

[jbroman, 2014-11-03 18:28:25]

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

[jbroman, 2014-11-03 18:28:26]

Just a drive-by comment; feel free to ignore me. :)

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:18: typedef RenderPassDrawQuad Type;
If you don't mind drive-by...

Since you only ever need the size of this, not the type itself (as far as I can
tell), why not have the template provide the size rather than the type, and put
"size_t Value = sizeof(RenderPassDrawQuad)" here, and similar below?

Considering one step further, with optimizations on, the following even has no
runtime cost (with optimizations off it may emit a static initializer, at least
until C++14 where this could be constexpr):

#include <algorithm>
const size_t kLargestDrawQuadSize = std::max(sizeof(RenderPassDrawQuad),
sizeof(StreamVideoDrawQuad));

If you were concerned about the static initializer in non-release builds, you
could instead write a function with this as its body (which would be trivially
inlined in optimized builds).

[jbroman, 2014-11-03 18:29:29]

jbroman@chromium.org changed reviewers:
- jbroman@chromium.org

[weiliangc, 2014-11-03 18:42:46]

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:8: #include "cc/quads/render_pass_draw_quad.h"
On 2014/11/03 18:14:21, danakj wrote:
> including this includes GL headers which was breaking the build before. is
this
> going to be okay? Or should we make a helper function and move all this to a
.cc
> file instead?

Have to include for sizeof(). I don't think it can be moved to source file
because that would move entire definition of kLargestDrawQuad.

[jbroman, 2014-11-03 18:44:27]

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:8: #include "cc/quads/render_pass_draw_quad.h"
On 2014/11/03 18:14:21, danakj wrote:
> including this includes GL headers which was breaking the build before. is
this
> going to be okay? Or should we make a helper function and move all this to a
.cc
> file instead?

I'm curious how this broke the build before; do you have a link?

Anyone who included this header (before this change, even) would need to also
include render_pass_draw_quad.h (in order to have a complete definition of
RenderPassDrawQuad, which it would need to evaluate its size).

[danakj, 2014-11-03 18:50:36]

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h
File cc/quads/largest_draw_quad.h (right):

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:8: #include "cc/quads/render_pass_draw_quad.h"
On 2014/11/03 18:44:27, jbroman wrote:
> On 2014/11/03 18:14:21, danakj wrote:
> > including this includes GL headers which was breaking the build before. is
> this
> > going to be okay? Or should we make a helper function and move all this to a
> .cc
> > file instead?
> 
> I'm curious how this broke the build before; do you have a link?
> 
> Anyone who included this header (before this change, even) would need to also
> include render_pass_draw_quad.h (in order to have a complete definition of
> RenderPassDrawQuad, which it would need to evaluate its size).

It was a matter of include files including include files and somewhere including
a GL header than conflicts with what this included on mac builds I think. We hit
it again on one of wei's patches maybe she can find the patch set. But if you
include this header in render_pass.h you can see it on trybots for yourself.

https://codereview.chromium.org/698053002/diff/1/cc/quads/largest_draw_quad.h...
cc/quads/largest_draw_quad.h:8: #include "cc/quads/render_pass_draw_quad.h"
On 2014/11/03 18:42:46, weiliangc wrote:
> On 2014/11/03 18:14:21, danakj wrote:
> > including this includes GL headers which was breaking the build before. is
> this
> > going to be okay? Or should we make a helper function and move all this to a
> .cc
> > file instead?
> 
> Have to include for sizeof(). I don't think it can be moved to source file
> because that would move entire definition of kLargestDrawQuad.

Ya, it would mean a function returning a size instead of a type. Similar to what
Jeremy suggested, though I guess you can just use std::max instead of a
templated magics.

[weiliangc, 2014-11-03 21:30:13]

Move to source file with function returning largest quad size.

[danakj, 2014-11-04 20:46:49]

LGTM

[weiliangc, 2014-11-04 20:50:08]

weiliangc@chromium.org changed reviewers:
+ kenrb@chromium.org

[weiliangc, 2014-11-04 20:50:24]

Add kenrb for cc_messages review.

[kenrb, 2014-11-05 17:08:21]

ipc lgtm

[weiliangc, 2014-11-05 17:30:15]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-11-05 17:31:12]

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

[commit-bot: I haz the power, 2014-11-05 18:20:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-05 18:21:07]

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