Allow Mojo structs as map keys

Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
   hash functions we already use for e.g. unordered_containers).

 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.

 * Modify the generated Struct/UnionTraits so they generate const accessors to
   the struct/union fields where possible (i.e. when there are not handle
   fields).

Committed: https://crrev.com/96a67c55783156f1b32a3915fcb54038d556e30f
Cr-Commit-Position: refs/heads/master@{#420578}

[tibell, 2016-09-19 23:17:02]

Add tests

[tibell, 2016-09-19 23:17:07]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 23:17:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-19 23:19:17]

Description was changed from

==========
WIP: Support structs as map keys

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

to

==========
Allow Mojo structs as map keys

This requires Mojo structs to comparable and hashable.
==========

[commit-bot: I haz the power, 2016-09-19 23:22:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-19 23:22:16]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2016-09-19 23:52:55]

Add blink mojom dependency

[tibell, 2016-09-19 23:52:59]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 23:53:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 00:04:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 00:04:56]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tibell, 2016-09-20 00:42:53]

Description was changed from

==========
Allow Mojo structs as map keys

This requires Mojo structs to comparable and hashable.
==========

to

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
hash functions we already use for e.g. unordered_containers).
 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.
 * Modify the generated Struct/UnionTraits so they generate const accessors to
the struct/union fields where possible (i.e. when there are not handle fields).
==========

[tibell, 2016-09-20 00:45:16]

Minor clean-ups

[tibell, 2016-09-20 00:45:21]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 00:46:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 00:56:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 00:56:03]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[tibell, 2016-09-20 04:57:22]

Deal with type-mapped structs

[tibell, 2016-09-20 04:57:27]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 04:57:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 05:11:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 05:11:25]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[tibell, 2016-09-20 05:33:13]

Override std::hash inside the right namespace

[tibell, 2016-09-20 05:33:19]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 05:33:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 05:45:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 05:45:35]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[tibell, 2016-09-20 05:58:26]

Address GCC syntax strictness

[tibell, 2016-09-20 05:58:32]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 05:58:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 06:08:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 06:08:51]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[tibell, 2016-09-20 06:30:39]

Remove left-over import

[tibell, 2016-09-20 06:30:45]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 06:30:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-20 06:32:57]

Description was changed from

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
hash functions we already use for e.g. unordered_containers).
 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.
 * Modify the generated Struct/UnionTraits so they generate const accessors to
the struct/union fields where possible (i.e. when there are not handle fields).
==========

to

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
   hash functions we already use for e.g. unordered_containers).
 
 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.
 
 * Modify the generated Struct/UnionTraits so they generate const accessors to
   the struct/union fields where possible (i.e. when there are not handle
   fields).
==========

[tibell, 2016-09-20 07:14:34]

tibell@chromium.org changed reviewers:
+ rockot@chromium.org, sammc@chromium.org, yzshen@chromium.org

[tibell, 2016-09-20 07:14:35]

This is now ready for a first review. I'm still adding some more tests.

[commit-bot: I haz the power, 2016-09-20 07:32:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 07:32:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yzshen1, 2016-09-20 23:44:40]

It mostly looks good. This is nice work. :)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/clone_equals_util.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:160: size_t HashCombine(size_t
seed, const T& value) {
Does it make sense to move hash-related functions to a separate file? (and the
same comment for wtf_clone_equals_util.h)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:199: return !value ?
HashCombine(seed, 0) : internal::Hash(seed, *value);
nit: is "internal::" necessary here?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:106: static unsigned
hash(const mojo::StructPtr<T>& value) {
nit: namespace is not needed (here and changes below)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:107: return
mojo::internal::Hash(mojo::internal::kHashSeed, *value.get());
*value.get(): Is it possible to dereference null ptr here?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:111: return
left->Equals(*right);
ditto

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:119: return
mojo::internal::Hash(mojo::internal::kHashSeed, *value.get());
ditto

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:123: return
left->Equals(*right);
ditto

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/map_traits_stl.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/map_traits_stl.h:97: template <typename IK, typename
IV>
nit: It is not very obvious what IK/IV mean. Please update map_traits.h and
comment about these types.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:58: static bool
Insert(WTF::HashMap<K, V>& input, const K& key, const V& value) {
Do we still need this method?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:202: // TODO(tibell): Is |value| nullable?
value.get() won't fail even if |value| is null.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (left):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:98: // converted to Testable to do == or
!= comparison.
If you would like to allow == and !=, please fix the TODO on line 104. :)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:188: // Forbid the == and != operators
explicitly, otherwise InlinedStructPtr will
Please consider fixing TODO on line 197. :)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:91: bool isHashTableDeletedValue() const {
Does it feel cleaner if we have these WTF-only functions in a separate class,
say StructPtrWTFHelper or something like that?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:217: NIL,
I think NULL might be more commonly used.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:4:
[u4]10                 // name
This should be 20. Otherwise we are not testing the right method.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:9:
[dist4]method10_params  // num_bytes
please update the name accordingly. This is method 20.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:24:
[dist8]key_struct_a_1
I don't think this test case matches the parameter definition of Method20.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom:94:
Method20(map<StructB, StructB> param0);
Will this break the current Java/JS bindings? You might need to skip this test
in Java/JS for now.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl:14:
{%-   set const = "" if field.kind|contains_handles else "const" %}
Does it make sense to name this variable "maybe_const"? Otherwise, at a glance
people may think {{const}} always means "const".

FYI, although it may be a corner case that we don't want to worry about, "not
containing handles" doesn't necessarily mean that "it can be serialized with a
const ref". For example:
// Mojom type:
struct Foo { Bar bar; }

// typemapping:
Foo=CustomFoo[copyable_pass_by_value]

// StructTraits<FooDataView, CustomFoo>:
...
static const Bar& bar(CustomFoo& foo); // Use a non-const ref as input, maybe it
wants to modify |foo| in-place.

In this case, an auto-generated struct having a Foo field will fail to compile.

But I think this is probably a corner case that no one will care about.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl:28:
static std::add_lvalue_reference<const
std::remove_reference<decltype(std::declval<{{mojom_type}}>().get_{{field.name}}())>::type>::type
This is quite hard to understand, maybe as you said, we should output the type,
instead of relying on template magic. (I was a little lazy when writing the
original code. :P)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:876: # TODO(tibell):
We don't know if user-defined types contain handles.
Native-only types are serialized using old IPC::ParamsTraits and considered as a
byte blob, so it shouldn't contain any handle.

[Sam McNally, 2016-09-21 04:57:58]

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/clone_equals_util.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:197: struct
HashTraits<base::Optional<T>, false> {
If we're still disallowing nullable structs, arrays and maps as map keys, I
don't think we need these specialisations yet. Alternatively, add some tests so
these don't rot.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:77: struct
HashTraits<WTF::Vector<T>, false> {
Ditto for these two.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:192: slot = InlinedStructPtr();
Should this be constructing an InlinedStructPtr in &slot like the StructPtr
version?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl:71:
switch (tag_) {
Should |tag_| influence the hash?

[tibell, 2016-09-21 07:08:33]

Address review comments

[tibell, 2016-09-21 07:08:39]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 07:09:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-21 07:10:51]

tibell@chromium.org changed reviewers:
+ erg@chromium.org

[tibell, 2016-09-21 07:10:54]

+erg@ for:
components/font_service/public/cpp/font_service_thread.cc

I have to comments left to address, but I'm replying to the other ones first in
the interest of saving a 24h round-trip for those. :)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/clone_equals_util.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:160: size_t HashCombine(size_t
seed, const T& value) {
On 2016/09/20 23:44:39, yzshen1 wrote:
> Does it make sense to move hash-related functions to a separate file? (and the
> same comment for wtf_clone_equals_util.h)
> 

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:197: struct
HashTraits<base::Optional<T>, false> {
On 2016/09/21 04:57:58, Sam McNally wrote:
> If we're still disallowing nullable structs, arrays and maps as map keys, I
> don't think we need these specialisations yet. Alternatively, add some tests
so
> these don't rot.

Done. Deleted.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/clone_equals_util.h:199: return !value ?
HashCombine(seed, 0) : internal::Hash(seed, *value);
On 2016/09/20 23:44:39, yzshen1 wrote:
> nit: is "internal::" necessary here?

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:77: struct
HashTraits<WTF::Vector<T>, false> {
On 2016/09/21 04:57:58, Sam McNally wrote:
> Ditto for these two.

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:106: static unsigned
hash(const mojo::StructPtr<T>& value) {
On 2016/09/20 23:44:39, yzshen1 wrote:
> nit: namespace is not needed (here and changes below)

Done.

These now all uses hashing/equality on the {Inlined}StructPtr itself, which
correctly takes nullability into account.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:107: return
mojo::internal::Hash(mojo::internal::kHashSeed, *value.get());
On 2016/09/20 23:44:39, yzshen1 wrote:
> *value.get(): Is it possible to dereference null ptr here?

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:111: return
left->Equals(*right);
On 2016/09/20 23:44:39, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:119: return
mojo::internal::Hash(mojo::internal::kHashSeed, *value.get());
On 2016/09/20 23:44:39, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:123: return
left->Equals(*right);
On 2016/09/20 23:44:39, yzshen1 wrote:
> ditto

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/map_traits_stl.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/map_traits_stl.h:97: template <typename IK, typename
IV>
On 2016/09/20 23:44:40, yzshen1 wrote:
> nit: It is not very obvious what IK/IV mean. Please update map_traits.h and
> comment about these types.

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/map_traits_wtf_hash_map.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/map_traits_wtf_hash_map.h:58: static bool
Insert(WTF::HashMap<K, V>& input, const K& key, const V& value) {
On 2016/09/20 23:44:40, yzshen1 wrote:
> Do we still need this method?

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:202: // TODO(tibell): Is |value| nullable?
On 2016/09/20 23:44:40, yzshen1 wrote:
> value.get() won't fail even if |value| is null.

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (left):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:98: // converted to Testable to do == or
!= comparison.
On 2016/09/20 23:44:40, yzshen1 wrote:
> If you would like to allow == and !=, please fix the TODO on line 104. :)

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:188: // Forbid the == and != operators
explicitly, otherwise InlinedStructPtr will
On 2016/09/20 23:44:40, yzshen1 wrote:
> Please consider fixing TODO on line 197. :)

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:192: slot = InlinedStructPtr();
On 2016/09/21 04:57:58, Sam McNally wrote:
> Should this be constructing an InlinedStructPtr in &slot like the StructPtr
> version?

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:217: NIL,
On 2016/09/20 23:44:40, yzshen1 wrote:
> I think NULL might be more commonly used.

It will collide with the standard NULL macro. :)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:4:
[u4]10                 // name
On 2016/09/20 23:44:40, yzshen1 wrote:
> This should be 20. Otherwise we are not testing the right method.

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:9:
[dist4]method10_params  // num_bytes
On 2016/09/20 23:44:40, yzshen1 wrote:
> please update the name accordingly. This is method 20.

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:24:
[dist8]key_struct_a_1
On 2016/09/20 23:44:40, yzshen1 wrote:
> I don't think this test case matches the parameter definition of Method20.

Done.

I changed my mind for the map value type half way through the CL and forgot to
update the mojom.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd20_good.data:34:
[u4]0 [u1]0 [u1]0 [u1]0 [u1]0  // manual padding for array alignment
Something is wrong with the alignment (in the updated version I'm about to
post). I can't figure it out. Each struct/array starts at a 8 byte alignment.
Can you see the error?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom:94:
Method20(map<StructB, StructB> param0);
On 2016/09/20 23:44:40, yzshen1 wrote:
> Will this break the current Java/JS bindings? You might need to skip this test
> in Java/JS for now.

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl:14:
{%-   set const = "" if field.kind|contains_handles else "const" %}
On 2016/09/20 23:44:40, yzshen1 wrote:
> Does it make sense to name this variable "maybe_const"? Otherwise, at a glance
> people may think {{const}} always means "const".

Done.

> FYI, although it may be a corner case that we don't want to worry about, "not
> containing handles" doesn't necessarily mean that "it can be serialized with a
> const ref". For example:
> // Mojom type:
> struct Foo { Bar bar; }
> 
> // typemapping:
> Foo=CustomFoo[copyable_pass_by_value]
> 
> // StructTraits<FooDataView, CustomFoo>:
> ...
> static const Bar& bar(CustomFoo& foo); // Use a non-const ref as input, maybe
it
> wants to modify |foo| in-place.
> 
> In this case, an auto-generated struct having a Foo field will fail to
compile.
> 
> But I think this is probably a corner case that no one will care about.

I'll leave a TODO for now. Do you have any idea how to detect this case and
generate the right constness for it?

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl:71:
switch (tag_) {
On 2016/09/21 04:57:58, Sam McNally wrote:
> Should |tag_| influence the hash?

Done.

We better if we have an empty union (e.g. an enum) or otherwise they will all
hash to the same value!

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:876: # TODO(tibell):
We don't know if user-defined types contain handles.
On 2016/09/20 23:44:40, yzshen1 wrote:
> Native-only types are serialized using old IPC::ParamsTraits and considered as
a
> byte blob, so it shouldn't contain any handle.

Done.

[tibell, 2016-09-21 07:17:31]

Revert no-longer-needed changes to clone_equals_util.h

[tibell, 2016-09-21 07:17:37]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 07:17:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 08:02:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 08:02:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Elliot Glaysher, 2016-09-21 17:27:30]

lgtm with nit

https://codereview.chromium.org/2339413004/diff/180001/components/font_servic...
File components/font_service/public/cpp/font_service_thread.cc (right):

https://codereview.chromium.org/2339413004/diff/180001/components/font_servic...
components/font_service/public/cpp/font_service_thread.cc:127: *out_valid =
bool(font_identity);
nit: font_identity.is_bound()

This is what the InterfacePtr is doing under the hood in the cast to Testable;
cast to bool trick, and the style guide is against c style casts.

[yzshen1, 2016-09-21 23:17:53]

A few more comments. Thanks!

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:91: bool isHashTableDeletedValue() const {
On 2016/09/20 23:44:40, yzshen1 (OOO Sep26-Oct30) wrote:
> Does it feel cleaner if we have these WTF-only functions in a separate class,
> say StructPtrWTFHelper or something like that?

This comment is not replied yet.
(Besides, if these methods are outside of the WTF namespace, it seems more
consistent to name them with uppercase initial character.)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:217: NIL,
On 2016/09/21 07:10:53, tibell wrote:
> On 2016/09/20 23:44:40, yzshen1 wrote:
> > I think NULL might be more commonly used.
> 
> It will collide with the standard NULL macro. :)

Ah, of course. :)

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl:14:
{%-   set const = "" if field.kind|contains_handles else "const" %}
On 2016/09/21 07:10:53, tibell wrote:
> On 2016/09/20 23:44:40, yzshen1 wrote:
> > Does it make sense to name this variable "maybe_const"? Otherwise, at a
glance
> > people may think {{const}} always means "const".
> 
> Done.
> 
> > FYI, although it may be a corner case that we don't want to worry about,
"not
> > containing handles" doesn't necessarily mean that "it can be serialized with
a
> > const ref". For example:
> > // Mojom type:
> > struct Foo { Bar bar; }
> > 
> > // typemapping:
> > Foo=CustomFoo[copyable_pass_by_value]
> > 
> > // StructTraits<FooDataView, CustomFoo>:
> > ...
> > static const Bar& bar(CustomFoo& foo); // Use a non-const ref as input,
maybe
> it
> > wants to modify |foo| in-place.
> > 
> > In this case, an auto-generated struct having a Foo field will fail to
> compile.
> > 
> > But I think this is probably a corner case that no one will care about.
> 
> I'll leave a TODO for now. Do you have any idea how to detect this case and
> generate the right constness for it?

It would require adding new attribute in typemap config files. I think it is
pretty low priority. We could revisit it when there is an actual use case, which
should be pretty rare.

It is up to you whether to leave a TODO. (If you decide to leave one,) it seems
a little more clear to say "... e.g., custom types which don't contain handles
but require non-const reference for serialization." WDYT?

https://codereview.chromium.org/2339413004/diff/180001/mojo/android/javatests...
File mojo/android/javatests/src/org/chromium/mojo/bindings/ValidationTest.java
(right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/android/javatests...
mojo/android/javatests/src/org/chromium/mojo/bindings/ValidationTest.java:64: if
(pathname.getName().startsWith("conformance_mthd19_good")) {
I just realized that the naming of conformance_mthd19_good is inconsistent:
usually if a validation test case is expected to fail, the name explains the
failure, e.g., conformance_mthd19_exceed_recursion_limit.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/hash_util.h:10: #include <unordered_map>
some of the includes are not needed, please remove.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:14: #include
"third_party/WebKit/Source/wtf/Optional.h"
Some of the includes are not needed, please remove.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/map_traits.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/map_traits.h:40: //     // message pipe receiving it to
be disconnected. |IK| and |IK| are
IK and *IV*

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:113: bool operator==(const StructPtr<T>&
other) const {
Something to think about: If we decide to have "==" do value comparison, maybe
we should get rid of Equals entirely. WDYT?

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:214: new (&slot) InlinedStructPtr();
Is |slot|'s constructor already called before passed into this function? This
calls constructor for |slot| again without destructing it first.

(and also line 108)

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom
(right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom:94:
Method20(map<StructB, uint8> param0);
It seems weird: reading the test case I thought the mojom definition should be
Method20(map<Struct*A*, uint8> param0);

Did the test pass?

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/js/validat...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/js/validat...
mojo/public/js/validation_unittests.js:240:
testFiles[i].indexOf("conformance_mthd20") != -1 ||
please also add a TODO for this.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:70: {#
hash_util.h includes template specializations that should be preswent for
preswent -> present

[tibell, 2016-09-22 05:16:19]

Address all review comments

[tibell, 2016-09-22 05:16:26]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 05:16:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-22 05:17:23]

PTAL

I've addressed all comments now. The decltype stuff is now much simpler.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:91: bool isHashTableDeletedValue() const {
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> On 2016/09/20 23:44:40, yzshen1 (OOO Sep26-Oct30) wrote:
> > Does it feel cleaner if we have these WTF-only functions in a separate
class,
> > say StructPtrWTFHelper or something like that?
> 
> This comment is not replied yet.
> (Besides, if these methods are outside of the WTF namespace, it seems more
> consistent to name them with uppercase initial character.)

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl:14:
{%-   set const = "" if field.kind|contains_handles else "const" %}
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> On 2016/09/21 07:10:53, tibell wrote:
> > On 2016/09/20 23:44:40, yzshen1 wrote:
> > > Does it make sense to name this variable "maybe_const"? Otherwise, at a
> glance
> > > people may think {{const}} always means "const".
> > 
> > Done.
> > 
> > > FYI, although it may be a corner case that we don't want to worry about,
> "not
> > > containing handles" doesn't necessarily mean that "it can be serialized
with
> a
> > > const ref". For example:
> > > // Mojom type:
> > > struct Foo { Bar bar; }
> > > 
> > > // typemapping:
> > > Foo=CustomFoo[copyable_pass_by_value]
> > > 
> > > // StructTraits<FooDataView, CustomFoo>:
> > > ...
> > > static const Bar& bar(CustomFoo& foo); // Use a non-const ref as input,
> maybe
> > it
> > > wants to modify |foo| in-place.
> > > 
> > > In this case, an auto-generated struct having a Foo field will fail to
> > compile.
> > > 
> > > But I think this is probably a corner case that no one will care about.
> > 
> > I'll leave a TODO for now. Do you have any idea how to detect this case and
> > generate the right constness for it?
> 
> It would require adding new attribute in typemap config files. I think it is
> pretty low priority. We could revisit it when there is an actual use case,
which
> should be pretty rare.
> 
> It is up to you whether to leave a TODO. (If you decide to leave one,) it
seems
> a little more clear to say "... e.g., custom types which don't contain handles
> but require non-const reference for serialization." WDYT?

Done.

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl:28:
static std::add_lvalue_reference<const
std::remove_reference<decltype(std::declval<{{mojom_type}}>().get_{{field.name}}())>::type>::type
On 2016/09/20 23:44:40, yzshen1 (OOO Sep26-Oct30) wrote:
> This is quite hard to understand, maybe as you said, we should output the
type,
> instead of relying on template magic. (I was a little lazy when writing the
> original code. :P)

Done.

https://codereview.chromium.org/2339413004/diff/180001/components/font_servic...
File components/font_service/public/cpp/font_service_thread.cc (right):

https://codereview.chromium.org/2339413004/diff/180001/components/font_servic...
components/font_service/public/cpp/font_service_thread.cc:127: *out_valid =
bool(font_identity);
On 2016/09/21 17:27:30, Elliot Glaysher wrote:
> nit: font_identity.is_bound()
> 
> This is what the InterfacePtr is doing under the hood in the cast to Testable;
> cast to bool trick, and the style guide is against c style casts.

Done (using is_null).

https://codereview.chromium.org/2339413004/diff/180001/mojo/android/javatests...
File mojo/android/javatests/src/org/chromium/mojo/bindings/ValidationTest.java
(right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/android/javatests...
mojo/android/javatests/src/org/chromium/mojo/bindings/ValidationTest.java:64: if
(pathname.getName().startsWith("conformance_mthd19_good")) {
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> I just realized that the naming of conformance_mthd19_good is inconsistent:
> usually if a validation test case is expected to fail, the name explains the
> failure, e.g., conformance_mthd19_exceed_recursion_limit.

Done.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/hash_util.h:10: #include <unordered_map>
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> some of the includes are not needed, please remove.

Done.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:14: #include
"third_party/WebKit/Source/wtf/Optional.h"
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> Some of the includes are not needed, please remove.

Done.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/map_traits.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/map_traits.h:40: //     // message pipe receiving it to
be disconnected. |IK| and |IK| are
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> IK and *IV*
> 

Done.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:113: bool operator==(const StructPtr<T>&
other) const {
On 2016/09/21 23:17:53, yzshen1 (OOO Sep26-Oct30) wrote:
> Something to think about: If we decide to have "==" do value comparison, maybe
> we should get rid of Equals entirely. WDYT?
> 

I added a TODO. Both Sam and I want to get rid of it. It might be a sufficiently
large change to deserve its own CL though.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:214: new (&slot) InlinedStructPtr();
On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> Is |slot|'s constructor already called before passed into this function? This
> calls constructor for |slot| again without destructing it first.
> 
> (and also line 108)

|slot| referred to a previous, real value that got deleted and had its
destructor run, so this is the first time the "deleted value" has its
constructor called.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom
(right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom:94:
Method20(map<StructB, uint8> param0);
On 2016/09/21 23:17:53, yzshen1 (OOO Sep26-Oct30) wrote:
> It seems weird: reading the test case I thought the mojom definition should be
> Method20(map<Struct*A*, uint8> param0);
> 
> Did the test pass?

No, it failed with alignment errors. I now think I understand why. :)

Fixed.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/js/validat...
File mojo/public/js/validation_unittests.js (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/js/validat...
mojo/public/js/validation_unittests.js:240:
testFiles[i].indexOf("conformance_mthd20") != -1 ||
On 2016/09/21 23:17:53, yzshen1 (OOO Sep26-Oct30) wrote:
> please also add a TODO for this.

Done.

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl:70: {#
hash_util.h includes template specializations that should be preswent for
On 2016/09/21 23:17:53, yzshen1 (OOO Sep26-Oct30) wrote:
> preswent -> present

Done.

[commit-bot: I haz the power, 2016-09-22 05:39:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 05:39:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-09-22 05:40:52]

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:70: return
mojo::internal::StructPtrWTFHelper<T>::ConstructDeletedValue(value);
Wat?

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:90: return
mojo::internal::InlinedStructPtrWTFHelper<T>::ConstructDeletedValue(
Ditto.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:202: return hash<std::string>()(value.get());
null and empty strings will have the same hash.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:107: bool operator==(const StructPtr<T>&
other) const {
How about an out of line
template <typename T>
bool operator==(const StructPtr<T>&, const StructPtr<T>&)?

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:262: slot.state_ =
InlinedStructPtr<Struct>::DELETEDc;
DELETEDc?

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/test_structs.mojom:396: Rect rect;
TypemappedRect

[tibell, 2016-09-22 07:18:05]

Address sammc's comments and improve Blink tests

[tibell, 2016-09-22 07:18:10]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 07:18:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-22 07:18:34]

PTAL

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:70: return
mojo::internal::StructPtrWTFHelper<T>::ConstructDeletedValue(value);
On 2016/09/22 05:40:51, Sam McNally wrote:
> Wat?

Done.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:90: return
mojo::internal::InlinedStructPtrWTFHelper<T>::ConstructDeletedValue(
On 2016/09/22 05:40:51, Sam McNally wrote:
> Ditto.

Done.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/string.h (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/string.h:202: return hash<std::string>()(value.get());
On 2016/09/22 05:40:51, Sam McNally wrote:
> null and empty strings will have the same hash.

Done.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:107: bool operator==(const StructPtr<T>&
other) const {
On 2016/09/22 05:40:51, Sam McNally wrote:
> How about an out of line
> template <typename T>
> bool operator==(const StructPtr<T>&, const StructPtr<T>&)?

Done.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:262: slot.state_ =
InlinedStructPtr<Struct>::DELETEDc;
On 2016/09/22 05:40:51, Sam McNally wrote:
> DELETEDc?

Done.

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/interfaces...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/2339413004/diff/200001/mojo/public/interfaces...
mojo/public/interfaces/bindings/tests/test_structs.mojom:396: Rect rect;
On 2016/09/22 05:40:52, Sam McNally wrote:
> TypemappedRect

Done.

[tibell, 2016-09-22 07:30:26]

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:31: struct WTFHashTraits;
This trait exists separately from HashTraits so that we

 * can eventually fix the TODO above (by calling DefaultHash<T>::Hash::hash
instead of std::hash there) and
 * have a natural place to put an instance for WTF::String.

I didn't have both HashTraits and WTFHashTraits initially. Worth keeping?

[commit-bot: I haz the power, 2016-09-22 08:35:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 08:35:13]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-09-22 23:48:57]

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/180001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:214: new (&slot) InlinedStructPtr();
On 2016/09/22 05:17:23, tibell wrote:
> On 2016/09/21 23:17:52, yzshen1 (OOO Sep26-Oct30) wrote:
> > Is |slot|'s constructor already called before passed into this function?
This
> > calls constructor for |slot| again without destructing it first.
> > 
> > (and also line 108)
> 
> |slot| referred to a previous, real value that got deleted and had its
> destructor run, so this is the first time the "deleted value" has its
> constructor called.

Please add a comment about this to the corresponding methods in
[Inlined]StructPtrWTFHelper.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:25: //     there are no general
instance for enums and their can't be an instance
I cannot parse this sentence. (Is it because "their" should be "there"?)

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl:12:
{#    We want the field accessor to be const whenever possible to allow #}
style nit: I think the other places use
{#  blah
    blah #}

instead of
{#  blah #}
{#  blah #}

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl:16:
{# We want the field accessor to be const whenever possible to allow
nit: use the same indent as line 15

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl:70:
size_t {{union.name}}::Hash(size_t seed) const {
Does this needs to call different hash functions according to for_blink value?
(just like what you did in wrapper_class_definition.tmpl)

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:161: def
GetQualifiedNameForKind(kind, internal=False, flatten_nested_kind=False):
Just something to think about, we could remove this now. (No need to do it in
this CL.)

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:195: if
(kind.native_only or
As we discussed, native-only types may be hashable.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:206: elif
mojom.IsArrayKind(kind):
Why array/map are not hashable?

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:302: purely a native
struct that must be type-mapped.
Nit: it doesn't have to be typemapped. We have NativeStruct defined for the
non-typemapped case. (But it should be very rare.)

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:308: which Java class
name to use to represent it in the generated
I thought we didn't support this?
(But we support a custom package name for the "module" definition.)
For struct, we support Native attribute.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:881: # Interfaces are
wrappers around handles.
Strictly speaking, associated interfaces are not handles (they are just
interface IDs).

Maybe this function can be called ContainsHandlesOrInterfaces?

[tibell, 2016-09-23 00:04:50]

Address yzshen@'s review comments

[tibell, 2016-09-23 00:04:57]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 00:05:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-23 00:07:08]

PTAL

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/wtf_hash_util.h (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/wtf_hash_util.h:25: //     there are no general
instance for enums and their can't be an instance
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> I cannot parse this sentence. (Is it because "their" should be "there"?)

Better now?

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/struct_traits_declaration.tmpl:12:
{#    We want the field accessor to be const whenever possible to allow #}
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> style nit: I think the other places use
> {#  blah
>     blah #}
> 
> instead of
> {#  blah #}
> {#  blah #}

Done.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/union_traits_declaration.tmpl:16:
{# We want the field accessor to be const whenever possible to allow
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> nit: use the same indent as line 15

Done.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl
(right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl:70:
size_t {{union.name}}::Hash(size_t seed) const {
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> Does this needs to call different hash functions according to for_blink value?
> (just like what you did in wrapper_class_definition.tmpl)

Done.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:161: def
GetQualifiedNameForKind(kind, internal=False, flatten_nested_kind=False):
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> Just something to think about, we could remove this now. (No need to do it in
> this CL.)

Acknowledged.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:195: if
(kind.native_only or
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> As we discussed, native-only types may be hashable.

Done.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:206: elif
mojom.IsArrayKind(kind):
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> Why array/map are not hashable?

We didn't think we'd need it. I've added a TODO. It should be easy to add .

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:302: purely a native
struct that must be type-mapped.
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> Nit: it doesn't have to be typemapped. We have NativeStruct defined for the
> non-typemapped case. (But it should be very rare.)

Done.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:308: which Java class
name to use to represent it in the generated
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> I thought we didn't support this?
> (But we support a custom package name for the "module" definition.)
> For struct, we support Native attribute.

Done.

https://codereview.chromium.org/2339413004/diff/220001/mojo/public/tools/bind...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:881: # Interfaces are
wrappers around handles.
On 2016/09/22 23:48:56, yzshen1 (OOO Sep26-Oct30) wrote:
> Strictly speaking, associated interfaces are not handles (they are just
> interface IDs).
> 
> Maybe this function can be called ContainsHandlesOrInterfaces?

Done.

[commit-bot: I haz the power, 2016-09-23 00:41:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 00:41:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yzshen1, 2016-09-23 01:08:23]

LGTM Thanks for the nice work!

The following comment is not replied yet:
> |slot| referred to a previous, real value that got deleted and had its
> destructor run, so this is the first time the "deleted value" has its
> constructor called.

Please add a comment about this to the corresponding methods in
[Inlined]StructPtrWTFHelper.

[tibell, 2016-09-23 01:10:18]

Add more tests for hashing

[tibell, 2016-09-23 01:10:25]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 01:10:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-23 01:12:11]

Add comment about deleted values

[tibell, 2016-09-23 01:12:17]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 01:12:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 01:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 01:16:58]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tibell, 2016-09-23 01:23:33]

Fix merge problem in BUILD.gn file

[tibell, 2016-09-23 01:23:40]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 01:24:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-09-23 01:35:04]

lgtm

https://codereview.chromium.org/2339413004/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:278: 
No blank line between namespace closing braces.

[tibell, 2016-09-23 01:39:04]

Remove a blank line

[tibell, 2016-09-23 01:39:13]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 01:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-23 01:39:35]

https://codereview.chromium.org/2339413004/diff/300001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/struct_ptr.h (right):

https://codereview.chromium.org/2339413004/diff/300001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/struct_ptr.h:278: 
On 2016/09/23 01:35:04, Sam McNally wrote:
> No blank line between namespace closing braces.

Done.

[tibell, 2016-09-23 01:49:25]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-09-23 01:49:33]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-09-23 01:49:34]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
yzshen@chromium.org, sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2339413004/#ps320001
(title: "Remove a blank line")

[commit-bot: I haz the power, 2016-09-23 01:50:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 03:37:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 03:37:53]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tibell, 2016-09-23 03:44:43]

Fix hash unit test on Windows

[tibell, 2016-09-23 03:44:50]

The CQ bit was checked by tibell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 03:45:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tibell, 2016-09-23 03:45:41]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2016-09-23 03:45:49]

The CQ bit was checked by tibell@chromium.org

[tibell, 2016-09-23 03:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
erg@chromium.org, sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2339413004/#ps340001
(title: "Fix hash unit test on Windows")

[commit-bot: I haz the power, 2016-09-23 03:46:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 04:48:44]

Description was changed from

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
   hash functions we already use for e.g. unordered_containers).
 
 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.
 
 * Modify the generated Struct/UnionTraits so they generate const accessors to
   the struct/union fields where possible (i.e. when there are not handle
   fields).
==========

to

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
   hash functions we already use for e.g. unordered_containers).
 
 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.
 
 * Modify the generated Struct/UnionTraits so they generate const accessors to
   the struct/union fields where possible (i.e. when there are not handle
   fields).
==========

[commit-bot: I haz the power, 2016-09-23 04:48:47]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-09-23 04:50:50]

Description was changed from

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
   hash functions we already use for e.g. unordered_containers).
 
 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.
 
 * Modify the generated Struct/UnionTraits so they generate const accessors to
   the struct/union fields where possible (i.e. when there are not handle
   fields).
==========

to

==========
Allow Mojo structs as map keys

 * The hash function is not resilient to hash flooding (but neither is the other
   hash functions we already use for e.g. unordered_containers).

 * Add std::hash and WTF::DefaultHash/HashTraits traits for {Inlined}StructPtr.

 * Modify the generated Struct/UnionTraits so they generate const accessors to
   the struct/union fields where possible (i.e. when there are not handle
   fields).

Committed: https://crrev.com/96a67c55783156f1b32a3915fcb54038d556e30f
Cr-Commit-Position: refs/heads/master@{#420578}
==========

[commit-bot: I haz the power, 2016-09-23 04:50:51]

Patchset 18 (id:??) landed as
https://crrev.com/96a67c55783156f1b32a3915fcb54038d556e30f
Cr-Commit-Position: refs/heads/master@{#420578}