Save Page As MHTML

chrome/browser/download/save_package_file_picker.cc

 // Copyright (c) 2011 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/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"
 #include "chrome/browser/prefs/pref_member.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/save_page_type.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::RenderProcessHost;
 using content::SavePageType;
 using content::WebContents;
 
 namespace {
 
 // If false, we don't prompt the user as to where to save the file.  This
 // exists only for testing.
 bool g_should_prompt_for_filename = true;
 
 // Used for mapping between SavePageType constants and the indexes above.
 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) {
   for (size_t i = 0; i < arraysize(kIndexToSaveType); ++i) {
     if (kIndexToSaveType[i] == type)
       return i;
   }
   NOTREACHED();
   return -1;
 }
 
 // Indexes used for specifying which element in the extensions dropdown
 // 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.

 };
 
 }
 
 SavePackageFilePicker::SavePackageFilePicker(
     content::WebContents* web_contents,
     const FilePath& suggested_path,
     const FilePath::StringType& default_extension,
     bool can_save_as_complete,
     DownloadPrefs* download_prefs,
     content::SaveFilePathPickedCallback callback)
     : render_process_id_(web_contents->GetRenderProcessHost()->GetID()),
       callback_(callback) {
   int file_type_index = SavePackageTypeToIndex(
       static_cast<SavePageType>(download_prefs->save_file_type()));
   DCHECK_NE(-1, file_type_index);
 
   SelectFileDialog::FileTypeInfo file_type_info;
 
   // If the contents can not be saved as complete-HTML, do not show the
   // file filters.
   if (can_save_as_complete) {

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

When is |can_save_as_complete| false? Should we be doing MHTML in that case?

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

can_save_as_complete = ((page_mime == "text/html") || (page_mime ==
"application/xhtml+xml"))

Can you think of any other mimes that we might want to save as mhtml?

     bool add_extra_extension = false;
     FilePath::StringType extra_extension;
     if (!suggested_path.Extension().empty() &&
         suggested_path.Extension().compare(FILE_PATH_LITERAL("htm")) &&
         suggested_path.Extension().compare(FILE_PATH_LITERAL("html"))) {
       add_extra_extension = true;
       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);
     file_type_info.extensions[kSelectFileHtmlOnlyIndex - 1].push_back(
         suggested_path.Extension());
 
     if (!file_type_info.extensions[kSelectFileHtmlOnlyIndex - 1][0].empty()) {
       // Drop the .
       file_type_info.extensions[kSelectFileHtmlOnlyIndex - 1][0].erase(0, 1);
     }
 
     file_type_info.include_all_files = true;
     file_type_index = 1;
   }
 
   if (g_should_prompt_for_filename) {
     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,
                                     web_contents,
                                     platform_util::GetTopLevel(
                                         web_contents->GetNativeView()),
                                     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);
   }
 }
 
 SavePackageFilePicker::~SavePackageFilePicker() {
 }
 
 void SavePackageFilePicker::SetShouldPromptUser(bool should_prompt) {
   g_should_prompt_for_filename = should_prompt;
 }
 
 void SavePackageFilePicker::FileSelected(const FilePath& path,
                                          int index,
                                          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();
     if (select_file_dialog_ &&
         select_file_dialog_->HasMultipleFileTypeChoices())
       prefs->SetInteger(prefs::kSaveFileType, save_type);
 
     StringPrefMember save_file_path;
     save_file_path.Init(prefs::kSaveFileDefaultDirectory, prefs, NULL);
 #if defined(OS_POSIX)
     std::string path_string = path.DirName().value();
 #elif defined(OS_WIN)
     std::string path_string = WideToUTF8(path.DirName().value());
 #endif
     // If user change the default saving directory, we will remember it just
     // like IE and FireFox.
     if (!process->GetBrowserContext()->IsOffTheRecord() &&
         save_file_path.GetValue() != path_string) {
       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;
 }
 
 void SavePackageFilePicker::FileSelectionCanceled(void* params) {
   delete this;
 }