Mojo C++ bindings: make Array/Map/String non-null by default.

Array<X> a; // Default construct an empty array. Array<X> b(nullptr); // Construct a null array. a.clear(); // Set to an empty array. a = nullptr; // Set to a null array. BUG=579634 TEST=None Review URL: https://codereview.chromium.org/1693943002 Cr-Commit-Position: refs/heads/master@{#375304}
marcosholgado · Feb 13, 2016 · 48f0c96 · 48f0c96
1 parent 07eb2f0
commit 48f0c96
Show file tree

Hide file tree

Showing 34 changed files with 99 additions and 79 deletions.
diff --git a/chrome/browser/media/router/media_router_mojo_impl_unittest.cc b/chrome/browser/media/router/media_router_mojo_impl_unittest.cc
@@ -923,7 +923,7 @@ TEST_F(MediaRouterMojoImplTest, PresentationSessionMessagesError) {
           expected_route_id, router()));
   run_loop.Run();
 
-  mojo_callback.Run(mojo::Array<interfaces::RouteMessagePtr>(0), true);
+  mojo_callback.Run(mojo::Array<interfaces::RouteMessagePtr>(), true);
   ProcessEventLoop();
 }
 

diff --git a/chrome/browser/ui/webui/engagement/site_engagement_ui.cc b/chrome/browser/ui/webui/engagement/site_engagement_ui.cc
@@ -36,7 +36,7 @@ class SiteEngagementUIHandlerImpl : public SiteEngagementUIHandler {
   // SiteEngagementUIHandler overrides:
   void GetSiteEngagementInfo(
       const GetSiteEngagementInfoCallback& callback) override {
-    mojo::Array<SiteEngagementInfoPtr> engagement_info(0);
+    mojo::Array<SiteEngagementInfoPtr> engagement_info;
 
     SiteEngagementService* service = SiteEngagementService::Get(profile_);
 

diff --git a/chrome/browser/ui/webui/plugins/plugins_handler.cc b/chrome/browser/ui/webui/plugins/plugins_handler.cc
@@ -90,7 +90,7 @@ base::string16 GetPluginDescription(const WebPluginInfo& plugin) {
 }
 
 mojo::Array<MimeTypePtr> GeneratePluginMimeTypes(const WebPluginInfo& plugin) {
-  mojo::Array<MimeTypePtr> mime_types(0u);
+  mojo::Array<MimeTypePtr> mime_types;
   for (const auto& plugin_mime_type : plugin.mime_types) {
     MimeTypePtr mime_type(MimeType::New());
     mime_type->description = mojo::String::From(plugin_mime_type.description);
@@ -227,7 +227,7 @@ mojo::Array<PluginDataPtr> PluginsHandler::GeneratePluginsData(
     groups[plugin->identifier()].push_back(&plugins[i]);
   }
 
-  mojo::Array<PluginDataPtr> plugins_data(0u);
+  mojo::Array<PluginDataPtr> plugins_data;
 
   for (PluginGroups::const_iterator it = groups.begin(); it != groups.end();
        ++it) {
@@ -242,7 +242,7 @@ mojo::Array<PluginDataPtr> PluginsHandler::GeneratePluginsData(
     const WebPluginInfo* active_plugin = nullptr;
     bool group_enabled = false;
 
-    mojo::Array<PluginFilePtr> plugin_files(0u);
+    mojo::Array<PluginFilePtr> plugin_files;
     for (const auto& group_plugin : group_plugins) {
       bool plugin_enabled = plugin_prefs->IsPluginEnabled(*group_plugin);
 

diff --git a/chrome/browser/usb/web_usb_permission_provider.cc b/chrome/browser/usb/web_usb_permission_provider.cc
@@ -84,7 +84,7 @@ void WebUSBPermissionProvider::HasDevicePermission(
   UsbChooserContext* chooser_context =
       UsbChooserContextFactory::GetForProfile(profile);
 
-  mojo::Array<mojo::String> allowed_guids(0);
+  mojo::Array<mojo::String> allowed_guids;
   for (size_t i = 0; i < requested_devices.size(); ++i) {
     const device::usb::DeviceInfoPtr& device = requested_devices[i];
     if (FindOriginInDescriptorSet(device->webusb_allowed_origins.get(),

diff --git a/components/filesystem/directory_impl.cc b/components/filesystem/directory_impl.cc
@@ -30,7 +30,7 @@ DirectoryImpl::~DirectoryImpl() {
 }
 
 void DirectoryImpl::Read(const ReadCallback& callback) {
-  mojo::Array<DirectoryEntryPtr> entries(0);
+  mojo::Array<DirectoryEntryPtr> entries;
   base::FileEnumerator directory_enumerator(
       directory_path_, false,
       base::FileEnumerator::DIRECTORIES | base::FileEnumerator::FILES);
@@ -221,18 +221,18 @@ void DirectoryImpl::ReadEntireFile(const mojo::String& raw_path,
   base::FilePath path;
   FileError error = ValidatePath(raw_path, directory_path_, &path);
   if (error != FileError::OK) {
-    callback.Run(error, mojo::Array<uint8_t>(0));
+    callback.Run(error, mojo::Array<uint8_t>());
     return;
   }
 
   if (base::DirectoryExists(path)) {
-    callback.Run(FileError::NOT_A_FILE, mojo::Array<uint8_t>(0));
+    callback.Run(FileError::NOT_A_FILE, mojo::Array<uint8_t>());
     return;
   }
 
   base::File base_file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
   if (!base_file.IsValid()) {
-    callback.Run(GetError(base_file), mojo::Array<uint8_t>(0));
+    callback.Run(GetError(base_file), mojo::Array<uint8_t>());
     return;
   }
 

diff --git a/components/mus/public/cpp/lib/window.cc b/components/mus/public/cpp/lib/window.cc
@@ -495,7 +495,7 @@ void Window::SetSharedPropertyInternal(const std::string& name,
     return;
 
   if (connection_) {
-    mojo::Array<uint8_t> transport_value;
+    mojo::Array<uint8_t> transport_value(nullptr);
     if (value) {
       transport_value.resize(value->size());
       if (value->size())

diff --git a/components/mus/public/cpp/lib/window_tree_client_impl.cc b/components/mus/public/cpp/lib/window_tree_client_impl.cc
@@ -299,7 +299,7 @@ void WindowTreeClientImpl::SetProperty(Window* window,
                                        mojo::Array<uint8_t> data) {
   DCHECK(tree_);
 
-  mojo::Array<uint8_t> old_value;
+  mojo::Array<uint8_t> old_value(nullptr);
   if (window->HasSharedProperty(name))
     old_value = mojo::Array<uint8_t>::From(window->properties_[name]);
 
@@ -435,8 +435,6 @@ Window* WindowTreeClientImpl::NewWindowImpl(
   if (properties) {
     transport_properties =
         mojo::Map<mojo::String, mojo::Array<uint8_t>>::From(*properties);
-  } else {
-    transport_properties.mark_non_null();
   }
   if (type == NewWindowType::CHILD) {
     tree_->NewWindow(change_id, window->id(), std::move(transport_properties));

diff --git a/components/mus/public/cpp/tests/window_tree_client_impl_unittest.cc b/components/mus/public/cpp/tests/window_tree_client_impl_unittest.cc
@@ -62,7 +62,7 @@ class WindowTreeClientImplPrivate {
     root_data->parent_id = 0;
     root_data->window_id = 1;
     root_data->bounds = mojo::Rect::From(gfx::Rect());
-    root_data->properties.mark_non_null();
+    root_data->properties.SetToEmpty();
     root_data->visible = true;
     root_data->drawn = true;
     root_data->viewport_metrics = mojom::ViewportMetrics::New();

diff --git a/components/mus/ws/window_tree_apptest.cc b/components/mus/ws/window_tree_apptest.cc
@@ -234,7 +234,7 @@ class TestWindowTreeClientImpl : public mojom::WindowTreeClient,
   bool SetWindowProperty(Id window_id,
                          const std::string& name,
                          const std::vector<uint8_t>* data) {
-    Array<uint8_t> mojo_data;
+    Array<uint8_t> mojo_data(nullptr);
     if (data)
       mojo_data = Array<uint8_t>::From(*data);
     const uint32_t change_id = GetAndAdvanceChangeId();

diff --git a/components/mus/ws/window_tree_impl.cc b/components/mus/ws/window_tree_impl.cc
@@ -397,7 +397,7 @@ void WindowTreeImpl::ProcessWindowPropertyChanged(
   if (!IsWindowKnown(window, &client_window_id))
     return;
 
-  Array<uint8_t> data;
+  Array<uint8_t> data(nullptr);
   if (new_data)
     data = Array<uint8_t>::From(*new_data);
 

diff --git a/components/mus/ws/window_tree_unittest.cc b/components/mus/ws/window_tree_unittest.cc
@@ -849,7 +849,6 @@ TEST_F(WindowTreeTest, NewTopLevelWindow) {
 
   // Create a new top level window.
   mojo::Map<mojo::String, mojo::Array<uint8_t>> properties;
-  properties.mark_non_null();
   const uint32_t initial_change_id = 17;
   // Explicitly use an id that does not contain the connection id.
   const ClientWindowId embed_window_id2_in_child(45 << 16 | 27);

diff --git a/content/browser/background_sync/background_sync_service_impl.cc b/content/browser/background_sync/background_sync_service_impl.cc
@@ -213,7 +213,7 @@ void BackgroundSyncServiceImpl::OnGetRegistrationsResult(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(result_registrations);
 
-  mojo::Array<content::SyncRegistrationPtr> mojo_registrations(0);
+  mojo::Array<content::SyncRegistrationPtr> mojo_registrations;
   for (BackgroundSyncRegistrationHandle* registration : *result_registrations) {
     active_handles_.AddWithID(registration, registration->handle_id());
     mojo_registrations.push_back(ToMojoRegistration(*registration));

diff --git a/content/browser/vr/vr_device_manager.cc b/content/browser/vr/vr_device_manager.cc
@@ -86,7 +86,7 @@ mojo::Array<VRDeviceInfoPtr> VRDeviceManager::GetVRDevices() {
   for (const auto& provider : providers_)
     provider->GetDevices(&devices);
 
-  mojo::Array<VRDeviceInfoPtr> out_devices(0);
+  mojo::Array<VRDeviceInfoPtr> out_devices;
   for (const auto& device : devices) {
     if (device->id() == VR_DEVICE_LAST_ID)
       continue;

diff --git a/device/serial/serial_device_enumerator_linux.cc b/device/serial/serial_device_enumerator_linux.cc
@@ -36,7 +36,7 @@ SerialDeviceEnumeratorLinux::SerialDeviceEnumeratorLinux() {
 SerialDeviceEnumeratorLinux::~SerialDeviceEnumeratorLinux() {}
 
 mojo::Array<serial::DeviceInfoPtr> SerialDeviceEnumeratorLinux::GetDevices() {
-  mojo::Array<serial::DeviceInfoPtr> devices(0);
+  mojo::Array<serial::DeviceInfoPtr> devices;
   ScopedUdevEnumeratePtr enumerate(udev_enumerate_new(udev_.get()));
   if (!enumerate) {
     LOG(ERROR) << "Serial device enumeration failed.";

diff --git a/device/serial/serial_device_enumerator_mac.cc b/device/serial/serial_device_enumerator_mac.cc
@@ -101,7 +101,7 @@ int Clamp(int value, int min, int max) {
 // enumerating serial devices (IOKit).  This new method gives more information
 // about the devices than the old method.
 mojo::Array<serial::DeviceInfoPtr> GetDevicesNew() {
-  mojo::Array<serial::DeviceInfoPtr> devices(0);
+  mojo::Array<serial::DeviceInfoPtr> devices;
 
   // Make a service query to find all serial devices.
   CFMutableDictionaryRef matchingDict =
@@ -180,7 +180,7 @@ mojo::Array<serial::DeviceInfoPtr> GetDevicesOld() {
   valid_patterns.insert("/dev/tty.*");
   valid_patterns.insert("/dev/cu.*");
 
-  mojo::Array<serial::DeviceInfoPtr> devices(0);
+  mojo::Array<serial::DeviceInfoPtr> devices;
   base::FileEnumerator enumerator(kDevRoot, false, kFilesAndSymLinks);
   do {
     const base::FilePath next_device_path(enumerator.Next());

diff --git a/device/serial/serial_device_enumerator_win.cc b/device/serial/serial_device_enumerator_win.cc
@@ -87,7 +87,7 @@ int Clamp(int value, int min, int max) {
 // enumerating serial devices (SetupDi).  This new method gives more information
 // about the devices than the old method.
 mojo::Array<serial::DeviceInfoPtr> GetDevicesNew() {
-  mojo::Array<serial::DeviceInfoPtr> devices(0);
+  mojo::Array<serial::DeviceInfoPtr> devices;
 
   // Make a device interface query to find all serial devices.
   HDEVINFO dev_info =
@@ -141,7 +141,7 @@ mojo::Array<serial::DeviceInfoPtr> GetDevicesNew() {
 mojo::Array<serial::DeviceInfoPtr> GetDevicesOld() {
   base::win::RegistryValueIterator iter_key(
       HKEY_LOCAL_MACHINE, L"HARDWARE\\DEVICEMAP\\SERIALCOMM\\");
-  mojo::Array<serial::DeviceInfoPtr> devices(0);
+  mojo::Array<serial::DeviceInfoPtr> devices;
   for (; iter_key.Valid(); ++iter_key) {
     serial::DeviceInfoPtr info(serial::DeviceInfo::New());
     info->path = base::UTF16ToASCII(iter_key.Value());

diff --git a/device/usb/mojo/device_manager_impl.cc b/device/usb/mojo/device_manager_impl.cc
@@ -135,7 +135,7 @@ void DeviceManagerImpl::OnGetDevices(EnumerationOptionsPtr options,
     filters = options->filters.To<std::vector<UsbDeviceFilter>>();
 
   std::map<std::string, scoped_refptr<UsbDevice>> device_map;
-  mojo::Array<DeviceInfoPtr> requested_devices(0);
+  mojo::Array<DeviceInfoPtr> requested_devices;
   for (const auto& device : devices) {
     if (filters.empty() || UsbDeviceFilter::MatchesAny(device, filters)) {
       device_map[device->guid()] = device;

diff --git a/extensions/browser/mojo/stash_backend.cc b/extensions/browser/mojo/stash_backend.cc
@@ -54,7 +54,7 @@ void StashServiceImpl::AddToStash(
 void StashServiceImpl::RetrieveStash(
     const mojo::Callback<void(mojo::Array<StashedObjectPtr>)>& callback) {
   if (!backend_) {
-    callback.Run(mojo::Array<StashedObjectPtr>(0));
+    callback.Run(mojo::Array<StashedObjectPtr>());
     return;
   }
   callback.Run(backend_->RetrieveStash());
@@ -114,7 +114,7 @@ void StashBackend::AddToStash(mojo::Array<StashedObjectPtr> stashed_objects) {
 
 mojo::Array<StashedObjectPtr> StashBackend::RetrieveStash() {
   has_notified_ = false;
-  mojo::Array<StashedObjectPtr> result(0);
+  mojo::Array<StashedObjectPtr> result;
   for (auto& entry : stashed_objects_) {
     result.push_back(entry->Release());
   }

diff --git a/extensions/browser/mojo/stash_backend_unittest.cc b/extensions/browser/mojo/stash_backend_unittest.cc
@@ -112,14 +112,14 @@ TEST_F(StashServiceTest, AddTwiceAndRetrieve) {
   StashedObjectPtr stashed_object(StashedObject::New());
   stashed_object->id = "test type";
   stashed_object->data.push_back(1);
-  stashed_object->stashed_handles = mojo::Array<mojo::ScopedHandle>(0);
+  stashed_object->stashed_handles = mojo::Array<mojo::ScopedHandle>();
   stashed_objects.push_back(std::move(stashed_object));
   stash_service_->AddToStash(std::move(stashed_objects));
   stashed_object = StashedObject::New();
   stashed_object->id = "test type2";
   stashed_object->data.push_back(2);
   stashed_object->data.push_back(3);
-  stashed_object->stashed_handles = mojo::Array<mojo::ScopedHandle>(0);
+  stashed_object->stashed_handles = mojo::Array<mojo::ScopedHandle>();
   stashed_objects.push_back(std::move(stashed_object));
   stash_service_->AddToStash(std::move(stashed_objects));
   stashed_objects = RetrieveStash();

diff --git a/mojo/public/cpp/bindings/array.h b/mojo/public/cpp/bindings/array.h
@@ -34,8 +34,10 @@ class Array {
   using Data_ =
       internal::Array_Data<typename internal::WrapperTraits<T>::DataType>;
 
-  // Constructs a new array that is null.
-  Array() : is_null_(true) {}
+  // Constructs an empty array.
+  Array() : is_null_(false) {}
+  // Constructs a null array.
+  Array(std::nullptr_t null_pointer) : is_null_(true) {}
 
   // Constructs a new non-null array of the specified size. The elements will
   // be value-initialized (meaning that they will be initialized by their
@@ -47,15 +49,26 @@ class Array {
   Array(std::vector<T>&& other) : vec_(std::move(other)), is_null_(false) {}
   Array(Array&& other) : is_null_(true) { Take(&other); }
 
+  Array& operator=(std::vector<T>&& other) {
+    vec_ = std::move(other);
+    is_null_ = false;
+    return *this;
+  }
   Array& operator=(Array&& other) {
     Take(&other);
     return *this;
   }
 
+  Array& operator=(std::nullptr_t null_pointer) {
+    is_null_ = true;
+    vec_.clear();
+    return *this;
+  }
+
   // Creates a non-null array of the specified size. The elements will be
   // value-initialized (meaning that they will be initialized by their default
   // constructor, if any, or else zero-initialized).
-  static Array New(size_t size) { return std::move(Array(size)); }
+  static Array New(size_t size) { return Array(size); }
 
   // Creates a new array with a copy of the contents of |other|.
   template <typename U>
@@ -69,12 +82,6 @@ class Array {
     return TypeConverter<U, Array>::Convert(*this);
   }
 
-  // Resets the contents of this array back to null.
-  void reset() {
-    vec_.clear();
-    is_null_ = true;
-  }
-
   // Indicates whether the array is null (which is distinct from empty).
   bool is_null() const { return is_null_; }
 
@@ -111,6 +118,9 @@ class Array {
     vec_.resize(size);
   }
 
+  // Sets the array to empty (even if previously it was null.)
+  void SetToEmpty() { resize(0); }
+
   // Returns a const reference to the |std::vector| managed by this class. If
   // the array is null, this will be an empty vector.
   const std::vector<T>& storage() const { return vec_; }
@@ -185,7 +195,7 @@ class Array {
   bool operator!=(const Array<U>& other) const = delete;
 
   void Take(Array* other) {
-    reset();
+    operator=(nullptr);
     Swap(other);
   }
 
@@ -228,7 +238,7 @@ struct TypeConverter<std::vector<E>, Array<T>> {
 template <typename T, typename E>
 struct TypeConverter<Array<T>, std::set<E>> {
   static Array<T> Convert(const std::set<E>& input) {
-    Array<T> result(0u);
+    Array<T> result;
     for (auto i : input)
       result.push_back(TypeConverter<T, E>::Convert(i));
     return std::move(result);

diff --git a/mojo/public/cpp/bindings/lib/array_serialization.h b/mojo/public/cpp/bindings/lib/array_serialization.h
@@ -462,7 +462,7 @@ inline bool Deserialize_(internal::Array_Data<F>* input,
       E, internal::ShouldUseNativeSerializer<E>::value>;
   if (input)
     return Strategy::template Deserialize<F>(input, output, context);
-  output->reset();
+  *output = nullptr;
   return true;
 }
 

diff --git a/mojo/public/cpp/bindings/lib/map_serialization.h b/mojo/public/cpp/bindings/lib/map_serialization.h
@@ -183,7 +183,7 @@ inline bool Deserialize_(internal::Map_Data<DataKey, DataValue>* input,
 
     *output = Map<MapKey, MapValue>(std::move(keys), std::move(values));
   } else {
-    output->reset();
+    *output = nullptr;
   }
   return success;
 }

diff --git a/mojo/public/cpp/bindings/lib/string_serialization.cc b/mojo/public/cpp/bindings/lib/string_serialization.cc
@@ -36,7 +36,7 @@ bool Deserialize_(internal::String_Data* input,
     String result(input->storage(), input->size());
     result.Swap(output);
   } else {
-    output->reset();
+    *output = nullptr;
   }
   return true;
 }