Allow installer::GetLocalizedString to return mode-specific strings.

chrome/installer/util/prebuild/create_string_rc.py

Index: chrome/installer/util/prebuild/create_string_rc.py
diff --git a/chrome/installer/util/prebuild/create_string_rc.py b/chrome/installer/util/prebuild/create_string_rc.py
index 9f73dacbfc2857f64f95d9245bf4b3ca43fe9fbd..05bca8f4b995c092de47a947feab3b35609a855c 100755
--- a/chrome/installer/util/prebuild/create_string_rc.py
+++ b/chrome/installer/util/prebuild/create_string_rc.py
@@ -45,32 +45,98 @@ from grit.extern import tclib
 # The IDs of strings we want to import from the .grd files and include in
 # setup.exe's resources.

[manzagop (departed), 2017/03/31 16:19:01]

nit: is it common practice to mention the alphabetical ordering?

[grt (UTC plus 2), 2017/04/03 11:59:42]

There's no technical requirement, so I don't think so. We generally sort things
without explicit comments (e.g., header inclusion in C++ files and various lists
in BUILD.gn files). I don't know that this is sufficiently "special" to need a
"keep this sorted" comment.

 STRING_IDS = [
-  'IDS_PRODUCT_NAME',
-  'IDS_SXS_SHORTCUT_NAME',
-  'IDS_PRODUCT_DESCRIPTION',
   'IDS_ABOUT_VERSION_COMPANY_NAME',
-  'IDS_INSTALL_HIGHER_VERSION',
+  'IDS_APP_SHORTCUTS_SUBDIR_NAME',
+  'IDS_APP_SHORTCUTS_SUBDIR_NAME_CANARY',
+  'IDS_INBOUND_MDNS_RULE_DESCRIPTION',
+  'IDS_INBOUND_MDNS_RULE_DESCRIPTION_CANARY',
+  'IDS_INBOUND_MDNS_RULE_NAME',
+  'IDS_INBOUND_MDNS_RULE_NAME_CANARY',
+  'IDS_INSTALL_EXISTING_VERSION_LAUNCHED',
   'IDS_INSTALL_FAILED',
-  'IDS_SAME_VERSION_REPAIR_FAILED',
-  'IDS_SETUP_PATCH_FAILED',
-  'IDS_INSTALL_OS_NOT_SUPPORTED',
+  'IDS_INSTALL_HIGHER_VERSION',
+  'IDS_INSTALL_INSUFFICIENT_RIGHTS',
+  'IDS_INSTALL_INVALID_ARCHIVE',
   'IDS_INSTALL_OS_ERROR',
+  'IDS_INSTALL_OS_NOT_SUPPORTED',
   'IDS_INSTALL_SINGLETON_ACQUISITION_FAILED',
   'IDS_INSTALL_TEMP_DIR_FAILED',
   'IDS_INSTALL_UNCOMPRESSION_FAILED',
-  'IDS_INSTALL_INVALID_ARCHIVE',
-  'IDS_INSTALL_INSUFFICIENT_RIGHTS',
-  'IDS_SHORTCUT_TOOLTIP',
+  'IDS_PRODUCT_DESCRIPTION',
+  'IDS_PRODUCT_NAME',
+  'IDS_SAME_VERSION_REPAIR_FAILED',
+  'IDS_SETUP_PATCH_FAILED',
   'IDS_SHORTCUT_NEW_WINDOW',
-  'IDS_APP_SHORTCUTS_SUBDIR_NAME',
-  'IDS_APP_SHORTCUTS_SUBDIR_NAME_CANARY',
-  'IDS_INBOUND_MDNS_RULE_NAME',
-  'IDS_INBOUND_MDNS_RULE_NAME_CANARY',
-  'IDS_INBOUND_MDNS_RULE_DESCRIPTION',
-  'IDS_INBOUND_MDNS_RULE_DESCRIPTION_CANARY',
-  'IDS_INSTALL_EXISTING_VERSION_LAUNCHED',
+  'IDS_SHORTCUT_TOOLTIP',
+  'IDS_SXS_SHORTCUT_NAME',
 ]
 
+# Certain strings are conditional on a brand's install mode (see
+# chrome/install_static/install_modes.h for details). This allows
+# installer::GetLocalizedString to return a resource specific to the current
+# install mode at runtime (e.g., "Google Chrome SxS" as IDS_SHORTCUT_NAME for
+# the localized shortcut name for Google Chrome's canary channel). This mapping
+# provides brand- and mode-specific string ids for a given input id as described
+# here:
+# {
+#   resource_id_1: {  # A resource ID for use with GetLocalizedString.
+#     brand: [  # 'google_chrome', for example.
+#       string_id_1,  # Strings listed in order of the brand's modes, as

[manzagop (departed), 2017/03/31 16:19:00]

Add a comment in the mode file that this needs to be changed if the order
changes?

[grt (UTC plus 2), 2017/04/03 11:59:42]

Done.

+#       string_id_2,  # specified in install_static::InstallConstantIndex.
+#       ...
+#       string_id_N,
+#     ],
+#     '': [  # Default brand fallback (Chromium).
+#       string_id_1,
+#     ],
+#   },
+#   resource_id_2:  ...
+# }
+# 'resource_id_1' may name an existing string ID or may be distinct. In the
+# former case, all calls to installer::GetLocalizedString with the existing
+# string ID will map to the mode-specific string. It becomes impossible to get
+# the general ("primary") variant of the string when a secondary install mode
+# is in use. By using a distinct ID, both the mode-specific and the general can
+# be retrieved as needed.

[manzagop (departed), 2017/03/31 16:19:01]

(rambling)

IIUC we have string ids, and a layer of indirection on top. The indirection can
be through:
- a string id, in which case we lose the ability to access exactly that string
id directly
- a pseudo string id

I can think of a simpler option: always use pseudo ids in our code, which
indirect to concrete string ids which are not visible to our code. (IE single
purpose pseudo string ids.)

Is the hybrid solution a desired end-state (because it supports X) or is it a
transitional convenience that (we'd want to move away from later)?

Hm. I guess we'd have lots useless identity mappings if always using pseudo
strings.

[grt (UTC plus 2), 2017/04/03 11:59:42]

I introduced the pseudo-id to deal with IDS_PRODUCT_NAME. It is used throughout
the product for various things that are independent of the install mode. The
installer, however, only uses it for the mode-specific shortcut name. Things I
considered:

- Introduce IDS_SHORTCUT_NAME for use by the installer. On the positive side,
all modes would use this for the shortcut. On the negative side, it would
duplicate an existing string across all translations.
- Map IDS_PRODUCT_NAME to (IDS_PRODUCT_NAME, IDS_SXS_SHORTCUT_NAME). Positive:
we can remove the concept of a pseudo string id. Negative: IDS_PRODUCT_NAME is
subtly different between the installer and the rest of Chrome.

I thought more about this after sending this CL up for review. I'm leaning
toward the second option above, as all uses of IDS_PRODUCT_NAME in the installer
really are asking for the mode-specific string. I would annotate these callsites
with comments pointing out this distinction.
I was hoping to only do the mapping for strings that required it.

[grt (UTC plus 2), 2017/04/03 11:59:42]

The hybrid was just something I dreamed up while trying to preserve
IDS_PRODUCT_NAME as a general purpose identifier for the product (constant
across all modes) that serves double-duty as the shortcut name for the primary
install, and IDS_SXS_SHORTCUT_NAME (a mode-specific shortcut name) that serves
as the shortcut name for a particular install mode.

Alternatives:
- introduce IDS_SHORTCUT_NAME for the primary mode's shortcut. It would use the
same value and translations as IDS_PRODUCT_NAME, so we'd have duplication in the
resource files for all translations.
- replace the pseudo id IDS_SHORTCUT_NAME with IDS_PRODUCT_NAME, as the only use
in the installer (where it matters) is to get the shortcut name.
Yeah. I didn't want to make extra hoops for all strings; just for the special
ones.

+MODE_SPECIFIC_STRINGS = {
+  'IDS_APP_SHORTCUTS_SUBDIR_NAME': {
+    'google_chrome': [

[manzagop (departed), 2017/03/31 16:19:01]

nit: use a constant? For the default brand too?

[grt (UTC plus 2), 2017/04/03 11:59:42]

This matches the -b BRAND value provided on the command line. I've added
parameter validation and some extra error handling. I've also removed the
automatic fallback to avoid silent fallbacks to chromium upon misspellings.

[manzagop (departed), 2017/04/03 13:06:33]

Oh, I meant introduce constants GOOGLE_BRAND and CHROMIUM_BRAND to reduce the
chance of typos as each string occurs a few times in the map.

[grt (UTC plus 2), 2017/04/03 13:31:30]

Okay. The new validation I added should catch any such typos.

+      'IDS_APP_SHORTCUTS_SUBDIR_NAME',
+      'IDS_APP_SHORTCUTS_SUBDIR_NAME_CANARY',
+    ],
+    '': [
+      'IDS_APP_SHORTCUTS_SUBDIR_NAME',
+    ],
+  },
+  'IDS_INBOUND_MDNS_RULE_DESCRIPTION': {
+    'google_chrome': [
+      'IDS_INBOUND_MDNS_RULE_DESCRIPTION',
+      'IDS_INBOUND_MDNS_RULE_DESCRIPTION_CANARY',
+    ],
+    '': [
+      'IDS_INBOUND_MDNS_RULE_DESCRIPTION',
+    ],
+  },
+  'IDS_INBOUND_MDNS_RULE_NAME': {
+    'google_chrome': [
+      'IDS_INBOUND_MDNS_RULE_NAME',
+      'IDS_INBOUND_MDNS_RULE_NAME_CANARY',
+    ],
+    '': [
+      'IDS_INBOUND_MDNS_RULE_NAME',
+    ],
+  },
+  'IDS_SHORTCUT_NAME': {
+    'google_chrome': [
+      'IDS_PRODUCT_NAME',

[manzagop (departed), 2017/03/31 16:19:01]

Can you speak to the tradeoffs of introducing a new IDS_SHORTCUT_NAME string
with the same value?

(Sorry, I'm having trouble wrapping my head around how we do this. Some strings
are for concepts (e.g. the product name), some are for purposes (shortcut), some
need to be mode-adapted, etc...)

[grt (UTC plus 2), 2017/04/03 11:59:42]

Does my response above address this question?

[manzagop (departed), 2017/04/03 13:06:33]

Yep! Thanks.

+      'IDS_SXS_SHORTCUT_NAME',
+    ],
+    '': [
+      'IDS_PRODUCT_NAME',
+    ],
+  },
+}
+
 # The ID of the first resource string.
 FIRST_RESOURCE_ID = 1600
 
@@ -215,8 +281,8 @@ class XtbHandler(sax.handler.ContentHandler):
 
 class StringRcMaker(object):
   """Makes .h and .rc files containing strings and translations."""
-  def __init__(self, name, inputs, outdir):
-    """Constructs a maker.
+  def __init__(self, name, inputs, outdir, brand):
+    """constructs a maker.

[manzagop (departed), 2017/03/31 16:19:01]

nitty question: no capital letter?

[grt (UTC plus 2), 2017/04/03 11:59:42]

I wasn't aware I'd changed that. Oops.

 
     Args:
       name: The base name of the generated files (e.g.,
@@ -227,6 +293,7 @@ class StringRcMaker(object):
     self.name = name
     self.inputs = inputs
     self.outdir = outdir
+    self.brand = brand
 
   def MakeFiles(self):
     translated_strings = self.__ReadSourceAndTranslatedStrings()
@@ -347,6 +414,7 @@ class StringRcMaker(object):
     lines = []
     do_languages_lines = ['\n#define DO_LANGUAGES']
     installer_string_mapping_lines = ['\n#define DO_INSTALLER_STRING_MAPPING']
+    do_mode_strings_lines = ['\n#define DO_MODE_STRINGS']
 
     # Write the values for how the languages ids are offset.
     seen_languages = set()
@@ -370,6 +438,18 @@ class StringRcMaker(object):
                                       resource_id))
       resource_id += 1
 
+    # Handle mode-specific strings.
+    for string_id, brands in MODE_SPECIFIC_STRINGS.iteritems():
+      # Write the synthetic resource ids for per-mode strings.
+      if string_id not in STRING_IDS:
+        lines.append('#define %s_BASE %s' % (string_id, resource_id))
+        resource_id += 1
+      # Populate the DO_MODE_STRINGS macro.
+      brand_strings = brands.get(self.brand, brands[''])
+      do_mode_strings_lines.append(
+        '  HANDLE_MODE_STRING(%s_BASE, %s)'
+        % (string_id, ', '.join([ ('%s_BASE' % s) for s in brand_strings])))

[manzagop (departed), 2017/03/31 16:19:00]

Should we validate that no brand string is a pseudo string?

[grt (UTC plus 2), 2017/04/03 11:59:42]

noop -- i've removed psuedo strings

+
     # Write out base ID values.
     for string_id in STRING_IDS:
       lines.append('#define %s_BASE %s_%s' % (string_id,
@@ -383,6 +463,7 @@ class StringRcMaker(object):
       outfile.write('\n#ifndef RC_INVOKED')
       outfile.write(' \\\n'.join(do_languages_lines))
       outfile.write(' \\\n'.join(installer_string_mapping_lines))
+      outfile.write(' \\\n'.join(do_mode_strings_lines))
       # .rc files must end in a new line
       outfile.write('\n#endif  // ndef RC_INVOKED\n')
 
@@ -398,6 +479,10 @@ def ParseCommandLine():
 
   parser = argparse.ArgumentParser(
     description='Generate .h and .rc files for installer strings.')
+  parser.add_argument('-b',
+                      required=True,
+                      help='identifier of the browser brand (e.g., chromium).',
+                      dest='brand')
   parser.add_argument('-i', action='append',
                       type=GrdPathAndXtbDirPair,
                       required=True,
@@ -417,7 +502,7 @@ def ParseCommandLine():
 
 def main():
   args = ParseCommandLine()
-  StringRcMaker(args.name, args.inputs, args.outdir).MakeFiles()
+  StringRcMaker(args.name, args.inputs, args.outdir, args.brand).MakeFiles()
   return 0