Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on data stored in the previous
kBrowserCrashDumpAttemptsRegistryPath will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable)
and Canary installed side-by-side per them sharing that key.

BUG=577820
Committed: https://crrev.com/7c45cfded56e2f0ee3f0d04ba9d926844c70e499
Cr-Commit-Position: refs/heads/master@{#387669}

[gab, 2016-01-25 20:06:46]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have properly been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

This means that reports generated from these entries could have
previously been incorrect on machines that had both Google Chrome
(Dev/Beta/Stable) and Canary installed side-by-side.

BUG=577820
==========

[gab, 2016-01-25 20:10:10]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have properly been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

This means that reports generated from these entries could have
previously been incorrect on machines that had both Google Chrome
(Dev/Beta/Stable) and Canary installed side-by-side.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have properly been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded key instead a Google\Chrome key but will now
properly use a "Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on this stored data
will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side.

BUG=577820
==========

[gab, 2016-01-25 20:10:35]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have properly been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded key instead a Google\Chrome key but will now
properly use a "Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on this stored data
will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded key instead a Google\Chrome key but will now
properly use a "Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on this stored data
will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side.

BUG=577820
==========

[gab, 2016-01-25 20:11:34]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded key instead a Google\Chrome key but will now
properly use a "Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on this stored data
will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on this stored data
will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side.

BUG=577820
==========

[gab, 2016-01-25 20:13:04]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on this stored data
will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on data stored in the previous
kBrowserCrashDumpAttemptsRegistryPath will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side per them sharing that key.

BUG=577820
==========

[gab, 2016-01-25 20:16:40]

gab@chromium.org changed reviewers:
+ grt@chromium.org, siggi@chromium.org, sky@chromium.org

[gab, 2016-01-25 20:16:40]

Siggi PTAL (and please ack side-effects noted in CL description).

CC scottmg FYI per side-effects on crash/exit code stats

@sky for chrome_constants as discussed @ https://crbug.com/577820#c10

@grt for rubberstamp on chrome/app/* side-effects

Thanks,
Gab

[Sigurður Ásgeirsson, 2016-01-25 20:50:24]

nice - thanks for the cleanup.

It is NOT OK to pass non-PODs across module boundaries. Therein lies madness.

https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...
chrome/chrome_watcher/chrome_watcher_main.cc:349: extern "C" int
WatcherMain(const base::string16& registry_path,
this crosses a module boundary and hence an allocation boundary. We pass only
PODs across those, hence the char16*

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
chrome/common/chrome_constants.cc:235: BrowserDistribution* chrome_distribution
=
will this return the same constants as previously, or will this abandon existing
data?

https://codereview.chromium.org/1630923002/diff/1/components/browser_watcher/...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/1630923002/diff/1/components/browser_watcher/...
components/browser_watcher/watcher_metrics_provider_win.h:23:
WatcherMetricsProviderWin(const base::string16& registry_path,
this'll force an extra copy. No biggie except we force these all over, and in
aggregate they're likely killing us. How about a StringPiece?

[grt (UTC plus 2), 2016-01-25 21:18:20]

it'll be nice to have this fixed. thanks for taking a stab at it!

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:138: base::string16
GetBrowserCrashDumpAttemptsRegistryPath();
is there a better place than a *_constants.{cc,h} pair for these functions? i
prefer keeping only constants in these sorts of files.

[gab, 2016-01-26 02:55:05]

Responses below for discussion (no code change yet)

https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...
chrome/chrome_watcher/chrome_watcher_main.cc:349: extern "C" int
WatcherMain(const base::string16& registry_path,
On 2016/01/25 20:50:24, Sigurður Ásgeirsson wrote:
> this crosses a module boundary and hence an allocation boundary. We pass only
> PODs across those, hence the char16*

Gotcha, would a base::string16* be okay? i.e given the call is synchronous and
the memory would be forever valid, right?

In fact the classes (BrowserMonitor et al.) could store a const base::string16*
instead of copying it to a local base::string16 member.

Another option is to do just that but pass the string through its c_str() as a 
"const base::char16*" and store it as that instead of copying the string?

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
File chrome/common/chrome_constants.cc (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
chrome/common/chrome_constants.cc:235: BrowserDistribution* chrome_distribution
=
On 2016/01/25 20:50:24, Sigurður Ásgeirsson wrote:
> will this return the same constants as previously, or will this abandon
existing
> data?

This will return the same constant as previously for Google Chrome
Dev/Beta/Stable but will return a new product specific constant for SxS Canary
(see CL description for details).

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:138: base::string16
GetBrowserCrashDumpAttemptsRegistryPath();
On 2016/01/25 21:18:20, grt wrote:
> is there a better place than a *_constants.{cc,h} pair for these functions? i
> prefer keeping only constants in these sorts of files.

How about chrome_constants_util_win.{cc,h} ?

https://codereview.chromium.org/1630923002/diff/1/components/browser_watcher/...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/1630923002/diff/1/components/browser_watcher/...
components/browser_watcher/watcher_metrics_provider_win.h:23:
WatcherMetricsProviderWin(const base::string16& registry_path,
On 2016/01/25 20:50:24, Sigurður Ásgeirsson wrote:
> this'll force an extra copy. No biggie except we force these all over, and in
> aggregate they're likely killing us. How about a StringPiece?

I don't think it does as the char* were replaced by const string& and both
result in a copy when reaching the base::string16 member of the class.

(see suggestion in other comment though to perhaps even avoid that copy)

[Sigurður Ásgeirsson, 2016-01-26 14:04:11]

On Mon, Jan 25, 2016 at 9:55 PM <gab@chromium.org> wrote:

> Responses below for discussion (no code change yet)
>
>
>
>
https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...
> File chrome/chrome_watcher/chrome_watcher_main.cc (right):
>
>
>
https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...
> chrome/chrome_watcher/chrome_watcher_main.cc:349
>
<https://codereview.chromium.org/1630923002/diff/1/chrome/chrome_watcher/chrom...>:
> extern "C" int
> WatcherMain(const base::string16& registry_path,
> On 2016/01/25 20:50:24, Sigurður Ásgeirsson wrote:
> > this crosses a module boundary and hence an allocation boundary. We
> pass only
> > PODs across those, hence the char16*
>
> Gotcha, would a base::string16* be okay? i.e given the call is
> synchronous and the memory would be forever valid, right?
>
> There are two general concerns with passing non-POD data across module
boundaries.
1. the allocation concerns - if you allocate a std::string in chrome.exe
and free in chrome.dll, well congrats, you've just corrupted two heaps.
2. the one definition rule. If the two modules don't share the same
definition of your data structure, your program is now undefined and will
*sometimes* crash in mysterious ways.

Given the declarations in this CL and the *expected* usage there's no
danger of either of those. However, because these are such an <expletive>
to hunt down, I always recommend strongly against passing non-PODs across
module boundaries. If you do it once, someone else is going to follow the
lead later and get it wrong.
Also, there's always the problem of usage creep, where tomorrow someone
else (SyzyASAN?) will start using the export in question and blow up.

Also, a non-POD pointer is a non-POD for the purposes of this discussion.
Also note that there's no difference between a std::string& and a
std::string* for the purposes of argument passing...

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[grt (UTC plus 2), 2016-01-26 16:13:04]

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:138: base::string16
GetBrowserCrashDumpAttemptsRegistryPath();
On 2016/01/26 02:55:05, gab wrote:
> On 2016/01/25 21:18:20, grt wrote:
> > is there a better place than a *_constants.{cc,h} pair for these functions?
i
> > prefer keeping only constants in these sorts of files.
> 
> How about chrome_constants_util_win.{cc,h} ?

WDYT about chrome/common/chrome_paths_win.cc? A registry path is a kind of
path...

https://codereview.chromium.org/1630923002/diff/1/components/browser_watcher/...
File components/browser_watcher/watcher_metrics_provider_win.h (right):

https://codereview.chromium.org/1630923002/diff/1/components/browser_watcher/...
components/browser_watcher/watcher_metrics_provider_win.h:23:
WatcherMetricsProviderWin(const base::string16& registry_path,
On 2016/01/26 02:55:05, gab wrote:
> On 2016/01/25 20:50:24, Sigurður Ásgeirsson wrote:
> > this'll force an extra copy. No biggie except we force these all over, and
in
> > aggregate they're likely killing us. How about a StringPiece?
> 
> I don't think it does as the char* were replaced by const string& and both
> result in a copy when reaching the base::string16 member of the class.
> 
> (see suggestion in other comment though to perhaps even avoid that copy)

I like the StringPiece suggestion since it doesn't require that the caller
create a base::string16. As far as I can tell, StringPiece is currently a POD
type. I'm not sure if this is guaranteed in the future, though, since the
comments don't seem to say "This is and will always be a POD type."

[Sigurður Ásgeirsson, 2016-01-26 18:18:38]

On Tue, Jan 26, 2016 at 11:13 AM <grt@chromium.org> wrote:

>
> I like the StringPiece suggestion since it doesn't require that the
> caller create a base::string16. As far as I can tell, StringPiece is
> currently a POD type. I'm not sure if this is guaranteed in the future,
> though, since the comments don't seem to say "This is and will always be
> a POD type."
>
> I don't think StringPiece qualifies as a POD, if only because it has a
user-defined constructor, though apparently this isn't super-well defined <
http://stackoverflow.com/questions/146452/what-are-pod-types-in-c>.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2016-01-26 18:23:29]

I guess then the best option is to grab the string16 on chrome_exe side and pass
it as a const base::char16* (i.e. via its c_str() and keep the old API on the
module side).

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
File chrome/common/chrome_constants.h (right):

https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
chrome/common/chrome_constants.h:138: base::string16
GetBrowserCrashDumpAttemptsRegistryPath();
On 2016/01/26 16:13:04, grt wrote:
> On 2016/01/26 02:55:05, gab wrote:
> > On 2016/01/25 21:18:20, grt wrote:
> > > is there a better place than a *_constants.{cc,h} pair for these
functions?
> i
> > > prefer keeping only constants in these sorts of files.
> > 
> > How about chrome_constants_util_win.{cc,h} ?
> 
> WDYT about chrome/common/chrome_paths_win.cc? A registry path is a kind of
> path...

I prefer "constants_util", _paths is already used in the codebase and only ever
used for file paths (and meant for PathService) so using it for registry paths +
without PathService feels wrong.

[gab, 2016-01-26 18:24:32]

On 2016/01/26 18:23:29, gab wrote:
> I guess then the best option is to grab the string16 on chrome_exe side and
pass
> it as a const base::char16* (i.e. via its c_str() and keep the old API on the
> module side).

Note that I think this is equivalent to passing it as a StringPiece in practice
today, but more explicit and less prone to the errors Siggi mentioned above
long-term.

> 
>
https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
> File chrome/common/chrome_constants.h (right):
> 
>
https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
> chrome/common/chrome_constants.h:138: base::string16
> GetBrowserCrashDumpAttemptsRegistryPath();
> On 2016/01/26 16:13:04, grt wrote:
> > On 2016/01/26 02:55:05, gab wrote:
> > > On 2016/01/25 21:18:20, grt wrote:
> > > > is there a better place than a *_constants.{cc,h} pair for these
> functions?
> > i
> > > > prefer keeping only constants in these sorts of files.
> > > 
> > > How about chrome_constants_util_win.{cc,h} ?
> > 
> > WDYT about chrome/common/chrome_paths_win.cc? A registry path is a kind of
> > path...
> 
> I prefer "constants_util", _paths is already used in the codebase and only
ever
> used for file paths (and meant for PathService) so using it for registry paths
+
> without PathService feels wrong.

[grt (UTC plus 2), 2016-01-28 13:45:53]

On 2016/01/26 18:23:29, gab wrote:
> I guess then the best option is to grab the string16 on chrome_exe side and
pass
> it as a const base::char16* (i.e. via its c_str() and keep the old API on the
> module side).
> 
>
https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
> File chrome/common/chrome_constants.h (right):
> 
>
https://codereview.chromium.org/1630923002/diff/1/chrome/common/chrome_consta...
> chrome/common/chrome_constants.h:138: base::string16
> GetBrowserCrashDumpAttemptsRegistryPath();
> On 2016/01/26 16:13:04, grt wrote:
> > On 2016/01/26 02:55:05, gab wrote:
> > > On 2016/01/25 21:18:20, grt wrote:
> > > > is there a better place than a *_constants.{cc,h} pair for these
> functions?
> > i
> > > > prefer keeping only constants in these sorts of files.
> > > 
> > > How about chrome_constants_util_win.{cc,h} ?
> > 
> > WDYT about chrome/common/chrome_paths_win.cc? A registry path is a kind of
> > path...
> 
> I prefer "constants_util", _paths is already used in the codebase and only
ever
> used for file paths (and meant for PathService)

While it is mainly used by PathService, it has other consumers. It's a (small)
grab-bag of Win-specific functions for getting paths that only houses funcs for
filesystem paths at the moment, but I don't see a big impedance mismatch to also
using it for registry paths. I'll leave it to you.

[gab, 2016-02-03 21:09:52]

Patchset #3 (id:40001) has been deleted

[gab, 2016-02-03 21:24:40]

Patchset #4 (id:80001) has been deleted

[gab, 2016-02-03 21:31:58]

All addressed I think, PTAL.

[Sigurður Ásgeirsson, 2016-02-03 21:57:08]

browser_watcher/* lgtm. Thanks!

[sky, 2016-02-03 22:20:08]

chrome/constants LGTM

[gab, 2016-02-04 01:37:57]

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

[commit-bot: I haz the power, 2016-02-04 01:39:02]

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

[commit-bot: I haz the power, 2016-02-04 01:43:21]

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

[commit-bot: I haz the power, 2016-02-04 01:43:22]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-02-04 15:15:29]

Thanks, @grt for chrome/app.

[grt (UTC plus 2), 2016-02-04 15:52:30]

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.cc (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:7: #include
"chrome/installer/util/browser_distribution.h"
common_constants doesn't depend on installer_util, so this shouldn't be allowed.
for some reason chrome_paths_win.cc has been doing this all along, but it's
wrong. either common_constants needs to depend on installer_util, or this
functionality needs to be in common (not common_constants), which does depend on
installer_util.

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:12: BrowserDistribution*
chrome_distribution =
  DCHECK_EQ(L'\\', *suffix);

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:17: return
chrome_distribution->GetRegistryPath() + suffix;
  DCHECK_NE(L'\\', the last char of GetRegistryPath());

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.h (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.h:13: // product. This is used by
breakpad and the metrics reporting.
isn't breakpad dead? should this say crashpad (or something else that doesn't
explicitly name the crash handler)?

[gab, 2016-03-07 21:49:22]

Patchset #6 (id:140001) has been deleted

[gab, 2016-03-07 21:51:55]

Patchset #6 (id:160001) has been deleted

[gab, 2016-03-07 21:53:55]

Sorry for the delay, @grt PTAL.

Thanks,
Gab

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.cc (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:7: #include
"chrome/installer/util/browser_distribution.h"
On 2016/02/04 15:52:30, grt wrote:
> common_constants doesn't depend on installer_util, so this shouldn't be
allowed.
> for some reason chrome_paths_win.cc has been doing this all along, but it's
> wrong. either common_constants needs to depend on installer_util, or this
> functionality needs to be in common (not common_constants), which does depend
on
> installer_util.

Good catch, I think it's reasonable to add the dependency since chrome_common
has it already.

I made it "win" only even though the installer_util target itself isn't, WDYT?

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:12: BrowserDistribution*
chrome_distribution =
On 2016/02/04 15:52:30, grt wrote:
>   DCHECK_EQ(L'\\', *suffix);

Done.

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:17: return
chrome_distribution->GetRegistryPath() + suffix;
On 2016/02/04 15:52:30, grt wrote:
>   DCHECK_NE(L'\\', the last char of GetRegistryPath());

Done.

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.h (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.h:13: // product. This is used by
breakpad and the metrics reporting.
On 2016/02/04 15:52:30, grt wrote:
> isn't breakpad dead? should this say crashpad (or something else that doesn't
> explicitly name the crash handler)?

It's used in chrome/app/chrome_crash_reporter_client.cc, made the comment
generic.

[grt (UTC plus 2), 2016-03-08 15:44:47]

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.cc (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:7: #include
"chrome/installer/util/browser_distribution.h"
On 2016/03/07 21:53:55, gab wrote:
> On 2016/02/04 15:52:30, grt wrote:
> > common_constants doesn't depend on installer_util, so this shouldn't be
> allowed.
> > for some reason chrome_paths_win.cc has been doing this all along, but it's
> > wrong. either common_constants needs to depend on installer_util, or this
> > functionality needs to be in common (not common_constants), which does
depend
> on
> > installer_util.
> 
> Good catch, I think it's reasonable to add the dependency since chrome_common
> has it already.

I'm concerned about making a "constants" target pull in installer_util since it
adds weight to something that sounds like it should be very light. Is there
another way?

> I made it "win" only even though the installer_util target itself isn't, WDYT?

https://codereview.chromium.org/1630923002/diff/220001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/1630923002/diff/220001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:355: deps += [ ":installer_util" ]
circular dependency: is this okay?

https://codereview.chromium.org/1630923002/diff/220001/chrome/common_constant...
File chrome/common_constants.gyp (right):

https://codereview.chromium.org/1630923002/diff/220001/chrome/common_constant...
chrome/common_constants.gyp:104: 'installer_util',
you'll minimally need a win-specific includes section that includes
chrome_installer_util.gypi for this to work. i'm not sure if more will be
needed.

[gab, 2016-03-09 20:29:35]

Reply below... Dependency mess..

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.cc (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:7: #include
"chrome/installer/util/browser_distribution.h"
On 2016/03/08 15:44:47, grt wrote:
> On 2016/03/07 21:53:55, gab wrote:
> > On 2016/02/04 15:52:30, grt wrote:
> > > common_constants doesn't depend on installer_util, so this shouldn't be
> > allowed.
> > > for some reason chrome_paths_win.cc has been doing this all along, but
it's
> > > wrong. either common_constants needs to depend on installer_util, or this
> > > functionality needs to be in common (not common_constants), which does
> depend
> > on
> > > installer_util.
> > 
> > Good catch, I think it's reasonable to add the dependency since
chrome_common
> > has it already.
> 
> I'm concerned about making a "constants" target pull in installer_util since
it
> adds weight to something that sounds like it should be very light. Is there
> another way?

Fair enough. Thought of naming these chrome_paths_win.cc/h (;-)) then and
putting them in chrome's "common" target proper.

But that only supports 2/4 places that need these methods as chrome/app doesn't
depend on "common"...

Actually AFAICT there is no official dependency to even "common_constants" in
chrome/app (nothing in chrome_exe.gypi at least..)?!

> 
> > I made it "win" only even though the installer_util target itself isn't,
WDYT?

https://codereview.chromium.org/1630923002/diff/220001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1630923002/diff/220001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:38: #include
"chrome/common/chrome_result_codes.h"
Interesting, another uncaught violation: "chrome/common/chrome_result_codes.h"
is in "common" not "common_constants". It compiles because it's merely a header
with no object, but the dependency is still missing...

[grt (UTC plus 2), 2016-03-10 19:14:42]

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
File chrome/common/chrome_constants_util_win.cc (right):

https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
chrome/common/chrome_constants_util_win.cc:7: #include
"chrome/installer/util/browser_distribution.h"
On 2016/03/09 20:29:35, gab wrote:
> On 2016/03/08 15:44:47, grt wrote:
> > On 2016/03/07 21:53:55, gab wrote:
> > > On 2016/02/04 15:52:30, grt wrote:
> > > > common_constants doesn't depend on installer_util, so this shouldn't be
> > > allowed.
> > > > for some reason chrome_paths_win.cc has been doing this all along, but
> it's
> > > > wrong. either common_constants needs to depend on installer_util, or
this
> > > > functionality needs to be in common (not common_constants), which does
> > depend
> > > on
> > > > installer_util.
> > > 
> > > Good catch, I think it's reasonable to add the dependency since
> chrome_common
> > > has it already.
> > 
> > I'm concerned about making a "constants" target pull in installer_util since
> it
> > adds weight to something that sounds like it should be very light. Is there
> > another way?
> 
> Fair enough. Thought of naming these chrome_paths_win.cc/h (;-)) then and
> putting them in chrome's "common" target proper.
> 
> But that only supports 2/4 places that need these methods as chrome/app
doesn't
> depend on "common"...
> 
> Actually AFAICT there is no official dependency to even "common_constants" in
> chrome/app (nothing in chrome_exe.gypi at least..)?!

Huh. Here's what is currently used by chrome.exe from common_constants.lib at
link time:

chrome::kBrowserExitCodesRegistryPath
- MainDllLoader::Launch (for watcher)
chrome::GetDefaultUserDataDirectory(class base::FilePath *)
- AttemptFastNotify
icon_resources::kApplicationIndex
- GoogleChromeDistribution::GetIconIndex
- BrowserDistribution::GetIconIndex
env_vars::kHeadless
- ChromeCrashReporterClient::IsRunningUnattended
switches::kProfileDirectory
- IsFastStartSwitch
chrome::RegisterPathProvider()
- MainDllLoader::Launch (for watcher)

The two from installer_util already depend on common_constants. Sure looks like
the chrome_initial target is missing a win-only dependency on common_constants,
and is only getting it transitively via installer_util.

[gab, 2016-03-14 18:47:19]

On 2016/03/10 19:14:42, grt (very slow) wrote:
>
https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
> File chrome/common/chrome_constants_util_win.cc (right):
> 
>
https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
> chrome/common/chrome_constants_util_win.cc:7: #include
> "chrome/installer/util/browser_distribution.h"
> On 2016/03/09 20:29:35, gab wrote:
> > On 2016/03/08 15:44:47, grt wrote:
> > > On 2016/03/07 21:53:55, gab wrote:
> > > > On 2016/02/04 15:52:30, grt wrote:
> > > > > common_constants doesn't depend on installer_util, so this shouldn't
be
> > > > allowed.
> > > > > for some reason chrome_paths_win.cc has been doing this all along, but
> > it's
> > > > > wrong. either common_constants needs to depend on installer_util, or
> this
> > > > > functionality needs to be in common (not common_constants), which does
> > > depend
> > > > on
> > > > > installer_util.
> > > > 
> > > > Good catch, I think it's reasonable to add the dependency since
> > chrome_common
> > > > has it already.
> > > 
> > > I'm concerned about making a "constants" target pull in installer_util
since
> > it
> > > adds weight to something that sounds like it should be very light. Is
there
> > > another way?
> > 
> > Fair enough. Thought of naming these chrome_paths_win.cc/h (;-)) then and
> > putting them in chrome's "common" target proper.
> > 
> > But that only supports 2/4 places that need these methods as chrome/app
> doesn't
> > depend on "common"...
> > 
> > Actually AFAICT there is no official dependency to even "common_constants"
in
> > chrome/app (nothing in chrome_exe.gypi at least..)?!
> 
> Huh. Here's what is currently used by chrome.exe from common_constants.lib at
> link time:
> 
> chrome::kBrowserExitCodesRegistryPath
> - MainDllLoader::Launch (for watcher)
> chrome::GetDefaultUserDataDirectory(class base::FilePath *)
> - AttemptFastNotify
> icon_resources::kApplicationIndex
> - GoogleChromeDistribution::GetIconIndex
> - BrowserDistribution::GetIconIndex
> env_vars::kHeadless
> - ChromeCrashReporterClient::IsRunningUnattended
> switches::kProfileDirectory
> - IsFastStartSwitch
> chrome::RegisterPathProvider()
> - MainDllLoader::Launch (for watcher)
> 
> The two from installer_util already depend on common_constants. Sure looks
like
> the chrome_initial target is missing a win-only dependency on
common_constants,
> and is only getting it transitively via installer_util.

I see, thanks for digging into this.

So given chrome.exe already depends on installer_util, I guess we could put the
two methods being introduced in this CL in a new common_constants_util_win
target (for lack of a better name) that depends only on installer_util? (adding
it straight in installer_util also feels out of place)

Then there are no new real dependencies added and we avoid common_constants
depending on installer_util which I agree is wrong.

WDYT?

[gab, 2016-04-07 02:01:23]

On 2016/03/14 18:47:19, gab wrote:
> On 2016/03/10 19:14:42, grt (very slow) wrote:
> >
>
https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
> > File chrome/common/chrome_constants_util_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
> > chrome/common/chrome_constants_util_win.cc:7: #include
> > "chrome/installer/util/browser_distribution.h"
> > On 2016/03/09 20:29:35, gab wrote:
> > > On 2016/03/08 15:44:47, grt wrote:
> > > > On 2016/03/07 21:53:55, gab wrote:
> > > > > On 2016/02/04 15:52:30, grt wrote:
> > > > > > common_constants doesn't depend on installer_util, so this shouldn't
> be
> > > > > allowed.
> > > > > > for some reason chrome_paths_win.cc has been doing this all along,
but
> > > it's
> > > > > > wrong. either common_constants needs to depend on installer_util, or
> > this
> > > > > > functionality needs to be in common (not common_constants), which
does
> > > > depend
> > > > > on
> > > > > > installer_util.
> > > > > 
> > > > > Good catch, I think it's reasonable to add the dependency since
> > > chrome_common
> > > > > has it already.
> > > > 
> > > > I'm concerned about making a "constants" target pull in installer_util
> since
> > > it
> > > > adds weight to something that sounds like it should be very light. Is
> there
> > > > another way?
> > > 
> > > Fair enough. Thought of naming these chrome_paths_win.cc/h (;-)) then and
> > > putting them in chrome's "common" target proper.
> > > 
> > > But that only supports 2/4 places that need these methods as chrome/app
> > doesn't
> > > depend on "common"...
> > > 
> > > Actually AFAICT there is no official dependency to even "common_constants"
> in
> > > chrome/app (nothing in chrome_exe.gypi at least..)?!
> > 
> > Huh. Here's what is currently used by chrome.exe from common_constants.lib
at
> > link time:
> > 
> > chrome::kBrowserExitCodesRegistryPath
> > - MainDllLoader::Launch (for watcher)
> > chrome::GetDefaultUserDataDirectory(class base::FilePath *)
> > - AttemptFastNotify
> > icon_resources::kApplicationIndex
> > - GoogleChromeDistribution::GetIconIndex
> > - BrowserDistribution::GetIconIndex
> > env_vars::kHeadless
> > - ChromeCrashReporterClient::IsRunningUnattended
> > switches::kProfileDirectory
> > - IsFastStartSwitch
> > chrome::RegisterPathProvider()
> > - MainDllLoader::Launch (for watcher)
> > 
> > The two from installer_util already depend on common_constants. Sure looks
> like
> > the chrome_initial target is missing a win-only dependency on
> common_constants,
> > and is only getting it transitively via installer_util.
> 
> I see, thanks for digging into this.
> 
> So given chrome.exe already depends on installer_util, I guess we could put
the
> two methods being introduced in this CL in a new common_constants_util_win
> target (for lack of a better name) that depends only on installer_util?
(adding
> it straight in installer_util also feels out of place)
> 
> Then there are no new real dependencies added and we avoid common_constants
> depending on installer_util which I agree is wrong.
> 
> WDYT?

ping for opinion on the above suggestion, thanks

[gab, 2016-04-11 16:07:12]

On 2016/04/07 02:01:23, gab wrote:
> On 2016/03/14 18:47:19, gab wrote:
> > On 2016/03/10 19:14:42, grt (very slow) wrote:
> > >
> >
>
https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
> > > File chrome/common/chrome_constants_util_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1630923002/diff/100001/chrome/common/chrome_c...
> > > chrome/common/chrome_constants_util_win.cc:7: #include
> > > "chrome/installer/util/browser_distribution.h"
> > > On 2016/03/09 20:29:35, gab wrote:
> > > > On 2016/03/08 15:44:47, grt wrote:
> > > > > On 2016/03/07 21:53:55, gab wrote:
> > > > > > On 2016/02/04 15:52:30, grt wrote:
> > > > > > > common_constants doesn't depend on installer_util, so this
shouldn't
> > be
> > > > > > allowed.
> > > > > > > for some reason chrome_paths_win.cc has been doing this all along,
> but
> > > > it's
> > > > > > > wrong. either common_constants needs to depend on installer_util,
or
> > > this
> > > > > > > functionality needs to be in common (not common_constants), which
> does
> > > > > depend
> > > > > > on
> > > > > > > installer_util.
> > > > > > 
> > > > > > Good catch, I think it's reasonable to add the dependency since
> > > > chrome_common
> > > > > > has it already.
> > > > > 
> > > > > I'm concerned about making a "constants" target pull in installer_util
> > since
> > > > it
> > > > > adds weight to something that sounds like it should be very light. Is
> > there
> > > > > another way?
> > > > 
> > > > Fair enough. Thought of naming these chrome_paths_win.cc/h (;-)) then
and
> > > > putting them in chrome's "common" target proper.
> > > > 
> > > > But that only supports 2/4 places that need these methods as chrome/app
> > > doesn't
> > > > depend on "common"...
> > > > 
> > > > Actually AFAICT there is no official dependency to even
"common_constants"
> > in
> > > > chrome/app (nothing in chrome_exe.gypi at least..)?!
> > > 
> > > Huh. Here's what is currently used by chrome.exe from common_constants.lib
> at
> > > link time:
> > > 
> > > chrome::kBrowserExitCodesRegistryPath
> > > - MainDllLoader::Launch (for watcher)
> > > chrome::GetDefaultUserDataDirectory(class base::FilePath *)
> > > - AttemptFastNotify
> > > icon_resources::kApplicationIndex
> > > - GoogleChromeDistribution::GetIconIndex
> > > - BrowserDistribution::GetIconIndex
> > > env_vars::kHeadless
> > > - ChromeCrashReporterClient::IsRunningUnattended
> > > switches::kProfileDirectory
> > > - IsFastStartSwitch
> > > chrome::RegisterPathProvider()
> > > - MainDllLoader::Launch (for watcher)
> > > 
> > > The two from installer_util already depend on common_constants. Sure looks
> > like
> > > the chrome_initial target is missing a win-only dependency on
> > common_constants,
> > > and is only getting it transitively via installer_util.
> > 
> > I see, thanks for digging into this.
> > 
> > So given chrome.exe already depends on installer_util, I guess we could put
> the
> > two methods being introduced in this CL in a new common_constants_util_win
> > target (for lack of a better name) that depends only on installer_util?
> (adding
> > it straight in installer_util also feels out of place)
> > 
> > Then there are no new real dependencies added and we avoid common_constants
> > depending on installer_util which I agree is wrong.
> > 
> > WDYT?
> 
> ping for opinion on the above suggestion, thanks

re-ping :-)

[gab, 2016-04-12 19:11:23]

Patchset #10 (id:260001) has been deleted

[gab, 2016-04-12 19:21:08]

@grt added metrics_constants_util_win target as discussed, but for some reason I
don't understand chrome.dll doesn't link (unresolved externals errors for the
two new calls -- other libraries work fine), both in GYP and GN builds, I've
been starring at it for a long time now and can't figure it out, could you
double-check that I'm doing this right?

Thanks,
Gab

https://codereview.chromium.org/1630923002/diff/300001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/1630923002/diff/300001/chrome/BUILD.gn#newcod...
chrome/BUILD.gn:137: "//chrome/common:constants",
Adding previously missing 'common_constants' dependency, ref:
https://codereview.chromium.org/1630923002/#msg33

https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (left):

https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:101: ['OS == "win"', {
Duplicate condition, merged with other one below.

https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:396: 'common_constants',
GYP-side of " Adding previously missing 'common_constants' dependency, ref:
https://codereview.chromium.org/1630923002/#msg33 "

[gab, 2016-04-14 20:37:44]

On 2016/04/12 19:21:08, gab wrote:
> @grt added metrics_constants_util_win target as discussed, but for some reason
I
> don't understand chrome.dll doesn't link (unresolved externals errors for the
> two new calls -- other libraries work fine), both in GYP and GN builds, I've
> been starring at it for a long time now and can't figure it out, could you
> double-check that I'm doing this right?

Figured out GYP error (http://crbug.com/603603) and fixed it. GN is still a
problem though, but I don't understand why. Kicked off chromium-dev thread for
it...

> 
> Thanks,
> Gab
> 
> https://codereview.chromium.org/1630923002/diff/300001/chrome/BUILD.gn
> File chrome/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1630923002/diff/300001/chrome/BUILD.gn#newcod...
> chrome/BUILD.gn:137: "//chrome/common:constants",
> Adding previously missing 'common_constants' dependency, ref:
> https://codereview.chromium.org/1630923002/#msg33
> 
> https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi
> File chrome/chrome_exe.gypi (left):
> 
>
https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi...
> chrome/chrome_exe.gypi:101: ['OS == "win"', {
> Duplicate condition, merged with other one below.
> 
> https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi
> File chrome/chrome_exe.gypi (right):
> 
>
https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi...
> chrome/chrome_exe.gypi:396: 'common_constants',
> GYP-side of " Adding previously missing 'common_constants' dependency, ref:
> https://codereview.chromium.org/1630923002/#msg33 "

[gab, 2016-04-15 15:21:15]

On 2016/04/14 20:37:44, gab wrote:
> On 2016/04/12 19:21:08, gab wrote:
> > @grt added metrics_constants_util_win target as discussed, but for some
reason
> I
> > don't understand chrome.dll doesn't link (unresolved externals errors for
the
> > two new calls -- other libraries work fine), both in GYP and GN builds, I've
> > been starring at it for a long time now and can't figure it out, could you
> > double-check that I'm doing this right?
> 
> Figured out GYP error (http://crbug.com/603603) and fixed it. GN is still a
> problem though, but I don't understand why. Kicked off chromium-dev thread for
> it...

Ok finally figured it out (the gypi_to_gn.py operation on chrome_browser.gypi
only imports file lists and doesn't import dependencies, so for the "browser"
target I still had to add the dependency to chrome/browser/BUILD.gn despite
chrome_metrics_service_client.cc and chrome_stability_metrics_provider.cc only
being listed in chrome_browser.gypi -- breaking my rule of
add-the-dependency-to-every-target-listing-the-depending-cc-files-themselves...).

@grt: ready for review now :-)

> 
> > 
> > Thanks,
> > Gab
> > 
> > https://codereview.chromium.org/1630923002/diff/300001/chrome/BUILD.gn
> > File chrome/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1630923002/diff/300001/chrome/BUILD.gn#newcod...
> > chrome/BUILD.gn:137: "//chrome/common:constants",
> > Adding previously missing 'common_constants' dependency, ref:
> > https://codereview.chromium.org/1630923002/#msg33
> > 
> >
https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi
> > File chrome/chrome_exe.gypi (left):
> > 
> >
>
https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi...
> > chrome/chrome_exe.gypi:101: ['OS == "win"', {
> > Duplicate condition, merged with other one below.
> > 
> >
https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi
> > File chrome/chrome_exe.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1630923002/diff/300001/chrome/chrome_exe.gypi...
> > chrome/chrome_exe.gypi:396: 'common_constants',
> > GYP-side of " Adding previously missing 'common_constants' dependency, ref:
> > https://codereview.chromium.org/1630923002/#msg33 "

[grt (UTC plus 2), 2016-04-15 15:29:33]

looks good

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3469: 'file_pre_reader',
how about putting the metrics_constants_util_win dependency down here?

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3840: 'dependencies': [
ignore previous comment. don't the metrics_constants_util_win and
file_pre_reader dependencies really belong here?

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_common.g...
chrome/chrome_common.gypi:660: # GN version:
//chrome/common:metrics_constants_util_win
i think it's entirely appropriate to put this in an OS=="win" conditional

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (left):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:101: ['OS == "win"', {
nice cleanup here!

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:445: ['win_console_app==1', {
pull this up and out of the msvs_settings block

https://codereview.chromium.org/1630923002/diff/360001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:354: # GYP version:
chrome/chrome_common.gypi:metrics_constants_util_win
how about defining this in an is_win block?

[gab, 2016-04-15 18:14:07]

Thanks, PTAL.

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3469: 'file_pre_reader',
On 2016/04/15 15:29:32, grt wrote:
> how about putting the metrics_constants_util_win dependency down here?

Replied below.

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3840: 'dependencies': [
On 2016/04/15 15:29:32, grt wrote:
> ignore previous comment. don't the metrics_constants_util_win and
> file_pre_reader dependencies really belong here?

No, I put the dependency in the same target that includes the
"chrome_browser_metrics_sources" (which include
chrome/browser/metrics/(chrome_metrics_service_client|chrome_stability_metrics_provider).cc)
as this is why this dependency is required in the first place.

The rule I've been following is
add-dependency-to-all-targets-that-include-cc-files-that-depend-on-it which
makes this one land in the != ios block.

I think this is correct even if the _win rules only subtract files not
dependencies. Worst case it means including an empty target on other platforms.
Another option is to define the metrics_constants_util_win target in a win-only
condition, but then we can only use it from win-only blocks and end up with more
ifs (i.e. exactly what auto _win.cc subtraction is meant to avoid).

EDIT: (left old text for context but changed my mind...) Okay I can now see why
we'd want all of this in a win-only section, but I still find it weird to have
the dependencies and sources split.

Done.

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_common.g...
chrome/chrome_common.gypi:660: # GN version:
//chrome/common:metrics_constants_util_win
On 2016/04/15 15:29:32, grt wrote:
> i think it's entirely appropriate to put this in an OS=="win" conditional

Didn't initially think that was the right way but gave it a shot now in latest
patch set.

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (left):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:101: ['OS == "win"', {
On 2016/04/15 15:29:32, grt wrote:
> nice cleanup here!

Ack :-)

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:445: ['win_console_app==1', {
On 2016/04/15 15:29:32, grt wrote:
> pull this up and out of the msvs_settings block

Done.

https://codereview.chromium.org/1630923002/diff/360001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/1630923002/diff/360001/chrome/common/BUILD.gn...
chrome/common/BUILD.gn:354: # GYP version:
chrome/chrome_common.gypi:metrics_constants_util_win
On 2016/04/15 15:29:32, grt wrote:
> how about defining this in an is_win block?

Done. The dependency for chrome_initial target was already in an is_win block,
the chrome_watcher one isn't but the whole target is win-only (it just isn't
explicit, nor is it in GYP, and I'm not going to address that), the new
'browser' dependency wasn't but I moved it to the is_win block too.

[grt (UTC plus 2), 2016-04-15 18:51:03]

lgtm!

[gab, 2016-04-15 19:12:11]

The CQ bit was checked by gab@chromium.org

[gab, 2016-04-15 19:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1630923002/#ps420001
(title: "de-dup is_win blocks in GN too")

[commit-bot: I haz the power, 2016-04-15 19:12:32]

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

[commit-bot: I haz the power, 2016-04-15 19:17:19]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on data stored in the previous
kBrowserCrashDumpAttemptsRegistryPath will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side per them sharing that key.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on data stored in the previous
kBrowserCrashDumpAttemptsRegistryPath will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side per them sharing that key.

BUG=577820
==========

[commit-bot: I haz the power, 2016-04-15 19:17:20]

Committed patchset #17 (id:420001)

[commit-bot: I haz the power, 2016-04-15 19:18:26]

Description was changed from

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on data stored in the previous
kBrowserCrashDumpAttemptsRegistryPath will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable) 
and Canary installed side-by-side per them sharing that key.

BUG=577820
==========

to

==========
Remove PRODUCT_STRING_PATH from chrome_constants.h on Windows.

Its last usage was from crash and exit code reporting registry keys
inside chrome_constants.cc which have now been replaced by
distribution friendly (e.g. Google Chrome versus Canary) helpers.

kBrowserCrashDumpAttemptsRegistryPath was previously handling Canary
through a hard-coded Google\Chrome key but will now properly use a
"Google\Chrome SxS" key per its distribution.

Note 1: This means that Canary stats depending on data stored in the previous
kBrowserCrashDumpAttemptsRegistryPath will be reset when this CL lands.

Note 2: kBrowserExitCodesRegistryPath wasn't handling Canary which means
that reports generated from these entries could have previously been
incorrect on machines that had both Google Chrome (Dev/Beta/Stable)
and Canary installed side-by-side per them sharing that key.

BUG=577820

Committed: https://crrev.com/7c45cfded56e2f0ee3f0d04ba9d926844c70e499
Cr-Commit-Position: refs/heads/master@{#387669}
==========

[commit-bot: I haz the power, 2016-04-15 19:18:28]

Patchset 17 (id:??) landed as
https://crrev.com/7c45cfded56e2f0ee3f0d04ba9d926844c70e499
Cr-Commit-Position: refs/heads/master@{#387669}