cc: Added raster source.

cc: Added raster source.

This patch adds a raster source that is an opaque interface
for picture pile impl. It also removes the picture pile base
ref count and adds it to the raster source. This makes it
possible to use a member picture pile, instead of a ref
pointer with one owner.

What is missing here is the next step: removing picture pile
base in favour of keeping shared data on the pile and copying
it over. This should allow us to clean up the data and only
share what is actually shared.

R=danakj, enne, reveman

Committed: https://crrev.com/394e2b51f40cbc5a522300073c7c058f95889901
Cr-Commit-Position: refs/heads/master@{#301667}

[vmpstr, 2014-10-24 21:23:29]

vmpstr@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, reveman@chromium.org

[vmpstr, 2014-10-24 21:25:01]

PTAL

[vmpstr, 2014-10-24 21:25:48]

On 2014/10/24 21:25:01, vmpstr wrote:
> PTAL

Let me know, if you feel like I should include the picture pile base split in
here as well. I haven't done the work yet, since I feel it should be a separate
change, but I can try and squeeze it in here.

[enne (OOO), 2014-10-24 22:27:35]

lgtm

https://codereview.chromium.org/666273002/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/666273002/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_unittest.cc:89: scoped_ptr<TestPicturePile> pile_;
This could probably just be a member too...

https://codereview.chromium.org/666273002/diff/80001/cc/resources/raster_sour...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/80001/cc/resources/raster_sour...
cc/resources/raster_source.h:27: // Raster a subrect of this PicturePileImpl
into the given canvas. It is
"PicturePileImpl"

https://codereview.chromium.org/666273002/diff/80001/cc/resources/raster_sour...
cc/resources/raster_source.h:30: // rasterizing to the stats if the respective
pointer is not NULL. When
Maybe the slow-down-raster-scale-factor bit should be a comment on the
PicturePileImpl interface?

[vmpstr, 2014-10-24 23:15:23]

PTAL

https://codereview.chromium.org/666273002/diff/80001/cc/resources/picture_pil...
File cc/resources/picture_pile_unittest.cc (right):

https://codereview.chromium.org/666273002/diff/80001/cc/resources/picture_pil...
cc/resources/picture_pile_unittest.cc:89: scoped_ptr<TestPicturePile> pile_;
On 2014/10/24 22:27:35, enne wrote:
> This could probably just be a member too...

I did that first and then I thought "but that changes behaviour, since we don't
reset it back to the default state". I'll change it and see if the tests are
happy

https://codereview.chromium.org/666273002/diff/80001/cc/resources/raster_sour...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/80001/cc/resources/raster_sour...
cc/resources/raster_source.h:27: // Raster a subrect of this PicturePileImpl
into the given canvas. It is
On 2014/10/24 22:27:35, enne wrote:
> "PicturePileImpl"

Oops. Done.

https://codereview.chromium.org/666273002/diff/80001/cc/resources/raster_sour...
cc/resources/raster_source.h:30: // rasterizing to the stats if the respective
pointer is not NULL. When
On 2014/10/24 22:27:35, enne wrote:
> Maybe the slow-down-raster-scale-factor bit should be a comment on the
> PicturePileImpl interface?

Done.

[reveman, 2014-10-27 14:39:46]

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:12: #include
"cc/debug/rendering_stats_instrumentation.h"
Can you forward declare RenderingStatsInstrumentation instead?

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:13: #include
"third_party/skia/include/core/SkCanvas.h"
forward declare SkCanvas instead?

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:25: };
Seems a bit silly to keep this struct when it's only used for solid color
detection. Why don't we rename AnalyzeInRect to IsSolidColor() instead?

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:31: virtual void RasterToBitmap(
hm, ToBitmap doesn't seem right. RasterToCanvas? or just Raster? or maybe even
ToCanvas? Or PlaybackToCanvas() to be consistent with the RasterBuffer API.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:35: RenderingStatsInstrumentation*
rendering_stats_instrumentation) const = 0;
Do we need to be passing this RenderingStatsInstrumentation pointer around?
Would be nice if that could just be handled by the implementation instead.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:53: virtual bool RasterCoversRect(const gfx::Rect&
content_rect,
CoversRect? RasterSource::CoversRect looks better to me than
RasterSource::RasterCoversRect.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:57: // fiend text.
typo: fiend

[vmpstr, 2014-10-27 15:55:11]

PTAL

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:12: #include
"cc/debug/rendering_stats_instrumentation.h"
On 2014/10/27 14:39:46, reveman wrote:
> Can you forward declare RenderingStatsInstrumentation instead?

Done.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:13: #include
"third_party/skia/include/core/SkCanvas.h"
On 2014/10/27 14:39:46, reveman wrote:
> forward declare SkCanvas instead?

Done.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:25: };
On 2014/10/27 14:39:46, reveman wrote:
> Seems a bit silly to keep this struct when it's only used for solid color
> detection. Why don't we rename AnalyzeInRect to IsSolidColor() instead?

I've kept this for now and added a TODO. Currently, the nice thing about the
struct is that we transport one opaque-ish variable instead of a bool and a
color. Secondly, if we do want to extend this, this currently requires no extra
plumbing, as all relevant information can be added to analysis. If we do decide
that we'll never really have any other type of information in here, then I can
remove the struct, but I don't want to be too hasty about it.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:31: virtual void RasterToBitmap(
On 2014/10/27 14:39:46, reveman wrote:
> hm, ToBitmap doesn't seem right. RasterToCanvas? or just Raster? or maybe even
> ToCanvas? Or PlaybackToCanvas() to be consistent with the RasterBuffer API.

Changed to PlaybackToCanvas

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:35: RenderingStatsInstrumentation*
rendering_stats_instrumentation) const = 0;
On 2014/10/27 14:39:46, reveman wrote:
> Do we need to be passing this RenderingStatsInstrumentation pointer around?
> Would be nice if that could just be handled by the implementation instead.

Good question. I'll talk to ernstm to see whether we actually need these stats
at all. Since most (all?) telemetry tests moved to tracing, I'm not sure we need
this. I've added a TODO for now until I find out for sure.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:53: virtual bool RasterCoversRect(const gfx::Rect&
content_rect,
On 2014/10/27 14:39:46, reveman wrote:
> CoversRect? RasterSource::CoversRect looks better to me than
> RasterSource::RasterCoversRect.

Done.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:57: // fiend text.
On 2014/10/27 14:39:46, reveman wrote:
> typo: fiend

Done.

[reveman, 2014-10-27 17:14:44]

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:25: };
On 2014/10/27 15:55:10, vmpstr wrote:
> On 2014/10/27 14:39:46, reveman wrote:
> > Seems a bit silly to keep this struct when it's only used for solid color
> > detection. Why don't we rename AnalyzeInRect to IsSolidColor() instead?
> 
> I've kept this for now and added a TODO. Currently, the nice thing about the
> struct is that we transport one opaque-ish variable instead of a bool and a
> color. Secondly, if we do want to extend this, this currently requires no
extra
> plumbing, as all relevant information can be added to analysis. If we do
decide
> that we'll never really have any other type of information in here, then I can
> remove the struct, but I don't want to be too hasty about it.

Ok, can you change the name to SolidColorAnalysis for now? I rather not have
unnecessarily abstract concepts just because we might want to extend it in the
future. Better to keep things as concrete and simple as possible and instead
rename it if we expand the usage.

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:35: RenderingStatsInstrumentation*
rendering_stats_instrumentation) const = 0;
On 2014/10/27 15:55:11, vmpstr wrote:
> On 2014/10/27 14:39:46, reveman wrote:
> > Do we need to be passing this RenderingStatsInstrumentation pointer around?
> > Would be nice if that could just be handled by the implementation instead.
> 
> Good question. I'll talk to ernstm to see whether we actually need these stats
> at all. Since most (all?) telemetry tests moved to tracing, I'm not sure we
need
> this. I've added a TODO for now until I find out for sure.

Acknowledged.

[vmpstr, 2014-10-27 18:46:40]

PTAL

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/100001/cc/resources/raster_sou...
cc/resources/raster_source.h:25: };
On 2014/10/27 17:14:43, reveman wrote:
> On 2014/10/27 15:55:10, vmpstr wrote:
> > On 2014/10/27 14:39:46, reveman wrote:
> > > Seems a bit silly to keep this struct when it's only used for solid color
> > > detection. Why don't we rename AnalyzeInRect to IsSolidColor() instead?
> > 
> > I've kept this for now and added a TODO. Currently, the nice thing about the
> > struct is that we transport one opaque-ish variable instead of a bool and a
> > color. Secondly, if we do want to extend this, this currently requires no
> extra
> > plumbing, as all relevant information can be added to analysis. If we do
> decide
> > that we'll never really have any other type of information in here, then I
can
> > remove the struct, but I don't want to be too hasty about it.
> 
> Ok, can you change the name to SolidColorAnalysis for now? I rather not have
> unnecessarily abstract concepts just because we might want to extend it in the
> future. Better to keep things as concrete and simple as possible and instead
> rename it if we expand the usage.   

Done.

[reveman, 2014-10-27 19:27:58]

lgtm after renaming AnalyzeInRect.

Also, can you land a small patch prior to this that makes PicturePileBase thread
safe ref-counted instead of just ref-counted in case we need to merge that fix
to a branch?

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_buf...
File cc/resources/raster_buffer.h (right):

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_buf...
cc/resources/raster_buffer.h:23: RenderingStatsInstrumentation* stats) = 0;
Note that all that is really needed by the RasterBuffer interface is the
PlaybackToCanvas part of the RasterSource interface. It seems a bit overkill to
create another interface just for that though.

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_sou...
cc/resources/raster_source.h:44: virtual void AnalyzeInRect(
nit: sorry I missed this in my last review. can you change this to
SolidColorAnalysis or maybe PerformSolidColorAnalysis?

[vmpstr, 2014-10-27 19:48:06]

I'll wait until the thread safe patch lands, and rebase in case there are
conflicts.

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_buf...
File cc/resources/raster_buffer.h (right):

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_buf...
cc/resources/raster_buffer.h:23: RenderingStatsInstrumentation* stats) = 0;
On 2014/10/27 19:27:58, reveman wrote:
> Note that all that is really needed by the RasterBuffer interface is the
> PlaybackToCanvas part of the RasterSource interface. It seems a bit overkill
to
> create another interface just for that though.

Acknowledged. (and I agree on both points)

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_sou...
File cc/resources/raster_source.h (right):

https://codereview.chromium.org/666273002/diff/140001/cc/resources/raster_sou...
cc/resources/raster_source.h:44: virtual void AnalyzeInRect(
On 2014/10/27 19:27:58, reveman wrote:
> nit: sorry I missed this in my last review. can you change this to
> SolidColorAnalysis or maybe PerformSolidColorAnalysis?

Done.

[vmpstr, 2014-10-27 21:05:39]

Patchset #9 (id:160001) has been deleted

[vmpstr, 2014-10-28 17:36:51]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-10-28 17:37:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666273002/220001

[commit-bot: I haz the power, 2014-10-28 18:38:21]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2014-10-28 18:38:59]

Patchset 11 (id:??) landed as
https://crrev.com/394e2b51f40cbc5a522300073c7c058f95889901
Cr-Commit-Position: refs/heads/master@{#301667}