Cosmetic cleanup to texture_layer mailboxes.

Cosmetic cleanup to texture_layer mailboxes.


BUG=


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

[alexst (slow to review), 2013-01-14 19:20:51]

Dana, it's followup to the mailbox changes to clean up the interface a bit.

This is a rough proposal of what I was thinking. I want to run this by you
because we also have a Mailbox struct in transferable_resources.h, but it's a
resource_provider construct and seems like texture_layer should not directly
depend on it and neither should the texture_layer users.

I want to reconcile this duplication with you.

[danakj, 2013-01-16 01:59:03]

I like this!

+piman

We should maybe rename the current Mailbox to TransferrableMailbox to denote
it's for ipcs?

https://codereview.chromium.org/11888010/diff/1/cc/texture_layer.cc
File cc/texture_layer.cc (right):

https://codereview.chromium.org/11888010/diff/1/cc/texture_layer.cc#newcode125
cc/texture_layer.cc:125: DCHECK(mailbox.name.empty() ==
mailbox.callback.is_null());
Can we enforce this inside TextureMailbox instead?

https://codereview.chromium.org/11888010/diff/1/cc/texture_layer.cc#newcode126
cc/texture_layer.cc:126: if (m_textureMailbox.name.compare(mailbox.name) == 0)
let's add an Equals or Compare method on TextureMailbox

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.cc
File cc/texture_mailbox.cc (right):

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.cc#newcode13
cc/texture_mailbox.cc:13: TextureMailbox::TextureMailbox(const std::string&
mailbox_name,
move first param to new line

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.cc#newcode31
cc/texture_mailbox.cc:31: if (!callback.is_null()) {
no {}

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.h
File cc/texture_mailbox.h (right):

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.h#newcode16
cc/texture_mailbox.h:16: struct CC_EXPORT TextureMailbox {
i prefer class even with struct-like public: things. it's impossible to remember
what to forward declare as a struct.

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.h#newcode24
cc/texture_mailbox.h:24: bool isZero() const;
IsEmpty?

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.h#newcode25
cc/texture_mailbox.h:25: void runCallback(unsigned sync_point);
Run*

[danakj, 2013-01-16 16:11:55]

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.h
File cc/texture_mailbox.h (right):

https://codereview.chromium.org/11888010/diff/1/cc/texture_mailbox.h#newcode25
cc/texture_mailbox.h:25: void runCallback(unsigned sync_point);
On 2013/01/16 01:59:03, danakj wrote:
> Run*

I mean, RunReleaseCallback().

[alexst (slow to review), 2013-01-16 20:03:20]

New patch.

[danakj, 2013-01-16 22:22:13]

https://codereview.chromium.org/11888010/diff/8001/cc/texture_mailbox.h
File cc/texture_mailbox.h (right):

https://codereview.chromium.org/11888010/diff/8001/cc/texture_mailbox.h#newco...
cc/texture_mailbox.h:25: const int8* Data() const;
I'd name this data() and move the body into the header, as it's just an
accessor.

https://codereview.chromium.org/11888010/diff/8001/cc/texture_mailbox.h#newco...
cc/texture_mailbox.h:29: std::string name_;
If these are meant to be public then drop the trailing underscore. That said, is
there a reason to not just add an accessor for them and make them private? This
class isn't for IPC so I think a class pattern would be preferable usually.

[piman, 2013-01-16 22:22:35]

Should we make cc::TextureMailbox a struct with a cc::Mailbox and the callback?
Among others, I'd like the idea of the string->char[64] conversion to happen
higher up in the stack, because sometimes we'll need to validate the string if
it comes from IPC before converting to char[64], and that's not really the role
of cc.

[alexst (slow to review), 2013-01-16 22:59:40]

On 2013/01/16 22:22:35, piman wrote:
> Should we make cc::TextureMailbox a struct with a cc::Mailbox and the
callback?
> Among others, I'd like the idea of the string->char[64] conversion to happen
> higher up in the stack, because sometimes we'll need to validate the string if
> it comes from IPC before converting to char[64], and that's not really the
role
> of cc.

I thought  about that, but implemented it this way initially because jamesr
expressed some concerns about introducing a dependency between resource provider
related classes and the TextureLayer.

Unless we want no header dependencies, it's a reasonable option, and I suppose
TextureLayer doesn't need to know how the underlying mailbox is stored.

We could also go middle of the road and store char[64] in TextureMailbox and do
all validation during construction.

Thoughts?

[piman, 2013-01-16 23:39:21]

On Wed, Jan 16, 2013 at 2:59 PM, <alexst@chromium.org> wrote:

> On 2013/01/16 22:22:35, piman wrote:
>
>> Should we make cc::TextureMailbox a struct with a cc::Mailbox and the
>>
> callback?
>
>> Among others, I'd like the idea of the string->char[64] conversion to
>> happen
>> higher up in the stack, because sometimes we'll need to validate the
>> string if
>> it comes from IPC before converting to char[64], and that's not really the
>>
> role
>
>> of cc.
>>
>
> I thought  about that, but implemented it this way initially because jamesr
> expressed some concerns about introducing a dependency between resource
> provider
> related classes and the TextureLayer.
>


> Unless we want no header dependencies, it's a reasonable option, and I
> suppose
> TextureLayer doesn't need to know how the underlying mailbox is stored.
>

cc::Mailbox is an "API" class, in the sense that it flows in an out of the
top-level API (OutputSurface, DelegatedRendererLayer), so it's not per-se a
ResourceProvider-specific class. It's used outside of cc, e.g. in the IPC
serialization.
Similarly, cc::TextureMailbox is an API class, so I don't see a problem
with having one depend on the other.


> We could also go middle of the road and store char[64] in TextureMailbox
> and do
> all validation during construction.
>

If needed, validation needs to happen close to the IPC layer so that it can
fail early (and possibly return failure on sync messages), instead of too
late: right now a string of the wrong size will cause a out-of-bounds read
in createResourceFromTextureMailbox (it's not caused by this patch, but
your previous one - we missed that). You can add a check
in createResourceFromTextureMailbox but you only have 2 options: DCHECK
(won't prevent issues in production) or fail to create the resource. If you
fail to create the resource, you may trigger other checks etc.
Passing a cc::Mailbox through IPC is safer, because if the data is
malformed, the IPC deserialization will reject the message. Hence the
suggestion to create a TextureMailbox out of the cc::Mailbox, that lets you
IPC cc::Mailbox around (or at least move the validation higher up in the
stack).


>
> Thoughts?
>
>
https://codereview.chromium.**org/11888010/<https://codereview.chromium.org/1...
>

[alexst (slow to review), 2013-01-17 20:52:52]

> If needed, validation needs to happen close to the IPC layer so that it can
> fail early (and possibly return failure on sync messages), instead of too
> late: right now a string of the wrong size will cause a out-of-bounds read
> in createResourceFromTextureMailbox (it's not caused by this patch, but
> your previous one - we missed that). You can add a check
> in createResourceFromTextureMailbox but you only have 2 options: DCHECK
> (won't prevent issues in production) or fail to create the resource. If you
> fail to create the resource, you may trigger other checks etc.
> Passing a cc::Mailbox through IPC is safer, because if the data is
> malformed, the IPC deserialization will reject the message. Hence the
> suggestion to create a TextureMailbox out of the cc::Mailbox, that lets you
> IPC cc::Mailbox around (or at least move the validation higher up in the
> stack).

Makes sense, thanks for the explanation. I put up a patch using cc::Mailbox for
storage, and added a DCHECK for string size until we can make the changes to
carry cc::Mailbox around through IPC instead of strings.

[piman, 2013-01-17 21:30:01]

Just couple of things left.

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.cc
File cc/texture_mailbox.cc (right):

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.cc#new...
cc/texture_mailbox.cc:19: DCHECK(mailbox_name.size() == sizeof(name_.name));
nit: could you make this a CHECK? If the check fails (because we missed
validation upstream), better crash safely than let an OOB read happen right
below.

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.h
File cc/texture_mailbox.h (right):

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.h#newc...
cc/texture_mailbox.h:33: void SetName(const int8* data);
Can we make this take a Mailbox as well? I think in almost all your use cases it
comes from a Mailbox anyway, except one where it could trivially be changed to
it.

[alexst (slow to review), 2013-01-17 22:10:11]

New patch

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.cc
File cc/texture_mailbox.cc (right):

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.cc#new...
cc/texture_mailbox.cc:19: DCHECK(mailbox_name.size() == sizeof(name_.name));
On 2013/01/17 21:30:01, piman wrote:
> nit: could you make this a CHECK? If the check fails (because we missed
> validation upstream), better crash safely than let an OOB read happen right
> below.

Done.

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.h
File cc/texture_mailbox.h (right):

https://codereview.chromium.org/11888010/diff/17001/cc/texture_mailbox.h#newc...
cc/texture_mailbox.h:33: void SetName(const int8* data);
On 2013/01/17 21:30:01, piman wrote:
> Can we make this take a Mailbox as well? I think in almost all your use cases
it
> comes from a Mailbox anyway, except one where it could trivially be changed to
> it.

Done.

[piman, 2013-01-17 22:20:42]

LGTM+nit

https://codereview.chromium.org/11888010/diff/21002/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11888010/diff/21002/cc/resource_provider.cc#n...
cc/resource_provider.cc:749: resource->mailbox.setName(name);
nit: I think you can simplify the 3 above lines to:
GLC(context3d, context3d->genMailboxCHROMIUM(resource->mailbox.name));

[alexst (slow to review), 2013-01-17 23:50:38]

Patch

https://codereview.chromium.org/11888010/diff/21002/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11888010/diff/21002/cc/resource_provider.cc#n...
cc/resource_provider.cc:749: resource->mailbox.setName(name);
On 2013/01/17 22:20:42, piman wrote:
> nit: I think you can simplify the 3 above lines to:
> GLC(context3d, context3d->genMailboxCHROMIUM(resource->mailbox.name));

Done. I was wondering if GLbyte name[GL_MAILBOX_SIZE_CHROMIUM]; was maybe there
for clarity, so I left it alone.

[piman, 2013-01-17 23:57:43]

lgtm

[danakj, 2013-01-18 00:13:08]

https://codereview.chromium.org/11888010/diff/25001/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11888010/diff/25001/cc/resource_provider.cc#n...
cc/resource_provider.cc:695: resource.mailbox.SetName(it->mailbox);
would it be safer to do resource.mailbox = TextureMailbox(it->mailbox); to
overwrite the callback with a null here?

https://codereview.chromium.org/11888010/diff/25001/cc/resource_provider.cc#n...
cc/resource_provider.cc:720: resource->mailbox.SetName(it->mailbox);
the name should already be set here since we gave it to the parent right? can we
just DCHECK they are equal here instead?

https://codereview.chromium.org/11888010/diff/25001/cc/texture_mailbox.h
File cc/texture_mailbox.h (right):

https://codereview.chromium.org/11888010/diff/25001/cc/texture_mailbox.h#newc...
cc/texture_mailbox.h:21: TextureMailbox(const std::string& name,
nit: same variable name as the .cc

[no sievers, 2013-01-18 01:27:40]

Cool stuff! Would be nice if there was also a sync point the layer could wait on
before consuming the texture  (for https://codereview.chromium.org/11861020/).
But I guess that's pretty easy to add when needed.

[alexst (slow to review), 2013-01-18 15:16:50]

On 2013/01/18 01:27:40, Daniel Sievers wrote:
> Cool stuff! Would be nice if there was also a sync point the layer could wait
on
> before consuming the texture  (for https://codereview.chromium.org/11861020/).
> But I guess that's pretty easy to add when needed.

Yeah, I think all we'll need to do is add another field to cc:TextureMailbox,
and
if(mailbox.sync_point())
  context->waitSyncPoint(..)
to the resource provider.

[alexst (slow to review), 2013-01-18 16:12:05]

And to  clarify my comment to Daniel, it's easy to add, but probably in a
separate patch since this one was only meant to cleanup and not add new
functionality.

https://codereview.chromium.org/11888010/diff/25001/cc/resource_provider.cc
File cc/resource_provider.cc (right):

https://codereview.chromium.org/11888010/diff/25001/cc/resource_provider.cc#n...
cc/resource_provider.cc:695: resource.mailbox.SetName(it->mailbox);
On 2013/01/18 00:13:08, danakj wrote:
> would it be safer to do resource.mailbox = TextureMailbox(it->mailbox); to
> overwrite the callback with a null here?

Since we just made this resource object, callback is already null, no need to
reset it again.

https://codereview.chromium.org/11888010/diff/25001/cc/resource_provider.cc#n...
cc/resource_provider.cc:720: resource->mailbox.SetName(it->mailbox);
On 2013/01/18 00:13:08, danakj wrote:
> the name should already be set here since we gave it to the parent right? can
we
> just DCHECK they are equal here instead?

There is already a DCHECK for that in cc::Mailbox, actually, but it does the
copy anyway. Putting the DCHECK here saves the copy.

[commit-bot: I haz the power, 2013-01-21 17:59:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexst@chromium.org/11888010/31001

[commit-bot: I haz the power, 2013-01-21 20:18:56]

Change committed as 177954