Switch on use_new_wrapper_types mode for content/common.

content/browser/leveldb_wrapper_impl.cc

Index: content/browser/leveldb_wrapper_impl.cc
diff --git a/content/browser/leveldb_wrapper_impl.cc b/content/browser/leveldb_wrapper_impl.cc
index 07d9fc33a7111662d5eea35534a50d26d24a0b10..ce04a26f2121a6f1cf7ee3808a0acdcac4f97233 100644
--- a/content/browser/leveldb_wrapper_impl.cc
+++ b/content/browser/leveldb_wrapper_impl.cc
@@ -40,7 +40,7 @@ size_t LevelDBWrapperImpl::CommitBatch::GetDataSize() const {
 
   size_t count = 0;
   for (const auto& pair : changed_values)
-    count += (pair.first.size(), pair.second.size());
+    count += (pair.first.size(), pair.second.value().size());

[yzshen1, 2016/08/02 16:20:09]

pair.second may be null, right? (e.g., see line 161) It seems you cannot
de-reference it directly in that case.

[michaeln, 2016/08/02 20:31:54]

also, this line looks weird, can you fix this to clearly add both size values

 count += first.size;
 if (second)
   count += second.value().size;

[leonhsl(Using Gerrit), 2016/08/03 03:38:48]

Done. And also confirmed there has no similar problem at other places about
possible null value in changed_values.

   return count;
 }
 
@@ -83,28 +83,25 @@ void LevelDBWrapperImpl::EnableAggressiveCommitDelay() {
   s_aggressive_flushing_enabled_ = true;
 }
 
-void LevelDBWrapperImpl::Put(mojo::Array<uint8_t> key,
-                             mojo::Array<uint8_t> value,
-                             const mojo::String& source,
+void LevelDBWrapperImpl::Put(const std::vector<uint8_t>& key,
+                             const std::vector<uint8_t>& value,
+                             const std::string& source,
                              const PutCallback& callback) {
   if (!map_) {
-    LoadMap(
-        base::Bind(&LevelDBWrapperImpl::Put, base::Unretained(this),
-                   base::Passed(&key), base::Passed(&value), source, callback));
+    LoadMap(base::Bind(&LevelDBWrapperImpl::Put, base::Unretained(this), key,
+                       value, source, callback));
     return;
   }
 
   bool has_old_item = false;
-  mojo::Array<uint8_t> old_value;
   size_t old_item_size = 0;
   auto found = map_->find(key);
   if (found != map_->end()) {
-    if (found->second.Equals(value)) {
+    if (*(found->second) == value) {

[yzshen1, 2016/08/02 16:20:09]

It seems here we also assume that found->second is not null. Is that intended?

(And also line 164, 210, 226)

[michaeln, 2016/08/02 20:31:54]

Values in this map_ may not be null, it might be good to DCHECK() that under
line 99.

Values in the CommitBatch's map may be null, thats how deletes are represented.

[leonhsl(Using Gerrit), 2016/08/03 03:38:48]

Yeah I think DCHECK() is proper here, as map_ is supposed always containing no
null value. Done and also handled other similar places. Thanks!

[yzshen1, 2016/08/03 17:23:05]

If map_ is not supposed to have null value, it would be better to not use
base::Optional<> for its value type. That will be more straightforward. WDYT?

[leonhsl(Using Gerrit), 2016/08/09 07:56:51]

Sorry for late response. Agree and addressed this at ps#4. Done.

       callback.Run(true);  // Key already has this value.
       return;
     }
-    old_value = std::move(found->second);
-    old_item_size = key.size() + old_value.size();
+    old_item_size = key.size() + found->second->size();

[yzshen1, 2016/08/02 16:20:09]

Previously the old value was removed from the map, but the new code doesn't. Is
that intended? The code may return on line 114. In that case the contents of
|map_| will be different before/after the change.

[michaeln, 2016/08/02 20:31:54]

yes, this fixes a bug

     has_old_item = true;
   }
   size_t new_item_size = key.size() + value.size();
@@ -119,35 +116,37 @@ void LevelDBWrapperImpl::Put(mojo::Array<uint8_t> key,
 
   if (database_) {
     CreateCommitBatchIfNeeded();
-    commit_batch_->changed_values[key.Clone()] = value.Clone();
+    commit_batch_->changed_values[key] = value;
   }
 
-  (*map_)[key.Clone()] = value.Clone();
+  std::vector<uint8_t> old_value;
+  if (has_old_item) {
+    old_value.swap(*((*map_)[key]));
+  }
+  (*map_)[key] = value;
   bytes_used_ = new_bytes_used;
   if (!has_old_item) {
     // We added a new key/value pair.
     observers_.ForAllPtrs(
         [&key, &value, &source](mojom::LevelDBObserver* observer) {
-          observer->KeyAdded(key.Clone(), value.Clone(), source);
+          observer->KeyAdded(key, value, source);
         });
   } else {
     // We changed the value for an existing key.
     observers_.ForAllPtrs(
         [&key, &value, &source, &old_value](mojom::LevelDBObserver* observer) {
-          observer->KeyChanged(
-              key.Clone(), value.Clone(), old_value.Clone(), source);
+          observer->KeyChanged(key, value, old_value, source);
         });
   }
   callback.Run(true);
 }
 
-void LevelDBWrapperImpl::Delete(mojo::Array<uint8_t> key,
-                                const mojo::String& source,
+void LevelDBWrapperImpl::Delete(const std::vector<uint8_t>& key,
+                                const std::string& source,
                                 const DeleteCallback& callback) {
   if (!map_) {
-    LoadMap(
-        base::Bind(&LevelDBWrapperImpl::Delete, base::Unretained(this),
-                   base::Passed(&key), source, callback));
+    LoadMap(base::Bind(&LevelDBWrapperImpl::Delete, base::Unretained(this), key,
+                       source, callback));
     return;
   }
 
@@ -159,21 +158,20 @@ void LevelDBWrapperImpl::Delete(mojo::Array<uint8_t> key,
 
   if (database_) {
     CreateCommitBatchIfNeeded();
-    commit_batch_->changed_values[key.Clone()] = mojo::Array<uint8_t>(nullptr);
+    commit_batch_->changed_values[key] = base::nullopt;
   }
 
-  mojo::Array<uint8_t> old_value = std::move(found->second);
+  std::vector<uint8_t> old_value(std::move(*(found->second)));
   map_->erase(found);
   bytes_used_ -= key.size() + old_value.size();
   observers_.ForAllPtrs(
       [&key, &source, &old_value](mojom::LevelDBObserver* observer) {
-        observer->KeyDeleted(
-            key.Clone(), old_value.Clone(), source);
+        observer->KeyDeleted(key, old_value, source);
       });
   callback.Run(true);
 }
 
-void LevelDBWrapperImpl::DeleteAll(const mojo::String& source,
+void LevelDBWrapperImpl::DeleteAll(const std::string& source,
                                    const DeleteAllCallback& callback) {
   if (!map_) {
     LoadMap(
@@ -196,24 +194,23 @@ void LevelDBWrapperImpl::DeleteAll(const mojo::String& source,
   callback.Run(true);
 }
 
-void LevelDBWrapperImpl::Get(mojo::Array<uint8_t> key,
+void LevelDBWrapperImpl::Get(const std::vector<uint8_t>& key,
                              const GetCallback& callback) {
   if (!map_) {
-    LoadMap(
-        base::Bind(&LevelDBWrapperImpl::Get, base::Unretained(this),
-                   base::Passed(&key), callback));
+    LoadMap(base::Bind(&LevelDBWrapperImpl::Get, base::Unretained(this), key,
+                       callback));
     return;
   }
 
   auto found = map_->find(key);
   if (found == map_->end()) {
-    callback.Run(false, mojo::Array<uint8_t>());
+    callback.Run(false, std::vector<uint8_t>());
     return;
   }
-  callback.Run(true, found->second.Clone());
+  callback.Run(true, *(found->second));
 }
 
-void LevelDBWrapperImpl::GetAll(const mojo::String& source,
+void LevelDBWrapperImpl::GetAll(const std::string& source,
                                 const GetAllCallback& callback) {
   if (!map_) {
     LoadMap(
@@ -222,11 +219,11 @@ void LevelDBWrapperImpl::GetAll(const mojo::String& source,
     return;
   }
 
-  mojo::Array<mojom::KeyValuePtr> all;
+  std::vector<mojom::KeyValuePtr> all;
   for (const auto& it : (*map_)) {
     mojom::KeyValuePtr kv = mojom::KeyValue::New();
-    kv->key = it.first.Clone();
-    kv->value = it.second.Clone();
+    kv->key = it.first;
+    kv->value = *(it.second);
     all.push_back(std::move(kv));
   }
   callback.Run(leveldb::mojom::DatabaseError::OK, std::move(all));
@@ -260,7 +257,7 @@ void LevelDBWrapperImpl::OnLoadComplete(
   DCHECK(!map_);
   map_.reset(new ValueMap);
   for (auto& it : data)
-    (*map_)[std::move(it->key)] = std::move(it->value);
+    (*map_)[it->key.PassStorage()] = it->value.PassStorage();
 
   // We proceed without using a backing store, nothing will be persisted but the
   // class is functional for the lifetime of the object.
@@ -334,12 +331,12 @@ void LevelDBWrapperImpl::CommitChanges() {
   for (auto& it : commit_batch_->changed_values) {
     leveldb::mojom::BatchedOperationPtr item =
         leveldb::mojom::BatchedOperation::New();
-    item->key = it.first.Clone();
-    if (it.second.is_null()) {
+    item->key = std::vector<uint8_t>(it.first);
+    if (!it.second) {
       item->type = leveldb::mojom::BatchOperationType::DELETE_KEY;
     } else {
       item->type = leveldb::mojom::BatchOperationType::PUT_KEY;
-      item->value = std::move(it.second);
+      item->value = std::move(*(it.second));
     }
     operations.push_back(std::move(item));
   }