mojo: Switch the clipboard interface over to using map<>.

mojo: Switch the clipboard interface over to using map<>.

BUG=413863
Committed: https://crrev.com/00a8f30e140e0e88a23fb1f14bfa74680cb4bd54
Cr-Commit-Position: refs/heads/master@{#299942}

[Elliot Glaysher, 2014-10-15 21:10:57]

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

[Elliot Glaysher, 2014-10-15 21:10:58]

Split off from the other patch.

[viettrungluu, 2014-10-15 21:21:13]

LGTM w/nits, a comment, and a question.

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
File mojo/services/clipboard/clipboard_standalone_impl.cc (right):

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
mojo/services/clipboard/clipboard_standalone_impl.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
"

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
mojo/services/clipboard/clipboard_standalone_impl.cc:7: #include <cstring>
nit: I think we usually use <string.h> rather than <cstring>.

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
File mojo/services/clipboard/clipboard_standalone_impl.h (right):

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
mojo/services/clipboard/clipboard_standalone_impl.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
drive-by nit: no "(c)" in current copyright statement

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
File mojo/services/clipboard/clipboard_standalone_unittest.cc (right):

https://codereview.chromium.org/646773005/diff/1/mojo/services/clipboard/clip...
mojo/services/clipboard/clipboard_standalone_unittest.cc:1: // Copyright (c)
2014 The Chromium Authors. All rights reserved.
" (etc.)

https://codereview.chromium.org/646773005/diff/1/mojo/services/html_viewer/we...
File mojo/services/html_viewer/webclipboard_impl.cc (right):

https://codereview.chromium.org/646773005/diff/1/mojo/services/html_viewer/we...
mojo/services/html_viewer/webclipboard_impl.cc:172:
data.insert(mojo::Clipboard::MIME_TYPE_TEXT,
I wonder if mojo::Map should have an operator[] (like mojo::Array does), to make
these things look more natural....

https://codereview.chromium.org/646773005/diff/1/mojo/services/public/interfa...
File mojo/services/public/interfaces/clipboard/clipboard.mojom (right):

https://codereview.chromium.org/646773005/diff/1/mojo/services/public/interfa...
mojo/services/public/interfaces/clipboard/clipboard.mojom:40: // sequence
number. In the case of an empty map, this will just
empty or null map?

[Elliot Glaysher, 2014-10-15 21:46:55]

ptal, especially at the move only type implementation in map_internal.h for
operator[].

https://codereview.chromium.org/646773005/diff/1/mojo/services/html_viewer/we...
File mojo/services/html_viewer/webclipboard_impl.cc (right):

https://codereview.chromium.org/646773005/diff/1/mojo/services/html_viewer/we...
mojo/services/html_viewer/webclipboard_impl.cc:172:
data.insert(mojo::Clipboard::MIME_TYPE_TEXT,
On 2014/10/15 21:21:13, viettrungluu wrote:
> I wonder if mojo::Map should have an operator[] (like mojo::Array does), to
make
> these things look more natural....

Done.

[viettrungluu, 2014-10-15 23:17:24]

Is there a unit test for Map (which operator[] should be tested in)?

https://codereview.chromium.org/646773005/diff/20001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/map_internal.h (right):

https://codereview.chromium.org/646773005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/map_internal.h:74: static inline ValueRefType
Index(
Hmmm. I still find the name "Index" a bit perplexing.

Maybe it should be "GetOrInsert" / "AtOrInsert" / ...? (Or maybe there should be
an explanatory comment?)

(An alternative might be to try to make "Insert" return a ValueRefType, etc. but
it's a bit awkward with the whole move-only business. Hrm.)

https://codereview.chromium.org/646773005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/map_internal.h:163: // on a non-existant key.
drive-by retroactive correction: "existant" -> "existent"

[Elliot Glaysher, 2014-10-15 23:30:44]

ptal, added test

https://codereview.chromium.org/646773005/diff/20001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/lib/map_internal.h (right):

https://codereview.chromium.org/646773005/diff/20001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/lib/map_internal.h:74: static inline ValueRefType
Index(
On 2014/10/15 23:17:23, viettrungluu wrote:
> Hmmm. I still find the name "Index" a bit perplexing.
> 
> Maybe it should be "GetOrInsert" / "AtOrInsert" / ...? (Or maybe there should
be
> an explanatory comment?)
> 
> (An alternative might be to try to make "Insert" return a ValueRefType, etc.
but
> it's a bit awkward with the whole move-only business. Hrm.)

Because it is the backing for the index operator. Still, changed the name to
"GetOrInsert" and added a comment.

[viettrungluu, 2014-10-15 23:54:07]

LGTM w/moar tests

https://codereview.chromium.org/646773005/diff/40001/mojo/public/cpp/bindings...
File mojo/public/cpp/bindings/tests/map_unittest.cc (right):

https://codereview.chromium.org/646773005/diff/40001/mojo/public/cpp/bindings...
mojo/public/cpp/bindings/tests/map_unittest.cc:56:
map[kStringIntData[i].string_data] = kStringIntData[i].int_data;
Two things:
- maybe test map[foo] when used strictly as an rvalue (right now, for all I know
map[foo] always clears the value at foo; possibly, you could test this by using
map[...] in the EXPECT_EQ below, though it's not entirely satisfactory)
- test with move-only values

[Elliot Glaysher, 2014-10-16 00:14:50]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-10-16 00:15:40]

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

[commit-bot: I haz the power, 2014-10-16 03:16:31]

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

[commit-bot: I haz the power, 2014-10-16 03:16:32]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Elliot Glaysher, 2014-10-16 17:46:19]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:47:49]

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

[commit-bot: I haz the power, 2014-10-16 18:45:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-16 18:46:12]

Patchset 4 (id:??) landed as
https://crrev.com/00a8f30e140e0e88a23fb1f14bfa74680cb4bd54
Cr-Commit-Position: refs/heads/master@{#299942}