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

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

Previously, callers needed to get string via a mode-specific string id
(e.g., IDS_PRODUCT_NAME or IDS_SXS_SHORTCUT_NAME for Chrome's localized
shortcut name). This meant that each caller needed to branch on the mode
(either explicitly, or implicitly via BrowserDistribution subclasses).

This change specifies mode-specific mappings for strings in
create_string_rc.py and plubing in GetLocalizedString so that callers
automatically get the proper string at runtime based on the mode at
play.

As a consequence, the following cleanups in this CL became possible:
- GoogleChromeSxSDistribution is removed, as it is no longer serves any
  purpose.
- InstallUtil::IsChromeSxSProcess is removed, as it no longer has any
  consumers.
- I took the liberty of sorting the STRING_IDS array in create_string_rc
  by ID name.

BUG=373987
Review-Url: https://codereview.chromium.org/2791593002
Cr-Commit-Position: refs/heads/master@{#461430}
Committed: https://chromium.googlesource.com/chromium/src/+/e8cc5ba9a51193c5759e775c263d2266e84a4a82

[grt (UTC plus 2), 2017-03-31 11:18:39]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 11:18:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-03-31 11:40:56]

grt@chromium.org changed reviewers:
+ manzagop@chromium.org

[grt (UTC plus 2), 2017-03-31 11:40:56]

Hi P-A. Do you have time to review this one? Thanks.

[commit-bot: I haz the power, 2017-03-31 12:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 12:45:41]

Dry run: This issue passed the CQ dry run.

[manzagop (departed), 2017-03-31 13:54:59]

On 2017/03/31 11:40:56, grt (UTC plus 2) wrote:
> Hi P-A. Do you have time to review this one? Thanks.

Sure thing! Having a look.

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

lgtm

Change looks good! I'm not familiar with our string usage though, so you'll see
a few rambling comments, mainly on the introduction of the indirection.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/l10n_...
File chrome/installer/util/l10n_string_util_unittest.cc (right):

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/l10n_...
chrome/installer/util/l10n_string_util_unittest.cc:38: 
Should we add a test for one brand/mode strings, for extra safety (brittleness)?

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:46: # setup.exe's resources.
nit: is it common practice to mention the alphabetical ordering?

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:84: #       string_id_1,  #
Strings listed in order of the brand's modes, as
Add a comment in the mode file that this needs to be changed if the order
changes?

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:100: # be retrieved as
needed.
(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.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:103: 'google_chrome': [
nit: use a constant? For the default brand too?

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:131: 'IDS_PRODUCT_NAME',
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...)

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:285: """constructs a maker.
nitty question: no capital letter?

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:451: % (string_id, ',
'.join([ ('%s_BASE' % s) for s in brand_strings])))
Should we validate that no brand string is a pseudo string?

[manzagop (departed), 2017-03-31 16:19:06]

lgtm

Change looks good! I'm not familiar with our string usage though, so you'll see
a few rambling comments, mainly on the introduction of the indirection.

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

PTAL

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/l10n_...
File chrome/installer/util/l10n_string_util_unittest.cc (right):

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/l10n_...
chrome/installer/util/l10n_string_util_unittest.cc:38: 
On 2017/03/31 16:19:00, manzagop wrote:
> Should we add a test for one brand/mode strings, for extra safety
(brittleness)?

I've added a new test that asserts the Google Chrome mapping is good.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:46: # setup.exe's resources.
On 2017/03/31 16:19:01, manzagop wrote:
> nit: is it common practice to mention the alphabetical ordering?

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.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:84: #       string_id_1,  #
Strings listed in order of the brand's modes, as
On 2017/03/31 16:19:00, manzagop wrote:
> Add a comment in the mode file that this needs to be changed if the order
> changes?

Done.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:100: # be retrieved as
needed.
On 2017/03/31 16:19:01, manzagop wrote:
> (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)?

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.

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

Yeah. I didn't want to make extra hoops for all strings; just for the special
ones.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:100: # be retrieved as
needed.
On 2017/03/31 16:19:01, manzagop wrote:
> (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)?

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.

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

I was hoping to only do the mapping for strings that required it.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:103: 'google_chrome': [
On 2017/03/31 16:19:01, manzagop wrote:
> nit: use a constant? For the default brand too?

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.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:131: 'IDS_PRODUCT_NAME',
On 2017/03/31 16:19:01, manzagop wrote:
> 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...)

Does my response above address this question?

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:285: """constructs a maker.
On 2017/03/31 16:19:01, manzagop wrote:
> nitty question: no capital letter?

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

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:451: % (string_id, ',
'.join([ ('%s_BASE' % s) for s in brand_strings])))
On 2017/03/31 16:19:00, manzagop wrote:
> Should we validate that no brand string is a pseudo string?

noop -- i've removed psuedo strings

[grt (UTC plus 2), 2017-04-03 11:59:50]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 12:00:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-04-03 12:26:49]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 12:27:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Great! lgtm % nits

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:103: 'google_chrome': [
On 2017/04/03 11:59:42, grt (UTC plus 2) wrote:
> On 2017/03/31 16:19:01, manzagop wrote:
> > nit: use a constant? For the default brand too?
> 
> 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.

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.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:131: 'IDS_PRODUCT_NAME',
On 2017/04/03 11:59:42, grt (UTC plus 2) wrote:
> On 2017/03/31 16:19:01, manzagop wrote:
> > 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...)
> 
> Does my response above address this question?

Yep! Thanks.

https://codereview.chromium.org/2791593002/diff/20001/chrome/installer/util/p...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://codereview.chromium.org/2791593002/diff/20001/chrome/installer/util/p...
chrome/installer/util/prebuild/create_string_rc.py:127: 'IDS_PRODUCT_NAME': {
May be a good idea to also call attention here to the modalization / difference
between the installer and the rest of Chrome and the intent.

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

Thanks.

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://codereview.chromium.org/2791593002/diff/1/chrome/installer/util/prebu...
chrome/installer/util/prebuild/create_string_rc.py:103: 'google_chrome': [
On 2017/04/03 13:06:33, manzagop wrote:
> On 2017/04/03 11:59:42, grt (UTC plus 2) wrote:
> > On 2017/03/31 16:19:01, manzagop wrote:
> > > nit: use a constant? For the default brand too?
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/2791593002/diff/20001/chrome/installer/util/p...
File chrome/installer/util/prebuild/create_string_rc.py (right):

https://codereview.chromium.org/2791593002/diff/20001/chrome/installer/util/p...
chrome/installer/util/prebuild/create_string_rc.py:127: 'IDS_PRODUCT_NAME': {
On 2017/04/03 13:06:33, manzagop wrote:
> May be a good idea to also call attention here to the modalization /
difference
> between the installer and the rest of Chrome and the intent.

Done. I also added a bit more text to the comment above explaining the
difference between using these strings in the installer vs. the rest of Chrome.

[grt (UTC plus 2), 2017-04-03 13:31:36]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-04-03 13:31:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
manzagop@chromium.org
Link to the patchset: https://codereview.chromium.org/2791593002/#ps60001
(title: "manzagop review part the deux")

[commit-bot: I haz the power, 2017-04-03 13:32:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 14:50:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491226296415770,
"parent_rev": "db5b2690ab058b240577cbf587c34bc9727a9b11", "commit_rev":
"e8cc5ba9a51193c5759e775c263d2266e84a4a82"}

[commit-bot: I haz the power, 2017-04-03 14:51:12]

Description was changed from

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

Previously, callers needed to get string via a mode-specific string id
(e.g., IDS_PRODUCT_NAME or IDS_SXS_SHORTCUT_NAME for Chrome's localized
shortcut name). This meant that each caller needed to branch on the mode
(either explicitly, or implicitly via BrowserDistribution subclasses).

This change specifies mode-specific mappings for strings in
create_string_rc.py and plubing in GetLocalizedString so that callers
automatically get the proper string at runtime based on the mode at
play.

As a consequence, the following cleanups in this CL became possible:
- GoogleChromeSxSDistribution is removed, as it is no longer serves any
  purpose.
- InstallUtil::IsChromeSxSProcess is removed, as it no longer has any
  consumers.
- I took the liberty of sorting the STRING_IDS array in create_string_rc
  by ID name.

BUG=373987
==========

to

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

Previously, callers needed to get string via a mode-specific string id
(e.g., IDS_PRODUCT_NAME or IDS_SXS_SHORTCUT_NAME for Chrome's localized
shortcut name). This meant that each caller needed to branch on the mode
(either explicitly, or implicitly via BrowserDistribution subclasses).

This change specifies mode-specific mappings for strings in
create_string_rc.py and plubing in GetLocalizedString so that callers
automatically get the proper string at runtime based on the mode at
play.

As a consequence, the following cleanups in this CL became possible:
- GoogleChromeSxSDistribution is removed, as it is no longer serves any
  purpose.
- InstallUtil::IsChromeSxSProcess is removed, as it no longer has any
  consumers.
- I took the liberty of sorting the STRING_IDS array in create_string_rc
  by ID name.

BUG=373987

Review-Url: https://codereview.chromium.org/2791593002
Cr-Commit-Position: refs/heads/master@{#461430}
Committed:
https://chromium.googlesource.com/chromium/src/+/e8cc5ba9a51193c5759e775c263d...
==========

[commit-bot: I haz the power, 2017-04-03 14:51:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e8cc5ba9a51193c5759e775c263d...