Updates file type selector for fileSystem API

chrome/browser/extensions/api/file_system/file_system_api.cc

Index: chrome/browser/extensions/api/file_system/file_system_api.cc
diff --git a/chrome/browser/extensions/api/file_system/file_system_api.cc b/chrome/browser/extensions/api/file_system/file_system_api.cc
index eb20c11c664c083d42ddf839fe2100ff43944ae2..5d56ffb333259b3f5358ba965f68876afae907ff 100644
--- a/chrome/browser/extensions/api/file_system/file_system_api.cc
+++ b/chrome/browser/extensions/api/file_system/file_system_api.cc
@@ -9,6 +9,8 @@
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/string_split.h"
+#include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/platform_util.h"
@@ -16,12 +18,15 @@
 #include "chrome/browser/ui/extensions/shell_window.h"
 #include "chrome/common/extensions/api/file_system.h"
 #include "chrome/common/extensions/permissions/api_permission.h"
+#include "grit/generated_resources.h"
+#include "net/base/mime_util.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "webkit/fileapi/file_system_util.h"
 #include "webkit/fileapi/isolated_context.h"
+#include "ui/base/l10n/l10n_util.h"
 
 using fileapi::IsolatedContext;
 
@@ -131,6 +136,101 @@ bool DoCheckWritableFile(const FilePath& path) {
          error == base::PLATFORM_FILE_ERROR_EXISTS;
 }
 
+// Parses a comma-seperated list of accept types into a std::vector, e.g.,
+// from "foo /bar, .TXT, hello/*" into ["foo/bar", ".txt", "hello/*"].
+std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) {
+  std::vector<std::string> type_list;
+  std::string accept_types(accept_types_);
+
+  RemoveChars(accept_types, kWhitespaceASCII, &accept_types);
+  StringToLowerASCII(&accept_types);
+
+  // Note that type_list might contain blank entries, e.g., if the source data
+  // was "foo,,bar", the output will be "foo", "", "bar".
+  base::SplitString(accept_types, ',', &type_list);
+  return type_list;
+}
+
+std::vector<FilePath::StringType> GetDescriptionIdAndExtensions(
+    const std::vector<std::string>& type_list,
+    int* description_id) {
+  std::set<FilePath::StringType> extensions;
+  bool has_valid_types = false;
+
+  *description_id = 0;
+
+  for (std::vector<std::string>::const_iterator iter = type_list.begin();
+       iter != type_list.end(); ++iter) {
+    std::vector<FilePath::StringType> inner;
+
+    if (iter->empty()) {
+      // Do nothing.
+    } else if ((*iter)[0] == '.') {
+      // Assume this is a file extension, and update the vector with just a
+      // single extension type.
+      FilePath::StringType ext(iter->begin(), iter->end());
+      inner.push_back(ext.substr(1));
+    } else if (*iter == "image/*") {
+      *description_id = IDS_IMAGE_FILES;
+      net::GetImageExtensions(&inner);
+    } else if (*iter == "audio/*") {
+      *description_id = IDS_AUDIO_FILES;
+      net::GetAudioExtensions(&inner);
+    } else if (*iter == "video/*") {
+      *description_id = IDS_VIDEO_FILES;
+      net::GetVideoExtensions(&inner);
+    } else {
+      net::GetExtensionsForMimeType(*iter, &inner);
+    }
+
+    if (inner.empty())
+      continue;
+
+    if (has_valid_types)
+      *description_id = 0;  // One valid accept type has already been seen; we
+                            // can no longer provide a fixed description.
+
+    has_valid_types = true;
+    extensions.insert(inner.begin(), inner.end());
+  }
+
+  return std::vector<FilePath::StringType>(extensions.begin(),
+      extensions.end());
+}
+
+bool GetFileTypesFromAcceptType(const std::string& accept_type,
+    std::vector<FilePath::StringType>* extensions,
+    string16* description) {
+  std::vector<std::string> type_list = ParseAcceptValue(accept_type);
+  int description_id;
+
+  *extensions = GetDescriptionIdAndExtensions(type_list, &description_id);
+
+  if (extensions->empty())
+    return false;
+
+  if (!description_id) {
+    // We don't have a description ID; build a string like

[benwells, 2012/07/27 05:59:24]

What happens if we don't supply a description? The file type info field being
set is called description_overrides, what is the non-overidden description?

[thorogood, 2012/07/27 06:21:23]

select_file_helper.cc:292 tells us that at least on Windows, the select file
dialog "uses the first extension in the list to form the description, like
"EHTML Files". This is not what we want.".

I've also just tried it (by commenting out the description append) on Linux, and
it generates almost exactly what we do here, but without the spaces (i.e.,
"*.ext1,*.ext2").

I can find out for completeness what Mac does, but I feel like defining it is
better.

+    // "*.ext1, *.ext2, *.other-ext".
+    *description = string16();
+    for (std::vector<FilePath::StringType>::const_iterator iter
+           = extensions->begin(); iter != extensions->end(); ++iter) {
+      if (!description->empty())
+        description->append(UTF8ToUTF16(", "));
+      description->append(UTF8ToUTF16("*."));
+#if defined(OS_WIN)  // FilePath::StringType is already string16.
+      description->append(*iter);
+#else
+      description->append(UTF8ToUTF16(*iter));
+#endif
+    }
+  } else {
+    *description = l10n_util::GetStringUTF16(description_id);
+  }
+
+  return true;
+}
+
 }  // namespace
 
 namespace extensions {
@@ -263,6 +363,7 @@ class FileSystemChooseFileFunction::FilePicker
   FilePicker(FileSystemChooseFileFunction* function,
              content::WebContents* web_contents,
              const FilePath& suggested_name,
+             const SelectFileDialog::FileTypeInfo& file_type_info,
              SelectFileDialog::Type picker_type,
              EntryType entry_type)
       : suggested_name_(suggested_name),
@@ -270,14 +371,6 @@ class FileSystemChooseFileFunction::FilePicker
         function_(function) {
     select_file_dialog_ = SelectFileDialog::Create(
         this, new ChromeSelectFilePolicy(web_contents));
-    SelectFileDialog::FileTypeInfo file_type_info;
-    FilePath::StringType extension = suggested_name.Extension();
-    if (!extension.empty()) {
-      extension.erase(extension.begin());  // drop the .
-      file_type_info.extensions.resize(1);
-      file_type_info.extensions[0].push_back(extension);
-    }
-    file_type_info.include_all_files = true;
     gfx::NativeWindow owning_window = web_contents ?
         platform_util::GetTopLevel(web_contents->GetNativeView()) : NULL;
 
@@ -333,6 +426,7 @@ class FileSystemChooseFileFunction::FilePicker
 
 bool FileSystemChooseFileFunction::ShowPicker(
     const FilePath& suggested_name,
+    const SelectFileDialog::FileTypeInfo& file_type_info,
     SelectFileDialog::Type picker_type,
     EntryType entry_type) {
   ShellWindowRegistry* registry = ShellWindowRegistry::Get(profile());
@@ -349,7 +443,7 @@ bool FileSystemChooseFileFunction::ShowPicker(
   // user has selected a file or cancelled the picker. At that point, the picker
   // will delete itself, which will also free the function instance.
   new FilePicker(this, shell_window->web_contents(), suggested_name,
-      picker_type, entry_type);
+      file_type_info, picker_type, entry_type);
   return true;
 }
 
@@ -389,11 +483,75 @@ void FileSystemChooseFileFunction::FileSelectionCanceled() {
   SendResponse(false);
 }
 
+void FileSystemChooseFileFunction::BuildFileTypeInfo(
+    SelectFileDialog::FileTypeInfo* file_type_info,
+    FilePath::StringType suggested_extension,

[benwells, 2012/07/27 05:59:24]

use const&

+    const std::vector<std::string>* accepts,

[benwells, 2012/07/27 05:59:24]

const&

[thorogood, 2012/07/27 06:21:23]

This is sourced from calling .get() on a scoped_ptr<std::vector<std::string>>.
If I dereference it and it is NULL, that would be bad.

+    const bool* acceptsAllTypes) {

[benwells, 2012/07/27 05:59:24]

bool

[thorogood, 2012/07/27 06:21:23]

as above.

+  if (accepts) {
+    for (std::vector<std::string>::const_iterator iter = accepts->begin();
+         iter != accepts->end(); ++iter) {
+      string16 description;
+      std::vector<FilePath::StringType> extensions;
+
+      if (!GetFileTypesFromAcceptType(*iter, &extensions, &description))
+        continue;  // No extensions were found.
+
+      file_type_info->extensions.push_back(extensions);
+      file_type_info->extension_description_overrides.push_back(description);
+
+      // Optionally hunt for our suggested_extension, ensuring it is one of the
+      // extensions provided through GetFileTypesFromAcceptType.
+      if (!suggested_extension.empty()) {
+        for (std::vector<FilePath::StringType>::const_iterator eiter
+              = extensions.begin(); eiter != extensions.end(); ++eiter) {
+          if (*eiter == suggested_extension) {
+            suggested_extension = FilePath::StringType();

[benwells, 2012/07/27 05:59:24]

Introduce a variable instead of using suggested_extension.empty() to indicate
something. You could call it need_all_files or something.

[thorogood, 2012/07/27 06:21:23]

I've called it need_suggestion, since we mark it as false when we find it - and
no longer need to keep searching. need_all_files is a harder state to maintain
knowledge of :)

+            break;
+          }
+        }
+      }
+    }
+  }
+
+  if (acceptsAllTypes && !file_type_info->extensions.empty()

[benwells, 2012/07/27 05:59:24]

What is the default behaviour if you have include_all_files false but
file_type_info->extensions empty or not containing the suggested extension? Does
the select file dialog do something sensible?

[thorogood, 2012/07/27 06:21:23]

At least on Linux, it doesn't do anything sensible - it prevents you from
selecting any files.

+      && suggested_extension.empty()) {
+    file_type_info->include_all_files = *acceptsAllTypes;
+  } else {
+    // There's nothing in our accepted extension list, or we couldn't find the
+    // suggested extension required; default to accepting all types.
+    file_type_info->include_all_files = true;
+  }
+}
+
+void FileSystemChooseFileFunction::BuildSuggestion(
+    const std::string *opt_name,
+    FilePath* suggested_name,
+    FilePath::StringType* suggested_extension) {
+  if (opt_name) {
+    *suggested_name = FilePath::FromUTF8Unsafe(*opt_name);
+
+    // Don't allow any path components; shorten to the base name. This should
+    // result in a relative path, but in some cases may not. Clear the
+    // suggestion for safety if this is the case.
+    *suggested_name = suggested_name->BaseName();
+    if (suggested_name->IsAbsolute()) {
+      *suggested_name = FilePath();
+    }
+    *suggested_extension = suggested_name->Extension();
+    if (!suggested_extension->empty()) {
+      suggested_extension->erase(suggested_extension->begin());  // drop the .
+    }
+  }
+}
+
 bool FileSystemChooseFileFunction::RunImpl() {
   scoped_ptr<ChooseFile::Params> params(ChooseFile::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   FilePath suggested_name;
+  scoped_ptr<SelectFileDialog::FileTypeInfo> file_type_info(
+      new SelectFileDialog::FileTypeInfo());
   EntryType entry_type = READ_ONLY;
   SelectFileDialog::Type picker_type = SelectFileDialog::SELECT_OPEN_FILE;
 
@@ -411,18 +569,12 @@ bool FileSystemChooseFileFunction::RunImpl() {
       }
     }
 
-    if (options->suggested_name.get()) {
-      suggested_name = FilePath::FromUTF8Unsafe(
-          *options->suggested_name.get());
+    FilePath::StringType suggested_extension;
+    BuildSuggestion(options->suggested_name.get(), &suggested_name,
+        &suggested_extension);
 
-      // Don't allow any path components; shorten to the base name. This should
-      // result in a relative path, but in some cases may not. Clear the
-      // suggestion for safety if this is the case.
-      suggested_name = suggested_name.BaseName();
-      if (suggested_name.IsAbsolute()) {
-        suggested_name = FilePath();
-      }
-    }
+    BuildFileTypeInfo(file_type_info.get(), options->accepts.get(),
+        options->accepts_all_types.get());

[benwells, 2012/07/27 05:59:24]

Doesn't this need another argument?

[thorogood, 2012/07/27 06:21:23]

Yep, I probably should have tested this. D'oh.

   }
 
   if (entry_type == WRITABLE && !HasFileSystemWritePermission()) {
@@ -430,7 +582,7 @@ bool FileSystemChooseFileFunction::RunImpl() {
     return false;
   }
 
-  return ShowPicker(suggested_name, picker_type, entry_type);
+  return ShowPicker(suggested_name, *file_type_info, picker_type, entry_type);
 }
 
 }  // namespace extensions