Move shared constants to //components/chrome_cleaner

Move shared constants to //components/chrome_cleaner

BUG=709035
Review-Url: https://codereview.chromium.org/2812223003
Cr-Commit-Position: refs/heads/master@{#465380}
Committed: https://chromium.googlesource.com/chromium/src/+/6e1d09bf3327be410524f9d594c7c218b5845de3

[ftirelo, 2017-04-12 14:39:48]

Description was changed from

==========
Move shared constants to //components/chrome_cleaner

BUG=
==========

to

==========
Move shared constants to //components/chrome_cleaner

BUG=709035
==========

[ftirelo, 2017-04-12 14:58:03]

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

[ftirelo, 2017-04-12 14:58:14]

ftirelo@chromium.org changed reviewers:
+ joenotcharles@chromium.org

[commit-bot: I haz the power, 2017-04-12 14:58:28]

Dry run: 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-12 15:10:46]

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

[commit-bot: I haz the power, 2017-04-12 15:10:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ftirelo, 2017-04-12 15:20:12]

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

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

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

[joenotcharles, 2017-04-12 15:44:41]

joenotcharles@google.com changed reviewers:
+ joenotcharles@google.com

[joenotcharles, 2017-04-12 15:44:42]

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:1449: "//components/chrome_cleaner/constants",
Should we make this public/contants to match public/interfaces?

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right):

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:494:
IN_PROC_BROWSER_TEST_F(SRTFetcherTest, NothingFound) {
Bad merge here.

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:500:
IN_PROC_BROWSER_TEST_F(SRTFetcherTest, CleanupNeeded) {
Bad merge here.

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:717:
IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
Bad merge here.

[commit-bot: I haz the power, 2017-04-12 17:17:09]

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

[commit-bot: I haz the power, 2017-04-12 17:17:10]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ftirelo, 2017-04-12 19:40:41]

PTAL

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:1449: "//components/chrome_cleaner/constants",
On 2017/04/12 15:44:42, joenotcharles wrote:
> Should we make this public/contants to match public/interfaces?

Done.

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right):

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:494:
IN_PROC_BROWSER_TEST_F(SRTFetcherTest, NothingFound) {
On 2017/04/12 15:44:42, joenotcharles wrote:
> Bad merge here.

Done.

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:500:
IN_PROC_BROWSER_TEST_F(SRTFetcherTest, CleanupNeeded) {
On 2017/04/12 15:44:42, joenotcharles wrote:
> Bad merge here.

Done.

https://codereview.chromium.org/2812223003/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:717:
IN_PROC_BROWSER_TEST_F(SRTFetcherTest, ReporterLogging_NoSBExtendedReporting) {
On 2017/04/12 15:44:42, joenotcharles wrote:
> Bad merge here.

Done.

[Joe Mason, 2017-04-13 17:54:02]

lgtm with nits

https://codereview.chromium.org/2812223003/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_global_error_win.cc (right):

https://codereview.chromium.org/2812223003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_global_error_win.cc:57: // Cleanup Tool to
indicate how the user interacted with the accept button.
Does it make sense to put these in public/constants too?

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
File components/chrome_cleaner/public/constants/constants.cc (right):

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.cc:38: const wchar_t
kStartTimeValueName[] = L"StartTime";
Nit: either put a blank line between all of these, or none of them. (I would
prefer this whole file have no blank lines between constants, just a blank
between the switches and paths and one between the paths and the values.

Also please sort these the same way they're sorted in the header file.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.cc:45: const wchar_t
kFoundUwsValueName[] = L"FoundUws";
Nit: please add a comment saying that the lower-case s can't be changed for
compatibility with older versions, so that people looking at this code in the
future don't "fix" it.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
File components/chrome_cleaner/public/constants/constants.h (right):

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:17: // 3: beta; 4:
stable.
Nit: I wouldn't list the exact values here. It can be looked up in
version_info::Channel, and if more channels are added the comment will get out
of date.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:23: // The switch to be
passed to the Software Reporter process with the Mojo pipe
Nit: The comment at the top of the namespace already says this is a switch for
the Software Reporter process. I would just say "The Mojo pipe token for IPC
communication between the Software Reporter and Chrome."

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:38: extern const char
kEnableCrashReporting[];
Nit: rename this kEnableCrashReportingSwitch while we're making changes?

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:59: extern const wchar_t
kCleanerSuffixRegistryKey[];
Nit: I prefer the other name, kCleanerSubKey, since this isn't a full key path.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:61: // Registry values
names where metrics are written to.
Nit: "value names", I think.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:68: extern const wchar_t
kScanTimesSubKey[];
Nit: This should go in the "registry paths" section above.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:74: const int
kSwReporterCleanupNeeded = 0;
Nit: these should be constexpr.

[ftirelo, 2017-04-18 15:26:48]

ftirelo@chromium.org changed reviewers:
+ jialiul@chromium.org
- joenotcharles@google.com

[ftirelo, 2017-04-18 15:26:49]

https://codereview.chromium.org/2812223003/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_global_error_win.cc (right):

https://codereview.chromium.org/2812223003/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_global_error_win.cc:57: // Cleanup Tool to
indicate how the user interacted with the accept button.
On 2017/04/13 17:54:01, Joe Mason wrote:
> Does it make sense to put these in public/constants too?

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
File components/chrome_cleaner/public/constants/constants.cc (right):

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.cc:38: const wchar_t
kStartTimeValueName[] = L"StartTime";
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: either put a blank line between all of these, or none of them. (I would
> prefer this whole file have no blank lines between constants, just a blank
> between the switches and paths and one between the paths and the values.
> 
> Also please sort these the same way they're sorted in the header file.

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.cc:45: const wchar_t
kFoundUwsValueName[] = L"FoundUws";
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: please add a comment saying that the lower-case s can't be changed for
> compatibility with older versions, so that people looking at this code in the
> future don't "fix" it.

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
File components/chrome_cleaner/public/constants/constants.h (right):

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:17: // 3: beta; 4:
stable.
On 2017/04/13 17:54:02, Joe Mason wrote:
> Nit: I wouldn't list the exact values here. It can be looked up in
> version_info::Channel, and if more channels are added the comment will get out
> of date.

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:23: // The switch to be
passed to the Software Reporter process with the Mojo pipe
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: The comment at the top of the namespace already says this is a switch for
> the Software Reporter process. I would just say "The Mojo pipe token for IPC
> communication between the Software Reporter and Chrome."

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:38: extern const char
kEnableCrashReporting[];
On 2017/04/13 17:54:02, Joe Mason wrote:
> Nit: rename this kEnableCrashReportingSwitch while we're making changes?

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:59: extern const wchar_t
kCleanerSuffixRegistryKey[];
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: I prefer the other name, kCleanerSubKey, since this isn't a full key
path.

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:61: // Registry values
names where metrics are written to.
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: "value names", I think.

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:68: extern const wchar_t
kScanTimesSubKey[];
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: This should go in the "registry paths" section above.

Done.

https://codereview.chromium.org/2812223003/diff/60001/components/chrome_clean...
components/chrome_cleaner/public/constants/constants.h:74: const int
kSwReporterCleanupNeeded = 0;
On 2017/04/13 17:54:01, Joe Mason wrote:
> Nit: these should be constexpr.

Done.

[ftirelo, 2017-04-18 15:27:28]

ftirelo@chromium.org changed reviewers:
+ waffles@chromium.org

[ftirelo, 2017-04-18 15:27:28]

+jialiu +waffles for OWNERS approvals.

[ftirelo, 2017-04-18 15:32:50]

ftirelo@chromium.org changed reviewers:
+ sorin@chromium.org
- waffles@chromium.org

[ftirelo, 2017-04-18 15:32:51]

-waffles +sorin for OWNERS approval, since waffles@ is OOO

[Sorin Jianu, 2017-04-18 16:13:47]

lgtm

Thank you!
component updater lgtm

[Sorin Jianu, 2017-04-18 16:13:49]

lgtm

Thank you!
component updater lgtm

[Jialiu Lin, 2017-04-18 19:48:45]

lgtm

[ftirelo, 2017-04-18 20:24:43]

The CQ bit was checked by ftirelo@chromium.org

[ftirelo, 2017-04-18 20:24:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
joenotcharles@chromium.org
Link to the patchset: https://codereview.chromium.org/2812223003/#ps80001
(title: "Code review")

[commit-bot: I haz the power, 2017-04-18 20:25: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-18 21:32:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492547083394230,
"parent_rev": "c38c29e5447a9e3b8188dcecd343a7a8533d3d0d", "commit_rev":
"6e1d09bf3327be410524f9d594c7c218b5845de3"}

[commit-bot: I haz the power, 2017-04-18 21:34:00]

Description was changed from

==========
Move shared constants to //components/chrome_cleaner

BUG=709035
==========

to

==========
Move shared constants to //components/chrome_cleaner

BUG=709035

Review-Url: https://codereview.chromium.org/2812223003
Cr-Commit-Position: refs/heads/master@{#465380}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e1d09bf3327be410524f9d594c7...
==========

[commit-bot: I haz the power, 2017-04-18 21:34:01]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6e1d09bf3327be410524f9d594c7...