Don't invert contentsDeviceTransform twice when rendering render passes.

Don't invert contentsDeviceTransform twice when rendering render passes.

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

[jamesr, 2012-11-06 23:53:43]

http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc
File cc/gl_renderer.cc (right):

http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc#newcode518
cc/gl_renderer.cc:518: WebTransformationMatrix deviceMatrix =
(frame.windowMatrix * frame.projectionMatrix * quadRectMatrix).to2dTransform();
what's the reason for the rename?

http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc#newcode725
cc/gl_renderer.cc:725: WebTransformationMatrix deviceMatrix =
WebTransformationMatrix(frame.windowMatrix * frame.projectionMatrix *
quad->quadTransform()).to2dTransform();
this rename seems somewhat arbitrary

http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.h
File cc/gl_renderer.h (right):

http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.h#newcode94
cc/gl_renderer.h:94: scoped_ptr<ScopedTexture>
drawBackgroundFilters(DrawingFrame&, const RenderPassDrawQuad*, const
WebKit::WebFilterOperations&, const WebKit::WebTransformationMatrix&, const
WebKit::WebTransformationMatrix&);
please provide names for the last two parameters. we omit parameter names when
they are clear from the type (i.e. "DrawingFrame& is probably a drawing frame),
but with two matrices it's impossible to guess the meaning from just the
signature

[shawnsingh, 2012-11-07 00:05:15]

Personally I think it's better to keep the original name, it makes it clear (for
those of us who have gotten familiar with it, at least?) that it transforms from
content space to device coordinate space.   "content" here is probably an
important qualifier to keep in the variable name.

As for renaming between Transform --> Matrix, someone could argue that there is
a distinction, perhaps not all matrices are meaningful transforms.  but for our
purposes, at least for now, I think the names are interchangable and maybe we
should not bother change it when we don't have to?

But I do like the cached inverse computation; it doesn't seem to increase
error-proneness or reduce readability to me.

[slavi, 2012-11-07 00:31:40]

On 2012/11/06 23:53:43, jamesr wrote:
> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc
> File cc/gl_renderer.cc (right):
> 
> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc#newcode518
> cc/gl_renderer.cc:518: WebTransformationMatrix deviceMatrix =
> (frame.windowMatrix * frame.projectionMatrix *
quadRectMatrix).to2dTransform();
> what's the reason for the rename?

No particular reason besides contentsDeviceTransform and
contentsDeviceTransformInv get a bit mouthful. I'm open to suggestions for
better names. contentMatrix?

 "There are only two hard problems in Computer Science:
  cache invalidation and naming things."

  -- Phil Karlton

> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc#newcode725
> cc/gl_renderer.cc:725: WebTransformationMatrix deviceMatrix =
> WebTransformationMatrix(frame.windowMatrix * frame.projectionMatrix *
> quad->quadTransform()).to2dTransform();
> this rename seems somewhat arbitrary

Acknoledged.

> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.h
> File cc/gl_renderer.h (right):
> 
> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.h#newcode94
> cc/gl_renderer.h:94: scoped_ptr<ScopedTexture>
> drawBackgroundFilters(DrawingFrame&, const RenderPassDrawQuad*, const
> WebKit::WebFilterOperations&, const WebKit::WebTransformationMatrix&, const
> WebKit::WebTransformationMatrix&);
> please provide names for the last two parameters. we omit parameter names when
> they are clear from the type (i.e. "DrawingFrame& is probably a drawing
frame),
> but with two matrices it's impossible to guess the meaning from just the
> signature

[danakj, 2012-11-07 00:37:37]

On Tue, Nov 6, 2012 at 7:31 PM,  <skaslev@chromium.org> wrote:
> On 2012/11/06 23:53:43, jamesr wrote:
>>
>> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc
>> File cc/gl_renderer.cc (right):
>
>
>>
>> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc#newcode518
>> cc/gl_renderer.cc:518: WebTransformationMatrix deviceMatrix =
>> (frame.windowMatrix * frame.projectionMatrix *
>
> quadRectMatrix).to2dTransform();
>>
>> what's the reason for the rename?
>
>
> No particular reason besides contentsDeviceTransform and
> contentsDeviceTransformInv get a bit mouthful. I'm open to suggestions for
> better names. contentMatrix?

I prefer the full names: contentsDeviceTransformInverse

>  "There are only two hard problems in Computer Science:
>   cache invalidation and naming things."
>
>   -- Phil Karlton
>
>
>>
>> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.cc#newcode725
>> cc/gl_renderer.cc:725: WebTransformationMatrix deviceMatrix =
>> WebTransformationMatrix(frame.windowMatrix * frame.projectionMatrix *
>> quad->quadTransform()).to2dTransform();
>> this rename seems somewhat arbitrary
>
>
> Acknoledged.
>
>
>> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.h
>> File cc/gl_renderer.h (right):
>
>
>> http://codereview.chromium.org/11361129/diff/1/cc/gl_renderer.h#newcode94
>> cc/gl_renderer.h:94: scoped_ptr<ScopedTexture>
>> drawBackgroundFilters(DrawingFrame&, const RenderPassDrawQuad*, const
>> WebKit::WebFilterOperations&, const WebKit::WebTransformationMatrix&,
>> const
>> WebKit::WebTransformationMatrix&);
>> please provide names for the last two parameters. we omit parameter names
>> when
>> they are clear from the type (i.e. "DrawingFrame& is probably a drawing
>
> frame),
>>
>> but with two matrices it's impossible to guess the meaning from just the
>> signature
>
>
>
> http://codereview.chromium.org/11361129/
>
> --
>
>

[slavi, 2012-11-07 01:02:14]

PTAL

The Transform -> Matrix is consistent with matrix naming in DrawingFrame:

http://code.google.com/searchframe#OAMlx_jo-ck/src/cc/direct_renderer.h&exact...

[danakj, 2012-11-07 01:10:40]

It's funny, because the various renderers are the only place left where we have
Matrix in variable names. Everywhere else we use Transform, and we've been
slowly converting variables with Matrix to Transform over time.

I don't feel strongly, but I do feel like we've been converging toward
*Transform.

Other than the Matrix vs Transform bit, I like the change. I defer to @enne
about the suffix bits.

[slavi, 2012-11-07 01:31:25]

Then Transform it is.

Coming from a gamedev background, Transform makes sense rather than Matrix.
Although, I have a thing for short names. I prefer projXform and projXformInv to
projectionTransform and projectionTranformInverse.

[danakj, 2012-11-07 01:35:18]

lgtm but bots hate me still. so you'll need enne/jamesr, or tbr one of them

http://codereview.chromium.org/11361129/diff/8/cc/software_renderer.cc
File cc/software_renderer.cc (left):

http://codereview.chromium.org/11361129/diff/8/cc/software_renderer.cc#oldcod...
cc/software_renderer.cc:261: 
nit: extraneous leftover change

[jamesr, 2012-11-07 01:37:37]

On 2012/11/07 01:31:25, slavi wrote:
> Then Transform it is.
> 
> Coming from a gamedev background, Transform makes sense rather than Matrix.
> Although, I have a thing for short names. I prefer projXform and projXformInv
to
> projectionTransform and projectionTranformInverse.

We've tended to use full words (since that's WebKit style) and I think it's a
net win.  It takes longer to write and a bit longer to read, but it's much
easier to scan and debug.

lgtm

[commit-bot: I haz the power, 2012-11-07 01:45:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11361129/8

[commit-bot: I haz the power, 2012-11-07 04:13:03]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-07 18:42:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/11361129/15003

[commit-bot: I haz the power, 2012-11-08 02:21:48]

Change committed as 166586