Add support for converting cc::FilterOperations into an SkImageFilter

Add support for converting cc::FilterOperations into an SkImageFilter

This defines a new type of cc::FilterOperation for reference filters, and
defines a method (RenderSurfaceFilters::BuildImageFilter) that builds an
SkImageFilter from a given cc::FilterOperations.

This also removes cc::Layer::SetFilter and cc::LayerImpl::SetFilter.
These were only used when we had a reference filter, but they are no
longer needed now that reference filters can be included in
FilterOperations.

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

[ajuma, 2013-07-31 18:40:37]

senorblanco@, PTAL.

This isn't in landable shape yet (e.g. I still need to add
tests), but I'm hoping to get some feedback on the
overall approach.

This CL represents Step 2 and Step 4 in the plan for
threading filter animations discussed in
https://docs.google.com/document/d/1HYB897iQzJMWcI0wA8pmoH3QyLWxpwjfDRvLY6CgWLg

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
File cc/output/render_surface_filters.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:20: #include
"third_party/skia/include/effects/SkTestImageFilters.h"
This is for SkComposeImageFilter, used to connect the SkImageFilter for a
reference filter (produced in Blink) to the SkImageFilter produced here for the
previous operations in the FilterOperations list. Should we move
SkComposeImageFilter to a different file in Skia (since it now has a non-test
use-case)?

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:325: class DropShadowImageFilter : public
SkImageFilter {
This is copied from Blink. SkiaImageFilterBuilder is the only consumer of this
class in Blink, so the Blink version can be removed once the DAG build is
happening in cc.

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:378: canvas.drawBitmap(src, dx_, dy_);
The version of DropShadowImageFilter currently in Blink uses -dy_ rather than
dy_ here, but that was producing incorrect results when I tested it.

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:600: // are only used for background filters
right now.
Is this worth doing? That is, are there any plans to use a DAG-build for
background filters?

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:620: op.image_filter().get(),
image_filter.get()));
The plan is to use a no-op filter as the SourceGraphic when building the
SkImageFilter for the reference filter in Blink. By using a color matrix filter
as the no-op filter, we'll preserve the optimization we currently have for the
case where the filter itself is just a color matrix.

[Stephen White, 2013-07-31 19:18:51]

This looks like a good start. I'd do the Skia change first, as noted below.

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/filter_op...
File cc/output/filter_operation.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/filter_op...
cc/output/filter_operation.cc:224: if (progress > 0.5)
Heh.. I guess it's better than nothing!  :)

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
File cc/output/render_surface_filters.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:20: #include
"third_party/skia/include/effects/SkTestImageFilters.h"
On 2013/07/31 18:40:37, ajuma wrote:
> This is for SkComposeImageFilter, used to connect the SkImageFilter for a
> reference filter (produced in Blink) to the SkImageFilter produced here for
the
> previous operations in the FilterOperations list. Should we move
> SkComposeImageFilter to a different file in Skia (since it now has a non-test
> use-case)?

Yes, I think it should be broken out into its own file in Skia first (ie.,
before landing this patch). We've done the same with most of the other filters
which started life in SkTestImageFilters.

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:377: canvas.saveLayer(0, &paint);
FYI, we can probably get rid of the saveLayer()/restore(), and just pass the
paint directly to drawBitmap. (This code dates from early days, when image
filters only worked in saveLayer/restore).

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:378: canvas.drawBitmap(src, dx_, dy_);
On 2013/07/31 18:40:37, ajuma wrote:
> The version of DropShadowImageFilter currently in Blink uses -dy_ rather than
> dy_ here, but that was producing incorrect results when I tested it.

Yeah, I think that was when we had TopDown textures in Blink but didn't know it.
Not doing the flip is probably correct (but I couldn't tell you why it's not
failing in Blink currently).

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:600: // are only used for background filters
right now.
On 2013/07/31 18:40:37, ajuma wrote:
> Is this worth doing? That is, are there any plans to use a DAG-build for
> background filters?

Check with zork@ about this filter; I think it's only used in ChromeOS, but I'm
not sure.

Eventually it would be nice for background filters to support the same path as
foreground filters (which will be just FilterOperations, I'm guessing, not
SkImageFilter directly).

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:620: op.image_filter().get(),
image_filter.get()));
On 2013/07/31 18:40:37, ajuma wrote:
> The plan is to use a no-op filter as the SourceGraphic when building the
> SkImageFilter for the reference filter in Blink. By using a color matrix
filter
> as the no-op filter, we'll preserve the optimization we currently have for the
> case where the filter itself is just a color matrix.

That seems reasonable. I like the use of the SkComposeImageFilter here, although
it does mean we miss the possibility for color matrix folding between shorthand
and reference filters that start with a color matrix, but contain other things
(e.g., sepia() -> matrix -> morphology). Probably not a big deal, though, and
maybe we can figure that out later.

The other thing we might be missing is a colorspace conversion from the
colorspace the shorthand filter is (device) in to the one the reference filter
is in (could be linear). This actually may also be broken in the Blink
implementation, so if it doesn't break tests, don't worry about it for now.

[ajuma, 2013-09-09 17:35:35]

senorblanco@, PTAL.

This now also removes cc::Layer::SetFilter (and, until the
Blink side of this lands, makes WebLayerImpl::setFilter use
a FilterOperation to wrap the SkImageFilter that it is given).

The Blink side of this is: http://crrev.com/23522020

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
File cc/output/render_surface_filters.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:20: #include
"third_party/skia/include/effects/SkTestImageFilters.h"
On 2013/07/31 19:18:51, Stephen White wrote:
> On 2013/07/31 18:40:37, ajuma wrote:
> > This is for SkComposeImageFilter, used to connect the SkImageFilter for a
> > reference filter (produced in Blink) to the SkImageFilter produced here for
> the
> > previous operations in the FilterOperations list. Should we move
> > SkComposeImageFilter to a different file in Skia (since it now has a
non-test
> > use-case)?
> 
> Yes, I think it should be broken out into its own file in Skia first (ie.,
> before landing this patch). We've done the same with most of the other filters
> which started life in SkTestImageFilters.

Done.

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:325: class DropShadowImageFilter : public
SkImageFilter {
On 2013/07/31 18:40:37, ajuma wrote:
> This is copied from Blink. SkiaImageFilterBuilder is the only consumer of this
> class in Blink, so the Blink version can be removed once the DAG build is
> happening in cc.

This is no longer needed, since DropShadowImageFilter was upstreamed to Skia.

https://chromiumcodereview.appspot.com/21154002/diff/2001/cc/output/render_su...
cc/output/render_surface_filters.cc:600: // are only used for background filters
right now.
On 2013/07/31 19:18:51, Stephen White wrote:
> On 2013/07/31 18:40:37, ajuma wrote:
> > Is this worth doing? That is, are there any plans to use a DAG-build for
> > background filters?
> 
> Check with zork@ about this filter; I think it's only used in ChromeOS, but
I'm
> not sure.
> 
> Eventually it would be nice for background filters to support the same path as
> foreground filters (which will be just FilterOperations, I'm guessing, not
> SkImageFilter directly).

I've added the code that was missing here for ZOOM filters.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/gl_rende...
File cc/output/gl_renderer.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/gl_rende...
cc/output/gl_renderer.cc:762: // when we have a reference filter.
I'm leaving this for a separate CL in order to avoid introducing behavior
changes (the ApplyImageFilter path can produce slightly different results than
the ApplyFilters path) in this already-large CL.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
File cc/output/software_renderer.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
cc/output/software_renderer.cc:437: if (quad->filters.HasReferenceFilter()) {
I'm checking for a reference filter here just to match the existing behavior.
But note that there doesn't appear to be any existing code handling the other
case.

[Stephen White, 2013-09-09 18:51:01]

LGTM, but compositor folks should look at this too.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
File cc/output/software_renderer.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
cc/output/software_renderer.cc:437: if (quad->filters.HasReferenceFilter()) {
On 2013/09/09 17:35:36, ajuma wrote:
> I'm checking for a reference filter here just to match the existing behavior.
> But note that there doesn't appear to be any existing code handling the other
> case.

IIRC, the software renderer only ever used setFilter(), for both the CSS
shorthands and for reference filters. Maybe skaslev@ can refresh my memory. If
so, we may want to skip the check and simply call BuildImageFilter()
unconditionally.

[danakj, 2013-09-09 22:04:49]

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/layers/layer_im...
File cc/layers/layer_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/layers/layer_im...
cc/layers/layer_impl_unittest.cc:269:
VERIFY_NO_NEEDS_UPDATE_DRAW_PROPERTIES(root->SetFilters(FilterOperations()));
This should be a non-empty filter ops. How about setting it up above like the
old one did?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operation.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.cc:181: NOTREACHED();
Can't create an image filter that does nothing?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.cc:225: if ((from && from->type() ==
FilterOperation::REFERENCE) ||
If the two types differ, you might get the |to| or |from| operation. But line
236 says you shouldn't. It seems like implementing CreateNoOpFilter would help
keep this function more clear and consistent, allowing you to move this stuff
down further after the earlyouts.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.cc:296: break;
Is there any data about the filter you can give here? How about countInputs()?
And canFilterImageGPU()?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operation.h (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.h:130: const skia::RefPtr<SkImageFilter>
image_filter) {
did you mean const& ?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operations.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations.cc:83: const FilterOperation op = operations_[i];
hmm, I think we meant const& here btw..

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations.cc:88: case FilterOperation::REFERENCE:
Should we DCHECK() that this is not called with REFERENCE since the outsets will
be invalid?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations.cc:108: const FilterOperation op = operations_[i];
hmm, I think we meant const& here btw..

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operations_unittest.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations_unittest.cc:526: FilterOperation filter =
FilterOperation::CreateReferenceFilter(image_filter);
nit: s/filter/from/

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
File cc/output/render_surface_filters.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
cc/output/render_surface_filters.cc:246: skia::RefPtr<SkImageFilter> input) {
const&?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
cc/output/render_surface_filters.cc:494: GetGrayscaleMatrix(1 - op.amount(),
matrix);
1.f here and below

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
cc/output/render_surface_filters.cc:576: } else if (image_filter.get()) {
do you mean else if (op.image_filter())?  if so, missing a test?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
File cc/output/software_renderer.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
cc/output/software_renderer.cc:437: if (quad->filters.HasReferenceFilter()) {
On 2013/09/09 17:35:36, ajuma wrote:
> I'm checking for a reference filter here just to match the existing behavior.
> But note that there doesn't appear to be any existing code handling the other
> case.

I think you should drop this if statement. It's not like we would be applying
the filters somewhere else.. Also, it sounds like we don't have any pixel tests
with software comp and filters. Would you care to make some/all of the
LayerTreeHostPixelTestFilterTest variants work with software comp too?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/test/render_pas...
File cc/test/render_pass_test_utils.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/test/render_pas...
cc/test/render_pass_test_utils.cc:96: FilterOperations filters,
const&?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/layer_tre...
File cc/trees/layer_tree_host_common.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/layer_tre...
cc/trees/layer_tree_host_common.cc:1550: layer->filters().HasReferenceFilter())
move this to be first so we don't call HasFilterThatMoves... ?

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/occlusion...
File cc/trees/occlusion_tracker.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/occlusion...
cc/trees/occlusion_tracker.cc:243:
finished_target->filters().HasReferenceFilter()) {
can't this just be removed?

[piman, 2013-09-09 23:29:14]

https://chromiumcodereview.appspot.com/21154002/diff/30001/content/common/cc_...
File content/common/cc_messages.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/content/common/cc_...
content/common/cc_messages.cc:49: switches::kAllowFiltersOverIPC))
nit: we already test the flag in ParamTraits<SkImageFilter>, I think we don't
need to test it twice. Maybe we can just unconditionally WriteParam it, and on
the reader side, unconditionally ReadParam it, and fix it up to an empty filter
if it comes out NULL. WDYT?

[ajuma, 2013-09-10 21:17:58]

Thanks for the review!

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/layers/layer_im...
File cc/layers/layer_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/layers/layer_im...
cc/layers/layer_impl_unittest.cc:269:
VERIFY_NO_NEEDS_UPDATE_DRAW_PROPERTIES(root->SetFilters(FilterOperations()));
On 2013/09/09 22:04:50, danakj wrote:
> This should be a non-empty filter ops. How about setting it up above like the
> old one did?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operation.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.cc:181: NOTREACHED();
On 2013/09/09 22:04:50, danakj wrote:
> Can't create an image filter that does nothing?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.cc:225: if ((from && from->type() ==
FilterOperation::REFERENCE) ||
On 2013/09/09 22:04:50, danakj wrote:
> If the two types differ, you might get the |to| or |from| operation. But line
> 236 says you shouldn't. It seems like implementing CreateNoOpFilter would help
> keep this function more clear and consistent, allowing you to move this stuff
> down further after the earlyouts.

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.cc:296: break;
On 2013/09/09 22:04:50, danakj wrote:
> Is there any data about the filter you can give here? How about countInputs()?
> And canFilterImageGPU()?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operation.h (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operation.h:130: const skia::RefPtr<SkImageFilter>
image_filter) {
On 2013/09/09 22:04:50, danakj wrote:
> did you mean const& ?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operations.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations.cc:83: const FilterOperation op = operations_[i];
On 2013/09/09 22:04:50, danakj wrote:
> hmm, I think we meant const& here btw..

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations.cc:88: case FilterOperation::REFERENCE:
On 2013/09/09 22:04:50, danakj wrote:
> Should we DCHECK() that this is not called with REFERENCE since the outsets
will
> be invalid?

Done. Also added a DCHECK to GetOutsets for the same reason.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations.cc:108: const FilterOperation op = operations_[i];
On 2013/09/09 22:04:50, danakj wrote:
> hmm, I think we meant const& here btw..

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
File cc/output/filter_operations_unittest.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/filter_o...
cc/output/filter_operations_unittest.cc:526: FilterOperation filter =
FilterOperation::CreateReferenceFilter(image_filter);
On 2013/09/09 22:04:50, danakj wrote:
> nit: s/filter/from/

This is used as the 'from' filter in the first two calls to Blend below, but as
the 'to' filter in the two calls that follow.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
File cc/output/render_surface_filters.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
cc/output/render_surface_filters.cc:246: skia::RefPtr<SkImageFilter> input) {
On 2013/09/09 22:04:50, danakj wrote:
> const&?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
cc/output/render_surface_filters.cc:494: GetGrayscaleMatrix(1 - op.amount(),
matrix);
On 2013/09/09 22:04:50, danakj wrote:
> 1.f here and below

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/render_s...
cc/output/render_surface_filters.cc:576: } else if (image_filter.get()) {
On 2013/09/09 22:04:50, danakj wrote:
> do you mean else if (op.image_filter())?  if so, missing a test?

No, this is checking if we've already build up a partial image_filter; if so, we
need to compose op.image_filter() with what we have so far, and if not there's
no need to compose.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
File cc/output/software_renderer.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
cc/output/software_renderer.cc:437: if (quad->filters.HasReferenceFilter()) {
On 2013/09/09 22:04:50, danakj wrote:
> On 2013/09/09 17:35:36, ajuma wrote:
> > I'm checking for a reference filter here just to match the existing
behavior.
> > But note that there doesn't appear to be any existing code handling the
other
> > case.
> 
> I think you should drop this if statement. It's not like we would be applying
> the filters somewhere else.. 

Done.

>Also, it sounds like we don't have any pixel tests
> with software comp and filters. Would you care to make some/all of the
> LayerTreeHostPixelTestFilterTest variants work with software comp too?

All but one of those tests use background filters, which the software renderer
ignores entirely right now (there's a TODO in
SoftwareRenderer::DrawRenderPassQuad for fixing that). The other test (that uses
a foreground filter) fails badly with the software compositor (all pixels
incorrect) both with and without this patch. There's an open bug for getting
filters working with the software compositor (http://crbug.com/160302). Perhaps
getting these pixel tests working with the software compositor should be done
once that work is complete (or done as part of that work).

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/test/render_pas...
File cc/test/render_pass_test_utils.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/test/render_pas...
cc/test/render_pass_test_utils.cc:96: FilterOperations filters,
On 2013/09/09 22:04:50, danakj wrote:
> const&?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/layer_tre...
File cc/trees/layer_tree_host_common.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/layer_tre...
cc/trees/layer_tree_host_common.cc:1550: layer->filters().HasReferenceFilter())
On 2013/09/09 22:04:50, danakj wrote:
> move this to be first so we don't call HasFilterThatMoves... ?

Done.

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/occlusion...
File cc/trees/occlusion_tracker.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/trees/occlusion...
cc/trees/occlusion_tracker.cc:243:
finished_target->filters().HasReferenceFilter()) {
On 2013/09/09 22:04:50, danakj wrote:
> can't this just be removed?

Yes, removed this and moved the TODO from above to
FilterOperations::HasFilterThatAffectsOpacity.

https://chromiumcodereview.appspot.com/21154002/diff/30001/content/common/cc_...
File content/common/cc_messages.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/content/common/cc_...
content/common/cc_messages.cc:49: switches::kAllowFiltersOverIPC))
On 2013/09/09 23:29:15, piman wrote:
> nit: we already test the flag in ParamTraits<SkImageFilter>, I think we don't
> need to test it twice. Maybe we can just unconditionally WriteParam it, and on
> the reader side, unconditionally ReadParam it, and fix it up to an empty
filter
> if it comes out NULL. WDYT?

Done. In fact, we don't even need to do any special fix up if the filter comes
out NULL, since setting the FilterOperation's image filter to NULL effectively
makes it a no-op filter.

[danakj, 2013-09-10 21:22:38]

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
File cc/output/software_renderer.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
cc/output/software_renderer.cc:437: if (quad->filters.HasReferenceFilter()) {
On 2013/09/10 21:17:58, ajuma wrote:
> On 2013/09/09 22:04:50, danakj wrote:
> > On 2013/09/09 17:35:36, ajuma wrote:
> > > I'm checking for a reference filter here just to match the existing
> behavior.
> > > But note that there doesn't appear to be any existing code handling the
> other
> > > case.
> > 
> > I think you should drop this if statement. It's not like we would be
applying
> > the filters somewhere else.. 
> 
> Done.
> 
> >Also, it sounds like we don't have any pixel tests
> > with software comp and filters. Would you care to make some/all of the
> > LayerTreeHostPixelTestFilterTest variants work with software comp too?
> 
> All but one of those tests use background filters, which the software renderer
> ignores entirely right now (there's a TODO in
> SoftwareRenderer::DrawRenderPassQuad for fixing that). The other test (that
uses
> a foreground filter) fails badly with the software compositor (all pixels
> incorrect) both with and without this patch. There's an open bug for getting
> filters working with the software compositor (http://crbug.com/160302).
Perhaps
> getting these pixel tests working with the software compositor should be done
> once that work is complete (or done as part of that work).

Do you know how the foreground filter test fails? It seems like this is the work
to get it going.

[ajuma, 2013-09-10 21:30:18]

On 2013/09/10 21:22:38, danakj wrote:
>
https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
> File cc/output/software_renderer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software...
> cc/output/software_renderer.cc:437: if (quad->filters.HasReferenceFilter()) {
> On 2013/09/10 21:17:58, ajuma wrote:
> > On 2013/09/09 22:04:50, danakj wrote:
> > > On 2013/09/09 17:35:36, ajuma wrote:
> > > > I'm checking for a reference filter here just to match the existing
> > behavior.
> > > > But note that there doesn't appear to be any existing code handling the
> > other
> > > > case.
> > > 
> > > I think you should drop this if statement. It's not like we would be
> applying
> > > the filters somewhere else.. 
> > 
> > Done.
> > 
> > >Also, it sounds like we don't have any pixel tests
> > > with software comp and filters. Would you care to make some/all of the
> > > LayerTreeHostPixelTestFilterTest variants work with software comp too?
> > 
> > All but one of those tests use background filters, which the software
renderer
> > ignores entirely right now (there's a TODO in
> > SoftwareRenderer::DrawRenderPassQuad for fixing that). The other test (that
> uses
> > a foreground filter) fails badly with the software compositor (all pixels
> > incorrect) both with and without this patch. There's an open bug for getting
> > filters working with the software compositor (http://crbug.com/160302).
> Perhaps
> > getting these pixel tests working with the software compositor should be
done
> > once that work is complete (or done as part of that work).
> 
> Do you know how the foreground filter test fails? It seems like this is the
work
> to get it going.

Yes, all pixels are (255, 255, 0, 0) instead of (0, 0, 255, 0). It seems like
there's more that needs to be done to get the software renderer to correctly
apply filters. We could add the test and mark it maximally fuzzy for now if that
seems like a good way to move this forward.

[ajuma, 2013-09-10 21:48:12]

> On 2013/09/10 21:22:38, danakj wrote:
> > Do you know how the foreground filter test fails? It seems like this is the
> work
> > to get it going.

Re-reading, perhaps I misunderstood the "how" in that question :) I'd be happy
to further debug the software renderer's handling of filters, but I think that
can be done separately from this CL.

[danakj, 2013-09-10 22:05:07]

On Tue, Sep 10, 2013 at 5:30 PM, <ajuma@chromium.org> wrote:

> On 2013/09/10 21:22:38, danakj wrote:
>
> https://chromiumcodereview.**appspot.com/21154002/diff/**
>
30001/cc/output/software_**renderer.cc<https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software_renderer.cc>
>
>> File cc/output/software_renderer.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/21154002/diff/**
>
30001/cc/output/software_**renderer.cc#newcode437<https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software_renderer.cc#newcode437>
>
>> cc/output/software_renderer.**cc:437: if
(quad->filters.**HasReferenceFilter())
>> {
>> On 2013/09/10 21:17:58, ajuma wrote:
>> > On 2013/09/09 22:04:50, danakj wrote:
>> > > On 2013/09/09 17:35:36, ajuma wrote:
>> > > > I'm checking for a reference filter here just to match the existing
>> > behavior.
>> > > > But note that there doesn't appear to be any existing code handling
>> the
>> > other
>> > > > case.
>> > >
>> > > I think you should drop this if statement. It's not like we would be
>> applying
>> > > the filters somewhere else..
>> >
>> > Done.
>> >
>> > >Also, it sounds like we don't have any pixel tests
>> > > with software comp and filters. Would you care to make some/all of the
>> > > LayerTreeHostPixelTestFilterTe**st variants work with software comp
>> too?
>> >
>> > All but one of those tests use background filters, which the software
>>
> renderer
>
>> > ignores entirely right now (there's a TODO in
>> > SoftwareRenderer::**DrawRenderPassQuad for fixing that). The other
>> test (that
>> uses
>> > a foreground filter) fails badly with the software compositor (all
>> pixels
>> > incorrect) both with and without this patch. There's an open bug for
>> getting
>> > filters working with the software compositor (http://crbug.com/160302).
>> Perhaps
>> > getting these pixel tests working with the software compositor should be
>>
> done
>
>> > once that work is complete (or done as part of that work).
>>
>
>  Do you know how the foreground filter test fails? It seems like this is
>> the
>>
> work
>
>> to get it going.
>>
>
> Yes, all pixels are (255, 255, 0, 0) instead of (0, 0, 255, 0). It seems
> like
> there's more that needs to be done to get the software renderer to
> correctly
> apply filters. We could add the test and mark it maximally fuzzy for now
> if that
> seems like a good way to move this forward.
>

Huh weird, thanks for investigating. Ok then I think we should keep the if
(HasReferenceFilter) {} in the software renderer for now, as the
SkImageFilter path is tested in renderer_pixeltests.cc and maybe add some
non-SkImageFilter tests to that file and remove the if() from the renderer
in a separate CL.


>
>
https://chromiumcodereview.**appspot.com/21154002/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2013-09-10 22:06:01]

On Tue, Sep 10, 2013 at 6:04 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Tue, Sep 10, 2013 at 5:30 PM, <ajuma@chromium.org> wrote:
>
>> On 2013/09/10 21:22:38, danakj wrote:
>>
>> https://chromiumcodereview.**appspot.com/21154002/diff/**
>>
30001/cc/output/software_**renderer.cc<https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software_renderer.cc>
>>
>>> File cc/output/software_renderer.cc (right):
>>>
>>
>>
>> https://chromiumcodereview.**appspot.com/21154002/diff/**
>>
30001/cc/output/software_**renderer.cc#newcode437<https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software_renderer.cc#newcode437>
>>
>>> cc/output/software_renderer.**cc:437: if
(quad->filters.**HasReferenceFilter())
>>> {
>>> On 2013/09/10 21:17:58, ajuma wrote:
>>> > On 2013/09/09 22:04:50, danakj wrote:
>>> > > On 2013/09/09 17:35:36, ajuma wrote:
>>> > > > I'm checking for a reference filter here just to match the existing
>>> > behavior.
>>> > > > But note that there doesn't appear to be any existing code
>>> handling the
>>> > other
>>> > > > case.
>>> > >
>>> > > I think you should drop this if statement. It's not like we would be
>>> applying
>>> > > the filters somewhere else..
>>> >
>>> > Done.
>>> >
>>> > >Also, it sounds like we don't have any pixel tests
>>> > > with software comp and filters. Would you care to make some/all of
>>> the
>>> > > LayerTreeHostPixelTestFilterTe**st variants work with software comp
>>> too?
>>> >
>>> > All but one of those tests use background filters, which the software
>>>
>> renderer
>>
>>> > ignores entirely right now (there's a TODO in
>>> > SoftwareRenderer::**DrawRenderPassQuad for fixing that). The other
>>> test (that
>>> uses
>>> > a foreground filter) fails badly with the software compositor (all
>>> pixels
>>> > incorrect) both with and without this patch. There's an open bug for
>>> getting
>>> > filters working with the software compositor (http://crbug.com/160302
>>> ).
>>> Perhaps
>>> > getting these pixel tests working with the software compositor should
>>> be
>>>
>> done
>>
>>> > once that work is complete (or done as part of that work).
>>>
>>
>>  Do you know how the foreground filter test fails? It seems like this is
>>> the
>>>
>> work
>>
>>> to get it going.
>>>
>>
>> Yes, all pixels are (255, 255, 0, 0) instead of (0, 0, 255, 0). It seems
>> like
>> there's more that needs to be done to get the software renderer to
>> correctly
>> apply filters. We could add the test and mark it maximally fuzzy for now
>> if that
>> seems like a good way to move this forward.
>>
>
> Huh weird, thanks for investigating. Ok then I think we should keep the if
> (HasReferenceFilter) {} in the software renderer for now, as the
> SkImageFilter path is tested in renderer_pixeltests.cc and maybe add some
> non-SkImageFilter tests to that file and remove the if() from the renderer
> in a separate CL.
>

Maybe with a TODO pointing to that bug you cited :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajuma, 2013-09-10 23:10:45]

On 2013/09/10 22:06:01, danakj wrote:
> On Tue, Sep 10, 2013 at 6:04 PM, Dana Jansens <mailto:danakj@chromium.org>
wrote:
> 
> > On Tue, Sep 10, 2013 at 5:30 PM, <mailto:ajuma@chromium.org> wrote:
> >
> >> On 2013/09/10 21:22:38, danakj wrote:
> >>
> >> https://chromiumcodereview.**appspot.com/21154002/diff/**
> >>
>
30001/cc/output/software_**renderer.cc<https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software_renderer.cc>
> >>
> >>> File cc/output/software_renderer.cc (right):
> >>>
> >>
> >>
> >> https://chromiumcodereview.**appspot.com/21154002/diff/**
> >>
>
30001/cc/output/software_**renderer.cc#newcode437<https://chromiumcodereview.appspot.com/21154002/diff/30001/cc/output/software_renderer.cc#newcode437>
> >>
> >>> cc/output/software_renderer.**cc:437: if
> (quad->filters.**HasReferenceFilter())
> >>> {
> >>> On 2013/09/10 21:17:58, ajuma wrote:
> >>> > On 2013/09/09 22:04:50, danakj wrote:
> >>> > > On 2013/09/09 17:35:36, ajuma wrote:
> >>> > > > I'm checking for a reference filter here just to match the existing
> >>> > behavior.
> >>> > > > But note that there doesn't appear to be any existing code
> >>> handling the
> >>> > other
> >>> > > > case.
> >>> > >
> >>> > > I think you should drop this if statement. It's not like we would be
> >>> applying
> >>> > > the filters somewhere else..
> >>> >
> >>> > Done.
> >>> >
> >>> > >Also, it sounds like we don't have any pixel tests
> >>> > > with software comp and filters. Would you care to make some/all of
> >>> the
> >>> > > LayerTreeHostPixelTestFilterTe**st variants work with software comp
> >>> too?
> >>> >
> >>> > All but one of those tests use background filters, which the software
> >>>
> >> renderer
> >>
> >>> > ignores entirely right now (there's a TODO in
> >>> > SoftwareRenderer::**DrawRenderPassQuad for fixing that). The other
> >>> test (that
> >>> uses
> >>> > a foreground filter) fails badly with the software compositor (all
> >>> pixels
> >>> > incorrect) both with and without this patch. There's an open bug for
> >>> getting
> >>> > filters working with the software compositor (http://crbug.com/160302
> >>> ).
> >>> Perhaps
> >>> > getting these pixel tests working with the software compositor should
> >>> be
> >>>
> >> done
> >>
> >>> > once that work is complete (or done as part of that work).
> >>>
> >>
> >>  Do you know how the foreground filter test fails? It seems like this is
> >>> the
> >>>
> >> work
> >>
> >>> to get it going.
> >>>
> >>
> >> Yes, all pixels are (255, 255, 0, 0) instead of (0, 0, 255, 0). It seems
> >> like
> >> there's more that needs to be done to get the software renderer to
> >> correctly
> >> apply filters. We could add the test and mark it maximally fuzzy for now
> >> if that
> >> seems like a good way to move this forward.
> >>
> >
> > Huh weird, thanks for investigating. Ok then I think we should keep the if
> > (HasReferenceFilter) {} in the software renderer for now, as the
> > SkImageFilter path is tested in renderer_pixeltests.cc and maybe add some
> > non-SkImageFilter tests to that file and remove the if() from the renderer
> > in a separate CL.
> >
> 
> Maybe with a TODO pointing to that bug you cited :)

Ok, done.

[piman, 2013-09-11 00:03:56]

LGTM for the content bits.

[ajuma, 2013-09-11 16:02:22]

On 2013/09/10 22:05:07, danakj wrote:
> Huh weird, thanks for investigating. Ok then I think we should keep the if
> (HasReferenceFilter) {} in the software renderer for now, as the
> SkImageFilter path is tested in renderer_pixeltests.cc and maybe add some
> non-SkImageFilter tests to that file and remove the if() from the renderer
> in a separate CL.

Fyi, the follow-up CL for this is at
https://chromiumcodereview.appspot.com/24090003.

[danakj, 2013-09-11 17:33:09]

Thanks, LGTM

https://chromiumcodereview.appspot.com/21154002/diff/81001/cc/output/render_s...
File cc/output/render_surface_filters.cc (right):

https://chromiumcodereview.appspot.com/21154002/diff/81001/cc/output/render_s...
cc/output/render_surface_filters.cc:579: } else if (image_filter.get()) {
nit: don't need .get() right?

[ajuma, 2013-09-11 17:41:30]

+cevans for content/common/cc_messsages.h
+enne for webkit/renderer/compositor_bindings

https://codereview.chromium.org/21154002/diff/81001/cc/output/render_surface_...
File cc/output/render_surface_filters.cc (right):

https://codereview.chromium.org/21154002/diff/81001/cc/output/render_surface_...
cc/output/render_surface_filters.cc:579: } else if (image_filter.get()) {
On 2013/09/11 17:33:09, danakj wrote:
> nit: don't need .get() right?

Done.

[Stephen White, 2013-09-11 17:56:24]

https://codereview.chromium.org/21154002/diff/75001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1550: if
(layer->filters().HasReferenceFilter() ||
It occurs to me that HasFilterThatMovesPixels() should probably return true
(conservatively) when it has a reference filter. Then we could just call that,
and skip the HasReferenceFilter() call here. Could be done in a followup.

That'll make it easier to clean this up once SkImageFilter gets the smarts.

[enne (OOO), 2013-09-11 17:57:23]

lgtm

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
cc/output/filter_operations.cc:89: DCHECK(op.type() !=
FilterOperation::REFERENCE);
This is a little awkward to DCHECK here because it means that calling code has
some subtlety (e.g. in layer_tree_host_common.cc) that they have to check for
HasReferenceFilter first before checking HasFilterThatMovesPixels.  I think it'd
be better to remove this DCHECK entirely.

https://codereview.chromium.org/21154002/diff/75001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:788: {
style nit: this is a seemingly unnecessary block.

[danakj, 2013-09-11 18:01:16]

https://codereview.chromium.org/21154002/diff/75001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1550: if
(layer->filters().HasReferenceFilter() ||
On 2013/09/11 17:56:25, Stephen White wrote:
> It occurs to me that HasFilterThatMovesPixels() should probably return true
> (conservatively) when it has a reference filter. Then we could just call that,
> and skip the HasReferenceFilter() call here. Could be done in a followup.
> 
> That'll make it easier to clean this up once SkImageFilter gets the smarts.

That's true, but elsewhere we check that and then use the outsets to tell how
far they moved. And with a reference filter those outsets are wrong. So I'd
prefer to keep it a dcheck as is.

[danakj, 2013-09-11 18:01:54]

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
cc/output/filter_operations.cc:89: DCHECK(op.type() !=
FilterOperation::REFERENCE);
On 2013/09/11 17:57:24, enne wrote:
> This is a little awkward to DCHECK here because it means that calling code has
> some subtlety (e.g. in layer_tree_host_common.cc) that they have to check for
> HasReferenceFilter first before checking HasFilterThatMovesPixels.  I think
it'd
> be better to remove this DCHECK entirely.

I'd agree if the damage tracker didn't use invalid data when
HasFilterThatMovesPixels is true for a reference filter.. what do you think of
that?

[danakj, 2013-09-11 18:02:46]

https://codereview.chromium.org/21154002/diff/75001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:788: {
On 2013/09/11 17:57:24, enne wrote:
> style nit: this is a seemingly unnecessary block.

This is to keep the raw pointer scoped and ensure it's not used outside of
putting it in an adopt ref. (See long discussion CL where I failed to add to
RefPtr support for this case.)

[Stephen White, 2013-09-11 18:04:19]

https://codereview.chromium.org/21154002/diff/75001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1550: if
(layer->filters().HasReferenceFilter() ||
On 2013/09/11 18:01:16, danakj wrote:
> On 2013/09/11 17:56:25, Stephen White wrote:
> > It occurs to me that HasFilterThatMovesPixels() should probably return true
> > (conservatively) when it has a reference filter. Then we could just call
that,
> > and skip the HasReferenceFilter() call here. Could be done in a followup.
> > 
> > That'll make it easier to clean this up once SkImageFilter gets the smarts.
> 
> That's true, but elsewhere we check that and then use the outsets to tell how
> far they moved. And with a reference filter those outsets are wrong. So I'd
> prefer to keep it a dcheck as is.

Ahh, ok. In that case, ignore me.

[enne (OOO), 2013-09-11 18:06:32]

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
cc/output/filter_operations.cc:89: DCHECK(op.type() !=
FilterOperation::REFERENCE);
On 2013/09/11 18:01:55, danakj wrote:
> On 2013/09/11 17:57:24, enne wrote:
> > This is a little awkward to DCHECK here because it means that calling code
has
> > some subtlety (e.g. in layer_tree_host_common.cc) that they have to check
for
> > HasReferenceFilter first before checking HasFilterThatMovesPixels.  I think
> it'd
> > be better to remove this DCHECK entirely.
> 
> I'd agree if the damage tracker didn't use invalid data when
> HasFilterThatMovesPixels is true for a reference filter.. what do you think of
> that?

So DCHECK in GetOutsets, but don't DCHECK here?

[danakj, 2013-09-11 18:18:16]

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
cc/output/filter_operations.cc:89: DCHECK(op.type() !=
FilterOperation::REFERENCE);
On 2013/09/11 18:06:33, enne wrote:
> On 2013/09/11 18:01:55, danakj wrote:
> > On 2013/09/11 17:57:24, enne wrote:
> > > This is a little awkward to DCHECK here because it means that calling code
> has
> > > some subtlety (e.g. in layer_tree_host_common.cc) that they have to check
> for
> > > HasReferenceFilter first before checking HasFilterThatMovesPixels.  I
think
> > it'd
> > > be better to remove this DCHECK entirely.
> > 
> > I'd agree if the damage tracker didn't use invalid data when
> > HasFilterThatMovesPixels is true for a reference filter.. what do you think
of
> > that?
> 
> So DCHECK in GetOutsets, but don't DCHECK here?

Hm, ya okay. I hadn't considered that originally. That's a good idea. SGTM
thanks

[ajuma, 2013-09-11 18:24:56]

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
File cc/output/filter_operations.cc (right):

https://codereview.chromium.org/21154002/diff/75001/cc/output/filter_operatio...
cc/output/filter_operations.cc:89: DCHECK(op.type() !=
FilterOperation::REFERENCE);
On 2013/09/11 18:18:17, danakj wrote:
> On 2013/09/11 18:06:33, enne wrote:
> > On 2013/09/11 18:01:55, danakj wrote:
> > > On 2013/09/11 17:57:24, enne wrote:
> > > > This is a little awkward to DCHECK here because it means that calling
code
> > has
> > > > some subtlety (e.g. in layer_tree_host_common.cc) that they have to
check
> > for
> > > > HasReferenceFilter first before checking HasFilterThatMovesPixels.  I
> think
> > > it'd
> > > > be better to remove this DCHECK entirely.
> > > 
> > > I'd agree if the damage tracker didn't use invalid data when
> > > HasFilterThatMovesPixels is true for a reference filter.. what do you
think
> of
> > > that?
> > 
> > So DCHECK in GetOutsets, but don't DCHECK here?
> 
> Hm, ya okay. I hadn't considered that originally. That's a good idea. SGTM
> thanks

Done.

[ajuma, 2013-09-12 19:25:36]

cevans, PTAL at the removal of a field in content/common/cc_messages.h.

[ajuma, 2013-09-18 16:58:47]

cevans@, friendly ping!

[ajuma, 2013-09-20 20:09:53]

+palmer for removing a field in cc_messages.h

[palmer, 2013-09-24 19:29:06]

IPC security LGTM

[commit-bot: I haz the power, 2013-09-24 22:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-25 03:47:36]

Retried try job too often on linux_chromeos for step(s) browser_tests,
content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-25 12:49:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-25 19:04:53]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-25 19:13:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-25 21:21:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-25 22:29:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-26 01:23:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-26 02:00:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/21154002/125001

[commit-bot: I haz the power, 2013-09-26 04:41:51]

Change committed as 225329