Make blink.mojom.DragItemFileSystemFile.file_system_id optional

In renderer to browser messages, this field isn't used and, because
WTF::String supports nullability, the value was serialized as null.

In addition, this CL adds and fixes some of the checks done in the
DragData Mojo type mapping.

Bug: 1136901
Change-Id: Ifc9f9343817d2e06d734470573ce3785cbe885d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2489897
Commit-Queue: Henrique Ferreiro <hferreiro@igalia.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821179}

content/browser/renderer_host/drop_data_util.cc

@@ -8,8 +8,10 @@
 #include <utility>
 #include <vector>
 
+#include "base/check.h"
 #include "base/containers/span.h"
 #include "base/files/file_path.h"
+#include "base/optional.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/file_system_access/native_file_system_manager_impl.h"
 #include "content/public/common/drop_data.h"
@@ -111,14 +113,25 @@ blink::mojom::DragDataPtr DropDataToDragData(
     items.push_back(blink::mojom::DragItem::NewString(std::move(item)));
   }
 
-  return blink::mojom::DragData::New(std::move(items),
-                                     base::UTF16ToUTF8(drop_data.filesystem_id),
-                                     drop_data.referrer_policy);
+  return blink::mojom::DragData::New(
+      std::move(items),
+      // While this shouldn't be a problem in production code, as the
+      // real file_system_id should never be empty if used in browser to
+      // renderer messages, some tests use this function to test renderer to
+      // browser messages, in which case the field is unused and this will hit
+      // a DCHECK.
+      drop_data.filesystem_id.empty()
+          ? base::nullopt
+          : base::Optional<std::string>(
+                base::UTF16ToUTF8(drop_data.filesystem_id)),
+      drop_data.referrer_policy);
 }
 
 DropData DragDataToDropData(const blink::mojom::DragData& drag_data) {
-  DropData result;
+  // This field should be empty when dragging from the renderer.
+  DCHECK(!drag_data.file_system_id);
 
+  DropData result;
   for (const blink::mojom::DragItemPtr& item : drag_data.items) {
     switch (item->which()) {
       case blink::mojom::DragItemDataView::Tag::STRING: {
@@ -168,10 +181,13 @@ DropData DragDataToDropData(const blink::mojom::DragData& drag_data) {
       case blink::mojom::DragItemDataView::Tag::FILE_SYSTEM_FILE: {
         const blink::mojom::DragItemFileSystemFilePtr& file_system_file_item =
             item->get_file_system_file();
+        // This field should be empty when dragging from the renderer.
+        DCHECK(!file_system_file_item->file_system_id);
+
         DropData::FileSystemFileInfo info;
         info.url = file_system_file_item->url;
         info.size = file_system_file_item->size;
-        info.filesystem_id = file_system_file_item->file_system_id;
+        info.filesystem_id = std::string();
         result.file_system_files.push_back(std::move(info));
         break;
       }

third_party/blink/public/mojom/page/drag.mojom

@@ -76,7 +76,7 @@ struct DragItemBinary {
 struct DragItemFileSystemFile {
   url.mojom.Url url;
   int64 size;
-  string file_system_id;
+  string? file_system_id;
 };
 
 union DragItem {

third_party/blink/renderer/platform/mojo/drag_mojom_traits.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/platform/mojo/drag_mojom_traits.h"
 
+#include "base/check.h"
 #include "base/containers/span.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_refptr.h"
@@ -176,6 +177,7 @@ int64_t StructTraits<
 WTF::String StructTraits<blink::mojom::DragItemFileSystemFileDataView,
                          blink::WebDragData::Item>::
     file_system_id(const blink::WebDragData::Item& item) {
+  DCHECK(item.file_system_id.IsNull());
   return item.file_system_id;
 }
 
@@ -185,7 +187,7 @@ StructTraits<blink::mojom::DragItemFileDataView, blink::WebDragData::Item>::
     native_file_system_token(const blink::WebDragData::Item& item) {
   // Should never have to send a transfer token information from the renderer
   // to the browser.
-  NOTREACHED();
+  DCHECK(!item.native_file_system_entry);
   return mojo::NullRemote();
 }