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..10b995bf050174857a0119d5479432da61090e09 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,85 @@ 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.,

[benwells, 2012/07/30 01:07:16]

/Parses/Parses and normalizes/

[thorogood, 2012/07/30 06:39:16]

Done.

+// from "foo /bar, .TXT, hello/*" into ["foo/bar", ".txt", "hello/*"].
+std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) {

[benwells, 2012/07/30 01:07:16]

Trailing underscore represents a field on a class. Don't use for parameter
names. Instead, have your local variable communicate how it is different e.g.
normalized_accept_types.

[thorogood, 2012/07/30 06:39:16]

Done.

+  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(

[benwells, 2012/07/30 01:07:16]

Now the description processing is so much simpler this can go back to the body
of GetFileTypesFromAcceptType.

[thorogood, 2012/07/30 06:39:16]

Done, PTAL.

+    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)

[benwells, 2012/07/30 01:07:16]

Does this mean something like "video/*,.xyz" will not use the video description
override?

[thorogood, 2012/07/30 06:39:16]

Yes. As discussed, I'm changing this code so that: if we see a major "foo/*"
with a matching description, we'll use that irrespective of what other
extensions we add to the list.

However on the corner case that we see more than one, e.g., "image/*" and
"video/*", I've change the logic to fall back to no description at all (urgh).

+      *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)
+    *description = l10n_util::GetStringUTF16(description_id);
+
+  return true;
+}
+
 }  // namespace
 
 namespace extensions {
@@ -263,6 +347,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 +355,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 +410,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 +427,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 +467,74 @@ void FileSystemChooseFileFunction::FileSelectionCanceled() {
   SendResponse(false);
 }
 
+void FileSystemChooseFileFunction::BuildFileTypeInfo(
+    SelectFileDialog::FileTypeInfo* file_type_info,
+    const FilePath::StringType& suggested_extension,
+    const std::vector<std::string>* accepts,
+    const bool* acceptsAllTypes) {
+  if (acceptsAllTypes)
+    file_type_info->include_all_files = *acceptsAllTypes;
+  else
+    file_type_info->include_all_files = false;

[benwells, 2012/07/30 01:07:16]

Default should be true.

[thorogood, 2012/07/30 06:39:16]

Nice catch, forgot about that from the IDL.

+
+  bool need_suggestion = !file_type_info->include_all_files &&
+                         !suggested_extension.empty();
+
+  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

[benwells, 2012/07/30 01:07:16]

Nit: what does optionally mean here?

[thorogood, 2012/07/30 06:39:16]

clarified this comment, it's now "If we still need to find suggested_extension,
hunt for it inside the extensions returned from GetFileTypesFromAcceptType.".

+      // extensions provided through GetFileTypesFromAcceptType.
+      if (need_suggestion && std::find(extensions.begin(),
+          extensions.end(),
+          suggested_extension) != extensions.end()) {
+        need_suggestion = false;
+      }
+    }
+  }
+
+  // If there's nothing in our accepted extension list or we couldn't find the
+  // suggested extension required, then default to accepting all types.
+  if (file_type_info->extensions.empty() || need_suggestion)
+    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()) {

[benwells, 2012/07/30 01:07:16]

Nit: don't need braces.

[thorogood, 2012/07/30 06:39:16]

Done.

+      *suggested_name = FilePath();
+    }
+    *suggested_extension = suggested_name->Extension();
+    if (!suggested_extension->empty()) {

[benwells, 2012/07/30 01:07:16]

Nit: don't need braces.

[thorogood, 2012/07/30 06:39:16]

Done.

+      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;
+  SelectFileDialog::FileTypeInfo file_type_info;
   EntryType entry_type = READ_ONLY;
   SelectFileDialog::Type picker_type = SelectFileDialog::SELECT_OPEN_FILE;
 
@@ -411,18 +552,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, suggested_extension,
+        options->accepts.get(), options->accepts_all_types.get());
   }
 
   if (entry_type == WRITABLE && !HasFileSystemWritePermission()) {
@@ -430,7 +565,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