[cc] Add nearest neighbor filtering for TextureLayer.

[cc] Add nearest neighbor filtering for TextureLayer.

Blink side is here (depends on this CL):
https://codereview.chromium.org/562583002/

This CL also depends on another blink side change here:
https://codereview.chromium.org/699103002/

BUG=134040
Committed: https://crrev.com/10c9af41a73ac7db787d64344630f8200925cfe5
Cr-Commit-Position: refs/heads/master@{#306768}

[jackhou1, 2014-09-11 08:33:49]

jackhou@chromium.org changed reviewers:
+ danakj@chromium.org

[jackhou1, 2014-09-11 08:33:50]

danakj, would you mind taking a look at this?

The goal is to allow accelerated canvases to be upscaled using nearest neighbor.

I couldn't quite get this to work. TextureLayerImpl::AppendQuads appends the
quads correctly, but those quads are never seen by
GLRenderer::FlushTextureQuadCache. I'm not sure what's happening in between, any
advice?

[jackhou1, 2014-09-15 06:23:01]

Ping.

[danakj, 2014-09-15 15:41:04]

If you want the GLRenderer in the browser compositor to know the filter, you
should be passing the filter in here and using it rather than hardcoded LINEAR:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...

Then we'll pass that filter to the browser compositor and it can see it also.

But I don't see anywhere that the compositor would override the current filter
on the texture you provide, either. Can you not set the filter in the code that
produces the texture, and just give it to CC and have it use it? Do you know
where CC is changing the filter to LINEAR? I only see that the filter changes in
2 places:

1. When making a new texture in ResourceProvider - which doesn't happen for a
texture given to TextureLayer
2. When using ResourceProvider::ScopedSamplerGL with a filter other than LINEAR.
Which doesn't even get called by FlushTextureQuadCache.

But, when you say FlushTextureQuadCache doesn't work, I'm not sure what you
mean. This function would happen in the browser process, not the renderer where
you first give the texture to the compositor (assuming this texture is coming
from the renderer).

[jackhou1, 2014-10-28 04:44:06]

PTAL

The feature seems to work now, but the CL is not finished. Please ignore the
printfs and commented out parts. I wanted to check that the approach is correct
first.

On 2014/09/15 15:41:04, danakj wrote:
> If you want the GLRenderer in the browser compositor to know the filter, you
> should be passing the filter in here and using it rather than hardcoded
LINEAR:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> 
> Then we'll pass that filter to the browser compositor and it can see it also.
> 
> But I don't see anywhere that the compositor would override the current filter
> on the texture you provide, either. Can you not set the filter in the code
that
> produces the texture, and just give it to CC and have it use it? Do you know
> where CC is changing the filter to LINEAR? I only see that the filter changes
in
> 2 places:
> 
> 1. When making a new texture in ResourceProvider - which doesn't happen for a
> texture given to TextureLayer
> 2. When using ResourceProvider::ScopedSamplerGL with a filter other than
LINEAR.
> Which doesn't even get called by FlushTextureQuadCache.
> 
> But, when you say FlushTextureQuadCache doesn't work, I'm not sure what you
> mean. This function would happen in the browser process, not the renderer
where
> you first give the texture to the compositor (assuming this texture is coming
> from the renderer).

It turns out this was because I did not update cc_messages.h.

[piman, 2014-10-28 05:09:49]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2014-10-28 05:09:51]

https://codereview.chromium.org/558083002/diff/40001/cc/blink/web_external_te...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/blink/web_external_te...
cc/blink/web_external_texture_layer_impl.cc:98:
mailbox->set_nearest_neighbor(nearest_neighbor_);
This should not be set indiscriminately, but based on what the client set on the
texture - i.e. this should be also added to WebExternalTextureMailbox and set by
blink.

https://codereview.chromium.org/558083002/diff/40001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:111: ResourceProvider::TextureHintDefault,
Why changing this? (and other places)
Immutability refers to the dimensions/format, not to the pixels or filtering
mode.

https://codereview.chromium.org/558083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2115: draw_cache_.nearest_neighbor);
You need an enum here (GL_LINEAR or GL_NEAREST), not a bool.

https://codereview.chromium.org/558083002/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:602: TextureHintDefault,
Why change this?

https://codereview.chromium.org/558083002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1348: TextureHintDefault,
And this

https://codereview.chromium.org/558083002/diff/40001/content/common/cc_messag...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/558083002/diff/40001/content/common/cc_messag...
content/common/cc_messages.h:216: IPC_STRUCT_TRAITS_MEMBER(nearest_neighbor)
please update cc_mesages_unittest.cc

[jackhou1, 2014-10-29 00:12:25]

https://codereview.chromium.org/558083002/diff/40001/cc/blink/web_external_te...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/blink/web_external_te...
cc/blink/web_external_texture_layer_impl.cc:98:
mailbox->set_nearest_neighbor(nearest_neighbor_);
On 2014/10/28 05:09:50, piman (Very slow to review) wrote:
> This should not be set indiscriminately, but based on what the client set on
the
> texture - i.e. this should be also added to WebExternalTextureMailbox and set
by
> blink.

Done.

https://codereview.chromium.org/558083002/diff/40001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:111: ResourceProvider::TextureHintDefault,
On 2014/10/28 05:09:50, piman (Very slow to review) wrote:
> Why changing this? (and other places)
> Immutability refers to the dimensions/format, not to the pixels or filtering
> mode.

Oops, I wasn't sure if this mattered. Removed.

https://codereview.chromium.org/558083002/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2115: draw_cache_.nearest_neighbor);
On 2014/10/28 05:09:50, piman (Very slow to review) wrote:
> You need an enum here (GL_LINEAR or GL_NEAREST), not a bool.

Done.

https://codereview.chromium.org/558083002/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/558083002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:602: TextureHintDefault,
On 2014/10/28 05:09:50, piman (Very slow to review) wrote:
> Why change this?

Done.

https://codereview.chromium.org/558083002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1348: TextureHintDefault,
On 2014/10/28 05:09:50, piman (Very slow to review) wrote:
> And this

Done.

https://codereview.chromium.org/558083002/diff/40001/content/common/cc_messag...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/558083002/diff/40001/content/common/cc_messag...
content/common/cc_messages.h:216: IPC_STRUCT_TRAITS_MEMBER(nearest_neighbor)
On 2014/10/28 05:09:51, piman (Very slow to review) wrote:
> please update cc_mesages_unittest.cc

Done.

[jackhou1, 2014-11-04 03:52:45]

PTAL

This depends on a minor change to WebExternalTextureMailbox.h:
http://crrev.com/699103002

[piman, 2014-11-04 04:48:03]

lgtm

[danakj, 2014-11-04 16:02:42]

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:68:
->SetNearestNeighbor(filter_level == SkPaint::kNone_FilterLevel);
You have multiple ways of setting nearest neighbour here? Why is that needed?
Isn't doing it through PrepareTextureMailbox enough?

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.h (right):

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:47: virtual void
setFilterLevel(SkPaint::FilterLevel filter_level);
since this function only handles none vs not-none, i think a setNearestNeighbour
would be more appropriate, but, see comment in the .cc.

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:69: bool nearest_neighbor_;
unused?

[piman, 2014-11-04 21:14:35]

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:68:
->SetNearestNeighbor(filter_level == SkPaint::kNone_FilterLevel);
On 2014/11/04 16:02:42, danakj wrote:
> You have multiple ways of setting nearest neighbour here? Why is that needed?
> Isn't doing it through PrepareTextureMailbox enough?

There's 2 things, that may or may not be in sync:
1- the state that's in the texture
2- how you want the quad to be displayed

This is #2. l.94 is #1.

[danakj, 2014-11-04 21:30:24]

This looks like a solid change to me too. A couple of test requests for you
though.

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:68:
->SetNearestNeighbor(filter_level == SkPaint::kNone_FilterLevel);
On 2014/11/04 21:14:35, piman (Very slow to review) wrote:
> On 2014/11/04 16:02:42, danakj wrote:
> > You have multiple ways of setting nearest neighbour here? Why is that
needed?
> > Isn't doing it through PrepareTextureMailbox enough?
> 
> There's 2 things, that may or may not be in sync:
> 1- the state that's in the texture
> 2- how you want the quad to be displayed
> 
> This is #2. l.94 is #1.

Ah okay, thanks.

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:599: mailbox.nearest_neighbor() ? GL_NEAREST :
GL_LINEAR,
can you add a test to resource_provider_unittest.cc for this change to show it's
working?

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1338: it->filter,
can you add a test to resource_provider_unittest.cc for this change to show it's
working?

[jackhou1, 2014-11-12 03:35:17]

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.h (right):

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:47: virtual void
setFilterLevel(SkPaint::FilterLevel filter_level);
On 2014/11/04 16:02:42, danakj wrote:
> since this function only handles none vs not-none, i think a
setNearestNeighbour
> would be more appropriate, but, see comment in the .cc.

I was recommended to use an enum on the other CL (crrev.com/562583002), but we
can discuss with junov/senorblanco if you feel strongly about it.

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:69: bool nearest_neighbor_;
On 2014/11/04 16:02:42, danakj wrote:
> unused?

Removed.

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:599: mailbox.nearest_neighbor() ? GL_NEAREST :
GL_LINEAR,
On 2014/11/04 21:30:23, danakj wrote:
> can you add a test to resource_provider_unittest.cc for this change to show
it's
> working?

I added a bit to ResourceProviderTest.TextureMailbox_GLTexture2D.

I realised it's possible for someone creating the mailbox to pass in a texture
with GL_LINEAR filters, but TextureMailbox::nearest_neighbor set to true. Would
it be better to set the filters on the texture here based on
TextureMailbox::nearest_neighbor, or set Resource::filter by checking what's set
on the texture?

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1338: it->filter,
On 2014/11/04 21:30:23, danakj wrote:
> can you add a test to resource_provider_unittest.cc for this change to show
it's
> working?

Huh, looks like Resource::filter is never read for anything if it's a software
resource. I guess the software renderer always assumes linear filtering? Is
there a way to force software rendering so I can fiddle with it?

[piman, 2014-11-12 04:43:26]

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:599: mailbox.nearest_neighbor() ? GL_NEAREST :
GL_LINEAR,
On 2014/11/12 03:35:17, jackhou1 wrote:
> On 2014/11/04 21:30:23, danakj wrote:
> > can you add a test to resource_provider_unittest.cc for this change to show
> it's
> > working?
> 
> I added a bit to ResourceProviderTest.TextureMailbox_GLTexture2D.
> 
> I realised it's possible for someone creating the mailbox to pass in a texture
> with GL_LINEAR filters, but TextureMailbox::nearest_neighbor set to true.

That's a user error then.

> Would
> it be better to set the filters on the texture here based on
> TextureMailbox::nearest_neighbor,

No, that would be redundant changes.

> or set Resource::filter by checking what's set
> on the texture?

No, reading what's on the texture would involve a round-trip (slow).

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1338: it->filter,
On 2014/11/12 03:35:17, jackhou1 wrote:
> On 2014/11/04 21:30:23, danakj wrote:
> > can you add a test to resource_provider_unittest.cc for this change to show
> it's
> > working?
> 
> Huh, looks like Resource::filter is never read for anything if it's a software
> resource. I guess the software renderer always assumes linear filtering? Is
> there a way to force software rendering so I can fiddle with it?

--disable-gpu

[jackhou1, 2014-11-12 05:05:54]

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/558083002/diff/140001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1338: it->filter,
On 2014/11/12 04:43:26, piman (Very slow to review) wrote:
> On 2014/11/12 03:35:17, jackhou1 wrote:
> > On 2014/11/04 21:30:23, danakj wrote:
> > > can you add a test to resource_provider_unittest.cc for this change to
show
> > it's
> > > working?
> > 
> > Huh, looks like Resource::filter is never read for anything if it's a
software
> > resource. I guess the software renderer always assumes linear filtering? Is
> > there a way to force software rendering so I can fiddle with it?
> 
> --disable-gpu

Looks like Patch Set 9 works fine with --disable-gpu. I guess blink is choosing
an already implemented code path.

[danakj, 2014-11-12 18:10:14]

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.h (right):

https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:47: virtual void
setFilterLevel(SkPaint::FilterLevel filter_level);
On 2014/11/12 03:35:17, jackhou1 wrote:
> On 2014/11/04 16:02:42, danakj wrote:
> > since this function only handles none vs not-none, i think a
> setNearestNeighbour
> > would be more appropriate, but, see comment in the .cc.
> 
> I was recommended to use an enum on the other CL (crrev.com/562583002), but we
> can discuss with junov/senorblanco if you feel strongly about it.

I guess different owners with different tastes. I'd prefer to optimize for the
simplest possible API now, rather than potential future changes that are
uncertain.

If they want an enum you can turn that into a bool at some level before the call
to this method?

[jackhou1, 2014-11-13 06:02:39]

On 2014/11/12 18:10:14, danakj wrote:
>
https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
> File cc/blink/web_external_texture_layer_impl.h (right):
> 
>
https://codereview.chromium.org/558083002/diff/140001/cc/blink/web_external_t...
> cc/blink/web_external_texture_layer_impl.h:47: virtual void
> setFilterLevel(SkPaint::FilterLevel filter_level);
> On 2014/11/12 03:35:17, jackhou1 wrote:
> > On 2014/11/04 16:02:42, danakj wrote:
> > > since this function only handles none vs not-none, i think a
> > setNearestNeighbour
> > > would be more appropriate, but, see comment in the .cc.
> > 
> > I was recommended to use an enum on the other CL (crrev.com/562583002), but
we
> > can discuss with junov/senorblanco if you feel strongly about it.
> 
> I guess different owners with different tastes. I'd prefer to optimize for the
> simplest possible API now, rather than potential future changes that are
> uncertain.
> 
> If they want an enum you can turn that into a bool at some level before the
call
> to this method?

Done.

[jackhou1, 2014-11-17 05:04:17]

danakj, ping.

[danakj, 2014-11-17 17:39:41]

https://codereview.chromium.org/558083002/diff/180001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.h (right):

https://codereview.chromium.org/558083002/diff/180001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:14: #include
"third_party/skia/include/core/SkPaint.h"
unused

https://codereview.chromium.org/558083002/diff/180001/cc/output/gl_renderer_d...
File cc/output/gl_renderer_draw_cache.h (right):

https://codereview.chromium.org/558083002/diff/180001/cc/output/gl_renderer_d...
cc/output/gl_renderer_draw_cache.h:36: bool nearest_neighbor;
please init this variable in the constructor

https://codereview.chromium.org/558083002/diff/180001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/558083002/diff/180001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:2673: // The sampler will reset these
because |nearest_neighbor| was set on the
It would be nice to test 2 TextureMailboxes, one with and one without nearest
neighbour set, so we can see the behaviour for each one.

[jackhou1, 2014-11-17 22:46:35]

https://codereview.chromium.org/558083002/diff/180001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.h (right):

https://codereview.chromium.org/558083002/diff/180001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.h:14: #include
"third_party/skia/include/core/SkPaint.h"
On 2014/11/17 17:39:41, danakj wrote:
> unused

Done.

https://codereview.chromium.org/558083002/diff/180001/cc/output/gl_renderer_d...
File cc/output/gl_renderer_draw_cache.h (right):

https://codereview.chromium.org/558083002/diff/180001/cc/output/gl_renderer_d...
cc/output/gl_renderer_draw_cache.h:36: bool nearest_neighbor;
On 2014/11/17 17:39:41, danakj wrote:
> please init this variable in the constructor

Done.

https://codereview.chromium.org/558083002/diff/180001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/558083002/diff/180001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:2673: // The sampler will reset these
because |nearest_neighbor| was set on the
On 2014/11/17 17:39:41, danakj wrote:
> It would be nice to test 2 TextureMailboxes, one with and one without nearest
> neighbour set, so we can see the behaviour for each one.

Refactored this test in a similar way to ResourceProviderTestTextureFilters.

[danakj, 2014-11-18 16:17:05]

https://codereview.chromium.org/558083002/diff/200001/cc/output/gl_renderer_d...
File cc/output/gl_renderer_draw_cache.cc (right):

https://codereview.chromium.org/558083002/diff/200001/cc/output/gl_renderer_d...
cc/output/gl_renderer_draw_cache.cc:10: : program_id(0), nearest_neighbor(false)
{}
Oh I guess you'll need to rebase :) I've since initialized everything in here.

https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:2676: // The sampler will reset these
because |nearest_neighbor| was set on the
These expectations will differ depending on nearest_neighbour| though, no?

[jackhou1, 2014-11-19 03:33:17]

https://codereview.chromium.org/558083002/diff/200001/cc/output/gl_renderer_d...
File cc/output/gl_renderer_draw_cache.cc (right):

https://codereview.chromium.org/558083002/diff/200001/cc/output/gl_renderer_d...
cc/output/gl_renderer_draw_cache.cc:10: : program_id(0), nearest_neighbor(false)
{}
On 2014/11/18 16:17:05, danakj wrote:
> Oh I guess you'll need to rebase :) I've since initialized everything in here.

Done.

https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:2676: // The sampler will reset these
because |nearest_neighbor| was set on the
On 2014/11/18 16:17:05, danakj wrote:
> These expectations will differ depending on nearest_neighbour| though, no?

Updated the comment.

At the moment the tests are set up so that the filter always changes, so these
get called either way. If you like, I can add the NearestToNearest and
LinearToLinear cases, but I'm not sure if there's much benefit to that.

[danakj, 2014-11-19 16:01:56]

https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:2676: // The sampler will reset these
because |nearest_neighbor| was set on the
On 2014/11/19 03:33:17, jackhou1 wrote:
> On 2014/11/18 16:17:05, danakj wrote:
> > These expectations will differ depending on nearest_neighbour| though, no?
> 
> Updated the comment.
> 
> At the moment the tests are set up so that the filter always changes, so these
> get called either way. If you like, I can add the NearestToNearest and
> LinearToLinear cases, but I'm not sure if there's much benefit to that.

Ah right I missed the sampler part, thanks for the comment.

Do we have any test that shows we don't redundantly set the texParameters with
every texture lock if it's not needed? I think that's something we want to
maintain (which the test used to test) so it's worth testing to not regress.

[jackhou1, 2014-12-01 06:42:02]

On 2014/11/19 16:01:56, danakj wrote:
>
https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
> File cc/resources/resource_provider_unittest.cc (right):
> 
>
https://codereview.chromium.org/558083002/diff/200001/cc/resources/resource_p...
> cc/resources/resource_provider_unittest.cc:2676: // The sampler will reset
these
> because |nearest_neighbor| was set on the
> On 2014/11/19 03:33:17, jackhou1 wrote:
> > On 2014/11/18 16:17:05, danakj wrote:
> > > These expectations will differ depending on nearest_neighbour| though, no?
> > 
> > Updated the comment.
> > 
> > At the moment the tests are set up so that the filter always changes, so
these
> > get called either way. If you like, I can add the NearestToNearest and
> > LinearToLinear cases, but I'm not sure if there's much benefit to that.
> 
> Ah right I missed the sampler part, thanks for the comment.
> 
> Do we have any test that shows we don't redundantly set the texParameters with
> every texture lock if it's not needed? I think that's something we want to
> maintain (which the test used to test) so it's worth testing to not regress.

Done.

[danakj, 2014-12-01 16:11:54]

LGTM

[jackhou1, 2014-12-02 04:00:50]

jackhou@chromium.org changed reviewers:
+ jamesr@chromium.org, jschuh@chromium.org

[jackhou1, 2014-12-02 04:00:51]

jschuh, please review for OWNERS in content/common/

jamesr, please review for OWNERS in mojo/

[jamesr, 2014-12-02 05:47:12]

mojo/ lgtm

(fyi the mojo/services/public code is maintained in the mojo repository so we'll
have to make this patch there, but i can land it there for you)

[jschuh, 2014-12-02 21:48:13]

ipc security lgtm. notes: additional bool

[jackhou1, 2014-12-03 03:30:49]

On 2014/12/02 05:47:12, jamesr wrote:
> mojo/ lgtm
> 
> (fyi the mojo/services/public code is maintained in the mojo repository so
we'll
> have to make this patch there, but i can land it there for you)

Thanks. Do we need to update the mojo repository before landing this? Or should
I just land this including the change to mojo/services/public?

[jamesr, 2014-12-03 05:17:54]

On 2014/12/03 03:30:49, jackhou1 wrote:
> On 2014/12/02 05:47:12, jamesr wrote:
> > mojo/ lgtm
> > 
> > (fyi the mojo/services/public code is maintained in the mojo repository so
> we'll
> > have to make this patch there, but i can land it there for you)
> 
> Thanks. Do we need to update the mojo repository before landing this? Or
should
> I just land this including the change to mojo/services/public?

You can land this patch as is and I'll take care of the rest. I have a patch
adapting this in the mojo repo with green try runs ready to go. Just letting you
know what the situation is in case anything goes wonky.

[jackhou1, 2014-12-03 05:26:33]

On 2014/12/03 05:17:54, jamesr wrote:
> On 2014/12/03 03:30:49, jackhou1 wrote:
> > On 2014/12/02 05:47:12, jamesr wrote:
> > > mojo/ lgtm
> > > 
> > > (fyi the mojo/services/public code is maintained in the mojo repository so
> > we'll
> > > have to make this patch there, but i can land it there for you)
> > 
> > Thanks. Do we need to update the mojo repository before landing this? Or
> should
> > I just land this including the change to mojo/services/public?
> 
> You can land this patch as is and I'll take care of the rest. I have a patch
> adapting this in the mojo repo with green try runs ready to go. Just letting
you
> know what the situation is in case anything goes wonky.

Awesome, thanks.

[jackhou1, 2014-12-04 05:21:20]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-12-04 05:22:23]

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

[commit-bot: I haz the power, 2014-12-04 05:24:54]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2014-12-04 05:25:38]

Patchset 16 (id:??) landed as
https://crrev.com/10c9af41a73ac7db787d64344630f8200925cfe5
Cr-Commit-Position: refs/heads/master@{#306768}