Let every "accelerated IO surface swapped" message have an identifier of the surface.

Let every "accelerated IO surface swapped" message have an identifier of the surface.

Currently, this is only used to DCHECK a currently implicit invariant, but I want to use this to let every surface container only remember its last painted-to surface, and not its last created surface.

No behavior change.

BUG=53165
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64317

[stuartmorgan, 2010-10-26 22:57:39]

Can you elaborate on what the benefit is here? You mention the container not
having to remember an ID, but this seems like an awful lot of code just to
remove one member variable somewhere.

http://codereview.chromium.org/4142004/diff/5001/6001
File app/surface/accelerated_surface_mac.cc (right):

http://codereview.chromium.org/4142004/diff/5001/6001#newcode276
app/surface/accelerated_surface_mac.cc:276: return
io_surface_support->IOSurfaceGetID(io_surface_);
Perhaps this ID should be cached when io_surface_ is created? This is going to
get called a *lot*

http://codereview.chromium.org/4142004/diff/5001/6002
File app/surface/accelerated_surface_mac.h (right):

http://codereview.chromium.org/4142004/diff/5001/6002#newcode61
app/surface/accelerated_surface_mac.h:61: // Returns 0 for transport DIB
surfaces.
Should actually say what it returns the rest of the time too.

http://codereview.chromium.org/4142004/diff/5001/6005
File chrome/browser/renderer_host/accelerated_surface_container_mac.cc (right):

http://codereview.chromium.org/4142004/diff/5001/6005#newcode211
chrome/browser/renderer_host/accelerated_surface_container_mac.cc:211:
DCHECK_EQ(surface_id, io_surface_support->IOSurfaceGetID(surface_));
Again, I'm a bit concerned about the potential performance impact.

http://codereview.chromium.org/4142004/diff/5001/6016
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/4142004/diff/5001/6016#newcode149
chrome/common/gpu_messages_internal.h:149: uint64 /* surface id, or 0 */)
Comment should be the variable name (we aren't always consistent, but that's the
rule I've been told).

http://codereview.chromium.org/4142004/diff/5001/6017
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/4142004/diff/5001/6017#newcode463
chrome/common/plugin_messages_internal.h:463: uint64 /* identifier for
IOSurface, or 0 */)
Same here

http://codereview.chromium.org/4142004/diff/5001/6018
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/4142004/diff/5001/6018#newcode2216
chrome/common/render_messages_internal.h:2216: uint64 /* identifier for
IOSurface, or 0 */)
And here.

http://codereview.chromium.org/4142004/diff/5001/6028
File gpu/command_buffer/service/gpu_processor.h (right):

http://codereview.chromium.org/4142004/diff/5001/6028#newcode86
gpu/command_buffer/service/gpu_processor.h:86: // Returns 0 of the current
IOSurface, or 0.
"0 of the" -> "id of the"

[Nico, 2010-10-27 00:19:59]

On Tue, Oct 26, 2010 at 3:57 PM,  <stuartmorgan@chromium.org> wrote:
> Can you elaborate on what the benefit is here? You mention the container not
> having to remember an ID, but this seems like an awful lot of code just to
> remove one member variable somewhere.

The immediate benefit is that it's possible to DCHECK an invariant
that's implicit and somewhat easy to break. When resizing a composited
tab, it's feasible that the ipcs to the browser process are
1. Resize IOSurface to 100, 100
2. Resize IOSurface to 110, 100
3. Resize IOSurface to 120, 100
4. Surface from 1 is swapped
5. Surface from 1 is swapped
6. Surface from 1 is swapped

…which would silently surprise the current code.

I think that this patch makes it also pretty easy to fix the garbage
that's visible during plugin / composited tab resizing;
proof-of-concept at http://codereview.chromium.org/4101002/show

While this CL touches many files, the actual change is relatively
minor, it just adds one parameter (and plumbs it through all IPC).

I will wait with addressing your comments below until we agree that
this change is worth doing :-)

>
>
> http://codereview.chromium.org/4142004/diff/5001/6001
> File app/surface/accelerated_surface_mac.cc (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6001#newcode276
> app/surface/accelerated_surface_mac.cc:276: return
> io_surface_support->IOSurfaceGetID(io_surface_);
> Perhaps this ID should be cached when io_surface_ is created? This is
> going to get called a *lot*
>
> http://codereview.chromium.org/4142004/diff/5001/6002
> File app/surface/accelerated_surface_mac.h (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6002#newcode61
> app/surface/accelerated_surface_mac.h:61: // Returns 0 for transport DIB
> surfaces.
> Should actually say what it returns the rest of the time too.
>
> http://codereview.chromium.org/4142004/diff/5001/6005
> File chrome/browser/renderer_host/accelerated_surface_container_mac.cc
> (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6005#newcode211
> chrome/browser/renderer_host/accelerated_surface_container_mac.cc:211:
> DCHECK_EQ(surface_id, io_surface_support->IOSurfaceGetID(surface_));
> Again, I'm a bit concerned about the potential performance impact.
>
> http://codereview.chromium.org/4142004/diff/5001/6016
> File chrome/common/gpu_messages_internal.h (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6016#newcode149
> chrome/common/gpu_messages_internal.h:149: uint64 /* surface id, or 0
> */)
> Comment should be the variable name (we aren't always consistent, but
> that's the rule I've been told).
>
> http://codereview.chromium.org/4142004/diff/5001/6017
> File chrome/common/plugin_messages_internal.h (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6017#newcode463
> chrome/common/plugin_messages_internal.h:463: uint64 /* identifier for
> IOSurface, or 0 */)
> Same here
>
> http://codereview.chromium.org/4142004/diff/5001/6018
> File chrome/common/render_messages_internal.h (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6018#newcode2216
> chrome/common/render_messages_internal.h:2216: uint64 /* identifier for
> IOSurface, or 0 */)
> And here.
>
> http://codereview.chromium.org/4142004/diff/5001/6028
> File gpu/command_buffer/service/gpu_processor.h (right):
>
> http://codereview.chromium.org/4142004/diff/5001/6028#newcode86
> gpu/command_buffer/service/gpu_processor.h:86: // Returns 0 of the
> current IOSurface, or 0.
> "0 of the" -> "id of the"
>
> http://codereview.chromium.org/4142004/show
>

[Nico, 2010-10-28 19:40:32]

http://codereview.chromium.org/4142004/diff/5001/6001
File app/surface/accelerated_surface_mac.cc (right):

http://codereview.chromium.org/4142004/diff/5001/6001#newcode276
app/surface/accelerated_surface_mac.cc:276: return
io_surface_support->IOSurfaceGetID(io_surface_);
On 2010/10/26 22:57:39, stuartmorgan wrote:
> Perhaps this ID should be cached when io_surface_ is created? This is going to
> get called a *lot*

Done.

http://codereview.chromium.org/4142004/diff/5001/6002
File app/surface/accelerated_surface_mac.h (right):

http://codereview.chromium.org/4142004/diff/5001/6002#newcode61
app/surface/accelerated_surface_mac.h:61: // Returns 0 for transport DIB
surfaces.
On 2010/10/26 22:57:39, stuartmorgan wrote:
> Should actually say what it returns the rest of the time too.

Done.

http://codereview.chromium.org/4142004/diff/5001/6005
File chrome/browser/renderer_host/accelerated_surface_container_mac.cc (right):

http://codereview.chromium.org/4142004/diff/5001/6005#newcode211
chrome/browser/renderer_host/accelerated_surface_container_mac.cc:211:
DCHECK_EQ(surface_id, io_surface_support->IOSurfaceGetID(surface_));
On 2010/10/26 22:57:39, stuartmorgan wrote:
> Again, I'm a bit concerned about the potential performance impact.

Done.

http://codereview.chromium.org/4142004/diff/5001/6016
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/4142004/diff/5001/6016#newcode149
chrome/common/gpu_messages_internal.h:149: uint64 /* surface id, or 0 */)
On 2010/10/26 22:57:39, stuartmorgan wrote:
> Comment should be the variable name (we aren't always consistent, but that's
the
> rule I've been told).

Done.

http://codereview.chromium.org/4142004/diff/5001/6017
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/4142004/diff/5001/6017#newcode463
chrome/common/plugin_messages_internal.h:463: uint64 /* identifier for
IOSurface, or 0 */)
On 2010/10/26 22:57:39, stuartmorgan wrote:
> Same here

Done.

http://codereview.chromium.org/4142004/diff/5001/6018
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/4142004/diff/5001/6018#newcode2216
chrome/common/render_messages_internal.h:2216: uint64 /* identifier for
IOSurface, or 0 */)
On 2010/10/26 22:57:39, stuartmorgan wrote:
> And here.

Done.

http://codereview.chromium.org/4142004/diff/5001/6028
File gpu/command_buffer/service/gpu_processor.h (right):

http://codereview.chromium.org/4142004/diff/5001/6028#newcode86
gpu/command_buffer/service/gpu_processor.h:86: // Returns 0 of the current
IOSurface, or 0.
On 2010/10/26 22:57:39, stuartmorgan wrote:
> "0 of the" -> "id of the"

Oops :-)

Done.

[stuartmorgan, 2010-10-28 21:29:52]

LGTM with nit.

http://codereview.chromium.org/4142004/diff/18001/19015
File chrome/browser/renderer_host/test/test_render_view_host.h (right):

http://codereview.chromium.org/4142004/diff/18001/19015#newcode122
chrome/browser/renderer_host/test/test_render_view_host.h:122: uint64);
Add variable name.