Mojo C++ bindings: introduce mojo::WTFMap for blink bindings.

Mojo C++ bindings: introduce mojo::WTFMap for blink bindings.

BUG=583738
Committed: https://crrev.com/5430084b388ddcf3913c32c7b86ba269a4f258a7
Cr-Commit-Position: refs/heads/master@{#400223}

[yzshen1, 2016-06-03 22:18:55]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org, yutak@chromium.org

[yzshen1, 2016-06-03 22:18:56]

Hi,

Would you please take a look? Thanks!

Ken: everything
Yuta: the usage of WTF::HashMap in wtf_map.h

Kentaro and Elliot: FYI. This change follows what I have done for array. So I
think there shouldn't be concerns about the general approach. Welcome to comment
if you are interested, of course.

[Ken Rockot(use gerrit already), 2016-06-03 23:14:23]

lgtm

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/blink_bindings_configuration.gni (left):

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/blink_bindings_configuration.gni:25: blacklist = [
Woohoo!

[haraken, 2016-06-06 04:18:36]

I haven't reviewed the details, but the approach LGTM.

[Yuta Kitamura, 2016-06-06 05:29:42]

LGTM, the use of WTF::HashMap in WTFMap appears sane.

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/wtf_map.h (right):

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/wtf_map.h:200: operator Testable() const { return
is_null_ ? 0 : &WTFMap::map_; }
Now you can safely use "explicit operator bool()". This lets you
avoid operator==() hacks below.

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/wtf_map.h:215: WTF::HashMap<Key, Value> map_;
What's a bit tricky is that WTF::HashMap does not allow
0 or -1 as a key if the key type is integer. Similarly,
nullptr as a key is prohibited if the key type is a pointer.
That may be worth documenting.

[yzshen1, 2016-06-06 20:48:57]

On 2016/06/06 05:29:42, Yuta Kitamura wrote:
> LGTM, the use of WTF::HashMap in WTFMap appears sane.
> 
>
https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/wtf_map.h (right):
> 
>
https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/wtf_map.h:200: operator Testable() const { return
> is_null_ ? 0 : &WTFMap::map_; }
> Now you can safely use "explicit operator bool()". This lets you
> avoid operator==() hacks below.

Done.

> 
>
https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/wtf_map.h:215: WTF::HashMap<Key, Value> map_;
> What's a bit tricky is that WTF::HashMap does not allow
> 0 or -1 as a key if the key type is integer. Similarly,
> nullptr as a key is prohibited if the key type is a pointer.
> That may be worth documenting.

This is pretty surprising. Why 0 or -1 cannot be integer map keys? I didn't see
it documented within HashMap header file.
Mojom map doesn't have such limitation. Then it seems problematic to use HashMap
as underlying storage. Otherwise what should we do when we receive a mojom map
who has a key 0 or -1?

Pointer is not an issue: mojom map only allows non-null string or primitive
types as key.

Another issue I saw was that there isn't currently a hash function defined for
bool. Do you think it should be added to HashFunctions.h?

[esprehn, 2016-06-06 21:20:24]

WTF maps are also not ordered. It's not a std::map, it's a specialized
std::hash_map that trades some key space for storing an uninitialized value.

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

[yzshen1, 2016-06-06 21:30:26]

On Mon, Jun 6, 2016 at 2:20 PM, Elliott Sprehn <esprehn@chromium.org> wrote:

> WTF maps are also not ordered. It's not a std::map, it's a specialized
> std::hash_map that trades some key space for storing an uninitialized value.
>

Thanks for the reply.

Not ordered is fine. The problem is that we cannot convert some legitimate
mojom map messages (containing -1 or 0 key) to WTF::HashMap.
Is it possible to eliminate this restriction?

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

[esprehn, 2016-06-06 21:43:45]

>> WTF maps are also not ordered. It's not a std::map, it's a specialized
std::hash_map that trades some key space for storing an uninitialized value.
>
>
> Thanks for the reply.
>
> Not ordered is fine. The problem is that we cannot convert some
legitimate mojom map messages (containing -1 or 0 key) to WTF::HashMap.
> Is it possible to eliminate this restriction?
>
>

Not without using more memory. The std containers use more memory to allow
this, that's also how they allow modification during iteration which wtf
forbids.

Since mojo is the IPC system for Chrome, and blink is the primary consumer
and customer, we should just impose this restriction on the IPC messages in
Chrome. :)

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

[esprehn, 2016-06-06 21:53:33]

Fwiw we could also try choosing different values. I don't know why we chose
-1, we could limit the min or max instead.

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

[Ken Rockot(use gerrit already), 2016-06-07 15:49:08]

On Mon, Jun 6, 2016 at 2:43 PM, Elliott Sprehn <esprehn@chromium.org> wrote:

> >> WTF maps are also not ordered. It's not a std::map, it's a specialized
> std::hash_map that trades some key space for storing an uninitialized value.
> >
> >
> > Thanks for the reply.
> >
> > Not ordered is fine. The problem is that we cannot convert some
> legitimate mojom map messages (containing -1 or 0 key) to WTF::HashMap.
> > Is it possible to eliminate this restriction?
> >
> >
>
> Not without using more memory. The std containers use more memory to allow
> this, that's also how they allow modification during iteration which wtf
> forbids.
>
> Since mojo is the IPC system for Chrome, and blink is the primary consumer
> and customer, we should just impose this restriction on the IPC messages in
> Chrome. :)
>
These constraints are understandable given the performance wins WTF map can
get from them. However they are not acceptible constraints for a
general-purpose mojom map, and Blink is not the only consumer and customer
of the services we're building in the chromium tree.

If we're going to have this restriction it should simply be documented as a
limitation of mojo+blink and enforced by the WTFMap traits so developers
can discover errors more easily when interfacing with blink.

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

[yzshen1, 2016-06-07 23:49:11]

Hi, reviewers.

According to our offline discussion, I have added better documentation and also
rejected invalid keys during message deserialization.

The new patch adds default hash traits and hash functions for enum types. That
doesn't affect behavior of any types that were previously supported. Please take
a look. Thanks!

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/wtf_map.h (right):

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/wtf_map.h:200: operator Testable() const { return
is_null_ ? 0 : &WTFMap::map_; }
On 2016/06/06 05:29:42, Yuta Kitamura wrote:
> Now you can safely use "explicit operator bool()". This lets you
> avoid operator==() hacks below.

Done. Good point!

https://codereview.chromium.org/2034273002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/wtf_map.h:215: WTF::HashMap<Key, Value> map_;
On 2016/06/06 05:29:42, Yuta Kitamura wrote:
> What's a bit tricky is that WTF::HashMap does not allow
> 0 or -1 as a key if the key type is integer. Similarly,
> nullptr as a key is prohibited if the key type is a pointer.
> That may be worth documenting.

I have added comments. And also added code to reject such keys during message
deserialization.

[Yuta Kitamura, 2016-06-09 09:46:20]

Still LGTM

[yzshen1, 2016-06-16 19:03:27]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-06-16 19:03:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rockot@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/2034273002/#ps160001
(title: "removed the code of adding enum key support to WTF::HashMap; which
turned out to be non-trivial; wi…")

[commit-bot: I haz the power, 2016-06-16 19:03:50]

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

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

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-16 19:10:15]

Description was changed from

==========
Mojo C++ bindings: introduce mojo::WTFMap for blink bindings.

BUG=583738
==========

to

==========
Mojo C++ bindings: introduce mojo::WTFMap for blink bindings.

BUG=583738

Committed: https://crrev.com/5430084b388ddcf3913c32c7b86ba269a4f258a7
Cr-Commit-Position: refs/heads/master@{#400223}
==========

[commit-bot: I haz the power, 2016-06-16 19:10:16]

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