cc: Turn DrawQuad into a struct-like class with public data members.

cc: Turn DrawQuad into a struct-like class with public data members.

We add a SetAll() method that sets everything to keep the compile-time guard
that verifies everything gets set from all appropriate callsites. This replaces
the constructor which now takes no arguments at all.

We will replace the constructor with an overriding SetAll() for
each subclass of DrawQuads and make them into structs as well.

Covered by existing tests.

TBR=aelias
BUG=152337
NOTRY=true

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168460

[danakj, 2012-11-16 22:56:02]

This patch does:

- Replace DrawQuad::DrawQuad() with DrawQuad::SetAll()
- Have each subclass of DrawQuad call SetAll instead of the constructor.
- Remove a lot of ()'s as things are now public data and not methods.

[aelias_OOO_until_Jul13, 2012-11-16 23:11:54]

I preferred having the base class implicitly set visibleRect.  The SetAll calls
are unreadable with three rects.

[danakj, 2012-11-16 23:16:30]

On 2012/11/16 23:11:54, aelias wrote:
> I preferred having the base class implicitly set visibleRect.  The SetAll
calls
> are unreadable with three rects.

But when restoring from serialization, we need to set the visibleRect also at
the SetAll time. The idea is to get compiler verification that we're indeed
setting every field, so that if a new field is added some time, our
serialization won't compile without it (especially since it will live so far
away in content/common).

I can add a temp called visibleRect in each subclass and pass that instead so
that it's clear, similar to opaqueRect. Would that work?

[aelias_OOO_until_Jul13, 2012-11-16 23:17:28]

Yes, adding the temp sounds good, thanks!

[danakj, 2012-11-16 23:18:07]

On 2012/11/16 23:16:30, danakj wrote:
> On 2012/11/16 23:11:54, aelias wrote:
> > I preferred having the base class implicitly set visibleRect.  The SetAll
> calls
> > are unreadable with three rects.
> 
> But when restoring from serialization, we need to set the visibleRect also at
> the SetAll time. The idea is to get compiler verification that we're indeed
> setting every field, so that if a new field is added some time, our
> serialization won't compile without it (especially since it will live so far
> away in content/common).
> 
> I can add a temp called visibleRect in each subclass and pass that instead so
> that it's clear, similar to opaqueRect. Would that work?

I'm playing with subclasses now, and I am thinking to do like this, for
checkerboard quad:

  void SetNew(const SharedQuadState* shared_quad_state,
              Material material,
              gfx::Rect rect,
              SkColor color);

  void SetAll(const SharedQuadState* shared_quad_state,
              Material material,
              gfx::Rect rect,
              gfx::Rect opaque_rect,
              gfx::Rect visible_rect,
              bool needs_blending,
              SkColor color);

So you have a minimal way to make quads in the child, but a compile-time check
that everything is being set as well when serializing.

[danakj, 2012-11-16 23:21:48]

On 2012/11/16 23:17:28, aelias wrote:
> Yes, adding the temp sounds good, thanks!

Done.

[aelias_OOO_until_Jul13, 2012-11-16 23:22:35]

Seems okay, but will the serializers be call SetAll on the base class at all,
anyway?  It seems they'll only be operating on the subclasses.

[danakj, 2012-11-16 23:25:13]

On 2012/11/16 23:22:35, aelias wrote:
> Seems okay, but will the serializers be call SetAll on the base class at all,
> anyway?  It seems they'll only be operating on the subclasses.

Right, it's protected in base class. The subclass' SetAll() passes on the things
the base class wants to its SetAll(), similar to constructors.

[aelias_OOO_until_Jul13, 2012-11-16 23:25:16]

Anyway, LGTM for this patch.

[danakj, 2012-11-16 23:27:09]

On Fri, Nov 16, 2012 at 6:25 PM,  <aelias@chromium.org> wrote:
> Anyway, LGTM for this patch.

Thanks :)

[commit-bot: I haz the power, 2012-11-17 16:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418047/2082

[commit-bot: I haz the power, 2012-11-17 18:16:19]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-17 18:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418047/2082

[commit-bot: I haz the power, 2012-11-17 19:29:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11418047/2082

[commit-bot: I haz the power, 2012-11-17 19:29:52]

Change committed as 168460