Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/common directory - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
    https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR6s/discussion

BUG=565545
Committed: https://crrev.com/1764f5c41e21cf607ffc4d644e077c096a7e78b9
Cr-Commit-Position: refs/heads/master@{#368502}

[Łukasz Anforowicz, 2015-12-17 23:47:20]

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

[Łukasz Anforowicz, 2015-12-17 23:47:20]

Asanka, would you be able to review this CL from content/browser/download
perspective?  This is a relatively low-priority CL, so if needed it can wait
until after the holidays :-).  Please see the CL description for more context
(i.e. the discussion at site.

Nick, can you do a review as a //content owner?  Or do you think we need more
discussion at
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...
?

[Łukasz Anforowicz, 2015-12-21 19:03:06]

Description was changed from

==========
Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/browser/downloads - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
   
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...

BUG=
==========

to

==========
Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/browser/downloads - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
   
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...

BUG=565545
==========

[Randy Smith (Not in Mondays), 2015-12-29 23:40:17]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-12-29 23:40:17]

content/browser/download LGTM modulo the naming/location issue I raise below. 
The other two (finding a subtler reviewer than I for the new type, including
MAX/MIN ints) are suggestions that are up to you.

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
File content/browser/download/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
content/browser/download/id_type.h:72: struct IdType {
This may run into a conflict with the content owners, but despite the fact that
it's only used by the download system currently, if it's named content::IdType,
I don't think that content/browser/download is the right place for it.  If the
content owners don't want it in a more central content/ location then I'd
recommend naming it DownloadIdType and renaming it if you later move it to a
more central location.

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
content/browser/download/id_type.h:86: bool operator<(const IdType& other) const
{ return value_ < other.value_; }
This kind of thing (making a type that "works like" a base type) is often fairly
subtle.  Even though this is going into content:: rather than base::, you might
want to have a base:: owner review it for gotchas.  Specifically, I suspect I
don't know all the ways in which this could break, so I'm not sure you should
rely on me for that level of review.  Having said that, this all looks good to
me.

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
File content/browser/download/id_type_unittest.cc (right):

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
content/browser/download/id_type_unittest.cc:169: ::testing::Values(-1, 0, 1,
123));
Worthwhile including MAX/MIN ints?

[Łukasz Anforowicz, 2015-12-30 20:37:23]

Thanks Randy.  Nick, can you take a look please? (note the question about
location of id_type.h that Randy has raised).

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
File content/browser/download/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
content/browser/download/id_type.h:72: struct IdType {
On 2015/12/29 23:40:17, Randy Smith - OOO until M 1-4 wrote:
> This may run into a conflict with the content owners, but despite the fact
that
> it's only used by the download system currently, if it's named
content::IdType,
> I don't think that content/browser/download is the right place for it.  If the
> content owners don't want it in a more central content/ location then I'd
> recommend naming it DownloadIdType and renaming it if you later move it to a
> more central location.  

Let's see what Nick says.  I am open to moving id_type.h to content/common or a
similar location.

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
content/browser/download/id_type.h:86: bool operator<(const IdType& other) const
{ return value_ < other.value_; }
On 2015/12/29 23:40:17, Randy Smith - OOO until M 1-4 wrote:
> This kind of thing (making a type that "works like" a base type) is often
fairly
> subtle.  Even though this is going into content:: rather than base::, you
might
> want to have a base:: owner review it for gotchas.  Specifically, I suspect I
> don't know all the ways in which this could break, so I'm not sure you should
> rely on me for that level of review.  Having said that, this all looks good to
> me.

Acknowledged.

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
File content/browser/download/id_type_unittest.cc (right):

https://codereview.chromium.org/1529363006/diff/1/content/browser/download/id...
content/browser/download/id_type_unittest.cc:169: ::testing::Values(-1, 0, 1,
123));
On 2015/12/29 23:40:17, Randy Smith - OOO until M 1-4 wrote:
> Worthwhile including MAX/MIN ints?

Done.

[ncarter (slow), 2016-01-07 00:48:50]

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
File content/browser/download/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:6: #define
CONTENT_BROWSER_DOWNLOAD_ID_TYPE_H_
Let's put this in content/common for now.

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:22: //   using BarId = content::IdType<Bar,
uint64_t, 0>;
Id prefer if we had a design that didn't require these typedefs: so that it
would be possible to use IdType<Bar> directly, with no other arguments.

Can we accomplish that with defaults, a traits type, or a convention where Bar
declares Bar::kInvalidIdValue?

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:71: template <typename TypeMarker, typename
WrappedType, WrappedType kInvalidValue>
I don't really know C++ templates well, so I wonder: if WrappedType is a non
primitive type like std::string, can you actually pass a kInvalidValue here?

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:72: struct IdType {
why struct? IdType has no public data members.

[Łukasz Anforowicz, 2016-01-07 18:02:13]

Nick, can you take another look please?

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
File content/browser/download/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:6: #define
CONTENT_BROWSER_DOWNLOAD_ID_TYPE_H_
On 2016/01/07 00:48:50, ncarter wrote:
> Let's put this in content/common for now.

Done.

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:22: //   using BarId = content::IdType<Bar,
uint64_t, 0>;
On 2016/01/07 00:48:50, ncarter wrote:
> Id prefer if we had a design that didn't require these typedefs: so that it
> would be possible to use IdType<Bar> directly, with no other arguments.

I agree that this is desirable, although:

1) I would qualify this further "possible to use IdType<Bar> directly, with no
other arguments, *and no includes or declarations except for id_type.h*".  This
qualification means that "foreign" / unrelated files don't have to include
foo_id.h (or define traits).

2) Just playing devil's advocate - putting all decisions in foo_id.h is better,
because it adheres better to the DRY principle.  For example - if you consider
the proposal below (of IdType32<TypeMarker> convenience alias), then the "32"
duplicates all over the place the decision that could be made once and local to
foo_id.h.  I think this is ok though.

> Can we accomplish that with defaults

Maybe.  For now I didn't modify kInvalidValue to make it default to 0, because I
think that IdType32 proposal below is sufficient and nicer in practice.  OTOH, I
am open to adding a default for kInvalidValue if you feel strongly about this.

> a traits type,

I'd rather not go there:

- Every point of use (i.e. wherever we need full declaration of the type) would
have to see the traits type (to invoke the default contructor one needs to know
the kInvalidValue).  This would mean having to include "foo_id.h" (so why not
just have a type alias inside "foo_id.h") or having to define traits all over
the place (even worse).

- I also think type traits would make things more complex (for users and
maintainers).

> or a convention where Bar declares Bar::kInvalidIdValue?

Maybe.  This would mean that TypeMarker is more than just a "marker", that it
needs to have its full declaration visible + has to have a specific structure. 
The latter means that one cannot use TypeMarker = types that you cannot change
(OTOH, I guess this can be worked around by using a TypeMarker =
new-artificial-type-that-you-can-change).


Let me propose something that I think is even better.  How about being able to
write something like this (notice the "32" in IdType32<Foo>):

  content::IdType32<Foo> foo_id;
  // or
  using FooId = content::IdType32<Foo>;

We can achieve this with:

  template <typename TypeMarker>
  using IdType32 = IdType<TypeMarker, uint32_t, 0>;
  template <typename TypeMarker>
  using IdType64 = IdType<TypeMarker, uint64_t, 0>;

Potential concerns:
- Naming (Id32 vs IdType32 vs IdTypeUnsigned32 :-)
- Default = 0 is something that we can also argue about... :-/

WDYT?  Let's ship it? :-)

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:71: template <typename TypeMarker, typename
WrappedType, WrappedType kInvalidValue>
On 2016/01/07 00:48:50, ncarter wrote:
> I don't really know C++ templates well, so I wonder: if WrappedType is a non
> primitive type like std::string, can you actually pass a kInvalidValue here?

I've tried this ans got an error:
../../content/common/id_type.h:71:66: error: a non-type template parameter
cannot have type 'std::basic_string<char, std::char_traits<char>, std::all
ocator<char> >'

http://en.cppreference.com/w/cpp/language/template_parameters says that the
values has to be of one of the following types:
- std::nullptr_t (since C++11);
- integral type;
- lvalue reference type (to object or to function);
- pointer type (to object or to function);
- pointer to member type (to member object or to member function);
- enumeration type.

I think it is ok to not support strings and similar types *for now*:
1) users of idtype copy
2) implementation of idtype copies
and copying of strings is more expensive than copying of ints.
Additionally, so far all the id types I saw were int-based.

I am not sure if we need to think about support for strings and similar types
*in the future*.  I guess one way to accomplish this would be to hard-code
invalid value to 0.  Another way would be traits route, but this would
complicate usage for users (even in presence of default traits if such thing is
possible).  *But*... I am going to propose to instead limit WrappedType(s) to
integers only (for now and for the long-term future):

- This restriction makes it easy to reason about base::IdType for *users* of the
type (i.e. users can assume that copying is cheap and that the types behave
similar to integers)

- This restriction limits the design space that implementers and maintainers of
base::IdType have to consider.  For example - after restricting WrappedType(s)
to integers only we don't have to worry about WrappedType = std::string OR
WrappedType = <move-only type>.


So - I've added a SFINAE saying that WrappedType has to be an integer.  This is
done via std::is_integral from type_traits [1].  I've tested that this prevents
using non-integer types (I've tested with an enum).  


[1] http://en.cppreference.com/w/cpp/types/is_integral

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:72: struct IdType {
On 2016/01/07 00:48:50, ncarter wrote:
> why struct? IdType has no public data members.

I changed to a class.

RE: why

No particular reason.  My C# background makes me thing struct = stack allocated
value type :-P.  And I based IdType on SurfaceId (although you are right that
this one has a public id field).

[ncarter (slow), 2016-01-07 20:14:32]

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
File content/browser/download/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:22: //   using BarId = content::IdType<Bar,
uint64_t, 0>;
On 2016/01/07 18:02:12, Łukasz Anforowicz wrote:
> On 2016/01/07 00:48:50, ncarter wrote:
> > Id prefer if we had a design that didn't require these typedefs: so that it
> > would be possible to use IdType<Bar> directly, with no other arguments.
> 
> I agree that this is desirable, although:
> 
> 1) I would qualify this further "possible to use IdType<Bar> directly, with no
> other arguments, *and no includes or declarations except for id_type.h*". 
This
> qualification means that "foreign" / unrelated files don't have to include
> foo_id.h (or define traits).
> 
> 2) Just playing devil's advocate - putting all decisions in foo_id.h is
better,
> because it adheres better to the DRY principle.  For example - if you consider
> the proposal below (of IdType32<TypeMarker> convenience alias), then the "32"
> duplicates all over the place the decision that could be made once and local
to
> foo_id.h.  I think this is ok though.
> 
> > Can we accomplish that with defaults
> 
> Maybe.  For now I didn't modify kInvalidValue to make it default to 0, because
I
> think that IdType32 proposal below is sufficient and nicer in practice.  OTOH,
I
> am open to adding a default for kInvalidValue if you feel strongly about this.
> 
> > a traits type,
> 
> I'd rather not go there:
> 
> - Every point of use (i.e. wherever we need full declaration of the type)
would
> have to see the traits type (to invoke the default contructor one needs to
know
> the kInvalidValue).  This would mean having to include "foo_id.h" (so why not
> just have a type alias inside "foo_id.h") or having to define traits all over
> the place (even worse).
> 
> - I also think type traits would make things more complex (for users and
> maintainers).
> 
> > or a convention where Bar declares Bar::kInvalidIdValue?
> 
> Maybe.  This would mean that TypeMarker is more than just a "marker", that it
> needs to have its full declaration visible + has to have a specific structure.

> The latter means that one cannot use TypeMarker = types that you cannot change
> (OTOH, I guess this can be worked around by using a TypeMarker =
> new-artificial-type-that-you-can-change).
> 
> 
> Let me propose something that I think is even better.  How about being able to
> write something like this (notice the "32" in IdType32<Foo>):
> 
>   content::IdType32<Foo> foo_id;
>   // or
>   using FooId = content::IdType32<Foo>;
> 
> We can achieve this with:
> 
>   template <typename TypeMarker>
>   using IdType32 = IdType<TypeMarker, uint32_t, 0>;
>   template <typename TypeMarker>
>   using IdType64 = IdType<TypeMarker, uint64_t, 0>;
> 
> Potential concerns:
> - Naming (Id32 vs IdType32 vs IdTypeUnsigned32 :-)
> - Default = 0 is something that we can also argue about... :-/
> 
> WDYT?  Let's ship it? :-)

I like this idea, a lot.

It looks like, in existing content/ code, signed int32s are far more common than
unsigned int32s for GeId() methods [see link below]. So I think IdType32 should
be signed, and IdTypeU32 sgtm for the unsigned cases.

https://code.google.com/p/chromium/codesearch#search/&q=file:content%20(u)?in...

I think zero is the right default. Routing IDs have a weird null behavior so
they'll have to use the full macro.

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:71: template <typename TypeMarker, typename
WrappedType, WrappedType kInvalidValue>
On 2016/01/07 18:02:12, Łukasz Anforowicz wrote:
> On 2016/01/07 00:48:50, ncarter wrote:
> > I don't really know C++ templates well, so I wonder: if WrappedType is a non
> > primitive type like std::string, can you actually pass a kInvalidValue here?
> 
> I've tried this ans got an error:
> ../../content/common/id_type.h:71:66: error: a non-type template parameter
> cannot have type 'std::basic_string<char, std::char_traits<char>, std::all
> ocator<char> >'
> 
> http://en.cppreference.com/w/cpp/language/template_parameters says that the
> values has to be of one of the following types:
> - std::nullptr_t (since C++11);
> - integral type;
> - lvalue reference type (to object or to function);
> - pointer type (to object or to function);
> - pointer to member type (to member object or to member function);
> - enumeration type.
> 
> I think it is ok to not support strings and similar types *for now*:
> 1) users of idtype copy
> 2) implementation of idtype copies
> and copying of strings is more expensive than copying of ints.
> Additionally, so far all the id types I saw were int-based.
> 
> I am not sure if we need to think about support for strings and similar types
> *in the future*.  I guess one way to accomplish this would be to hard-code
> invalid value to 0.  Another way would be traits route, but this would
> complicate usage for users (even in presence of default traits if such thing
is
> possible).  *But*... I am going to propose to instead limit WrappedType(s) to
> integers only (for now and for the long-term future):
> 
> - This restriction makes it easy to reason about base::IdType for *users* of
the
> type (i.e. users can assume that copying is cheap and that the types behave
> similar to integers)
> 
> - This restriction limits the design space that implementers and maintainers
of
> base::IdType have to consider.  For example - after restricting WrappedType(s)
> to integers only we don't have to worry about WrappedType = std::string OR
> WrappedType = <move-only type>.
> 
> 
> So - I've added a SFINAE saying that WrappedType has to be an integer.  This
is
> done via std::is_integral from type_traits [1].  I've tested that this
prevents
> using non-integer types (I've tested with an enum).  
> 
> 
> [1] http://en.cppreference.com/w/cpp/types/is_integral

I really like the solution of expose a ready-to-use, single-parameter IdType32 /
IdType64 combo.

There are cases in the code where we might want to wrap a string, but it's best
to leave it as a separate effort, when somebody needs it.

https://codereview.chromium.org/1529363006/diff/100001/content/common/id_type.h
File content/common/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/100001/content/common/id_type...
content/common/id_type.h:14: // Problem:
For templates like this, the definitions that users care about often wind up
buried in the middle of the file, so reading the code top-down is pretty hard.
So the comment needs to do a good job of summarizing the useful parts.

I find the "Problem:/Solution:" intro a little terse, since it kind of requires
you to spot the swapped-parameters bug. If such bugs were easy to spot, we
wouldn't need IdType! Here's my attempt at an intro:

// IdType32<>, IdType64<>, IdType<>: wrap an integer id in a custom type.
//
// IdType32<Foo> is an alternative to int, for a class Foo with methods like:
//
//    int GetId() { return id_; };
//    static Foo* FromId(int id) { return g_all_foos_by_id[id]; }
//
// Such methods are a standard means of safely referring to objects across
// thread and process boundaries. But if a nearby classes Bar also represents
// its IDs as a bare int, horrific mixups are possible -- one example, of many,
// is http://crrev.com/365437. IdType<> offers compile-time protection against
// such mishaps, since IdType32<Foo> is incompatible with IdType32<Bar>, even
// though both just compile down to an int32_t.
//
// Templates in this file:
//   IdType32<T> / IdTypeU32<T>: Signed / unsigned 32-bit IDs
//   IdType64<T> / IdTypeU64<T>: Signed / unsigned 64-bit IDs
//   IdType<>: For when you need a different underlying type

https://codereview.chromium.org/1529363006/diff/100001/content/common/id_type...
content/common/id_type.h:53: //      - copy construction and assignment
Would it be better to talk about what the supported operations let you do?

"All IdType<> variants support comparison and hashing, so you can use them as
arguments to ASSERT_EQ or as keys in STL containers."

[Łukasz Anforowicz, 2016-01-07 21:43:01]

Nick, can you take one more look please?

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
File content/browser/download/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:22: //   using BarId = content::IdType<Bar,
uint64_t, 0>;
On 2016/01/07 20:14:32, ncarter wrote:
> On 2016/01/07 18:02:12, Łukasz Anforowicz wrote:
> > On 2016/01/07 00:48:50, ncarter wrote:
> > > Id prefer if we had a design that didn't require these typedefs: so that
it
> > > would be possible to use IdType<Bar> directly, with no other arguments.
> > 
> > I agree that this is desirable, although:
> > 
> > 1) I would qualify this further "possible to use IdType<Bar> directly, with
no
> > other arguments, *and no includes or declarations except for id_type.h*". 
> This
> > qualification means that "foreign" / unrelated files don't have to include
> > foo_id.h (or define traits).
> > 
> > 2) Just playing devil's advocate - putting all decisions in foo_id.h is
> better,
> > because it adheres better to the DRY principle.  For example - if you
consider
> > the proposal below (of IdType32<TypeMarker> convenience alias), then the
"32"
> > duplicates all over the place the decision that could be made once and local
> to
> > foo_id.h.  I think this is ok though.
> > 
> > > Can we accomplish that with defaults
> > 
> > Maybe.  For now I didn't modify kInvalidValue to make it default to 0,
because
> I
> > think that IdType32 proposal below is sufficient and nicer in practice. 
OTOH,
> I
> > am open to adding a default for kInvalidValue if you feel strongly about
this.
> > 
> > > a traits type,
> > 
> > I'd rather not go there:
> > 
> > - Every point of use (i.e. wherever we need full declaration of the type)
> would
> > have to see the traits type (to invoke the default contructor one needs to
> know
> > the kInvalidValue).  This would mean having to include "foo_id.h" (so why
not
> > just have a type alias inside "foo_id.h") or having to define traits all
over
> > the place (even worse).
> > 
> > - I also think type traits would make things more complex (for users and
> > maintainers).
> > 
> > > or a convention where Bar declares Bar::kInvalidIdValue?
> > 
> > Maybe.  This would mean that TypeMarker is more than just a "marker", that
it
> > needs to have its full declaration visible + has to have a specific
structure.
> 
> > The latter means that one cannot use TypeMarker = types that you cannot
change
> > (OTOH, I guess this can be worked around by using a TypeMarker =
> > new-artificial-type-that-you-can-change).
> > 
> > 
> > Let me propose something that I think is even better.  How about being able
to
> > write something like this (notice the "32" in IdType32<Foo>):
> > 
> >   content::IdType32<Foo> foo_id;
> >   // or
> >   using FooId = content::IdType32<Foo>;
> > 
> > We can achieve this with:
> > 
> >   template <typename TypeMarker>
> >   using IdType32 = IdType<TypeMarker, uint32_t, 0>;
> >   template <typename TypeMarker>
> >   using IdType64 = IdType<TypeMarker, uint64_t, 0>;
> > 
> > Potential concerns:
> > - Naming (Id32 vs IdType32 vs IdTypeUnsigned32 :-)
> > - Default = 0 is something that we can also argue about... :-/
> > 
> > WDYT?  Let's ship it? :-)
> 
> I like this idea, a lot.
> 
> It looks like, in existing content/ code, signed int32s are far more common
than
> unsigned int32s for GeId() methods [see link below]. So I think IdType32
should
> be signed, and IdTypeU32 sgtm for the unsigned cases.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=file:content%20(u)?in...
> 
> I think zero is the right default. Routing IDs have a weird null behavior so
> they'll have to use the full macro.

Done.  (changed IdType32/64 to wrap a signed integer + introduced IdTypeU32/64 +
kept using 0 as the default value here)

https://codereview.chromium.org/1529363006/diff/40001/content/browser/downloa...
content/browser/download/id_type.h:71: template <typename TypeMarker, typename
WrappedType, WrappedType kInvalidValue>
On 2016/01/07 20:14:32, ncarter wrote:
> On 2016/01/07 18:02:12, Łukasz Anforowicz wrote:
> > On 2016/01/07 00:48:50, ncarter wrote:
> > > I don't really know C++ templates well, so I wonder: if WrappedType is a
non
> > > primitive type like std::string, can you actually pass a kInvalidValue
here?
> > 
> > I've tried this ans got an error:
> > ../../content/common/id_type.h:71:66: error: a non-type template parameter
> > cannot have type 'std::basic_string<char, std::char_traits<char>, std::all
> > ocator<char> >'
> > 
> > http://en.cppreference.com/w/cpp/language/template_parameters says that the
> > values has to be of one of the following types:
> > - std::nullptr_t (since C++11);
> > - integral type;
> > - lvalue reference type (to object or to function);
> > - pointer type (to object or to function);
> > - pointer to member type (to member object or to member function);
> > - enumeration type.
> > 
> > I think it is ok to not support strings and similar types *for now*:
> > 1) users of idtype copy
> > 2) implementation of idtype copies
> > and copying of strings is more expensive than copying of ints.
> > Additionally, so far all the id types I saw were int-based.
> > 
> > I am not sure if we need to think about support for strings and similar
types
> > *in the future*.  I guess one way to accomplish this would be to hard-code
> > invalid value to 0.  Another way would be traits route, but this would
> > complicate usage for users (even in presence of default traits if such thing
> is
> > possible).  *But*... I am going to propose to instead limit WrappedType(s)
to
> > integers only (for now and for the long-term future):
> > 
> > - This restriction makes it easy to reason about base::IdType for *users* of
> the
> > type (i.e. users can assume that copying is cheap and that the types behave
> > similar to integers)
> > 
> > - This restriction limits the design space that implementers and maintainers
> of
> > base::IdType have to consider.  For example - after restricting
WrappedType(s)
> > to integers only we don't have to worry about WrappedType = std::string OR
> > WrappedType = <move-only type>.
> > 
> > 
> > So - I've added a SFINAE saying that WrappedType has to be an integer.  This
> is
> > done via std::is_integral from type_traits [1].  I've tested that this
> prevents
> > using non-integer types (I've tested with an enum).  
> > 
> > 
> > [1] http://en.cppreference.com/w/cpp/types/is_integral
> 
> I really like the solution of expose a ready-to-use, single-parameter IdType32
/
> IdType64 combo.
> 
> There are cases in the code where we might want to wrap a string, but it's
best
> to leave it as a separate effort, when somebody needs it.

Acknowledged.

https://codereview.chromium.org/1529363006/diff/100001/content/common/id_type.h
File content/common/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/100001/content/common/id_type...
content/common/id_type.h:14: // Problem:
On 2016/01/07 20:14:32, ncarter wrote:
> For templates like this, the definitions that users care about often wind up
> buried in the middle of the file, so reading the code top-down is pretty hard.
> So the comment needs to do a good job of summarizing the useful parts.
> 
> I find the "Problem:/Solution:" intro a little terse, since it kind of
requires
> you to spot the swapped-parameters bug. If such bugs were easy to spot, we
> wouldn't need IdType! Here's my attempt at an intro:
> 
> // IdType32<>, IdType64<>, IdType<>: wrap an integer id in a custom type.
> //
> // IdType32<Foo> is an alternative to int, for a class Foo with methods like:
> //
> //    int GetId() { return id_; };
> //    static Foo* FromId(int id) { return g_all_foos_by_id[id]; }
> //
> // Such methods are a standard means of safely referring to objects across
> // thread and process boundaries. But if a nearby classes Bar also represents
> // its IDs as a bare int, horrific mixups are possible -- one example, of
many,
> // is http://crrev.com/365437. IdType<> offers compile-time protection against
> // such mishaps, since IdType32<Foo> is incompatible with IdType32<Bar>, even
> // though both just compile down to an int32_t.
> //
> // Templates in this file:
> //   IdType32<T> / IdTypeU32<T>: Signed / unsigned 32-bit IDs
> //   IdType64<T> / IdTypeU64<T>: Signed / unsigned 64-bit IDs
> //   IdType<>: For when you need a different underlying type

Good point.  Done.  OTOH I tweaked your suggestion slightly, so please take one
more brief look.

PS. Does one say "are a standard means" or "are standard means"?  I am getting
confused here as a non-native speaker :-/

PPS. I see that you are a one-space-after-a-period-person :-)  Other comments in
id_type.h were typed by me (a two-spaces-after-a-period-person).  I guess I need
to pick one convention and stick to it.

https://codereview.chromium.org/1529363006/diff/100001/content/common/id_type...
content/common/id_type.h:53: //      - copy construction and assignment
On 2016/01/07 20:14:32, ncarter wrote:
> Would it be better to talk about what the supported operations let you do?
> 
> "All IdType<> variants support comparison and hashing, so you can use them as
> arguments to ASSERT_EQ or as keys in STL containers."

Done.

I've added a paragraph following your suggestion (and also mentioning memcpy
[which I think is relatively important], restraining myself from mentioning
other random operations [like stream output or ipc pickling], and finally
mentioning that some things like addition or multiplication are not supported).

In the spirit of simplicity and brevity I've also yanked out the "Typically
FooId would be declared like this" and I've moved item 4 to id_type_unittest.cc.

[dcheng, 2016-01-07 22:07:28]

A few drive bys.

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type.h
File content/common/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type.h:53: bool is_null() const { return value_ ==
kInvalidValue; }
Bikeshedding... seems a bit odd that the first is named "is valid", and the
second is named "is null".

In practice, I think providing is_null() is good enough. !is_null() is the exact
same number of characters as is_valid(), and doesn't leave someone wondering if
null might be valid, or there's a third possible state that's not valid and not
null.

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
File content/common/id_type_unittest.cc (right):

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type_unittest.cc:10: #include <unordered_map>
Not yet allowed =)

(Though it's very close)

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type_unittest.cc:59: EXPECT_EQ(sizeof(uint32_t), sizeof(id));
It seems like these should be static asserts... somewhere.

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type_unittest.cc:74: EXPECT_EQ(std::string("456"), ss.str());
Nit: std::string should have a comparison operator for const char* and
std::string, so I don't think the explicit string construction is required.

[ncarter (slow), 2016-01-07 22:46:59]

lgtm!!!

[ncarter (slow), 2016-01-07 22:49:42]

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type.h
File content/common/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type.h:53: bool is_null() const { return value_ ==
kInvalidValue; }
On 2016/01/07 22:07:28, dcheng wrote:
> Bikeshedding... seems a bit odd that the first is named "is valid", and the
> second is named "is null".
> 
> In practice, I think providing is_null() is good enough. !is_null() is the
exact
> same number of characters as is_valid(), and doesn't leave someone wondering
if
> null might be valid, or there's a third possible state that's not valid and
not
> null.

Continuing the bikeshed: If we go to only one getter, I think is_valid() (or
is_invalid()) is the better one to keep, since sometimes "null" actually would
mean "has a value of MSG_ROUTING_NONE / -2" whereas "null" to me suggests a zero
value?

[dcheng, 2016-01-07 22:50:37]

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type.h
File content/common/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type.h:53: bool is_null() const { return value_ ==
kInvalidValue; }
On 2016/01/07 at 22:49:42, ncarter wrote:
> On 2016/01/07 22:07:28, dcheng wrote:
> > Bikeshedding... seems a bit odd that the first is named "is valid", and the
> > second is named "is null".
> > 
> > In practice, I think providing is_null() is good enough. !is_null() is the
exact
> > same number of characters as is_valid(), and doesn't leave someone wondering
if
> > null might be valid, or there's a third possible state that's not valid and
not
> > null.
> 
> Continuing the bikeshed: If we go to only one getter, I think is_valid() (or
is_invalid()) is the better one to keep, since sometimes "null" actually would
mean "has a value of MSG_ROUTING_NONE / -2" whereas "null" to me suggests a zero
value?

Either way is fine. Just that having two different spellings for the same thing
feels funky.

[Łukasz Anforowicz, 2016-01-07 23:12:43]

Thanks for reviewing.

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type.h
File content/common/id_type.h (right):

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type.h:53: bool is_null() const { return value_ ==
kInvalidValue; }
On 2016/01/07 22:07:28, dcheng wrote:
> Bikeshedding... seems a bit odd that the first is named "is valid", and the
> second is named "is null".
> 
> In practice, I think providing is_null() is good enough. !is_null() is the
exact
> same number of characters as is_valid(), and doesn't leave someone wondering
if
> null might be valid, or there's a third possible state that's not valid and
not
> null.

Done.  Thanks for chiming in.  I don't actually remember why I have is_valid
above (I am guessing that I started with is_valid and then realized that
SurfaceId has is_null and then ended up with 2 without thinking about it much). 
I agree that having only is_null is better.

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type.h:53: bool is_null() const { return value_ ==
kInvalidValue; }
On 2016/01/07 22:50:37, dcheng wrote:
> On 2016/01/07 at 22:49:42, ncarter wrote:
> > On 2016/01/07 22:07:28, dcheng wrote:
> > > Bikeshedding... seems a bit odd that the first is named "is valid", and
the
> > > second is named "is null".
> > > 
> > > In practice, I think providing is_null() is good enough. !is_null() is the
> exact
> > > same number of characters as is_valid(), and doesn't leave someone
wondering
> if
> > > null might be valid, or there's a third possible state that's not valid
and
> not
> > > null.
> > 
> > Continuing the bikeshed: If we go to only one getter, I think is_valid() (or
> is_invalid()) is the better one to keep, since sometimes "null" actually would
> mean "has a value of MSG_ROUTING_NONE / -2" whereas "null" to me suggests a
zero
> value?
> 
> Either way is fine. Just that having two different spellings for the same
thing
> feels funky.

I'd prefer "is_null", because this is the name of an existing SurfaceId::is_null
method.  Having the same name will make it easier to "reimplement" SurfaceId on
top of IdType in the future.

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
File content/common/id_type_unittest.cc (right):

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type_unittest.cc:10: #include <unordered_map>
On 2016/01/07 22:07:28, dcheng wrote:
> Not yet allowed =)
> 
> (Though it's very close)

Done - removed std::unordered_container support.

I think the right thing to do is to pull unordered_map support for now:

- This support is not needed (because as you point out unordered_map is
forbidden for now and therefore not used anywhere in Chromium)

- The removal is safe - i.e. won't cause any latent surprises - compilation will
simply fail when somebody tries to use IdType as a key of an
std::unordered_container.

We can revisit later when 1) std::unordered_container are allowed and 2) when
retrying to move IdType to //base.  At that point we can also discuss
desirability of defining std::hash functor for IdType (which might be slightly
frowned upon because of incompatibility with future hashing frameworks?).

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type_unittest.cc:59: EXPECT_EQ(sizeof(uint32_t), sizeof(id));
On 2016/01/07 22:07:28, dcheng wrote:
> It seems like these should be static asserts... somewhere.

Done.  I think somewhere = here is fine (there are other static_asserts
elsewhere, in a separate, not-width-specific unit test).

https://codereview.chromium.org/1529363006/diff/120001/content/common/id_type...
content/common/id_type_unittest.cc:74: EXPECT_EQ(std::string("456"), ss.str());
On 2016/01/07 22:07:28, dcheng wrote:
> Nit: std::string should have a comparison operator for const char* and
> std::string, so I don't think the explicit string construction is required.

Done.

[Łukasz Anforowicz, 2016-01-08 00:30:06]

Description was changed from

==========
Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/browser/downloads - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
   
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...

BUG=565545
==========

to

==========
Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/common directory - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
   
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...

BUG=565545
==========

[Łukasz Anforowicz, 2016-01-08 23:45:24]

FYI - I've pinged [1] cxx@ discussion list and asked them to shout if they think
that adding a new specialization of BASE_HASH_NAMESPACE::hash<...> for IdType is
undesirable.  Since I haven't heard back (and we can always transition to an
explicit [2] hasher later), I'll just go ahead and push to CQ as-is.

[1] https://groups.google.com/a/chromium.org/d/msg/cxx/nCdjQqnouO4/LlVWe3k6DwAJ
[2] something like the 3rd template argument below:
        base::hash_map<SaveItemId, SaveItem*, SaveItemId::hasher>
saved_success_items_;

[Łukasz Anforowicz, 2016-01-08 23:45:50]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-01-08 23:45:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1529363006/#ps160001
(title: "Addressed CR feedback from Daniel.")

[commit-bot: I haz the power, 2016-01-08 23:47:52]

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

[commit-bot: I haz the power, 2016-01-09 01:43:02]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-09 01:44:02]

Description was changed from

==========
Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/common directory - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
   
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...

BUG=565545
==========

to

==========
Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

This CL:
- Introduces content::IdType<...> template and unit tests.
- Uses content::IdType<...> to introduce SavePackageId and SaveItemId.

For now id_type.h is in content/common directory - if it proves useful
for other things, then we can move it around (see also [1], [2] and [3]).

[1] abandoned CL - crrev.com/1496103002:
    Reusing base::IdType<...> to implement SurfaceId.

[2] abandoned CL - crrev.com/1492413002:
    Adding a compile-time safe base::IdType<...> into //base.

[3] Id type discussion at chromium.org / site-isolation-dev list:
   
https://groups.google.com/a/chromium.org/d/topic/site-isolation-dev/4YWsj6keR...

BUG=565545

Committed: https://crrev.com/1764f5c41e21cf607ffc4d644e077c096a7e78b9
Cr-Commit-Position: refs/heads/master@{#368502}
==========

[commit-bot: I haz the power, 2016-01-09 01:44:02]

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