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

chrome/installer/setup/install_worker.cc

Index: chrome/installer/setup/install_worker.cc
diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc
index cd17feea47e150abf09e29bb9bc847b780027d0f..737fdc35b00044d0f2ceb40be56ff2f7e4268f83 100644
--- a/chrome/installer/setup/install_worker.cc
+++ b/chrome/installer/setup/install_worker.cc
@@ -9,6 +9,7 @@
 
 #include <oaidl.h>
 #include <shlobj.h>
+#include <string>

[grt (UTC plus 2), 2012/05/01 04:17:03]

style: c++ headers follow c headers, so move this to just above <vector>.  i
like to separate the two groups with a newline, but i could be anti-style when i
do so.

[gab, 2012/05/01 22:19:02]

I agree that a space makes it clearer (and I'm pretty sure I wouldn't have made
the mistake to misplace the new include in the first place had they been
separated originally).

 #include <time.h>
 #include <vector>
 
@@ -17,7 +18,9 @@
 #include "base/file_util.h"
 #include "base/logging.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/version.h"
 #include "base/win/registry.h"
@@ -42,6 +45,76 @@
 
 using base::win::RegKey;
 
+namespace {
+
+// Elements that make up install target paths.
+const wchar_t kDictionaries[] = L"Dictionaries";
+const wchar_t kImages[] = L"Images";
+const wchar_t kLogoImage[] = L"Logo.png";
+const wchar_t kSmallLogoImage[] = L"SmallLogo.png";
+const wchar_t kSplashScreenImage[] = L"splash-620x300.png";
+const wchar_t kVisualElementsManifest[] = L"VisualElementsManifest.xml";
+const wchar_t kWowHelperExe[] = L"wow_helper.exe";
+
+// Adds work items to |install_list| to lay down a version specific
+// VisualElementsManifest.xml beside chrome.exe.
+void AddVisualElementsInstallWorkItems(const FilePath& src_path,

[grt (UTC plus 2), 2012/05/01 04:17:03]

i think this should be split up into two parts.  here's what i envision:

the first part should just write the file out and return true/false accordingly.
 i think this should live in install.cc and be called early in that file's
InstallOrUpdateProduct (before the call to InstallNewVersion).  it should be
possible to write a unit test for this new function.

is the manifest required for metro Chrome to work at all?  if so, do we want to
abort installation if we can't make the manifest?  in this case, we should have
a new InstallStatus constant in util_constants.h for "failed to generate visual
elements manifest" or perhaps a more generic "failed to generate installable
files" so we can see an error when this fails.

we also want to know if the installer outright crashes or times out during this
operation.  for that, we need a corresponding InstallerStage constant and a call
to installer_state.UpdateStage() in InstallOrUpdateProduct just before calling
this new function.

the second part should be more or less what was previously in
AddInstallWorkItems: if the manifest is present in src_path, add a work item to
move it into the right place.  no magic there.

[gab, 2012/05/02 20:55:58]

Done. unit test to come in next patch set.
We do not need to abort if the manifest fails to be created (nor do we want to..
as discussed with Robert, it appears we don't stop when failing to create
shortcuts either which is a similar case). The manifest not being present simply
means we get the default icons instead of Metro tiles (as if we were non-default
browser), but with the full functionality (including metro launch) of the
default browser.
Done. Is it normal that there isn't an InstallerStage for InstallNewVersion()? I
guess we are only adding work items in it, not executing them, but if it somehow
fails the stage will be on CREATING_VISUAL_MANIFEST in the current code.
Some magic actually, need to delete it if it's not present in the update (could
be an error creating it in the archive_dir). Otherwise we keep an old manifest
pointing to an old version dir bound to be deleted =S!

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

it's inside InstallNewVersion.
excellent point.

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

Oh, how did I miss that...! I'll change CreateVisualElementsManifest to do this
too.

+                                       const FilePath& target_path,
+                                       const FilePath& temp_path,
+                                       const Version& new_version,
+                                       WorkItemList* install_list) {
+  DCHECK(install_list);
+
+  string16 images_dir = ASCIIToUTF16(new_version.GetString());

[grt (UTC plus 2), 2012/05/01 04:17:03]

i'm told that this is the encouraged style rather than using =
  string16 images_dir(ASCIIToUTF16(new_version.GetString()));
please switch to that for everything other than the fundamental types (ints,
pointers, etc)

[gab, 2012/05/01 22:19:02]

Done.

[gab, 2012/05/01 22:19:02]

Done.

+  images_dir.push_back(FilePath::kSeparators[0]);
+  images_dir.append(kImages);
+  images_dir.push_back(FilePath::kSeparators[0]);
+
+  // Some distributions of Chromium may not include visual elements. Only
+  // proceed if this distribution does.
+  if (!file_util::PathExists(src_path.Append(images_dir + kLogoImage))) {
+    LOG(WARNING) << "No visual elements found, skipping related work items.";

[grt (UTC plus 2), 2012/05/01 04:17:03]

is VLOG(1) more appropriate here, or is this really something worth warning us
about?

[gab, 2012/05/01 22:19:02]

Technically all distributions should have visual elements imo, but chrome.exe
itself can work without them (so it's not an error if they're absent, but should
be brought to the attention; as discussed verbose logging should be sufficient).

+  } else {
+    const wchar_t manifest_template[] =

[grt (UTC plus 2), 2012/05/01 04:17:03]

i think it'd be helpful to put a comment above the template listing what the
expected inputs are; e.g. (you might want to read my other comments first):
// A printf_p-style format string for generating the visual elements manifest.
// - localized display name for the product
// - version number

[grt (UTC plus 2), 2012/05/01 04:17:03]

- make this static to be sure that uses of it are pointing straight into the
.rdata segment
- use char (and ASCIIToUTF16 below) rather than wchar_t in the interest of
saving bytes

[gab, 2012/05/01 22:19:02]

Done.

[gab, 2012/05/01 22:19:02]

Done.
Done.

+        L"<Application\r\n"
+        L"  xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\">\r\n"

[grt (UTC plus 2), 2012/05/01 04:17:03]

remove this namespace decl since it isn't used.

[gab, 2012/05/01 22:19:02]

Done.

+        L"  <VisualElements\r\n"
+        L"    DisplayName=\"%ls\"\r\n"

[grt (UTC plus 2), 2012/05/01 04:17:03]

since the attributes are conceptually a continuation of the opening of the
element, i think it's consistent with Chromium style to do four-space indent
here.

[grt (UTC plus 2), 2012/05/01 04:17:03]

i think the template will look cleaner with single-quotes around the attribute
values rather than escaped double-quotes.  for example:
       L"    DisplayName='%ls'\r\n"

[gab, 2012/05/01 22:19:02]

Makes sense, done.

[gab, 2012/05/01 22:19:02]

Didn't know that XML didn't care, good to know, and I definitely agree :)!

+        L"    Logo=\"%ls\"\r\n"

[grt (UTC plus 2), 2012/05/01 04:17:03]

the bits underlying StringPrintf appear to support positional parameters.  if
this is true, i think something like this should work:
        L"      Logo='%2$ls\\Images\\Logo.png'\r\n"
        L"      SmallLogo='%2$ls\\Images\\SmallLogo.png'\r\n"
...
        L"    <SplashScreen Image='%2$ls\\Images\\splash-620x300.png'/>\r\n"
...
string16 manifest(base::StringPrintf(ASCIIToUTF16(manifest_template),
    display_name.c_str(),
    ASCIIToUTF16(new_version.GetString()).c_str());

[gab, 2012/05/02 20:55:58]

This works, done in previous patchset.

+        L"    SmallLogo=\"%ls\"\r\n"
+        L"    ForegroundText=\"light\"\r\n"
+        L"    BackgroundColor=\"white\">\r\n"
+        L"    <DefaultTile ShowName=\"allLogos\"/>\r\n"

[grt (UTC plus 2), 2012/05/01 04:17:03]

the two-space indent is good here and on the next line.

[gab, 2012/05/01 22:19:02]

Right.

+        L"    <SplashScreen Image=\"%ls\"/>\r\n"
+        L"  </VisualElements>\r\n"
+        L"</Application>";
+
+    // Fill the manifest with the desired values.
+    string16 manifest;
+    BrowserDistribution* dist = BrowserDistribution::GetDistribution();

[grt (UTC plus 2), 2012/05/01 04:17:03]

this will return the distribution of whatever product happens to be getting
installed.  in the land of multi-install, this is rarely what we want in the
installer itself (since it could be installing Chrome, Chrome Frame, or both). 
for a Chrome Frame installation, I think it's a good idea to lay down the
manifest in support of a future Chrome Frame -> Chrome quick-enable scenario. 
since the manifest only applies to Chrome, change this to:
    BrowserDistribution* dist = BrowserDistribution::GetSpecificDistribution(
        BrowserDistribution::CHROME_BROWSER);
i can explain in person tomorrow if this doesn't make any sense.  :-)

[gab, 2012/05/01 22:19:02]

Wouldn't that trigger Chromium to have display name "Google Chrome"...?!

[grt (UTC plus 2), 2012/05/02 15:09:43]

No.  The base BrowserDistribution class (rather than the
GoogleChromeBrowserDistribution class) will be selected in that case.

[gab, 2012/05/02 20:55:58]

Ah, I see, CHROME_BROWSER != GOOGLE_CHROME :)!

Done.

+    const string16 display_name = dist->GetAppShortCutName();

[grt (UTC plus 2), 2012/05/01 04:17:03]

XML attribute values may not contain '<' or '&'.  to be safe, this value should
be run through a filter that converts all '&' into "&amp;" and all '<' into
"&lt;".

[grt (UTC plus 2), 2012/05/01 04:23:24]

oh yeah:
// TODO(grt): http://crbug.com/75152 Write a reference to a localized resource.
unless you'd like it to be TODO(gab)!

[gab, 2012/05/01 22:19:02]

Done.

I think however fixes the localization issue across the installer should also
fix this. Given all the TODOs are under your name for now, I'll also put this
one under your name. Whoever ends up owning bug #75152 can take ownership of the
TODOs.

[gab, 2012/05/01 22:19:02]

Done. Not sure where you found documentation saying there was no need to escape
'>' in this case, but I'll take your word for it.

[grt (UTC plus 2), 2012/05/02 15:09:43]

http://www.w3.org/TR/2008/REC-xml-20081126/#NT-AttValue

[gab, 2012/05/02 20:55:58]

Great, thanks, added it to the comment.

+    const string16 logo_path = images_dir + kLogoImage;
+    const string16 small_logo_path = images_dir + kSmallLogoImage;
+    const string16 splash_path = images_dir + kSplashScreenImage;
+    base::SStringPrintf(&manifest, manifest_template, display_name, logo_path,

[grt (UTC plus 2), 2012/05/01 04:17:03]

is there a benefit to using SStringPrintf over StringPrintf?  i think the latter
is more clear since the first arg isn't an out param (which looks odd in
Chromium code).

[gab, 2012/05/01 22:19:02]

Looks like I overlooked the code and assumed StringPrintf was just like printf
(i.e. printing to stdout). I definitely also prefer StringPrintf (i.e. with no
out param).

+                        small_logo_path, splash_path);
+
+    // Write the manifest to the directory where the archive was extracted.
+    const std::string manifest_utf8 = UTF16ToUTF8(manifest);
+    file_util::WriteFile(src_path.Append(kVisualElementsManifest),

[grt (UTC plus 2), 2012/05/01 04:17:03]

check the return value and, in case of error, log it and return false.

[gab, 2012/05/01 22:19:02]

Done.

+                         manifest_utf8.c_str(), manifest_utf8.size());
+
+    // Add a work item to move the new manifest to |target_path|.
+    install_list->AddMoveTreeWorkItem(
+        src_path.Append(kVisualElementsManifest).value(),
+        target_path.Append(kVisualElementsManifest).value(),
+        temp_path.value(),
+        WorkItem::ALWAYS_MOVE);
+  }
+}
+
+}  // namespace
+
 namespace installer {
 
 // Local helper to call AddRegisterComDllWorkItems for all DLLs in a set of
@@ -769,41 +842,15 @@ void AddInstallWorkItems(const InstallationState& original_state,
       base::win::OSInfo::WOW64_DISABLED &&
       base::win::GetVersion() <= base::win::VERSION_VISTA) {
     install_list->AddMoveTreeWorkItem(
-        src_path.Append(installer::kWowHelperExe).value(),
-        target_path.Append(installer::kWowHelperExe).value(),
+        src_path.Append(kWowHelperExe).value(),
+        target_path.Append(kWowHelperExe).value(),
         temp_path.value(),
         WorkItem::ALWAYS_MOVE);
   }
 
   if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
-    // Desktop only (i.e. not supporting Metro) versions of Chromium do not have
-    // to include visual elements.
-    scoped_ptr<WorkItemList> win8_work_items(
-        WorkItem::CreateConditionalWorkItemList(new ConditionRunIfFileExists(
-            src_path.Append(L"visualelementsmanifest.xml"))));
-    // TODO (gab): All of these hard-coded strings are temporary and will be
-    // deleted in the patch following this one.
-    win8_work_items->AddMoveTreeWorkItem(
-        src_path.Append(L"visualelementsmanifest.xml").value(),
-        target_path.Append(L"visualelementsmanifest.xml").value(),
-        temp_path.value(),
-        WorkItem::ALWAYS_MOVE);
-    win8_work_items->AddMoveTreeWorkItem(
-        src_path.Append(L"logo.png").value(),
-        target_path.Append(L"logo.png").value(),
-        temp_path.value(),
-        WorkItem::ALWAYS_MOVE);
-    win8_work_items->AddMoveTreeWorkItem(
-        src_path.Append(L"smalllogo.png").value(),
-        target_path.Append(L"smalllogo.png").value(),
-        temp_path.value(),
-        WorkItem::ALWAYS_MOVE);
-    win8_work_items->AddMoveTreeWorkItem(
-        src_path.Append(L"splash-620x300.png").value(),
-        target_path.Append(L"splash-620x300.png").value(),
-        temp_path.value(),
-        WorkItem::ALWAYS_MOVE);
-    install_list->AddWorkItem(win8_work_items.release());
+    AddVisualElementsInstallWorkItems(src_path, target_path, temp_path,
+                                      new_version, install_list);
   }
 
   // In the past, we copied rather than moved for system level installs so that
@@ -826,8 +873,8 @@ void AddInstallWorkItems(const InstallationState& original_state,
   // TODO(grt): Use AddMoveTreeWorkItem in a conditional WorkItemList, which
   // will be more efficient in space and time.
   install_list->AddCopyTreeWorkItem(
-      src_path.Append(installer::kDictionaries).value(),
-      target_path.Append(installer::kDictionaries).value(),
+      src_path.Append(kDictionaries).value(),
+      target_path.Append(kDictionaries).value(),
       temp_path.value(), WorkItem::IF_NOT_PRESENT);
 
   // Delete any old_chrome.exe if present (ignore failure if it's in use).