From 00a8f30e140e0e88a23fb1f14bfa74680cb4bd54 Mon Sep 17 00:00:00 2001 From: erg Date: Thu, 16 Oct 2014 11:45:11 -0700 Subject: mojo: Switch the clipboard interface over to using map<>. BUG=413863 Review URL: https://codereview.chromium.org/646773005 Cr-Commit-Position: refs/heads/master@{#299942} --- mojo/public/cpp/bindings/lib/map_internal.h | 26 ++++++-- mojo/public/cpp/bindings/map.h | 5 ++ mojo/public/cpp/bindings/tests/map_unittest.cc | 71 +++++++++++++++++++++- .../clipboard/clipboard_standalone_impl.cc | 63 +++++++------------ .../services/clipboard/clipboard_standalone_impl.h | 7 ++- .../clipboard/clipboard_standalone_unittest.cc | 42 +++---------- mojo/services/html_viewer/webclipboard_impl.cc | 36 +++-------- .../public/interfaces/clipboard/clipboard.mojom | 11 +--- 8 files changed, 142 insertions(+), 119 deletions(-) (limited to 'mojo') diff --git a/mojo/public/cpp/bindings/lib/map_internal.h b/mojo/public/cpp/bindings/lib/map_internal.h index aa0fe7e..f006cd0 100644 --- a/mojo/public/cpp/bindings/lib/map_internal.h +++ b/mojo/public/cpp/bindings/lib/map_internal.h @@ -57,7 +57,7 @@ struct MapTraits { static inline ValueRefType at(std::map* m, KeyForwardType key) { // We don't have C++11 library support yet, so we have to emulate the crash - // on a non-existant key. + // on a non-existent key. auto it = m->find(key); MOJO_CHECK(it != m->end()); return it->second; @@ -66,11 +66,17 @@ struct MapTraits { const std::map* m, KeyForwardType key) { // We don't have C++11 library support yet, so we have to emulate the crash - // on a non-existant key. + // on a non-existent key. auto it = m->find(key); MOJO_CHECK(it != m->end()); return it->second; } + static inline ValueRefType GetOrInsert( + std::map* m, + KeyForwardType key) { + // This is the backing for the index operator (operator[]). + return (*m)[key]; + } static inline void Insert(std::map* m, KeyForwardType key, ValueForwardType value) { @@ -146,7 +152,7 @@ struct MapTraits { static inline ValueRefType at(std::map* m, KeyForwardType key) { // We don't have C++11 library support yet, so we have to emulate the crash - // on a non-existant key. + // on a non-existent key. auto it = m->find(key); MOJO_CHECK(it != m->end()); return GetValue(it); @@ -155,11 +161,23 @@ struct MapTraits { const std::map* m, KeyForwardType key) { // We don't have C++11 library support yet, so we have to emulate the crash - // on a non-existant key. + // on a non-existent key. auto it = m->find(key); MOJO_CHECK(it != m->end()); return GetValue(it); } + static inline ValueRefType GetOrInsert( + std::map* m, + KeyForwardType key) { + // This is the backing for the index operator (operator[]). + auto it = m->find(key); + if (it == m->end()) { + it = m->insert(std::make_pair(key, ValueStorageType())).first; + new (it->second.buf) Value(); + } + + return GetValue(it); + } static inline void Insert(std::map* m, KeyForwardType key, ValueRefType value) { diff --git a/mojo/public/cpp/bindings/map.h b/mojo/public/cpp/bindings/map.h index f09937a..7aa7b6f 100644 --- a/mojo/public/cpp/bindings/map.h +++ b/mojo/public/cpp/bindings/map.h @@ -88,6 +88,11 @@ class Map { return Traits::at(&map_, key); } + ValueRefType operator[](KeyForwardType key) { + is_null_ = false; + return Traits::GetOrInsert(&map_, key); + } + void Swap(Map* other) { std::swap(is_null_, other->is_null_); map_.swap(other->map_); diff --git a/mojo/public/cpp/bindings/tests/map_unittest.cc b/mojo/public/cpp/bindings/tests/map_unittest.cc index 159afb9..6f7db16 100644 --- a/mojo/public/cpp/bindings/tests/map_unittest.cc +++ b/mojo/public/cpp/bindings/tests/map_unittest.cc @@ -50,6 +50,49 @@ TEST_F(MapTest, InsertWorks) { } } +TEST_F(MapTest, TestIndexOperator) { + Map map; + for (size_t i = 0; i < kStringIntDataSize; ++i) + map[kStringIntData[i].string_data] = kStringIntData[i].int_data; + + for (size_t i = 0; i < kStringIntDataSize; ++i) { + EXPECT_EQ(kStringIntData[i].int_data, + map.at(kStringIntData[i].string_data)); + } +} + +TEST_F(MapTest, TestIndexOperatorAsRValue) { + Map map; + for (size_t i = 0; i < kStringIntDataSize; ++i) + map.insert(kStringIntData[i].string_data, kStringIntData[i].int_data); + + for (size_t i = 0; i < kStringIntDataSize; ++i) { + EXPECT_EQ(kStringIntData[i].int_data, map[kStringIntData[i].string_data]); + } +} + +TEST_F(MapTest, TestIndexOperatorMoveOnly) { + ASSERT_EQ(0u, MoveOnlyType::num_instances()); + mojo::Map> map; + std::vector value_ptrs; + + for (size_t i = 0; i < kStringIntDataSize; ++i) { + const char* key = kStringIntData[i].string_data; + Array array(1); + array[0] = kStringIntData[i].int_data; + map[key] = array.Pass(); + EXPECT_TRUE(map); + } + + // We now read back that data, to test the behavior of operator[]. + for (size_t i = 0; i < kStringIntDataSize; ++i) { + auto it = map.find(kStringIntData[i].string_data); + ASSERT_TRUE(it != map.end()); + ASSERT_EQ(1u, it.GetValue().size()); + EXPECT_EQ(kStringIntData[i].int_data, it.GetValue()[0]); + } +} + TEST_F(MapTest, ConstructedFromArray) { Array keys(kStringIntDataSize); Array values(kStringIntDataSize); @@ -153,7 +196,33 @@ TEST_F(MapTest, Insert_MoveOnly) { // a lot more boring. map.reset(); - EXPECT_EQ(0u, CopyableType::num_instances()); + EXPECT_EQ(0u, MoveOnlyType::num_instances()); +} + +TEST_F(MapTest, IndexOperator_MoveOnly) { + ASSERT_EQ(0u, MoveOnlyType::num_instances()); + mojo::Map map; + std::vector value_ptrs; + + for (size_t i = 0; i < kStringIntDataSize; ++i) { + const char* key = kStringIntData[i].string_data; + MoveOnlyType value; + value_ptrs.push_back(value.ptr()); + map[key] = value.Pass(); + ASSERT_EQ(i + 1, map.size()); + ASSERT_EQ(i + 1, value_ptrs.size()); + EXPECT_EQ(map.size() + 1, MoveOnlyType::num_instances()); + EXPECT_TRUE(map.at(key).moved()); + EXPECT_EQ(value_ptrs[i], map.at(key).ptr()); + map.at(key).ResetMoved(); + EXPECT_TRUE(map); + } + + // std::map doesn't have a capacity() method like std::vector so this test is + // a lot more boring. + + map.reset(); + EXPECT_EQ(0u, MoveOnlyType::num_instances()); } TEST_F(MapTest, STLToMojo) { diff --git a/mojo/services/clipboard/clipboard_standalone_impl.cc b/mojo/services/clipboard/clipboard_standalone_impl.cc index e1c7e2c..404eded 100644 --- a/mojo/services/clipboard/clipboard_standalone_impl.cc +++ b/mojo/services/clipboard/clipboard_standalone_impl.cc @@ -1,12 +1,12 @@ -// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Copyright 2014 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include "mojo/services/clipboard/clipboard_standalone_impl.h" -namespace mojo { +#include -typedef std::vector ByteVector; +namespace mojo { // ClipboardData contains data copied to the Clipboard for a variety of formats. // It mostly just provides APIs to cleanly access and manipulate this data. @@ -15,35 +15,25 @@ class ClipboardStandaloneImpl::ClipboardData { ClipboardData() {} ~ClipboardData() {} - std::vector GetMimeTypes() const { - std::vector types; - for (std::map::const_iterator it = - data_types_.begin(); - it != data_types_.end(); - ++it) { - types.push_back(it->first); - } - - return types; - } + Array GetMimeTypes() const { + Array types(data_types_.size()); + int i = 0; + for (auto it = data_types_.begin(); it != data_types_.end(); ++it, ++i) + types[i] = it.GetKey(); - void SetData(std::map* data) { - std::swap(data_types_, *data); + return types.Pass(); } - bool GetData(const std::string& mime_type, ByteVector* data) const { - std::map::const_iterator it = - data_types_.find(mime_type); - if (it != data_types_.end()) { - *data = it->second; - return true; - } + void SetData(Map> data) { data_types_ = data.Pass(); } - return false; + void GetData(const String& mime_type, mojo::Array* data) const { + auto it = data_types_.find(mime_type); + if (it != data_types_.end()) + *data = it.GetValue().Clone(); } private: - std::map data_types_; + Map> data_types_; DISALLOW_COPY_AND_ASSIGN(ClipboardData); }; @@ -67,34 +57,23 @@ void ClipboardStandaloneImpl::GetSequenceNumber( void ClipboardStandaloneImpl::GetAvailableMimeTypes( Clipboard::Type clipboard_type, const mojo::Callback)>& callback) { - mojo::Array types = mojo::Array::From( - clipboard_state_[clipboard_type]->GetMimeTypes()); - callback.Run(types.Pass()); + callback.Run(clipboard_state_[clipboard_type]->GetMimeTypes().Pass()); } void ClipboardStandaloneImpl::ReadMimeType( Clipboard::Type clipboard_type, const mojo::String& mime_type, const mojo::Callback)>& callback) { - ByteVector mime_data; - if (clipboard_state_[clipboard_type]->GetData( - mime_type.To(), &mime_data)) { - callback.Run(mojo::Array::From(mime_data).Pass()); - return; - } - - callback.Run(mojo::Array().Pass()); + mojo::Array mime_data; + clipboard_state_[clipboard_type]->GetData(mime_type, &mime_data); + callback.Run(mime_data.Pass()); } void ClipboardStandaloneImpl::WriteClipboardData( Clipboard::Type clipboard_type, - mojo::Array data) { - std::map mime_data; - for (size_t i = 0; i < data.size(); ++i) - mime_data[data[i]->mime_type] = data[i]->data; - + mojo::Map> data) { sequence_number_[clipboard_type]++; - clipboard_state_[clipboard_type]->SetData(&mime_data); + clipboard_state_[clipboard_type]->SetData(data.Pass()); } } // namespace mojo diff --git a/mojo/services/clipboard/clipboard_standalone_impl.h b/mojo/services/clipboard/clipboard_standalone_impl.h index 09d7424..ccb8c54 100644 --- a/mojo/services/clipboard/clipboard_standalone_impl.h +++ b/mojo/services/clipboard/clipboard_standalone_impl.h @@ -1,4 +1,4 @@ -// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Copyright 2014 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -38,8 +38,9 @@ class ClipboardStandaloneImpl : public InterfaceImpl { Clipboard::Type clipboard_type, const mojo::String& mime_type, const mojo::Callback)>& callback) override; - virtual void WriteClipboardData(Clipboard::Type clipboard_type, - mojo::Array data) override; + virtual void WriteClipboardData( + Clipboard::Type clipboard_type, + mojo::Map> data) override; private: uint64_t sequence_number_[kNumClipboards]; diff --git a/mojo/services/clipboard/clipboard_standalone_unittest.cc b/mojo/services/clipboard/clipboard_standalone_unittest.cc index c297b94..cdf0087 100644 --- a/mojo/services/clipboard/clipboard_standalone_unittest.cc +++ b/mojo/services/clipboard/clipboard_standalone_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Copyright 2014 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -89,11 +89,8 @@ class ClipboardStandaloneTest : public testing::Test { } void SetStringText(const std::string& data) { - Array mime_data; - MimeTypePairPtr text_data(MimeTypePair::New()); - text_data->mime_type = mojo::Clipboard::MIME_TYPE_TEXT; - text_data->data = Array::From(data).Pass(); - mime_data.push_back(text_data.Pass()); + Map> mime_data; + mime_data[mojo::Clipboard::MIME_TYPE_TEXT] = Array::From(data); clipboard_->WriteClipboardData(mojo::Clipboard::TYPE_COPY_PASTE, mime_data.Pass()); } @@ -127,16 +124,11 @@ TEST_F(ClipboardStandaloneTest, CanReadBackText) { } TEST_F(ClipboardStandaloneTest, CanSetMultipleDataTypesAtOnce) { - Array mime_data; - MimeTypePairPtr text_data(MimeTypePair::New()); - text_data->mime_type = mojo::Clipboard::MIME_TYPE_TEXT; - text_data->data = Array::From(std::string(kPlainTextData)).Pass(); - mime_data.push_back(text_data.Pass()); - - MimeTypePairPtr html_data(MimeTypePair::New()); - html_data->mime_type = mojo::Clipboard::MIME_TYPE_HTML; - html_data->data = Array::From(std::string(kHtmlData)).Pass(); - mime_data.push_back(html_data.Pass()); + Map> mime_data; + mime_data[mojo::Clipboard::MIME_TYPE_TEXT] = + Array::From(std::string(kPlainTextData)); + mime_data[mojo::Clipboard::MIME_TYPE_HTML] = + Array::From(std::string(kHtmlData)); clipboard_->WriteClipboardData(mojo::Clipboard::TYPE_COPY_PASTE, mime_data.Pass()); @@ -150,22 +142,6 @@ TEST_F(ClipboardStandaloneTest, CanSetMultipleDataTypesAtOnce) { EXPECT_EQ(kHtmlData, data); } -TEST_F(ClipboardStandaloneTest, CanClearClipboardWithNull) { - std::string data; - SetStringText(kPlainTextData); - EXPECT_EQ(1ul, GetSequenceNumber()); - - EXPECT_TRUE(GetDataOfType(mojo::Clipboard::MIME_TYPE_TEXT, &data)); - EXPECT_EQ(kPlainTextData, data); - - Array mime_data; - clipboard_->WriteClipboardData(mojo::Clipboard::TYPE_COPY_PASTE, - mime_data.Pass()); - - EXPECT_EQ(2ul, GetSequenceNumber()); - EXPECT_FALSE(GetDataOfType(mojo::Clipboard::MIME_TYPE_TEXT, &data)); -} - TEST_F(ClipboardStandaloneTest, CanClearClipboardWithZeroArray) { std::string data; SetStringText(kPlainTextData); @@ -174,7 +150,7 @@ TEST_F(ClipboardStandaloneTest, CanClearClipboardWithZeroArray) { EXPECT_TRUE(GetDataOfType(mojo::Clipboard::MIME_TYPE_TEXT, &data)); EXPECT_EQ(kPlainTextData, data); - Array mime_data(0); + Map> mime_data; clipboard_->WriteClipboardData(mojo::Clipboard::TYPE_COPY_PASTE, mime_data.Pass()); diff --git a/mojo/services/html_viewer/webclipboard_impl.cc b/mojo/services/html_viewer/webclipboard_impl.cc index 94b16ca..9b7ebff 100644 --- a/mojo/services/html_viewer/webclipboard_impl.cc +++ b/mojo/services/html_viewer/webclipboard_impl.cc @@ -168,11 +168,8 @@ blink::WebString WebClipboardImpl::readCustomData( } void WebClipboardImpl::writePlainText(const blink::WebString& text) { - Array data; - MimeTypePairPtr text_data(MimeTypePair::New()); - text_data->mime_type = mojo::Clipboard::MIME_TYPE_TEXT; - text_data->data = Array::From(text).Pass(); - data.push_back(text_data.Pass()); + Map> data; + data[mojo::Clipboard::MIME_TYPE_TEXT] = Array::From(text); clipboard_->WriteClipboardData(mojo::Clipboard::TYPE_COPY_PASTE, data.Pass()); } @@ -181,28 +178,13 @@ void WebClipboardImpl::writeHTML(const blink::WebString& htmlText, const blink::WebURL& url, const blink::WebString& plainText, bool writeSmartPaste) { - Array data; - MimeTypePairPtr text_data(MimeTypePair::New()); - text_data->mime_type = mojo::Clipboard::MIME_TYPE_TEXT; - text_data->data = Array::From(plainText).Pass(); - data.push_back(text_data.Pass()); - - MimeTypePairPtr html_data(MimeTypePair::New()); - text_data->mime_type = mojo::Clipboard::MIME_TYPE_HTML; - text_data->data = Array::From(htmlText).Pass(); - data.push_back(html_data.Pass()); - - MimeTypePairPtr url_data(MimeTypePair::New()); - url_data->mime_type = mojo::Clipboard::MIME_TYPE_URL; - url_data->data = Array::From(url.string()).Pass(); - data.push_back(url_data.Pass()); - - if (writeSmartPaste) { - MimeTypePairPtr smart_paste(MimeTypePair::New()); - url_data->mime_type = kMimeTypeWebkitSmartPaste; - url_data->data = Array::From(blink::WebString()).Pass(); - data.push_back(smart_paste.Pass()); - } + Map> data; + data[mojo::Clipboard::MIME_TYPE_TEXT] = Array::From(plainText); + data[mojo::Clipboard::MIME_TYPE_HTML] = Array::From(htmlText); + data[mojo::Clipboard::MIME_TYPE_URL] = Array::From(url.string()); + + if (writeSmartPaste) + data[kMimeTypeWebkitSmartPaste] = Array::From(blink::WebString()); clipboard_->WriteClipboardData(mojo::Clipboard::TYPE_COPY_PASTE, data.Pass()); } diff --git a/mojo/services/public/interfaces/clipboard/clipboard.mojom b/mojo/services/public/interfaces/clipboard/clipboard.mojom index eca47b9..92ef822 100644 --- a/mojo/services/public/interfaces/clipboard/clipboard.mojom +++ b/mojo/services/public/interfaces/clipboard/clipboard.mojom @@ -4,13 +4,6 @@ module mojo { -// A wrapper type which is just a Key/Value pair. Workaround until we get -// proper maps in mojom. -struct MimeTypePair { - string mime_type; - array data; -}; - interface Clipboard { enum Type { COPY_PASTE = 0, @@ -44,9 +37,9 @@ interface Clipboard { ReadMimeType(Type clipboard_type, string mime_type) => (array? data); // Writes a set of mime types to the clipboard. This will increment the - // sequence number. In the case of an empty or NULL list, this will just + // sequence number. In the case of an empty or null map, this will just // clear the clipboard. - WriteClipboardData(Type clipboard_type, array? data); + WriteClipboardData(Type clipboard_type, map>? data); }; } // module mojo -- cgit v1.1