Use gpu::Mailbox in IPCs instead of std::string

Use gpu::Mailbox in IPCs instead of std::string

gpu::Mailbox is safer since the size is checked in the deserializer. It's also
overall less code.

BUG=None


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

[piman, 2013-03-06 21:18:44]

Alex: browser_plugin and cc/
Daniel: content/browser/renderer_host, content/common/gpu, ui/compositor
Justin: IPC (same as before)

[alexst (slow to review), 2013-03-06 21:54:27]

BrowserPlugin and CC lgtm with a couple small nits.

https://codereview.chromium.org/12440005/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/12440005/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_embedder.h:29: namespace gpu {
We shouldn't need this here, it's not referenced anywhere in the file.

https://codereview.chromium.org/12440005/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/12440005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:89:
context->consumeTextureCHROMIUM( GL_TEXTURE_2D, mailbox_name.name);
There is an extra space here
                       
consumeTextureCHROMIUM( GL_TEXTURE_2D
                       ^

[no sievers, 2013-03-06 23:19:23]

Nice! LGTM. Just a few nits/comments.

https://codereview.chromium.org/12440005/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12440005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/image_transport_factory.cc:168: if
(mailbox_name.IsZero())
I guess you could remove that now, since it's already checked by the caller. I
had originally added it out of paranoia since with the string it was riskier to
access the buffer if the size wasn't right (now I guess you'd just get a GL
error)...

https://codereview.chromium.org/12440005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/image_transport_factory.cc:180: if
(!mailbox_name_.IsZero()) {
...and I think this could be a DCHECK() now since we should never try to return
a texture that didn't have a valid mailbox name when we received it (we would
skip the frame).

https://codereview.chromium.org/12440005/diff/1/content/common/gpu/texture_im...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/12440005/diff/1/content/common/gpu/texture_im...
content/common/gpu/texture_image_transport_surface.cc:404:
name.SetName(new_mailbox_name.key);
hmm, should MailboxName have an operator maybe?

https://codereview.chromium.org/12440005/diff/1/content/common/gpu/texture_im...
content/common/gpu/texture_image_transport_surface.cc:446: MailboxName name;
Also  maybe a constructor from gpu::Mailbox?

https://codereview.chromium.org/12440005/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12440005/diff/1/ui/compositor/compositor.cc#n...
ui/compositor/compositor.cc:14: #include "base/string_util.h"
I think string_util.h is not needed anymore after removing EmptyString() below.

[piman, 2013-03-07 01:48:02]

https://codereview.chromium.org/12440005/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/image_transport_factory.cc (right):

https://codereview.chromium.org/12440005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/image_transport_factory.cc:168: if
(mailbox_name.IsZero())
On 2013/03/06 23:19:23, Daniel Sievers wrote:
> I guess you could remove that now, since it's already checked by the caller. I
> had originally added it out of paranoia since with the string it was riskier
to
> access the buffer if the size wasn't right (now I guess you'd just get a GL
> error)...

Done.

https://codereview.chromium.org/12440005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/image_transport_factory.cc:180: if
(!mailbox_name_.IsZero()) {
On 2013/03/06 23:19:23, Daniel Sievers wrote:
> ...and I think this could be a DCHECK() now since we should never try to
return
> a texture that didn't have a valid mailbox name when we received it (we would
> skip the frame).

Done.

https://codereview.chromium.org/12440005/diff/1/content/common/gpu/texture_im...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://codereview.chromium.org/12440005/diff/1/content/common/gpu/texture_im...
content/common/gpu/texture_image_transport_surface.cc:404:
name.SetName(new_mailbox_name.key);
On 2013/03/06 23:19:23, Daniel Sievers wrote:
> hmm, should MailboxName have an operator maybe?

cast operators are discouraged by the style guide. We could make a function, but
since there's only one caller, there's little value. What I would really want to
do is make MailboxName a gpu::Mailbox, but I would do that as a separate change.

https://codereview.chromium.org/12440005/diff/1/content/common/gpu/texture_im...
content/common/gpu/texture_image_transport_surface.cc:446: MailboxName name;
On 2013/03/06 23:19:23, Daniel Sievers wrote:
> Also  maybe a constructor from gpu::Mailbox?

Done.

https://codereview.chromium.org/12440005/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/12440005/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:89:
context->consumeTextureCHROMIUM( GL_TEXTURE_2D, mailbox_name.name);
On 2013/03/06 21:54:27, alexst wrote:
> There is an extra space here
>                        
> consumeTextureCHROMIUM( GL_TEXTURE_2D
>                        ^

Done.

https://codereview.chromium.org/12440005/diff/1/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12440005/diff/1/ui/compositor/compositor.cc#n...
ui/compositor/compositor.cc:14: #include "base/string_util.h"
On 2013/03/06 23:19:23, Daniel Sievers wrote:
> I think string_util.h is not needed anymore after removing EmptyString()
below.

Done.

[jschuh, 2013-03-07 03:24:05]

strings to a structured type. yay! rubberstamp lgtm.

[commit-bot: I haz the power, 2013-03-07 03:35:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/12440005/17004

[commit-bot: I haz the power, 2013-03-07 05:33:43]

Change committed as 186627