Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=wolenetz@chromium.org, sky@chromium.org

Committed: https://crrev.com/2573ce7d143bee9c1f88dac18b9213c03597daae
Cr-Commit-Position: refs/heads/master@{#375620}

[Łukasz Anforowicz, 2016-02-05 18:04:19]

Description was changed from

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern
for gpu::CommandBufferId should hopefully help avoid bugs like 514815.

BUG=514815 565545
==========

to

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern
for gpu::CommandBufferId should hopefully help avoid bugs like 514815.

BUG=514815, 565545
==========

[Łukasz Anforowicz, 2016-02-05 20:56:26]

Description was changed from

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern
for gpu::CommandBufferId should hopefully help avoid bugs like 514815.

BUG=514815, 565545
==========

to

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern
for gpu::CommandBufferId should hopefully help avoid bugs like 514815.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Łukasz Anforowicz, 2016-02-05 23:41:41]

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

[Łukasz Anforowicz, 2016-02-05 23:41:41]

Antoine, could you please take a look?

[piman, 2016-02-05 23:53:53]

Excellent! LGTM++

[Łukasz Anforowicz, 2016-02-06 00:25:38]

lukasza@chromium.org changed reviewers:
+ thakis@chromium.org

[Łukasz Anforowicz, 2016-02-06 00:25:39]

Nico, could you please take a look?

Some context:

- An old CL (crrev.com/1492413002) for putting id_type.h into //base has been
abandoned after Dana's feedback that it was back then too early for adding it to
base.  There was some follow-up discussion about putting id_type.h into base
(mostly in [1], some feedback in [2]) but it has more-or-less died out. 

- Right now id_type.h is in //content/common and with recent updates (i.e.
crrev.com/1665383002) I think it is ready for moving it to //base.  The current
CL:

  1) shows how id_type.h could prevent http://crrev.com/365437 mixup
  and
  2) is a 2nd use of id_type.h (with //content/browser/downloads being
     original user).

- Ideally this CL would be reviewed by Dana (original reviewer of
crrev.com/1492413002), but she's OOO until Feb 22nd, so I thought for now I'll
try to get it reviewed by others (for //base and for the other directories
touched by this CL).

WDYT? :-)

[1]
https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/4YWsj6keR6s...
[2] https://codereview.chromium.org/1492413002/#msg6

[Łukasz Anforowicz, 2016-02-06 00:47:30]

Description was changed from

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern
for gpu::CommandBufferId should hopefully help avoid bugs like 514815.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Nico, 2016-02-09 20:04:29]

Before this CL, IdType was used in one file. After this CL, it's used in 3 as
far as I can see. I agree with Dana that this doesn't meet the base bar from
https://code.google.com/p/chromium/codesearch#chromium/src/base/OWNERS&l=6

(it also serves a somewhat similar purpose as base/id_map.h, though in a
different way)

Since this is only used in targets gpu and up, maybe this could live in gpu?
(having a template class for this doesn't seem much shorter than just writing a
manual wrapper class for an int in the three uses -- this would allow putting
other functionality going with these ints in the various usecases in these
classes too.)

(note to self: related to n3515)

https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:999: };
This isn't more type-safe than just sending the inner int type, right? Why is it
necessary to send IdTypes over ipc?

[piman, 2016-02-09 21:47:36]

On Tue, Feb 9, 2016 at 12:04 PM, <thakis@chromium.org> wrote:

> Before this CL, IdType was used in one file. After this CL, it's used in 3 as
> far as I can see. I agree with Dana that this doesn't meet the base bar
fromhttps://code.google.com/p/chromium/codesearch#chromium/src/base/OWNERS&l=6
>
> (it also serves a somewhat similar purpose as base/id_map.h, though in a
> different way)
>
> Since this is only used in targets gpu and up, maybe this could live in gpu?
> (having a template class for this doesn't seem much shorter than just writing
a
> manual wrapper class for an int in the three uses -- this would allow putting
> other functionality going with these ints in the various usecases in these
> classes too.)
>
> (note to self: related to n3515)
>
> https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h
> File ipc/ipc_message_utils.h (right):
>
https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...
> ipc/ipc_message_utils.h:999
<https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...>:
};
> This isn't more type-safe than just sending the inner int type, right?
> Why is it necessary to send IdTypes over ipc?
>
>
It keeps type safety as long as possible (until you hit the wire). It makes
messages themselves (e.g. their constructors) type-safe. It catches bugs.
See linked ones.

> https://codereview.chromium.org/1548443002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2016-02-09 21:59:55]

On Tue, Feb 9, 2016 at 1:47 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Feb 9, 2016 at 12:04 PM, <thakis@chromium.org> wrote:
>
>> Before this CL, IdType was used in one file. After this CL, it's used in 3
as
>> far as I can see. I agree with Dana that this doesn't meet the base bar
fromhttps://code.google.com/p/chromium/codesearch#chromium/src/base/OWNERS&l=6
>>
>> (it also serves a somewhat similar purpose as base/id_map.h, though in a
>> different way)
>>
>> Not really IMO, this is all about type-safety, which is fairly orthogonal
to what IdMap is used for (ids in IDMap are not type safe), though IDMap
could use IdType for its ids, and that would be a double-win.

>
>> Since this is only used in targets gpu and up, maybe this could live in gpu?
>> (having a template class for this doesn't seem much shorter than just writing
a
>> manual wrapper class for an int in the three uses -- this would allow
putting
>> other functionality going with these ints in the various usecases in these
>> classes too.)
>>
>>
FYI, discussion where I claim this is useful generally in Chrome for
fundamental reasons at
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/4YW...


>
>> (note to self: related to n3515)
>>
>>
https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h
>> File ipc/ipc_message_utils.h (right):
>>
https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...
>> ipc/ipc_message_utils.h:999
<https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...>:
};
>> This isn't more type-safe than just sending the inner int type, right?
>> Why is it necessary to send IdTypes over ipc?
>>
>>
> It keeps type safety as long as possible (until you hit the wire). It
> makes messages themselves (e.g. their constructors) type-safe. It catches
> bugs. See linked ones.
>
>> https://codereview.chromium.org/1548443002/
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2016-02-09 22:03:33]

On Tue, Feb 9, 2016 at 4:59 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Feb 9, 2016 at 1:47 PM, Antoine Labour <piman@chromium.org> wrote:
>
>>
>>
>> On Tue, Feb 9, 2016 at 12:04 PM, <thakis@chromium.org> wrote:
>>
>>> Before this CL, IdType was used in one file. After this CL, it's used in 3
as
>>> far as I can see. I agree with Dana that this doesn't meet the base bar
fromhttps://code.google.com/p/chromium/codesearch#chromium/src/base/OWNERS&l=6
>>>
>>> (it also serves a somewhat similar purpose as base/id_map.h, though in a
>>> different way)
>>>
>>> Not really IMO, this is all about type-safety, which is fairly
> orthogonal to what IdMap is used for (ids in IDMap are not type safe),
> though IDMap could use IdType for its ids, and that would be a double-win.
>
>> Since this is only used in targets gpu and up, maybe this could live in gpu?
>>> (having a template class for this doesn't seem much shorter than just
writing a
>>> manual wrapper class for an int in the three uses -- this would allow
putting
>>> other functionality going with these ints in the various usecases in these
>>> classes too.)
>>>
>>>
> FYI, discussion where I claim this is useful generally in Chrome for
> fundamental reasons at
>
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/4YW...
>
>

There are many things that are generally useful in Chrome. Some people like
algebraic data types like TypeId<> (and Optional<>, etc). Others like other
styles. Currently, Chrome is mostly written in an OOP style with not too
much algebraic data types / pattern matching stuff going on. It's cool to
experiment with this, but `cat base/OWNERS`


> (note to self: related to n3515)
>>>
>>>
https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h
>>> File ipc/ipc_message_utils.h (right):
>>>
https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...
>>> ipc/ipc_message_utils.h:999
<https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...>:
};
>>> This isn't more type-safe than just sending the inner int type, right?
>>> Why is it necessary to send IdTypes over ipc?
>>>
>>>
>> It keeps type safety as long as possible (until you hit the wire). It
>> makes messages themselves (e.g. their constructors) type-safe. It catches
>> bugs. See linked ones.
>>
>>> https://codereview.chromium.org/1548443002/
>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Łukasz Anforowicz, 2016-02-09 23:06:32]

Thanks for reviewing Nico.  I've moved the code out of //base, so technically
your sign-off is not needed, but I would still appreciate an ack that everything
looks ok.

On 2016/02/09 20:04:29, Nico wrote:
> Before this CL, IdType was used in one file. After this CL, it's used in 3 as
> far as I can see. I agree with Dana that this doesn't meet the base bar from
> https://code.google.com/p/chromium/codesearch#chromium/src/base/OWNERS&l=6
> 
> (it also serves a somewhat similar purpose as base/id_map.h, though in a
> different way)
> 
> Since this is only used in targets gpu and up, maybe this could live in gpu?

Yes - I guess I can move it to gpu.  I think gpu/command_buffer/common sounds ok
(there is no other "common" / "base"-like dir under //gpu AFAICT).

Pros of the move:
- Accomplishes the goal of enabling reuse from //gpu and //content
- Forces folding IPC::ParamTraits into id_type.h, which I think is a good thing.

Cons of the move:
- It looks a bit weird that content::SaveItemId uses gpu::IdType, but I guess
this is ok (when I talked with nick@ he asked for plans of typedefing from
content namespace, but I think things look ok as-is;  let's see what he says
when it is his turn to review :-).

> (having a template class for this doesn't seem much shorter than just writing
a
> manual wrapper class for an int in the three uses -- this would allow putting
> other functionality going with these ints in the various usecases in these
> classes too.)

I don't like this idea:
- All the cases would have to repeat IPC::ParamTraits, Hasher, <<, etc.  This
isn't the end of the world, but it strikes me as rather icky.
- It seems to me that the cost of having to introduce a separate struct is a
barrier that prevented introducing type-safety earlier.  I hope that easily
reusable IdType will fix that.
- Separate structs lead to extraneous, ad-hoc code in random places (i.e. see
separate pickling definition for SurfaceId in content/common/cc_messages.h).


On 2016/02/09 22:03:33, Nico wrote:
> On Tue, Feb 9, 2016 at 4:59 PM, Antoine Labour <mailto:piman@chromium.org>
wrote:
> > FYI, discussion where I claim this is useful generally in Chrome for
> > fundamental reasons at
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/4YW...
> >
> 
> There are many things that are generally useful in Chrome. Some people like
> algebraic data types like TypeId<> (and Optional<>, etc). Others like other
> styles. Currently, Chrome is mostly written in an OOP style with not too
> much algebraic data types / pattern matching stuff going on. It's cool to
> experiment with this, but `cat base/OWNERS`
> 
I think algebraic data types are a red herring - in most places the code would
just say content::SaveItemId or gpu::CommandBufferId - they look and read like
perfectly normal / opaque types.

https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/1548443002/diff/40001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:999: };
On 2016/02/09 20:04:29, Nico wrote:
> This isn't more type-safe than just sending the inner int type, right? Why is
it
> necessary to send IdTypes over ipc?

It *is* more type-safe to use gpu::CommandBufferId instead of a raw integer in a
definition of an IPC message (i.e. PpapiHostMsg_PPBGraphics3D_Create in
crrev.com/1548443002/patch/40001/50064).  This means that the constructor of the
message is type-safe (i.e. won't accept a random integer) as are the handlers of
the message (i.e. PPB_Graphics3D_Proxy::OnMsgCreate in
crrev.com/1548443002/patch/40001/50065).  In other words - having IPC support
avoids unnecessary GetUnsafeValue / FromUnsafeValue calls.

[piman, 2016-02-09 23:29:32]

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
File content/browser/download/save_types.h (right):

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
content/browser/download/save_types.h:22: using SavePackageId =
gpu::IdType32<SavePackage>;
Yeah, this is technically permitted, but really really odd.

https://codereview.chromium.org/1548443002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/id_type.h (right):

https://codereview.chromium.org/1548443002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/id_type.h:15: #include "ipc/ipc_param_traits.h"
gpu/command_buffer shouldn't depend on IPC. You can move the ParamTraits into
gpu/ipc/  (either gpu_command_buffer_traits.h or in a separate header).

[Łukasz Anforowicz, 2016-02-10 22:18:13]

lukasza@chromium.org changed reviewers:
+ nick@chromium.org

[Łukasz Anforowicz, 2016-02-10 22:18:14]

+Nick for //content review.  Could you please take a look?

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
File content/browser/download/save_types.h (right):

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
content/browser/download/save_types.h:22: using SavePackageId =
gpu::IdType32<SavePackage>;
On 2016/02/09 23:29:32, piman wrote:
> Yeah, this is technically permitted, but really really odd.

Let me add nick@ to the reviewer's line.  Nick - WDYT?

https://codereview.chromium.org/1548443002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/id_type.h (right):

https://codereview.chromium.org/1548443002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/id_type.h:15: #include "ipc/ipc_param_traits.h"
On 2016/02/09 23:29:32, piman wrote:
> gpu/command_buffer shouldn't depend on IPC. You can move the ParamTraits into
> gpu/ipc/  (either gpu_command_buffer_traits.h or in a separate header).

Done (into a separate gpu/ipc/id_type_traits.h header).

I am a bit sad that including id_type.h won't anymore be sufficient when using
it in IPC messages - one will also have to make sure that id_type_traits.h are
included next to message definitions (unlike with traits defined in
ipc/ipc_message_utils.h which we can only do if id_type.h lives in //base or
other layer below //ipc and unlike with traits defined within id_type.h header
itself). :-(

[piman, 2016-02-10 22:54:04]

lgtm

[ncarter (slow), 2016-02-10 23:29:40]

lgtm

[Nico, 2016-02-10 23:31:18]

lgtm, thanks!

https://codereview.chromium.org/1548443002/diff/80001/gpu/command_buffer/comm...
File gpu/command_buffer/common/id_type_unittest.cc (right):

https://codereview.chromium.org/1548443002/diff/80001/gpu/command_buffer/comm...
gpu/command_buffer/common/id_type_unittest.cc:200: }  // namespace base
namespace gpu

[ncarter (slow), 2016-02-10 23:34:16]

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
File content/browser/download/save_types.h (right):

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
content/browser/download/save_types.h:22: using SavePackageId =
gpu::IdType32<SavePackage>;
On 2016/02/10 22:18:14, Łukasz Anforowicz wrote:
> On 2016/02/09 23:29:32, piman wrote:
> > Yeah, this is technically permitted, but really really odd.
> 
> Let me add nick@ to the reviewer's line.  Nick - WDYT?

I think it should go in base/, but if we can't put it there, I guess we can live
with this.

The other approach is to leave content/common/id_type intact and hide the ugly
import of gpu from there?

[Łukasz Anforowicz, 2016-02-11 16:51:24]

lukasza@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@chromium.org, fsamuel@chromium.org,
sky@chromium.org, wolenetz@chromium.org

[Łukasz Anforowicz, 2016-02-11 16:51:25]

+dcheng for gpu/ipc
+fsamuel for components/mus/gles2
+wolenetz for media/
+sky for mojo/
+bbudge for ppapi/

Thanks for reviewing :-)

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
File content/browser/download/save_types.h (right):

https://codereview.chromium.org/1548443002/diff/60001/content/browser/downloa...
content/browser/download/save_types.h:22: using SavePackageId =
gpu::IdType32<SavePackage>;
On 2016/02/10 23:34:16, ncarter wrote:
> On 2016/02/10 22:18:14, Łukasz Anforowicz wrote:
> > On 2016/02/09 23:29:32, piman wrote:
> > > Yeah, this is technically permitted, but really really odd.
> > 
> > Let me add nick@ to the reviewer's line.  Nick - WDYT?
> 
> I think it should go in base/, but if we can't put it there, I guess we can
live
> with this.

Ok.
> 
> The other approach is to leave content/common/id_type intact and hide the ugly
> import of gpu from there?

Right - we could include gpu/.../id_type.h from content/common/id_type.h and
have 5 type aliases from |content| namespace into |gpu| namespace like:

  namespace content {

  template<T1,T2,T3>
  using IdType<T1,T2,T3> = gpu::IdType<T1,T2,T3>;

  template<T>
  using IdType32<T> = gpu::IdType32<T>;

I think I prefer the lesser evil of what we currently have in the CL, because

1. The proposed type aliases would only help users of IdType under //content
(i.e. the type aliases don't help with adopting IdType elsewhere).

2. The extra file makes IdType approach look messier IMO (i.e. it makes IdType
"implementation" spread across even more files).

https://codereview.chromium.org/1548443002/diff/80001/gpu/command_buffer/comm...
File gpu/command_buffer/common/id_type_unittest.cc (right):

https://codereview.chromium.org/1548443002/diff/80001/gpu/command_buffer/comm...
gpu/command_buffer/common/id_type_unittest.cc:200: }  // namespace base
On 2016/02/10 23:31:18, Nico wrote:
> namespace gpu

Ooops - thanks for catching this.  Done.

[bbudge, 2016-02-11 17:02:01]

ppapi lgtm

[Fady Samuel, 2016-02-11 17:02:36]

components/mus/gles2 lgtm

[dcheng, 2016-02-12 19:23:17]

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:121: "[%d:%llX] %llu",
static_cast<int>(p.namespace_id()),
How about:

*l += base::StringPrintf("[%d:" PRIu64 "] " PRIu64,
    static_cast<p.namespace_id()),
    p.command_buffer_id().GetUnsafeValue(), p.release_count());

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits.h
File gpu/ipc/id_type_traits.h (right):

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:19: typedef gpu::IdType<TypeMarker, WrappedType,
kInvalidValue> param_type;
using param_type = gpu::IdType<TypeMarker, WrappedType, kInvalidValue>;

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:22: base_param_traits::GetSize(sizer,
p.GetUnsafeValue());
Use the type deducing helpers =)

GetParamSize(sizer, p.GetUnsafeValue())

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:25: base_param_traits::Write(m, p.GetUnsafeValue());
WriteParam(m, p.GetUnsafeValue()

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:31: if (!base_param_traits::Read(m, iter, &value))
ReadParam(m, iter, &value)

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:37: base_param_traits::Log(p.GetUnsafeValue(), l);
LogParam(p.getUnsafeValue(), l)

[Łukasz Anforowicz, 2016-02-12 22:33:24]

Daniel - can you take another look please?

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:121: "[%d:%llX] %llu",
static_cast<int>(p.namespace_id()),
On 2016/02/12 19:23:17, dcheng wrote:
> How about:
> 
> *l += base::StringPrintf("[%d:" PRIu64 "] " PRIu64,
>     static_cast<p.namespace_id()),
>     p.command_buffer_id().GetUnsafeValue(), p.release_count());

Done - thanks for catching this.  Can you please double-check what I did?
- I switched to PRIX64 and PRIu64 to avoid %llu and static casts to unsigned
long long.  The last 2 arguments are both uint64_t.
- I also wanted to get rid of the static_cast on the first placeholder argument.
 The cast is not needed, because argv will pass them as int.  For consistency I
switched from "%d" to |"%" PRId8|.

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits.h
File gpu/ipc/id_type_traits.h (right):

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:19: typedef gpu::IdType<TypeMarker, WrappedType,
kInvalidValue> param_type;
On 2016/02/12 19:23:17, dcheng wrote:
> using param_type = gpu::IdType<TypeMarker, WrappedType, kInvalidValue>;

Done - good point (my poor excuse for not doing it earlier is that previously it
was matching the style of other nearby code in ipc_message_utils.h - I should
have changed to "using" after moving this code to id_type_traits.h).

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:22: base_param_traits::GetSize(sizer,
p.GetUnsafeValue());
On 2016/02/12 19:23:17, dcheng wrote:
> Use the type deducing helpers =)
> 
> GetParamSize(sizer, p.GetUnsafeValue())

Doh... thanks!

Done (I also removed no longer needed |base_param_traits| type alias).

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:25: base_param_traits::Write(m, p.GetUnsafeValue());
On 2016/02/12 19:23:17, dcheng wrote:
> WriteParam(m, p.GetUnsafeValue()

Done.

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:31: if (!base_param_traits::Read(m, iter, &value))
On 2016/02/12 19:23:17, dcheng wrote:
> ReadParam(m, iter, &value)

Done.

https://codereview.chromium.org/1548443002/diff/120001/gpu/ipc/id_type_traits...
gpu/ipc/id_type_traits.h:37: base_param_traits::Log(p.GetUnsafeValue(), l);
On 2016/02/12 19:23:17, dcheng wrote:
> LogParam(p.getUnsafeValue(), l)

Done.

[dcheng, 2016-02-12 22:38:19]

https://codereview.chromium.org/1548443002/diff/140001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1548443002/diff/140001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:121: "[%" PRId8 ":%" PRIX64 "] %" PRIu64,
p.namespace_id(),
Should this be lowercase 'x'?

[Łukasz Anforowicz, 2016-02-12 22:47:49]

https://codereview.chromium.org/1548443002/diff/140001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1548443002/diff/140001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:121: "[%" PRId8 ":%" PRIX64 "] %" PRIu64,
p.namespace_id(),
On 2016/02/12 22:38:19, dcheng wrote:
> Should this be lowercase 'x'?

I think this should be an uppercase 'X', because
1. The old code used an upper case 'X' ("%llX").
2. C++ reference says that there are 2 separate PRIx64 and PRIX64 (for uppercase
vs lowercase hex digits) - http://en.cppreference.com/w/cpp/types/integer

[dcheng, 2016-02-12 22:52:45]

ipc changes lgtm

[Łukasz Anforowicz, 2016-02-16 17:01:59]

Description was changed from

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=wolenetz@chromium.org, sky@chromium.org
==========

[Łukasz Anforowicz, 2016-02-16 18:04:10]

sky@ + wolenetz@, can you please take a look when you have a chance?  I plan to
TBR you on this CL (as changes under media/ and mojo/ are mostly mechanical) and
land it later today if everything looks ok to CQ.

[Łukasz Anforowicz, 2016-02-16 18:04:20]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-02-16 18:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
thakis@chromium.org, piman@chromium.org, bbudge@chromium.org,
fsamuel@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1548443002/#ps160001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-02-16 18:04:52]

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

[commit-bot: I haz the power, 2016-02-16 19:17:34]

Committed patchset #9 (id:160001)

[sky, 2016-02-16 20:09:24]

LGTM

[commit-bot: I haz the power, 2016-02-16 22:53:49]

Description was changed from

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=wolenetz@chromium.org, sky@chromium.org
==========

to

==========
Introducing gpu::CommandBufferId as a distinct, IdType<...>-based type.

This CL generalizes the pattern used by content::SavePackageId and
cc::SurfaceId and puts it into //base/id_type.h.  Using this pattern for
gpu::CommandBufferId should hopefully help avoid bugs like the mixup
fixed by https://crrev.com/365437.

BUG=514815, 565545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=wolenetz@chromium.org, sky@chromium.org

Committed: https://crrev.com/2573ce7d143bee9c1f88dac18b9213c03597daae
Cr-Commit-Position: refs/heads/master@{#375620}
==========

[commit-bot: I haz the power, 2016-02-16 22:53:50]

Patchset 9 (id:??) landed as
https://crrev.com/2573ce7d143bee9c1f88dac18b9213c03597daae
Cr-Commit-Position: refs/heads/master@{#375620}

[Nico, 2016-02-17 19:45:19]

this caused https://code.google.com/p/chromium/issues/detail?id=587558