Extracting and unittesting PrepareDropDataForChildProcess function.

content/browser/fileapi/browser_file_system_helper.h

Index: content/browser/fileapi/browser_file_system_helper.h
diff --git a/content/browser/fileapi/browser_file_system_helper.h b/content/browser/fileapi/browser_file_system_helper.h
index c5702e73281631e88d65107a64ded88381796c68..be0b768a4bafcd2bfd42e1aaca3674c410ae5aff 100644
--- a/content/browser/fileapi/browser_file_system_helper.h
+++ b/content/browser/fileapi/browser_file_system_helper.h
@@ -17,6 +17,8 @@ class FileSystemURL;
 namespace content {
 
 class BrowserContext;
+class ChildProcessSecurityPolicyImpl;
+struct DropData;
 
 // Helper method that returns FileSystemContext constructed for
 // the browser process.
@@ -36,6 +38,21 @@ CONTENT_EXPORT void SyncGetPlatformPath(storage::FileSystemContext* context,
                                         int process_id,
                                         const GURL& path,
                                         base::FilePath* platform_path);
+

[ncarter (slow), 2017/04/24 22:01:51]

Maybe this as an intro sentence:

// Make it possible for a |drop_data|'s resources to be read by |child_id|'s
// process -- by granting permissions, rewriting |drop_data|, or both.
//

[Łukasz Anforowicz, 2017/04/24 22:18:10]

Done.

+// |drop_data| can include references to local files and filesystem files that
+// were accessible to the child process that is the source of the drag and drop,
+// but might not (yet) be accessible the child process that is the target of the
+// drop.  PrepareDropDataForChildProcess makes sure that |child_id| has access
+// to files referred to by |drop_data| - this method will 1) mutate |drop_data|
+// as needed (e.g. to refer to files in a new isolated filesystem, rather than
+// the original filesystem files) and 2) use |security_policy| to grant
+// |child_id| appropriate file access.
+CONTENT_EXPORT void PrepareDropDataForChildProcess(

[Łukasz Anforowicz, 2017/04/24 21:30:24]

TBH I don't understand why |CONTENT_EXPORT| is needed here.  I am guessing that
things don't link without it, because there is an (unexpected to me) component
build boundary between here and
content/browser/renderer_host/render_widget_host_impl.cc.  FWIW, other
content-internal things in this file use CONTENT_EXPORT, so this is probably
fine.

[ncarter (slow), 2017/04/24 22:01:51]

In the component build, both render_widget_host_impl.cc and
browser_file_system_helper.cc get compiled into content.dll. If you didn't have
a unittest, you wouldn't need CONTENT_EXPORT.

You need the CONTENT_EXPORT because otherwise your unittest (which calls this
function directly) doesn't know to look in content.dll for this function.

[Łukasz Anforowicz, 2017/04/24 22:18:10]

Oh, ok - thanks for the explanation.  I guess I should have looked closer at the
error message from the linker to realize that it is complaining about the unit
tests, not about the product code.

+    DropData* drop_data,
+    ChildProcessSecurityPolicyImpl* security_policy,

[Łukasz Anforowicz, 2017/04/24 21:30:23]

This has to be CPSPImpl, because CPSP doesn't expose
GrantRequestSpecificFileURL.

[ncarter (slow), 2017/04/24 22:01:51]

That's fine.

+    int child_id,
+    const storage::FileSystemContext* file_system_context);
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_FILEAPI_BROWSER_FILE_SYSTEM_HELPER_H_