cc: Encapsulate rastering related data in RasteringInfo.

cc: Encapsulate rastering related data in RasteringInfo.

This patches adds new struct which encapsulates all rastering related
information. So all the functions that need rastering related data,
the single variable is passed to use data from.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[prashant.n, 2016-04-06 10:35:38]

Description was changed from

==========
cc: Encapsulate rastering related data in RasteringInfo.

This patches adds new struct which encapsulates all rastering related
information. So all the functions that need rastering related data,
the single variable is passed to use data from.

BUG=599863
==========

to

==========
cc: Encapsulate rastering related data in RasteringInfo.

This patches adds new struct which encapsulates all rastering related
information. So all the functions that need rastering related data,
the single variable is passed to use data from.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-04-06 10:41:08]

prashant.n@samsung.com changed reviewers:
+ ericrk@chromium.org, reveman@chromium.org, vmpstr@chromium.org

[prashant.n, 2016-04-06 10:41:08]

wip - do let me know your kind opinion.

I'll modify in patchset2 all the functions like AcquireBufferForRaster,
ReleaseBufferForRaster, Playback to take single argument RasteringInfo. This
will simplify the functions used on tasks.

[reveman, 2016-04-06 12:07:39]

I'm failing to see the benefit of this. RasterTaskImpl is already an appropriate
place to store  all this state imo.

[prashant.n, 2016-04-06 14:26:46]

On 2016/04/06 12:07:39, reveman wrote:
> I'm failing to see the benefit of this. RasterTaskImpl is already an
appropriate
> place to store  all this state imo.

Yes, but RasterTaskImpl also contains few members derived from Task, so I
thought to have separate struct. But as I'm thinking to pass as pointer, there
would not be a problem.

Main intent of this patch is -
1. Make argument passing simplified to just one argument passed.
2. Make all functions which are getting called on worker, not to return any
value. Just store that to the one argument which is passed; so that later we can
encapsulate easily in closures.

[prashant.n, 2016-04-06 14:34:09]

> 2. Make all functions which are getting called on worker, not to return any

In RunOnWorkerThread().

[vmpstr, 2016-04-06 18:03:25]

On 2016/04/06 14:26:46, prashant.n wrote:
> On 2016/04/06 12:07:39, reveman wrote:
> > I'm failing to see the benefit of this. RasterTaskImpl is already an
> appropriate
> > place to store  all this state imo.
> 
> Yes, but RasterTaskImpl also contains few members derived from Task, so I
> thought to have separate struct. But as I'm thinking to pass as pointer, there
> would not be a problem.
> 
> Main intent of this patch is -
> 1. Make argument passing simplified to just one argument passed.
> 2. Make all functions which are getting called on worker, not to return any
> value. Just store that to the one argument which is passed; so that later we
can
> encapsulate easily in closures.

As the code stands now, I tend to agree with reveman@ in that I don't see a
benefit of doing this. In fact, this adds more indirection. I would see this
being useful if we can use a lot of default values and only set some, but this
struct needs all of the members set.  

Can you elaborate on the second point a little bit? Maybe that will make it
clear why this is a good change.

[prashant.n, 2016-04-06 22:33:27]

> Can you elaborate on the second point a little bit? Maybe that will make it
> clear why this is a good change.

For avoiding indirection we can store the members in RaterTaskImpl itself and
send that as an argument.

2nd point is RasterTaskImpl itself maintains all the states, modifications than
locals in functions being called in RunOnWorkerThread().

e.g. In case of one copy,

RunOnWorkerThread() {
raster_buffer_->Playback(this);
}

RasterBufferImpl::Playback(RasterTaskImpl* rt) {
worker_pool_->PlaybackAndCopyOnWorkerThread(resource_, &lock_, rt);
}

This way it will simplify passing of rastering info further deep in calls,
saving number of arguments being sent in a function call. This way
RasterTaskImpl itself will maintain state of all variables.

Then it becomes easier to encapsulate those in a closure and in future have
these closures splitted (multiple work items) if needed. So in
RunOnWorkerThread(), it can execute work items sequentially to get the needed
job done by Task.