Updates file type selector for fileSystem API

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/file_system/file_system_api.h"
 
 #include "base/bind.h"
 #include "base/file_path.h"
 #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"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #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;
 
 const char kInvalidParameters[] = "Invalid parameters";
 const char kSecurityError[] = "Security error";
 const char kInvalidCallingPage[] = "Invalid calling page";
 const char kUserCancelled[] = "User cancelled";
 const char kWritableFileError[] = "Invalid file for writing";
 const char kRequiresFileSystemWriteError[] =
     "Operation requires fileSystemWrite permission";
@@ skipping 89 matching lines @@
   // Create the file if it doesn't already exist.
   base::PlatformFileError error = base::PLATFORM_FILE_OK;
   int creation_flags = base::PLATFORM_FILE_CREATE |
                        base::PLATFORM_FILE_READ |
                        base::PLATFORM_FILE_WRITE;
   base::CreatePlatformFile(path, creation_flags, NULL, &error);
   return error == base::PLATFORM_FILE_OK ||
          error == base::PLATFORM_FILE_ERROR_EXISTS;
 }
 
+std::vector<std::string> ParseAcceptValue(const std::string& accept_types_) {

[benwells, 2012/07/27 03:43:56]

Put a comment explaining how it's being parsed.

[thorogood, 2012/07/27 04:49:55]

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/27 03:43:56]

This is all convoluted, can be simpler by stopping the vector -> set -> vector
transformation and simplifying the 'has valid description' logic.

To do this change GetDescriptionIdAndExtension to have this signature:
std::vector<FilePath:StringType> Get...ions(const std::vector<std::string>
type_list, int* description_id, bool* has_description).

This gets the extensions and description id for all the types, i.e. it has the
loop, and a bit of logic to track if it gets multiple valid description ids.
Internally it uses a set and at return converts to a vector.

[thorogood, 2012/07/27 04:49:55]

I certainly agree that it's all convoluted. :)
So, I didn't implement your suggestion verbatim. Issues:
- net::Get*Extensions* methods operate on a std::vector, so I've had to use an
"inner" vector and then add to the set (so we still have the vector -> set ->
vector, but at least it's within one method)
- has_description felt superfluous, since I ended up only using it for internal
state inside GetDescriptionIdAndExtensions (I can clear description_id if I've
already seen a previous description)

What I've done is generally simplified the logic. Basically now the only time
you'll see a fixed description is if you call this method with a single "magic"
type, i.e., "image/*", "audio/*", or "video/*". If you specify more than one
valid type, we'll always fall out to expanding the whole list (*.jpeg, *.jpg,
...), and I think this method is easier to grok because of it.

+    const std::string& type,
+    int *description_id) {

[benwells, 2012/07/27 03:43:56]

int *des -> int* des

[thorogood, 2012/07/27 04:49:55]

Done.

+  std::vector<FilePath::StringType> extensions;
+
+  if (type.empty())
+    return extensions;
+
+  if (type[0] == '.') {
+    // Assume this is a file extension, and return a list with just a single
+    // extension type.
+    FilePath::StringType ext(type.begin(), type.end());
+    extensions.push_back(ext.substr(1));
+  } else if (type == "image/*") {
+    *description_id = IDS_IMAGE_FILES;
+    net::GetImageExtensions(&extensions);
+  } else if (type == "audio/*") {
+    *description_id = IDS_AUDIO_FILES;
+    net::GetAudioExtensions(&extensions);
+  } else if (type == "video/*") {
+    *description_id = IDS_VIDEO_FILES;
+    net::GetVideoExtensions(&extensions);
+  } else {
+    net::GetExtensionsForMimeType(type, &extensions);
+  }
+
+  return extensions;
+}
+
+bool GetFileTypesFromAcceptType(const std::string& accept_type,
+    std::vector<FilePath::StringType>* extensions,
+    string16 *description) {

[benwells, 2012/07/27 03:43:56]

string16 *des -> string16* des

[thorogood, 2012/07/27 04:49:55]

Done.

+  std::vector<std::string> type_list = ParseAcceptValue(accept_type);
+  std::set<FilePath::StringType> extension_set;
+
+  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();
+
+    std::vector<FilePath::StringType> local = GetDescriptionIdAndExtensions(
+          *iter, &description_id);
+    extension_set.insert(local.begin(), local.end());
+
+    if (extension_set.size() > old_extension_size)
+      valid_type_count++;
+  }
+
+  if (valid_type_count == 0)

[benwells, 2012/07/27 03:43:56]

This can change to if (extension_set.size() > 0)

[thorogood, 2012/07/27 04:49:55]

Done, well, to if (extension_set.empty())...

+    return false;
+
+  if (!description_id || valid_type_count > 1) {

[benwells, 2012/07/27 03:43:56]

This can change to if (has_valid_description) { .... } else { .... }

[thorogood, 2012/07/27 04:49:55]

I've changed it just to "!description_id", since that's all we return now from
GetDsecriptionIdAndExtensions.

+    // We don't have a description ID, or we have one but need to include
+    // other valid types (e.g. "image/*,text/html"); build a string like
+    // "*.ext1, *.ext2, *.other-ext".
+    *description = string16();
+    for (std::set<FilePath::StringType>::const_iterator iter
+           = extension_set.begin(); iter != extension_set.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);
+  }
+  extensions->assign(extension_set.begin(), extension_set.end());
+
+  return true;
+}
+
 }  // namespace
 
 namespace extensions {
 
 bool FileSystemGetDisplayPathFunction::RunImpl() {
   std::string filesystem_name;
   std::string filesystem_path;
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &filesystem_name));
   EXTENSION_FUNCTION_VALIDATE(args_->GetString(1, &filesystem_path));
 
@@ skipping 112 matching lines @@
 }
 
 // Handles showing a dialog to the user to ask for the filename for a file to
 // save or open.
 class FileSystemChooseFileFunction::FilePicker
     : public SelectFileDialog::Listener {
  public:
   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),
         entry_type_(entry_type),
         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;
 
     if (g_skip_picker_for_test) {
       if (g_path_to_be_picked_for_test) {
         content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
             base::Bind(
                 &FileSystemChooseFileFunction::FilePicker::FileSelected,
                 base::Unretained(this), *g_path_to_be_picked_for_test, 1,
                 static_cast<void*>(NULL)));
@@ skipping 35 matching lines @@
   EntryType entry_type_;
 
   scoped_refptr<SelectFileDialog> select_file_dialog_;
   scoped_refptr<FileSystemChooseFileFunction> function_;
 
   DISALLOW_COPY_AND_ASSIGN(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());
   DCHECK(registry);
   ShellWindow* shell_window = registry->GetShellWindowForRenderViewHost(
       render_view_host());
   if (!shell_window) {
     error_ = kInvalidCallingPage;
     return false;
   }
 
   // The file picker will hold a reference to this function instance, preventing
   // its destruction (and subsequent sending of the function response) until the
   // 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;
 }
 
 // static
 void FileSystemChooseFileFunction::SkipPickerAndAlwaysSelectPathForTest(
     FilePath* path) {
   g_skip_picker_for_test = true;
   g_path_to_be_picked_for_test = path;
 }
 
@@ skipping 19 matching lines @@
 
   // Don't need to check the file, it's for reading.
   RegisterFileSystemAndSendResponse(path, READ_ONLY);
 }
 
 void FileSystemChooseFileFunction::FileSelectionCanceled() {
   error_ = kUserCancelled;
   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

[benwells, 2012/07/27 03:43:56]

Implement this now or remove the suggested_extension calculation

[thorogood, 2012/07/27 04:49:55]

Done, taken the implemented approach.

+      // 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;
+  }
+}
+
+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;
 
   file_system::ChooseFileOptions* options = params->options.get();
   if (options) {
     if (options->type.get()) {
       if (*options->type == kOpenWritableFileOption) {
         entry_type = WRITABLE;
       } else if (*options->type == kSaveFileOption) {
         entry_type = WRITABLE;
         picker_type = SelectFileDialog::SELECT_SAVEAS_FILE;
       } else if (*options->type != kOpenFileOption) {
         error_ = kUnknownChooseFileType;
         return false;
       }
     }
 
-    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());
   }
 
   if (entry_type == WRITABLE && !HasFileSystemWritePermission()) {
     error_ = kRequiresFileSystemWriteError;
     return false;
   }
 
-  return ShowPicker(suggested_name, picker_type, entry_type);
+  return ShowPicker(suggested_name, *file_type_info, picker_type, entry_type);
 }
 
 }  // namespace extensions