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

chrome/installer/util/prebuild/create_string_rc.py

 #!/usr/bin/env python
 # Copyright (c) 2012 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.
 
 """Generates .h and .rc files for installer strings. Run "python
 create_string_rc.py" for usage details.
 
 This script generates an rc file and header (NAME.{rc,h}) to be included in
 setup.exe. The rc file includes translations for strings pulled from the given
@@ skipping 25 matching lines @@
 import sys
 from xml import sax
 
 BASEDIR = os.path.dirname(os.path.abspath(__file__))
 sys.path.append(os.path.join(BASEDIR, '../../../../tools/grit'))
 sys.path.append(os.path.join(BASEDIR, '../../../../tools/python'))
 
 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_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_INSTALL_HIGHER_VERSION',
-  'IDS_INSTALL_FAILED',
-  'IDS_SAME_VERSION_REPAIR_FAILED',
-  'IDS_SETUP_PATCH_FAILED',
+  'IDS_INSTALL_INSUFFICIENT_RIGHTS',
+  'IDS_INSTALL_INVALID_ARCHIVE',
+  'IDS_INSTALL_OS_ERROR',
   'IDS_INSTALL_OS_NOT_SUPPORTED',
-  'IDS_INSTALL_OS_ERROR',
   'IDS_INSTALL_SINGLETON_ACQUISITION_FAILED',
   'IDS_INSTALL_TEMP_DIR_FAILED',
   'IDS_INSTALL_UNCOMPRESSION_FAILED',
-  'IDS_INSTALL_INVALID_ARCHIVE',
-  'IDS_INSTALL_INSUFFICIENT_RIGHTS',
+  'IDS_PRODUCT_DESCRIPTION',
+  'IDS_PRODUCT_NAME',
+  'IDS_SAME_VERSION_REPAIR_FAILED',
+  'IDS_SETUP_PATCH_FAILED',
+  'IDS_SHORTCUT_NEW_WINDOW',
   'IDS_SHORTCUT_TOOLTIP',
-  '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_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
 
 
 class GrdHandler(sax.handler.ContentHandler):
   """Extracts selected strings from a .grd file.
 
   Attributes:
     messages: A dict mapping string identifiers to their corresponding messages.
   """
@@ skipping 124 matching lines @@
       translated_string = ''.join(self.__text_scraps).strip()
       for string_id in self.__string_ids:
         self.translations[string_id] = translated_string
       self.__string_ids = None
       self.__text_scraps = []
       self.__characters_callback = None
 
 
 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.,
         'installer_util_strings').
       inputs: A list of (grd_file, xtb_dir) pairs containing the source data.
       outdir: The directory into which the files will be generated.
     """
     self.name = name
     self.inputs = inputs
     self.outdir = outdir
+    self.brand = brand
 
   def MakeFiles(self):
     translated_strings = self.__ReadSourceAndTranslatedStrings()
     self.__WriteRCFile(translated_strings)
     self.__WriteHeaderFile(translated_strings)
 
   class __TranslationData(object):
     """A container of information about a single translation."""
     def __init__(self, resource_id_str, language, translation):
       self.resource_id_str = resource_id_str
@@ skipping 100 matching lines @@
                        escaped_text))
       outfile.write(FOOTER_TEXT)
 
   def __WriteHeaderFile(self, translated_strings):
     """Writes a .h file with resource ids."""
     # TODO(grt): Stream the lines to the file rather than building this giant
     # list of lines first.
     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()
     offset_id = 0
     for translation_data in translated_strings:
       lang = translation_data.language
       if lang not in seen_languages:
         seen_languages.add(lang)
         lines.append('#define IDS_L10N_OFFSET_%s %s' % (lang, offset_id))
         do_languages_lines.append('  HANDLE_LANGUAGE(%s, IDS_L10N_OFFSET_%s)'
                                   % (lang.replace('_', '-').lower(), lang))
         offset_id += 1
       else:
         break
 
     # Write the resource ids themselves.
     resource_id = FIRST_RESOURCE_ID
     for translation_data in translated_strings:
       lines.append('#define %s %s' % (translation_data.resource_id_str + '_' +
                                       translation_data.language,
                                       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,
                                               string_id,
                                               translated_strings[0].language))
       installer_string_mapping_lines.append('  HANDLE_STRING(%s_BASE, %s)'
                                             % (string_id, string_id))
 
     with open(os.path.join(self.outdir, self.name + '.h'), 'wb') as outfile:
       outfile.write('\n'.join(lines))
       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')
 
 
 def ParseCommandLine():
   def GrdPathAndXtbDirPair(string):
     """Returns (grd_path, xtb_dir) given a colon-separated string of the same.
     """
     parts = string.split(':')
     if len(parts) is not 2:
       raise argparse.ArgumentTypeError('%r is not grd_path:xtb_dir')
     return (parts[0], parts[1])
 
   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,
                       help='path to .grd file:relative path to .xtb dir',
                       metavar='GRDFILE:XTBDIR',
                       dest='inputs')
   parser.add_argument('-o',
                       required=True,
                       help='output directory for generated .rc and .h files',
                       dest='outdir')
   parser.add_argument('-n',
                       required=True,
                       help='base name of generated .rc and .h files',
                       dest='name')
   return parser.parse_args()
 
 
 def main():
   args = ParseCommandLine()
-  StringRcMaker(args.name, args.inputs, args.outdir).MakeFiles()
+  StringRcMaker(args.name, args.inputs, args.outdir, args.brand).MakeFiles()
   return 0
 
 
 if '__main__' == __name__:
   sys.exit(main())