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) {
+  // 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 {
 
 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
+      // 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;
 
   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;
       }

[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());
 
       // 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 .

[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()) {
     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