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..21975b423bba6f6545bf6353ffebf87211dcf66d 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,90 @@ bool DoCheckWritableFile(const FilePath& path) {
          error == base::PLATFORM_FILE_ERROR_EXISTS;
 }
 
+std::vector<std::string> ParseAcceptValue(const std::string& accept_types) {
+  // NOTE: Mostly stolen from ppb_file_chooser_impl.cc
+  std::vector<std::string> type_list;
+  if (accept_types.empty())
+    return type_list;
+  base::SplitString(accept_types, ',', &type_list);
+  std::vector<std::string> normalized_type_list;
+  normalized_type_list.reserve(type_list.size());
+
+  for (std::vector<std::string>::const_iterator iter = type_list.begin();
+       iter != type_list.end(); ++iter) {
+    std::string type;
+    TrimWhitespaceASCII(*iter, TRIM_ALL, &type);
+
+    if (type.empty())
+      continue;
+
+    StringToLowerASCII(&type);
+    normalized_type_list.push_back(type);
+  }
+
+  return normalized_type_list;
+}

[benwells, 2012/07/26 05:40:45]

I think the ParseAcceptValue function could be made simpler by removing spaces
and making lower case before splitting. Then you just need to split.

[thorogood, 2012/07/26 07:39:05]

Done, although now I feel like inlining the method just inside
GetFileTypesFromAcceptType. Thoughts, now that GetFileTypesFromAcceptType is
much shorter?

[benwells, 2012/07/26 08:10:23]

I prefer it like this.

+
+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 = 0;
+  int valid_type_count = 0;
+
+  for (std::vector<std::string>::const_iterator iter = type_list.begin();
+       iter != type_list.end(); ++iter) {
+    size_t old_extension_size = extensions->size();
+
+    if ((*iter)[0] == '.') {
+      // Assume this is a file extension, and add it to its own place inside
+      // the list.
+      FilePath::StringType ext(iter->begin(), iter->end());
+      extensions->push_back(ext.substr(1));
+    } else if (*iter == "image/*") {
+      description_id = IDS_IMAGE_FILES;
+      net::GetImageExtensions(extensions);
+    } else if (*iter == "audio/*") {
+      description_id = IDS_AUDIO_FILES;
+      net::GetAudioExtensions(extensions);
+    } else if (*iter == "video/*") {
+      description_id = IDS_VIDEO_FILES;
+      net::GetVideoExtensions(extensions);
+    } else {
+      net::GetExtensionsForMimeType(*iter, extensions);
+    }
+    // TODO(thorogood): It's possible to add the same extension multiple times.
+

[benwells, 2012/07/26 05:40:45]

I think you should split this whole loop out into a new function
std::vector<std::string> GetDescriptionIdAndExtensions(string, int*) and use a
set when constructing the return value. This will encapsulate this ugly list of
if's and handle duplicate extensions.

[thorogood, 2012/07/26 07:39:05]

I'm not sure if I completely understand you, but what I've done is:

- written GetDescriptionIdAndExtensions, and made it return a std::vector
- insert everything from that vector into a std::set held over the lifetime of
GetFileTypesFromAcceptType
- at the end of GetFileTypesFromAcceptType, copy everything from the std::set
into the output variable "extensions"

I've also updated the comment now on line 208 to be clearer as to why we only
sometimes generate a description based on explicit definitions.

[benwells, 2012/07/26 08:10:23]

Hmm, I meant somethign slightly different, but this works. With this you may as
well return a set, not vector.

It would be nice to clean up the description logic somehow, it's pretty
confusing. One approach would be to only update the desktop in the
GetDescriptionIdAndExtensions function if there it is currently blank. Otherwise
set it to -1 or something. But -1 magic value is not that nice either.

[thorogood, 2012/07/27 03:14:38]

I've gone over this a few times in my head and I don't think there's any better
way to have the logic.

I'm also not fussed about returning a set. I think by returning a vector we make
the caller's life easier (since the SelectFileDialog::FileTypeInfo struct
requires a vector). So I've left this unless you have strong feelings otherwise.

+    if (extensions->size() > old_extension_size)
+      valid_type_count++;
+  }
+
+  if (valid_type_count == 0)

[benwells, 2012/07/26 05:40:45]

Can valid_type_count be replaced with extensions->size(), and old_extension_size
removed?

[thorogood, 2012/07/26 07:39:05]

Probably not. The idea here is that we have a few magical types: image/*,
audio/* and video/*, which actually expand to a much larger list each (but this
also applies to say, text/html, which expands to "htm" and "html"). So, checking
for extensions->size() == 1 doesn't really do what we want here.

The alternative is to change the logic which sets description_id; to only set if
it its currently unset, and set it to a dummy "generate our own description"
value if we see anything that isn't "image/*", "audio/*" or "video/*".

Phew.

[benwells, 2012/07/26 08:10:23]

Oh, ok, i see it is used in two places, not just the ==0.

+    return false;
+
+  if (!description_id || valid_type_count > 1) {
+    // We don't have a description ID, or we have multiple valid types; build
+    // a string like "*.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 +352,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 +360,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 +415,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 +432,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 +472,40 @@ void FileSystemChooseFileFunction::FileSelectionCanceled() {
   SendResponse(false);
 }
 
+void FileSystemChooseFileFunction::BuildFileTypeInfo(
+    SelectFileDialog::FileTypeInfo* file_type_info,
+    const std::vector<std::string>* accepts,
+    const bool* acceptsAllTypes) {
+  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)) {
+        file_type_info->extensions.push_back(extensions);
+        file_type_info->extension_description_overrides.push_back(description);
+      }
+      // TODO(thorogood): If suggested_extension is non-empty, ensure that
+      // at least once of our extensions list includes it.
+    }
+  }
+
+  if (acceptsAllTypes && !file_type_info->extensions.empty()) {
+    file_type_info->include_all_files = *acceptsAllTypes;
+  } else {
+    // There's nothing in our accepted extension list; default to accepting
+    // all types.
+    file_type_info->include_all_files = true;
+  }
+}
+
 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(

[benwells, 2012/07/26 05:40:45]

This declaration should go just before where it is first used.

[thorogood, 2012/07/26 07:39:05]

It is already, more or less, since it is updated inside the "if (options) {"
block.

[benwells, 2012/07/26 08:10:23]

Ah, ok.

+      new SelectFileDialog::FileTypeInfo());
   EntryType entry_type = READ_ONLY;
   SelectFileDialog::Type picker_type = SelectFileDialog::SELECT_OPEN_FILE;
 
@@ -411,6 +523,7 @@ bool FileSystemChooseFileFunction::RunImpl() {
       }

[benwells, 2012/07/26 05:40:45]

Consider splitting this chunk out into a new function

[thorogood, 2012/07/27 03:14:38]

I've considered it, but I think the interaction with error_ and multiple
variables is too much for now, it's a bit contrived. This section isn't that
long anyway.

     }
 
+    FilePath::StringType suggested_extension;
     if (options->suggested_name.get()) {
       suggested_name = FilePath::FromUTF8Unsafe(
           *options->suggested_name.get());
@@ -422,7 +535,14 @@ bool FileSystemChooseFileFunction::RunImpl() {
       if (suggested_name.IsAbsolute()) {
         suggested_name = FilePath();
       }
+      suggested_extension = suggested_name.Extension();
+      if (!suggested_extension.empty()) {
+        suggested_extension.erase(suggested_extension.begin());  // drop the .

[benwells, 2012/07/26 05:40:45]

And this chunk

[thorogood, 2012/07/27 03:14:38]

Done, I've even added a test.

+      }
     }
+
+    BuildFileTypeInfo(file_type_info.get(), options->accepts.get(),
+        options->accepts_all_types.get());
   }
 
   if (entry_type == WRITABLE && !HasFileSystemWritePermission()) {
@@ -430,7 +550,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