cc: Use DrawingFrame as state, not function argument

cc: Use DrawingFrame as state, not function argument

Most DirectRenderer functions take a DrawingFrame*, and (with one
exception), all of these functions use the DrawingFrame that is
allocated on the stack in DirectRenderer::DrawFrame.

Since this parameter is state of DirectRenderer, store it as state,
namely, a pointer to that stack variable, as
DirectRenderer::current_frame_.

To ensure that no functions touch current_frame_ while outside of
DirectRenderer::DrawFrame, set current_frame_ to nullptr outside of
that scope.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2680203005
Cr-Commit-Position: refs/heads/master@{#449561}
Committed: https://chromium.googlesource.com/chromium/src/+/59f7c65c351f8fd43229710e65413df8f3da7c72

[ccameron, 2017-02-08 22:12:28]

Description was changed from

==========
cc: Use DrawingFrame as state, not function argument

Most DirectRenderer functions take a DrawingFrame*, and (with one
exception), all of these functions use the DrawingFrame that is
allocated on the stack in DirectRenderer::DrawFrame.

Since this parameter is state of DirectRenderer, store it as state,
namely, a pointer to that stack variable, as
DirectRenderer::current_frame_.

To ensure that no functions touch current_frame_ while outside of
DirectRenderer::DrawFrame, set current_frame_ to nullptr outside of
that scope.

BUG=667966
==========

to

==========
cc: Use DrawingFrame as state, not function argument

Most DirectRenderer functions take a DrawingFrame*, and (with one
exception), all of these functions use the DrawingFrame that is
allocated on the stack in DirectRenderer::DrawFrame.

Since this parameter is state of DirectRenderer, store it as state,
namely, a pointer to that stack variable, as
DirectRenderer::current_frame_.

To ensure that no functions touch current_frame_ while outside of
DirectRenderer::DrawFrame, set current_frame_ to nullptr outside of
that scope.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-02-08 22:19:16]

ccameron@chromium.org changed reviewers:
+ enne@chromium.org

[ccameron, 2017-02-08 22:19:16]

For color conversion, I need to access DrawingFrame::device_color_space in a
bunch of places ...

I noticed that DrawingFrame is passed all over the place, giving the impression
that we may be using different DrawingFrames in each place.

In practice, with the exception of one function (which should probably be made
static, though that would require a bit less mechanical refactoring), we use the
same DrawingFrame in every instance.

[enne (OOO), 2017-02-08 22:32:40]

https://codereview.chromium.org/2680203005/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2680203005/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:241: current_frame_ = &frame;
In general I'm pretty fine with keeping the frame as a member instead of passing
it around everywhere.  That seems like good cleanup.

I'm a little <_< about member pointer to stack variable though.

What about just making current_frame_ a DrawingFrame and not a pointer, and
adding a reset() function or something that you could call here instead?

[ccameron, 2017-02-08 22:35:59]

https://codereview.chromium.org/2680203005/diff/1/cc/output/direct_renderer.cc
File cc/output/direct_renderer.cc (right):

https://codereview.chromium.org/2680203005/diff/1/cc/output/direct_renderer.c...
cc/output/direct_renderer.cc:241: current_frame_ = &frame;
On 2017/02/08 22:32:40, enne wrote:
> In general I'm pretty fine with keeping the frame as a member instead of
passing
> it around everywhere.  That seems like good cleanup.
> 
> I'm a little <_< about member pointer to stack variable though.
> 
> What about just making current_frame_ a DrawingFrame and not a pointer, and
> adding a reset() function or something that you could call here instead?

That was what I originally had in the patch, but then I really wanted it to
explode if you tried to access the current frame outside of DrawFrame(). So then
I added accessor functions and DCHECKs, and it seemed a bit much, so I started
with this.

[ccameron, 2017-02-08 22:48:38]

Mmh, on second (well, third, possibly fourth) thought, yah, I like that better.

[enne (OOO), 2017-02-09 22:23:18]

lgtm

[ccameron, 2017-02-10 06:48:02]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-02-10 06:48:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 07:40:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486709282946060,
"parent_rev": "987b6f51e04ababc70167151f7c9adfd861a238a", "commit_rev":
"59f7c65c351f8fd43229710e65413df8f3da7c72"}

[commit-bot: I haz the power, 2017-02-10 07:41:16]

Description was changed from

==========
cc: Use DrawingFrame as state, not function argument

Most DirectRenderer functions take a DrawingFrame*, and (with one
exception), all of these functions use the DrawingFrame that is
allocated on the stack in DirectRenderer::DrawFrame.

Since this parameter is state of DirectRenderer, store it as state,
namely, a pointer to that stack variable, as
DirectRenderer::current_frame_.

To ensure that no functions touch current_frame_ while outside of
DirectRenderer::DrawFrame, set current_frame_ to nullptr outside of
that scope.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use DrawingFrame as state, not function argument

Most DirectRenderer functions take a DrawingFrame*, and (with one
exception), all of these functions use the DrawingFrame that is
allocated on the stack in DirectRenderer::DrawFrame.

Since this parameter is state of DirectRenderer, store it as state,
namely, a pointer to that stack variable, as
DirectRenderer::current_frame_.

To ensure that no functions touch current_frame_ while outside of
DirectRenderer::DrawFrame, set current_frame_ to nullptr outside of
that scope.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2680203005
Cr-Commit-Position: refs/heads/master@{#449561}
Committed:
https://chromium.googlesource.com/chromium/src/+/59f7c65c351f8fd43229710e6541...
==========

[commit-bot: I haz the power, 2017-02-10 07:41:17]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/59f7c65c351f8fd43229710e6541...