Save Page As MHTML

chrome/browser/download/save_package_file_picker.cc

Index: chrome/browser/download/save_package_file_picker.cc
diff --git a/chrome/browser/download/save_package_file_picker.cc b/chrome/browser/download/save_package_file_picker.cc
index e56c1bbf05a4e91ced5a14f18d55f5a77c3bdbb8..38136b640a0cad41b09c3e3ba2344d212e9dfc65 100644
--- a/chrome/browser/download/save_package_file_picker.cc
+++ b/chrome/browser/download/save_package_file_picker.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/download/save_package_file_picker.h"
 
+#include "base/metrics/histogram.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/platform_util.h"
@@ -33,6 +34,7 @@ const SavePageType kIndexToSaveType[] = {
   content::SAVE_PAGE_TYPE_UNKNOWN,
   content::SAVE_PAGE_TYPE_AS_ONLY_HTML,
   content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML,
+  content::SAVE_PAGE_TYPE_AS_MHTML,
 };
 
 int SavePackageTypeToIndex(SavePageType type) {
@@ -48,10 +50,14 @@ int SavePackageTypeToIndex(SavePageType type) {
 // the user chooses when picking a save type.
 const int kSelectFileHtmlOnlyIndex = 1;
 const int kSelectFileCompleteIndex = 2;
+const int kSelectFileCompleteSingleFileIndex = 3;
 
 // Used for mapping between the IDS_ string identifiers and the indexes above.
 const int kIndexToIDS[] = {
-  0, IDS_SAVE_PAGE_DESC_HTML_ONLY, IDS_SAVE_PAGE_DESC_COMPLETE,
+  0,
+  IDS_SAVE_PAGE_DESC_HTML_ONLY,
+  IDS_SAVE_PAGE_DESC_COMPLETE,
+  IDS_SAVE_PAGE_DESC_COMPLETE_SINGLE_FILE,

[cbentzel, 2012/04/17 14:45:34]

Would be nice to call this MHTML to be consistent - enums such as SavePageType
indicate MHTML, whereas the descriptor here references COMPLETE_SINGLE_FILE.

[benjhayden, 2012/04/17 17:14:36]

Done.

 };
 
 }
@@ -83,31 +89,49 @@ SavePackageFilePicker::SavePackageFilePicker(
       extra_extension = suggested_path.Extension().substr(1);
     }
 
-    file_type_info.extensions.resize(2);
+    static const size_t kNumberExtensions = arraysize(kIndexToIDS) - 1;
+    file_type_info.extensions.resize(kNumberExtensions);

[cbentzel, 2012/04/17 14:45:34]

This resize and direct indexing is kinda gross. Yeah, it existed before but it
would be nice to clean up by just doing push_back's into the data structures.
Seems less prone to problems as well. 

[benjhayden, 2012/04/17 17:14:36]

If this used push_back and somebody re-ordered them, then FileSelected()'s use
of |index| would be broken.

[cbentzel, 2012/04/17 20:53:33]

I'm not too worried about that case, but it is reasonable. Could you at least
use a local to have the 0-based index to make this clearer?

+    file_type_info.extension_description_overrides.resize(kNumberExtensions);
+
+    // NOTE: indices into kIndexToIDS are 1-based where as indices into
+    // file_type_info.extensions are 0-based. Hence the '-1's.
+
+    file_type_info.extension_description_overrides[
+      kSelectFileHtmlOnlyIndex - 1] = l10n_util::GetStringUTF16(kIndexToIDS[
+          kSelectFileHtmlOnlyIndex]);
     file_type_info.extensions[kSelectFileHtmlOnlyIndex - 1].push_back(
         FILE_PATH_LITERAL("htm"));
     file_type_info.extensions[kSelectFileHtmlOnlyIndex - 1].push_back(
         FILE_PATH_LITERAL("html"));
-
     if (add_extra_extension) {
       file_type_info.extensions[kSelectFileHtmlOnlyIndex - 1].push_back(
           extra_extension);
     }
 
-    file_type_info.extension_description_overrides.push_back(
-        l10n_util::GetStringUTF16(kIndexToIDS[kSelectFileCompleteIndex - 1]));
+    file_type_info.extension_description_overrides[
+      kSelectFileCompleteIndex - 1] = l10n_util::GetStringUTF16(kIndexToIDS[
+          kSelectFileCompleteIndex]);
     file_type_info.extensions[kSelectFileCompleteIndex - 1].push_back(
         FILE_PATH_LITERAL("htm"));
     file_type_info.extensions[kSelectFileCompleteIndex - 1].push_back(
         FILE_PATH_LITERAL("html"));
-
     if (add_extra_extension) {
       file_type_info.extensions[kSelectFileCompleteIndex - 1].push_back(
           extra_extension);
     }
 
-    file_type_info.extension_description_overrides.push_back(
-        l10n_util::GetStringUTF16(kIndexToIDS[kSelectFileCompleteIndex]));
+    file_type_info.extension_description_overrides[
+      kSelectFileCompleteSingleFileIndex - 1] = l10n_util::GetStringUTF16(
+          kIndexToIDS[kSelectFileCompleteSingleFileIndex]);
+    file_type_info.extensions[kSelectFileCompleteSingleFileIndex - 1].push_back(
+        FILE_PATH_LITERAL("mht"));
+    file_type_info.extensions[kSelectFileCompleteSingleFileIndex - 1].push_back(
+        FILE_PATH_LITERAL("mhtml"));
+    if (add_extra_extension) {
+      file_type_info.extensions[
+        kSelectFileCompleteSingleFileIndex - 1].push_back(extra_extension);
+    }
+
     file_type_info.include_all_files = false;
   } else {
     file_type_info.extensions.resize(1);
@@ -127,7 +151,7 @@ SavePackageFilePicker::SavePackageFilePicker(
     select_file_dialog_ = SelectFileDialog::Create(this);
     select_file_dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,
                                     string16(),
-                                    suggested_path,
+                                    suggested_path.RemoveExtension(),

[cbentzel, 2012/04/17 14:45:34]

Why are you doing this here? Looks like different behavior from before.

[benjhayden, 2012/04/17 17:14:36]

Randy suggested doing this so that we could ensure that the extension for MHTML
files is 'mht'. Changing the save_type in the file picker dialog to mhtml does
not and can not change the extension of the suggested filename to mht, so it's
likely that users would accidentally save MHTML files with the default 'html'
extension. Chrome would not render such mis-named files correctly.
This is what gimp does when you first save new files, but not when you save
existing files.
I'm thinking about other ways of doing this, and I'm open to suggestions.

                                     &file_type_info,
                                     file_type_index,
                                     default_extension,
@@ -137,7 +161,8 @@ SavePackageFilePicker::SavePackageFilePicker(
                                     NULL);
   } else {
     // Just use 'suggested_path' instead of opening the dialog prompt.
-    callback.Run(suggested_path, kIndexToSaveType[file_type_index]);
+    // Go through FileSelected() to pick the extension back up.
+    FileSelected(suggested_path.RemoveExtension(), file_type_index, NULL);
   }
 }
 
@@ -153,11 +178,13 @@ void SavePackageFilePicker::FileSelected(const FilePath& path,
                                          void* params) {
   // The option index is not zero-based.
   DCHECK(index >= kSelectFileHtmlOnlyIndex &&
-         index <= kSelectFileCompleteIndex);
+         index <= kSelectFileCompleteSingleFileIndex);
 
   RenderProcessHost* process = RenderProcessHost::FromID(render_process_id_);
   if (process) {
     SavePageType save_type = kIndexToSaveType[index];
+    UMA_HISTOGRAM_ENUMERATION(
+        "Download.SavePageType", save_type, content::SAVE_PAGE_TYPE_MAX);
     Profile* profile =
         Profile::FromBrowserContext(process->GetBrowserContext());
     PrefService* prefs = profile->GetPrefs();
@@ -179,7 +206,14 @@ void SavePackageFilePicker::FileSelected(const FilePath& path,
       save_file_path.SetValue(path_string);
     }
 
-    callback_.Run(path, save_type);
+    FilePath mutable_path = path;

[cbentzel, 2012/04/17 14:45:34]

Why is this done here? Why not done before preparing the data for SelectFile()?
It seems like this would override what the user is intending for the final path
to be.

[benjhayden, 2012/04/17 17:14:36]

See my reply to your comment on L154 of this PS.

+    if (index == kSelectFileCompleteSingleFileIndex) {
+      mutable_path = path.ReplaceExtension(FILE_PATH_LITERAL("mht"));
+    } else {
+      mutable_path = path.ReplaceExtension(FILE_PATH_LITERAL("html"));
+    }
+
+    callback_.Run(mutable_path, save_type);
   }
 
   delete this;