Switch on use_new_wrapper_types mode for content/common.

content/browser/leveldb_wrapper_impl.cc

 // Copyright 2016 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 "content/browser/leveldb_wrapper_impl.h"
 
 #include "base/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/public/browser/browser_thread.h"
 #include "mojo/common/common_type_converters.h"
@@ skipping 22 matching lines @@
 
 LevelDBWrapperImpl::CommitBatch::CommitBatch() : clear_all_first(false) {}
 LevelDBWrapperImpl::CommitBatch::~CommitBatch() {}
 
 size_t LevelDBWrapperImpl::CommitBatch::GetDataSize() const {
   if (changed_values.empty())
     return 0;
 
   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;
 }
 
 LevelDBWrapperImpl::LevelDBWrapperImpl(
     leveldb::mojom::LevelDBDatabase* database,
     const std::string& prefix,
     size_t max_size,
     base::TimeDelta default_commit_delay,
     int max_bytes_per_hour,
     int max_commits_per_hour,
@@ skipping 22 matching lines @@
 }
 
 void LevelDBWrapperImpl::AddObserver(mojom::LevelDBObserverPtr observer) {
   observers_.AddPtr(std::move(observer));
 }
 
 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();
   size_t new_bytes_used = bytes_used_ - old_item_size + new_item_size;
 
   // Only check quota if the size is increasing, this allows
   // shrinking changes to pre-existing maps that are over budget.
   if (new_item_size > old_item_size && new_bytes_used > max_size_) {
     callback.Run(false);
     return;
   }
 
   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;
   }
 
   auto found = map_->find(key);
   if (found == map_->end()) {
     callback.Run(true);
     return;
   }
 
   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(
         base::Bind(&LevelDBWrapperImpl::DeleteAll, base::Unretained(this),
                     source, callback));
     return;
   }
 
   if (database_ && !map_->empty()) {
     CreateCommitBatchIfNeeded();
     commit_batch_->clear_all_first = true;
     commit_batch_->changed_values.clear();
   }
   map_->clear();
   bytes_used_ = 0;
   observers_.ForAllPtrs(
       [&source](mojom::LevelDBObserver* observer) {
         observer->AllDeleted(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(
         base::Bind(&LevelDBWrapperImpl::GetAll, base::Unretained(this),
                    source, callback));
     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));
   observers_.ForAllPtrs(
       [source](mojom::LevelDBObserver* observer) {
         observer->GetAllComplete(source);
       });
 }
 
 void LevelDBWrapperImpl::OnConnectionError() {
@@ skipping 13 matching lines @@
                          base::Bind(&LevelDBWrapperImpl::OnLoadComplete,
                                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void LevelDBWrapperImpl::OnLoadComplete(
     leveldb::mojom::DatabaseError status,
     mojo::Array<leveldb::mojom::KeyValuePtr> data) {
   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.
   // TODO(michaeln): Uma here or in the DB file?
   if (status != leveldb::mojom::DatabaseError::OK)
     database_ = nullptr;
 
   std::vector<base::Closure> tasks;
   on_load_complete_tasks_.swap(tasks);
   for (auto& task : tasks)
@@ skipping 53 matching lines @@
   if (commit_batch_->clear_all_first) {
     leveldb::mojom::BatchedOperationPtr item =
         leveldb::mojom::BatchedOperation::New();
     item->type = leveldb::mojom::BatchOperationType::DELETE_PREFIXED_KEY;
     item->key = mojo::Array<uint8_t>::From(std::string(prefix_));
     operations.push_back(std::move(item));
   }
   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));
   }
   commit_batch_.reset();
 
   ++commit_batches_in_flight_;
 
   // TODO(michaeln): Currently there is no guarantee LevelDBDatabaseImp::Write
   // will run during a clean shutdown. We need that to avoid dataloss.
   database_->Write(std::move(operations),
                    base::Bind(&LevelDBWrapperImpl::OnCommitComplete,
                               weak_ptr_factory_.GetWeakPtr()));
 }
 
 void LevelDBWrapperImpl::OnCommitComplete(leveldb::mojom::DatabaseError error) {
   // TODO(michaeln): What if it fails, uma here or in the DB class?
   --commit_batches_in_flight_;
   StartCommitTimer();
 }
 
 }  // namespace content