Create VisualElementsManifest.xml from template -- install VisualElements in the version directory

chrome/installer/setup/install.cc

Index: chrome/installer/setup/install.cc
diff --git a/chrome/installer/setup/install.cc b/chrome/installer/setup/install.cc
index 2ceac7d014e88894a874b857919800e55d7a0409..7915940163839aa9483ad24008732ede7b87d174 100644
--- a/chrome/installer/setup/install.cc
+++ b/chrome/installer/setup/install.cc
@@ -6,6 +6,8 @@
 
 #include <shlobj.h>
 #include <time.h>
+
+#include <string>
 #include <vector>
 
 #include "base/command_line.h"
@@ -14,7 +16,9 @@
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
+#include "base/string16.h"
 #include "base/string_util.h"
+#include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
@@ -254,6 +258,76 @@ void RegisterChromeOnMachine(const InstallerState& installer_state,
   }
 }
 
+// Creates VisualElementsManifest.xml beside chrome.exe in |src_path| if
+// |src_path|\VisualElements exists.
+// Returns true unless the manifest is supposed to be created, but fails to be.
+bool CreateVisualElementsManifest(const FilePath& src_path,
+                                  const Version& new_version) {

[grt (UTC plus 2), 2012/05/03 17:49:37]

nit: within the context of this function, i think just "version" is appropriate
for this parameter.

[gab, 2012/05/03 19:03:48]

Done.

+
+  // Construct the relative path to the versioned VisualElements directory.
+  string16 elements_dir(ASCIIToUTF16(new_version.GetString()));
+  elements_dir.push_back(FilePath::kSeparators[0]);
+  elements_dir.append(installer::kVisualElements);
+
+  // Some distributions of Chromium may not include visual elements. Only
+  // proceed if this distribution does.
+  if (!file_util::PathExists(src_path.Append(elements_dir))) {
+    VLOG(1) << "No visual elements found, not writing "
+            << installer::kVisualElementsManifest << " to " << src_path.value();
+    return true;
+  } else {
+    // A printf_p-style format string for generating the visual elements
+    // manifest. Required arguments, in order, are:
+    //   - Localized display name for the product.
+    //   - Relative path to the VisualElements directory.
+    static const char kManifestTemplate[] =
+        "<Application>\r\n"
+        "  <VisualElements\r\n"
+        "      DisplayName='%1$ls'\r\n"
+        "      Logo='%2$ls\\Logo.png'\r\n"
+        "      SmallLogo='%2$ls\\SmallLogo.png'\r\n"
+        "      ForegroundText='light'\r\n"
+        "      BackgroundColor='white'>\r\n"
+        "    <DefaultTile ShowName='allLogos'/>\r\n"
+        "    <SplashScreen Image='%2$ls\\splash-620x300.png'/>\r\n"
+        "  </VisualElements>\r\n"
+        "</Application>";
+
+    const string16 manifest_template16(ASCIIToUTF16(kManifestTemplate));

[grt (UTC plus 2), 2012/05/03 17:49:37]

manifest_template16 -> manifest_template

[gab, 2012/05/03 19:03:48]

Done.

+
+    BrowserDistribution* dist = BrowserDistribution::GetSpecificDistribution(
+        BrowserDistribution::CHROME_BROWSER);
+    // TODO(grt): http://crbug.com/75152 Write a reference to a localized
+    // resource for |display_name|.
+    string16 display_name(dist->GetAppShortCutName());
+    // Escape the display name as per the XML AttValue production
+    // (http://www.w3.org/TR/2008/REC-xml-20081126/#NT-AttValue) for a value in
+    // single quotes.
+    ReplaceChars(display_name, L"&", L"&amp", &display_name);

[grt (UTC plus 2), 2012/05/03 17:49:37]

&amp -> &

[gab, 2012/05/03 19:03:48]

Done.

+    ReplaceChars(display_name, L"'", L"&apos", &display_name);

[grt (UTC plus 2), 2012/05/03 17:49:37]

&apos -> '

[gab, 2012/05/03 19:03:48]

Done.

+    ReplaceChars(display_name, L"<", L"&lt", &display_name);

[grt (UTC plus 2), 2012/05/03 17:49:37]

&lt -> <

[gab, 2012/05/03 19:03:48]

Done.

+
+    // Fill the manifest with the desired values.
+    string16 manifest16(base::StringPrintf(manifest_template16.c_str(),
+                                           display_name.c_str(),
+                                           elements_dir.c_str()));
+
+    // Write the manifest to |src_path|.
+    const std::string manifest(UTF16ToUTF8(manifest16));
+    if (file_util::WriteFile(
+            src_path.Append(installer::kVisualElementsManifest),
+            manifest.c_str(), manifest.size())) {
+      VLOG(1) << "Successfully wrote " << installer::kVisualElementsManifest
+              << " to " << src_path.value();
+      return true;
+    } else {
+      PLOG(ERROR) << "Error writing " << installer::kVisualElementsManifest
+                  << " to " << src_path.value();
+      return false;
+    }
+  }
+}
+
 // This function installs a new version of Chrome to the specified location.
 //
 // setup_path: Path to the executable (setup.exe) as it will be copied
@@ -377,6 +451,17 @@ InstallStatus InstallOrUpdateProduct(
     }
   }
 
+  // On Windows 8 and above: create VisualElementManifest.xml in |src_path| (if
+  // required) so that it looks as if it had been extracted from the archive
+  // when calling InstallNewVersion() below.
+  if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
+    installer_state.UpdateStage(installer::CREATING_VISUAL_MANIFEST);
+    if (!CreateVisualElementsManifest(src_path, new_version)) {
+      LOG(WARNING) << "Error creating " << kVisualElementsManifest
+                   << ". Proceeding with the install anyways...";

[grt (UTC plus 2), 2012/05/03 17:49:37]

the log message in CreateVisualElementsManifest is sufficient, no?

[gab, 2012/05/03 19:03:48]

I debated that. If we say the log is sufficient in CreateVisualElementsManifest
then that function does not need to return bool. Having a log here shows how we
handled the error reported by CreateVisualElementsManifest.

We could potentially decide in the future that we want to stop the install if
the function fails in which case we still want the error log inside
CreateVisualElementsManifest, but need to change this part of the code.

i.e. this separates concerns:
CreateVisualElementsManifest => reports error writing file
InstallOrUpdateProduct => reports how it proceeded given the error

Let me know if you still think I should proceed otherwise (and thus probably
make CreateVisualElementsManifest return void).

[grt (UTC plus 2), 2012/05/03 19:37:58]

It doesn't need to return bool in the current use, but it's a nice interface.
I argue that it adds nothing but noise to the log file.  The previous line would
have detailed error information, and the next line would make it clear that the
installation was proceeding anyway.  That aside, anyone reading the log can (and
will) consult the code.  The log is meant to be used in conjunction with the
code to diagnose problems.
I say leave it as-is and remove the logging here.  The func returning bool is a
good interface.  That doesn't mean that the caller must take action based on the
return value.

[gab, 2012/05/03 19:50:24]

Done.

+    }
+  }
+
   scoped_ptr<Version> existing_version;
   InstallStatus result = InstallNewVersion(original_state, installer_state,
       setup_path, archive_path, src_path, install_temp_path, new_version,