cc: Eliminate tile.h's dependency on tile_manager.h

cc: Eliminate tile.h's dependency on tile_manager.h

Tile needs the raster mode and tile priority, which are
currently included via raster_worker_pool, which is included
by the tile manager. Since tile at the layer's level, it really
shouldn't include raster worker pool at any point.

This patch moves raster mode and tile priority to a separate
tile data file. This would also be a good spot for combining
content_rect and contents_scale into TileRect (which is something
I'd like to do)

BUG=253972
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210098

[vmpstr, 2013-06-24 18:45:45]

Please take a look.

[reveman, 2013-06-30 05:47:50]

Can we just remove RasterTaskMetadata instead? See my comments for some ideas of
how we might be able to accomplish this.

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:38: RasterMode raster_mode,
how about we just pass a SkDrawFilter* instead of a raster mode? we could then
move the definition of RasterTask back to the tile manager as TileRasterMode.

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:39: bool use_color_estimator,
this is the same for every task. how about we move this property to the
PicturePileImpl instead?

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:70: base::TimeDelta duration =
rendering_stats_->EndRecording(start_time);
can we make this consistent with raster and add PicturePileImpl::AnalysisStats?

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:74: analysis_.is_solid_color);
maybe move this to TileManager

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:76: analysis_.is_solid_color);
btw, s/AddTileAnalysisResult/AddAnalysisResult/ or move this to TileManager

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:85: "metadata",
TracedValue::FromValue(metadata_.AsValue().release()));
do we need all that data in this trace event? can some of it be moved to
TileManager::OnRasterTaskCompleted?

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:118: PicturePileImpl::RasterStats
raster_stats;
How about we remove |rendering_stats_| and add these parameters to
RasterWorkerPoolTaskImpl ctor:
PicturePileImpl::RasterStats* raster_stats,
PicturePileImpl::AnalysisStats* analysis_stats

that way all this logic can go into the tile manager.

[vmpstr, 2013-07-01 18:41:37]

I've address some of the comments. With regards to rendering stats, I'd like to
push that off to a different CL. I think we might have to rename
RenderingStatsInstrumentation or rename variables of that type to something that
involves analysis. Either that or we have to agree that analysis is part of
rendering. Please take another look.

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:38: RasterMode raster_mode,
On 2013/06/30 05:47:50, David Reveman wrote:
> how about we just pass a SkDrawFilter* instead of a raster mode? we could then
> move the definition of RasterTask back to the tile manager as TileRasterMode.

As it stands right now, TileManager isn't even aware that we're using skia since
all of the actual raster code is in the rwp and picture_pile_impl. I think it
would look a bit clumsy if we create a SkDrawFilter in tile manager, pass that
along, then create SkCanvas and attach the given filter. I very much prefer
passing the intent (such as a raster mode), instead of passing a particular
implementation (such as SkDrawFilter). We could pass two bools, like use lcd and
use low quality. However, I think raster mode is the right kind of abstraction
in this case... Wdyt?

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:39: bool use_color_estimator,
On 2013/06/30 05:47:50, David Reveman wrote:
> this is the same for every task. how about we move this property to the
> PicturePileImpl instead?

This will be removed in https://codereview.chromium.org/18370002/

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:70: base::TimeDelta duration =
rendering_stats_->EndRecording(start_time);
On 2013/06/30 05:47:50, David Reveman wrote:
> can we make this consistent with raster and add
PicturePileImpl::AnalysisStats?

This is RenderingStatsInstrumentation (which is a big class)... It would be
quite a big change to add a parallel class for analysis. Do you think it's worth
it? Maybe just rename the variable here to mean something better?

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:74: analysis_.is_solid_color);
On 2013/06/30 05:47:50, David Reveman wrote:
> maybe move this to TileManager

Removed in a different CL.

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:76: analysis_.is_solid_color);
On 2013/06/30 05:47:50, David Reveman wrote:
> btw, s/AddTileAnalysisResult/AddAnalysisResult/ or move this to TileManager

Done.

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:85: "metadata",
TracedValue::FromValue(metadata_.AsValue().release()));
On 2013/06/30 05:47:50, David Reveman wrote:
> do we need all that data in this trace event? can some of it be moved to
> TileManager::OnRasterTaskCompleted?

I think having this information on RunRasterOnThread is more useful than
OnRasterTaskCompleted for me. When I look at traces, and I look for what data is
associated with a tile, it's a lot easier for me to check the RunRasterOnThread
slice instead of the (much smaller, and on a different thread)
OnRasterTaskCompleted... That being said, I usually just use this information:
resolution, is tile in pending tree now bin, and raster mode (this isn't a part
of the metadata, but a separate parameter). It's not everything, but I think
that's most of it.

https://codereview.chromium.org/17625002/diff/5001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:118: PicturePileImpl::RasterStats
raster_stats;
On 2013/06/30 05:47:50, David Reveman wrote:
> How about we remove |rendering_stats_| and add these parameters to
> RasterWorkerPoolTaskImpl ctor:
> PicturePileImpl::RasterStats* raster_stats,
> PicturePileImpl::AnalysisStats* analysis_stats
> 
> that way all this logic can go into the tile manager.

It only really gets the time it takes to run the function (in analysis, I mean).
I can make this just a float parameter. Or do you prefer a struct?

[vmpstr, 2013-07-02 20:34:26]

I rebased this. PTAL.

[reveman, 2013-07-02 21:46:17]

I don't like the idea of a file named tile_data.h. I think the use of the
generic term 'data' is a good indication we're doing something wrong.

You could put RasterMode in a raster_mode.h file if you like but I'm ok keeping
it in raster_worker_pool.h too.

Just remove RasterTaskMetadata and add its members as parameters to
RasterWorkerPool::CreateRasterTask please, I hate that struct :)

[vmpstr, 2013-07-03 16:31:56]

PTAL. I've also removed some unnecessary includes in the headers

[reveman, 2013-07-03 16:56:12]

https://codereview.chromium.org/17625002/diff/13001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17625002/diff/13001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:41: int source_frame_number_) {
no "_" suffixes please. however, I think this would be better as a non-static
private member function of RasterWorkerPoolTaskImpl.
RasterWorkerPoolTaskImpl::DataAsValue() or maybe even just AsValue().

https://codereview.chromium.org/17625002/diff/13001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:83: "metadata",
lets remove all references to metadata. "data" here is fine unless you can come
up with something better.

[vmpstr, 2013-07-03 17:33:33]

PTAL.

https://codereview.chromium.org/17625002/diff/13001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17625002/diff/13001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:41: int source_frame_number_) {
On 2013/07/03 16:56:12, David Reveman wrote:
> no "_" suffixes please. however, I think this would be better as a non-static
> private member function of RasterWorkerPoolTaskImpl.
> RasterWorkerPoolTaskImpl::DataAsValue() or maybe even just AsValue().

Oops, sorry about the trailing underscores, it was a very automated change. I
moved this to be DataAsValue (I prefer to keep Data there, since we're not
returning all of the information about the task here)

https://codereview.chromium.org/17625002/diff/13001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:83: "metadata",
On 2013/07/03 16:56:12, David Reveman wrote:
> lets remove all references to metadata. "data" here is fine unless you can
come
> up with something better.

Done.

[reveman, 2013-07-03 18:06:55]

lgtm

[commit-bot: I haz the power, 2013-07-03 19:08:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/17625002/19001

[commit-bot: I haz the power, 2013-07-04 01:44:18]

Change committed as 210098