cc: Remove redundant is_overlay arguments

cc: Remove redundant overlay arguments

Whether or not a quad can be made into an overlay is dependent on
whether there is a GLImage backing the texture resources that the quad
uses.

Prior to this patch, this information was passed along to the layer or
quad via a side-channel, in the form of a "allow overlay" argument.

Now, store this information in the cc::Resource, in particular, through
the cc::TextureMailbox that it is created from, and have the overlay
strategy query the property of the cc::Resource.

Add appropriate plumbing of the parameters from the cc::TextureMailbox,
through the cc::Resource, to cc::TransferableResource, and finally to
a cc::Resource in other compositors.

R=reveman,fsamuel,dcheng
TBR=danakj
BUG=533690
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/268c09fdae64b08957aba9b31797adb93525c90f
Cr-Commit-Position: refs/heads/master@{#353128}

[ccameron, 2015-10-07 00:45:14]

ccameron@chromium.org changed reviewers:
+ alexst@chromium.org, reveman@chromium.org

[ccameron, 2015-10-07 00:47:58]

Turned out to be fairly simple!

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.cc:1576: resource->dirty_image = false;
In a follow-on patch, I will set bound_to_gl_image to true at this point,
whereupon tiles and textures on Mac can become overlay candidates (!)

[reveman, 2015-10-07 09:32:59]

Been thinking a bit more about this and I'm not sure the bound_to_image naming
for this is the best. It leaks a bit too much details of the implementation to
code that should not be aware of the concept of GLImages and we might want to
have overlay candidates in the future that are not necessarily GLImage backed
and this breaks that.

Re-factor done by this patch looks good in general. I would just prefer if we
kept the "allow_overlay" name or maybe even better use "overlay_candidate" as
that seems to represent the intent even better afaict.

https://codereview.chromium.org/1387283002/diff/1/cc/quads/io_surface_draw_qu...
File cc/quads/io_surface_draw_quad.h (right):

https://codereview.chromium.org/1387283002/diff/1/cc/quads/io_surface_draw_qu...
cc/quads/io_surface_draw_quad.h:31: Orientation orientation);
Btw, what is this quad type used for these days? npapi plugins? Can we remove
it?

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:500: bool bound_to_image : 1;
I think this is a bit confusing as we already have Resource::bound_image_id. I
think "allow_overlay" or simply "overlay_candidate" would be better.

[ccameron, 2015-10-07 19:13:11]

Good ideas -- updated.

https://codereview.chromium.org/1387283002/diff/1/cc/quads/io_surface_draw_qu...
File cc/quads/io_surface_draw_quad.h (right):

https://codereview.chromium.org/1387283002/diff/1/cc/quads/io_surface_draw_qu...
cc/quads/io_surface_draw_quad.h:31: Orientation orientation);
On 2015/10/07 09:32:58, reveman wrote:
> Btw, what is this quad type used for these days? npapi plugins? Can we remove
> it?

This is used by videos (h264 and vp9). We could probably convert these to just
be texture quads. There may be other instances that I don't know of.

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:500: bool bound_to_image : 1;
On 2015/10/07 09:32:58, reveman wrote:
> I think this is a bit confusing as we already have Resource::bound_image_id. I
> think "allow_overlay" or simply "overlay_candidate" would be better.

Yeah the original name promises a bit too much. Changed to
"is_overlay_candidate" (because "overlay_candidate" name suggests the type is
not bool).

[reveman, 2015-10-07 19:24:18]

lgtm

https://codereview.chromium.org/1387283002/diff/1/cc/quads/io_surface_draw_qu...
File cc/quads/io_surface_draw_quad.h (right):

https://codereview.chromium.org/1387283002/diff/1/cc/quads/io_surface_draw_qu...
cc/quads/io_surface_draw_quad.h:31: Orientation orientation);
On 2015/10/07 at 19:13:11, ccameron wrote:
> On 2015/10/07 09:32:58, reveman wrote:
> > Btw, what is this quad type used for these days? npapi plugins? Can we
remove
> > it?
> 
> This is used by videos (h264 and vp9). We could probably convert these to just
be texture quads. There may be other instances that I don't know of.

That would be great!

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1387283002/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:500: bool bound_to_image : 1;
On 2015/10/07 at 19:13:11, ccameron wrote:
> On 2015/10/07 09:32:58, reveman wrote:
> > I think this is a bit confusing as we already have Resource::bound_image_id.
I
> > think "allow_overlay" or simply "overlay_candidate" would be better.
> 
> Yeah the original name promises a bit too much. Changed to
"is_overlay_candidate" (because "overlay_candidate" name suggests the type is
not bool).

Good point.

[ccameron, 2015-10-07 20:07:10]

ccameron@chromium.org changed reviewers:
+ fsamuel@chromium.org

[ccameron, 2015-10-07 20:07:10]

Thanks! Adding fsamuel for mojo

[Fady Samuel, 2015-10-07 21:00:52]

mojo lgtm

[ccameron, 2015-10-07 21:15:34]

Thanks!

[ccameron, 2015-10-07 21:15:40]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-07 21:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1387283002/#ps60001
(title: "More mojo changes")

[commit-bot: I haz the power, 2015-10-07 21:17:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387283002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387283002/60001

[commit-bot: I haz the power, 2015-10-07 21:32:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-07 21:32:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2015-10-07 21:41:03]

ccameron@chromium.org changed reviewers:
+ dcheng@chromium.org

[ccameron, 2015-10-07 21:41:03]

Adding dcheng@ for content/common/*message*

[dcheng, 2015-10-08 04:07:14]

ipc changes lgtm

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (left):

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:23: QuadList& quad_list =
render_passes->back()->quad_list;
FYI, the style guide does allow mutable references, just not mutable reference
arguments.

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:42: for (auto overlap_iter =
quad_list->cbegin();
If you're going to change this, you may as well just change it to for (const
auto& quad : quad_list) { ... }

https://codereview.chromium.org/1387283002/diff/60001/cc/quads/draw_quad_unit...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/1387283002/diff/60001/cc/quads/draw_quad_unit...
cc/quads/draw_quad_unittest.cc:389: CREATE_QUAD_5_NEW(IOSurfaceDrawQuad,
opaque_rect, visible_rect, size,
This can wait for a future patch, but I suspect a variadic macro will make life
a lot easier here.

[ccameron, 2015-10-08 04:36:51]

Thanks!

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (left):

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:23: QuadList& quad_list =
render_passes->back()->quad_list;
On 2015/10/08 04:07:14, dcheng wrote:
> FYI, the style guide does allow mutable references, just not mutable reference
> arguments.

I wonder how line 36 was okay then.

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:42: for (auto overlap_iter =
quad_list->cbegin();
On 2015/10/08 04:07:14, dcheng wrote:
> If you're going to change this, you may as well just change it to for (const
> auto& quad : quad_list) { ... }

Done.

https://codereview.chromium.org/1387283002/diff/60001/cc/quads/draw_quad_unit...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/1387283002/diff/60001/cc/quads/draw_quad_unit...
cc/quads/draw_quad_unittest.cc:389: CREATE_QUAD_5_NEW(IOSurfaceDrawQuad,
opaque_rect, visible_rect, size,
On 2015/10/08 04:07:14, dcheng wrote:
> This can wait for a future patch, but I suspect a variadic macro will make
life
> a lot easier here.

Mmh, good point.

[ccameron, 2015-10-08 04:36:58]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-08 04:36:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, reveman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1387283002/#ps80001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2015-10-08 04:37:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387283002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387283002/80001

[dcheng, 2015-10-08 04:45:20]

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (left):

https://codereview.chromium.org/1387283002/diff/60001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:23: QuadList& quad_list =
render_passes->back()->quad_list;
On 2015/10/08 at 04:36:51, ccameron wrote:
> On 2015/10/08 04:07:14, dcheng wrote:
> > FYI, the style guide does allow mutable references, just not mutable
reference
> > arguments.
> 
> I wonder how line 36 was okay then.

It's not style guide compliant: I imagine it was just an accident.

[commit-bot: I haz the power, 2015-10-08 04:45:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 04:45:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2015-10-08 05:12:52]

On 2015/10/08 04:45:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

TBRing owners for cc_messages_unittest.cc (cause there's just one).

[ccameron, 2015-10-08 05:22:19]

https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:42: for (auto overlap_iter :
*quad_list) {
Hmm, this broke things. I'll put this in the category of "types improve
readability, why again is C++ trying to hide them?"

[ccameron, 2015-10-08 05:22:25]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-08 05:22:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, reveman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1387283002/#ps100001
(title: "Undo changes on overlay_strategy_single_on_top.cc")

[commit-bot: I haz the power, 2015-10-08 05:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387283002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387283002/100001

[dcheng, 2015-10-08 05:44:15]

https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
File cc/output/overlay_strategy_single_on_top.cc (right):

https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
cc/output/overlay_strategy_single_on_top.cc:42: for (auto overlap_iter :
*quad_list) {
On 2015/10/08 at 05:22:18, ccameron wrote:
> Hmm, this broke things. I'll put this in the category of "types improve
readability, why again is C++ trying to hide them?"

I'm not sure why this broke things, but note that:

for (auto overlap_iter : *quads_list) {

is copying a DrawQuad. Usually you want:

for (const auto& quad : *quads_list) {

or:

for (auto& quad : *quads_list) {

[commit-bot: I haz the power, 2015-10-08 06:25:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-08 06:25:37]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[danakj, 2015-10-08 16:24:39]

On 2015/10/08 05:12:52, ccameron wrote:
> On 2015/10/08 04:45:26, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> TBRing owners for cc_messages_unittest.cc (cause there's just one).

unittest changes LGTM

[ccameron, 2015-10-08 18:55:16]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-10-08 18:57:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387283002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387283002/100001

[ccameron, 2015-10-08 19:01:29]

On 2015/10/08 05:44:15, dcheng wrote:
>
https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
> File cc/output/overlay_strategy_single_on_top.cc (right):
> 
>
https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
> cc/output/overlay_strategy_single_on_top.cc:42: for (auto overlap_iter :
> *quad_list) {
> On 2015/10/08 at 05:22:18, ccameron wrote:
> > Hmm, this broke things. I'll put this in the category of "types improve
> readability, why again is C++ trying to hide them?"
> 
> I'm not sure why this broke things, but note that:
> 
> for (auto overlap_iter : *quads_list) {
> 
> is copying a DrawQuad. Usually you want:

Good point  ...

> for (const auto& quad : *quads_list) {

... but this one didn't work ...

> for (auto& quad : *quads_list) {

... and this one didn't compile.

So ... I don't know C++ anymore :(

[weiliangc, 2015-10-08 20:00:09]

On 2015/10/08 at 19:01:29, ccameron wrote:
> On 2015/10/08 05:44:15, dcheng wrote:
> >
https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
> > File cc/output/overlay_strategy_single_on_top.cc (right):
> > 
> >
https://codereview.chromium.org/1387283002/diff/80001/cc/output/overlay_strat...
> > cc/output/overlay_strategy_single_on_top.cc:42: for (auto overlap_iter :
> > *quad_list) {
> > On 2015/10/08 at 05:22:18, ccameron wrote:
> > > Hmm, this broke things. I'll put this in the category of "types improve
> > readability, why again is C++ trying to hide them?"
> > 
> > I'm not sure why this broke things, but note that:
> > 
> > for (auto overlap_iter : *quads_list) {
> > 
> > is copying a DrawQuad. Usually you want:

It's actually a pointer to DrawQuad so should be fine if you want to use it this
way.
> 
> Good point  ...
> 
> > for (const auto& quad : *quads_list) {
> 
> ... but this one didn't work ...

This should work. Possibly that you need to break the loop when it reaches
candidate_iterator, not end().
> 
> > for (auto& quad : *quads_list) {
> 
> ... and this one didn't compile.
> 
> So ... I don't know C++ anymore :(

[commit-bot: I haz the power, 2015-10-08 20:23:55]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-08 20:25:22]

Patchset 6 (id:??) landed as
https://crrev.com/268c09fdae64b08957aba9b31797adb93525c90f
Cr-Commit-Position: refs/heads/master@{#353128}