Split GN installer_util into one that has no strings.

Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because it doesn't include all of these generated strings. The strings are not needed by chrome.exe, but there is no sub-part of chrome/installer/util that can be factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if you don't. This is rather precarious so I named the non-string-including target in a very detailed way. The result is the final link matches GYP in this respect, and saves about 200KB on chrome.exe

TBR=dpranke@chromium.org

Committed: https://crrev.com/d1ceb87cd865e2777059c5afe031dae57c8e7914
Cr-Commit-Position: refs/heads/master@{#366618}

[brettw, 2015-12-17 21:04:08]

Description was changed from

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect.
==========

to

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe
==========

[brettw, 2015-12-17 21:04:08]

brettw@chromium.org changed reviewers:
+ dpranke@chromium.org

[brettw, 2015-12-17 21:04:20]

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

[commit-bot: I haz the power, 2015-12-17 21:08:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534903002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534903002/1

[commit-bot: I haz the power, 2015-12-17 23:11:35]

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

[commit-bot: I haz the power, 2015-12-17 23:11:36]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2015-12-18 19:02:15]

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

[grt (UTC plus 2), 2015-12-18 19:02:15]

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD.gn
File chrome/installer/util/BUILD.gn (right):

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD...
chrome/installer/util/BUILD.gn:18: # chrome/installer/util has generated strings
on Windows. These appear as
I think this comment is incorrect. Chrome calls many functions in installer_util
that use strings, and does not require that it be linked with the string
resources. ChromeBrowserMainPartsWin::PreMainMessageLoopStart takes care of
installing a TranslationDelegate for Chrome so that installer_util string
lookups are routed to l10n_util::GetStringUTF16 rather than
AtlGetStringResourceImage. In the GYP world, targets that don't install a custom
TranslationDelegate link with installer_util_strings.rc to get the string
resources. At the moment, this is installer_util_unittests.exe, setup.exe,
setup_unittests.exe, and unit_tests.exe. Hope this helps clear things up a bit.

[Dirk Pranke, 2015-12-20 19:53:46]

I leave this to you and grt@ to sort out.

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD.gn
File chrome/installer/util/BUILD.gn (right):

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD...
chrome/installer/util/BUILD.gn:23: # without calling any functions that use the
strings. And historitally in the
nit: s/historitally/historically

[brettw, 2015-12-21 17:33:00]

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD.gn
File chrome/installer/util/BUILD.gn (right):

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD...
chrome/installer/util/BUILD.gn:18: # chrome/installer/util has generated strings
on Windows. These appear as
This comment is referring specifically to chrome.exe, not "Chrome" (which is
mostly chrome.dll). This all happens long before ChromeBrowserMain, and there is
no TranslationDelegate in this module until chrome.dll is loaded. Since the code
looked clearly wrong, I actually audited all the calls from chrome.exe to
installer_util and none result in getting any strings.

[grt (UTC plus 2), 2015-12-21 19:45:06]

grt@chromium.org changed reviewers:
- grt@chromium.org

[grt (UTC plus 2), 2015-12-21 19:45:06]

I don't consider myself to have GN readability, so I'll duck out here. Take it
away, Dirk.

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD.gn
File chrome/installer/util/BUILD.gn (right):

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD...
chrome/installer/util/BUILD.gn:18: # chrome/installer/util has generated strings
on Windows. These appear as
On 2015/12/21 17:33:00, brettw wrote:
> This comment is referring specifically to chrome.exe, not "Chrome" (which is
> mostly chrome.dll). This all happens long before ChromeBrowserMain, and there
is
> no TranslationDelegate in this module until chrome.dll is loaded. Since the
code
> looked clearly wrong, I actually audited all the calls from chrome.exe to
> installer_util and none result in getting any strings.

Ah, now I see what you mean. :-(

[brettw, 2015-12-21 22:56:22]

I think Dirk is out and he already kind of looked at this. I'm going to TBR him
for now. Happy to make any changes in a follow-up.

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD.gn
File chrome/installer/util/BUILD.gn (right):

https://codereview.chromium.org/1534903002/diff/1/chrome/installer/util/BUILD...
chrome/installer/util/BUILD.gn:18: # chrome/installer/util has generated strings
on Windows. These appear as
I made this distinction more clear in the comment here.

[brettw, 2015-12-21 22:56:26]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-12-21 22:56:39]

Description was changed from

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe
==========

to

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe

TBR=dpranke@chromium.org
==========

[commit-bot: I haz the power, 2015-12-21 22:56:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534903002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534903002/20001

[commit-bot: I haz the power, 2015-12-21 23:10:22]

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

[commit-bot: I haz the power, 2015-12-21 23:10:22]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2015-12-22 17:48:30]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2015-12-22 17:48:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534903002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534903002/20001

[commit-bot: I haz the power, 2015-12-22 18:16:09]

Description was changed from

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe

TBR=dpranke@chromium.org
==========

to

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe

TBR=dpranke@chromium.org
==========

[commit-bot: I haz the power, 2015-12-22 18:16:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-22 18:17:23]

Description was changed from

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe

TBR=dpranke@chromium.org
==========

to

==========
Split GN installer_util into one that has no strings.

After a lot of study, the GYP build of chrome.exe is smaller partially because
it doesn't include all of these generated strings. The strings are not needed by
chrome.exe, but there is no sub-part of chrome/installer/util that can be
factored such that the part that chrome.exe uses does not depend on the strings.

This patch makes two targets, one for if you want the strings, and one for if
you don't. This is rather precarious so I named the non-string-including target
in a very detailed way. The result is the final link matches GYP in this
respect, and saves about 200KB on chrome.exe

TBR=dpranke@chromium.org

Committed: https://crrev.com/d1ceb87cd865e2777059c5afe031dae57c8e7914
Cr-Commit-Position: refs/heads/master@{#366618}
==========

[commit-bot: I haz the power, 2015-12-22 18:17:24]

Patchset 2 (id:??) landed as
https://crrev.com/d1ceb87cd865e2777059c5afe031dae57c8e7914
Cr-Commit-Position: refs/heads/master@{#366618}

[Dirk Pranke, 2015-12-23 03:30:32]

lgtm