Move crash keys for command-line switches to components/crash so they can be set

Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552
Committed: https://crrev.com/0edd36dd4ac523ad6989fd2e387b15c68d22379e
Cr-Commit-Position: refs/heads/master@{#362437}

[Joe Mason, 2015-11-27 15:11:09]

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

[commit-bot: I haz the power, 2015-11-27 15:11:46]

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

[commit-bot: I haz the power, 2015-11-27 15:11:47]

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

[commit-bot: I haz the power, 2015-11-27 15:11:48]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Joe Mason, 2015-11-27 15:28:02]

joenotcharles@chromium.org changed reviewers:
+ grt@chromium.org, rsesek@chromium.org

[Joe Mason, 2015-11-27 15:28:02]

grt@chromium.org: Please review changes in chrome/installer

rsesek@chromium.org: Please review changes in chrome/common and components/crash

[Joe Mason, 2015-11-27 15:31:09]

Description was changed from

==========
Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552
==========

to

==========
Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552
==========

[grt (UTC plus 2), 2015-11-27 18:51:21]

code reuse FTW!

https://codereview.chromium.org/1481933002/diff/1/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/1481933002/diff/1/chrome/common/crash_keys.cc...
chrome/common/crash_keys.cc:255: return
SetSwitchesFromCommandLine(*command_line,
hmm, the name collision here is a bit confusing. wdyt of naming the one in
components/crash something like SetFilteredSwitchesFromCommandLine? ooh, i like
SetCrashKeysFromCommandLine in installer. how about using that here, too?

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:112: static char
formatted_keys[kSwitchesMaxCount][sizeof(kSwitch) + 1] = {{ 0 }};
oh my. this is an accident waiting to happen. sizeof(kSwitch) is
platform-dependent (PRIuS could be any length). it just so happens that it's two
chars on all current platforms, but that's fragile. sigh. with the current
values, sizeof(kswitch) - 1 is really all the space that is needed. since you're
copying existing code, i'll just look the other way. you know, how about adding
this:
  static_assert(sizeof(formatted_keys[0]) >= 10,
                "insufficient space for \"switch-NN\"");

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:160: if (i >
crash_keys::kSwitchesMaxCount)
i -> key_i

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:168: for (; key_i <=
kSwitchesMaxCount; ++key_i) {
nit: omit braces. :-)

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:35: void
SetSwitchesFromCommandLine(const base::CommandLine& command_line);
rather than overloading the function, i think it's better to have callers pass
in either an empty callback or a null function pointer (my preference).

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:40: const base::Callback<bool(const
std::string &)>& skip_filter);
callback seems like overkill. how about starting with a simple function pointer
for now? this can be expanded to a callback in the future if needed.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:92: extern const char kSwitch[];
may as well document this for what it is:
// A printf-style format string naming the set of crash keys corresponding to at
most |kSwitchesMaxCount| command line switches.

i wouldn't object to you renaming this to kSwitchFormat or something like that
since it's a format string rather than an actual crash key name.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:93: // The total number of switches,
used to report the total in case more than
nit: blank line before comment

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys_unittest.cc (right):

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys_unittest.cc:108: static bool
IsBoringFlag(const std::string& flag) {
nit: put this in an unnamed namespace rather than using static.

[Joe Mason, 2015-11-27 21:05:41]

Address review comments

[Joe Mason, 2015-11-27 21:16:47]

https://codereview.chromium.org/1481933002/diff/1/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/1481933002/diff/1/chrome/common/crash_keys.cc...
chrome/common/crash_keys.cc:255: return
SetSwitchesFromCommandLine(*command_line,
On 2015/11/27 18:51:21, grt wrote:
> hmm, the name collision here is a bit confusing. wdyt of naming the one in
> components/crash something like SetFilteredSwitchesFromCommandLine? ooh, i
like
> SetCrashKeysFromCommandLine in installer. how about using that here, too?

Done.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:112: static char
formatted_keys[kSwitchesMaxCount][sizeof(kSwitch) + 1] = {{ 0 }};
On 2015/11/27 18:51:21, grt wrote:
> oh my. this is an accident waiting to happen. sizeof(kSwitch) is
> platform-dependent (PRIuS could be any length). it just so happens that it's
two
> chars on all current platforms, but that's fragile. sigh. with the current
> values, sizeof(kswitch) - 1 is really all the space that is needed. since
you're
> copying existing code, i'll just look the other way. you know, how about
adding
> this:
>   static_assert(sizeof(formatted_keys[0]) >= 10,
>                 "insufficient space for \"switch-NN\"");

Done.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:160: if (i >
crash_keys::kSwitchesMaxCount)
On 2015/11/27 18:51:21, grt wrote:
> i -> key_i

Good catch! Fixed.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.cc:168: for (; key_i <=
kSwitchesMaxCount; ++key_i) {
On 2015/11/27 18:51:21, grt wrote:
> nit: omit braces. :-)

Done.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:35: void
SetSwitchesFromCommandLine(const base::CommandLine& command_line);
On 2015/11/27 18:51:21, grt wrote:
> rather than overloading the function, i think it's better to have callers pass
> in either an empty callback or a null function pointer (my preference).

Done.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:40: const base::Callback<bool(const
std::string &)>& skip_filter);
On 2015/11/27 18:51:21, grt wrote:
> callback seems like overkill. how about starting with a simple function
pointer
> for now? this can be expanded to a callback in the future if needed.

Done.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:92: extern const char kSwitch[];
On 2015/11/27 18:51:21, grt wrote:
> may as well document this for what it is:
> // A printf-style format string naming the set of crash keys corresponding to
at
> most |kSwitchesMaxCount| command line switches.
> 
> i wouldn't object to you renaming this to kSwitchFormat or something like that
> since it's a format string rather than an actual crash key name.

Done.

https://codereview.chromium.org/1481933002/diff/1/components/crash/core/commo...
components/crash/core/common/crash_keys.h:93: // The total number of switches,
used to report the total in case more than
On 2015/11/27 18:51:21, grt wrote:
> nit: blank line before comment

Done.

[grt (UTC plus 2), 2015-11-27 21:32:53]

lgtm with a niggle

https://codereview.chromium.org/1481933002/diff/20001/components/crash/core/c...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/20001/components/crash/core/c...
components/crash/core/common/crash_keys.h:41: SwitchFilterFunction skip_filter =
nullptr);
in general, default arguments are banned
(https://google.github.io/styleguide/cppguide.html#Default_Arguments). i think
it's fine to pass nullptr explicitly at the one callsite. if that unappealing to
you, then i think the overload is better. sorry for the back-and-forth.

[Joe Mason, 2015-11-27 22:26:36]

Get rid of default argument

[Joe Mason, 2015-11-27 22:27:13]

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

[Joe Mason, 2015-11-27 22:27:14]

jochen@chromium.org: Please review changes in
chrome/browser/chrome_browser_main.cc

[Joe Mason, 2015-11-27 22:29:14]

https://codereview.chromium.org/1481933002/diff/20001/components/crash/core/c...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/20001/components/crash/core/c...
components/crash/core/common/crash_keys.h:41: SwitchFilterFunction skip_filter =
nullptr);
On 2015/11/27 21:32:53, grt wrote:
> in general, default arguments are banned
> (https://google.github.io/styleguide/cppguide.html#Default_Arguments). i think
> it's fine to pass nullptr explicitly at the one callsite. if that unappealing
to
> you, then i think the overload is better. sorry for the back-and-forth.

Done.

[grt (UTC plus 2), 2015-11-28 02:04:30]

still lgtm

https://codereview.chromium.org/1481933002/diff/40001/chrome/common/crash_keys.h
File chrome/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/40001/chrome/common/crash_key...
chrome/common/crash_keys.h:25: // Sets the kNumSwitches key and a set of keys
named using kSwitchFormat based
comment nit to be addressed if you need to make other changes:
"the set of keys named" rather than "a set of keys named" since the set of names
is fixed (there can be only one).

https://codereview.chromium.org/1481933002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/installer_crash_reporting.cc (right):

https://codereview.chromium.org/1481933002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/installer_crash_reporting.cc:131:
std::vector<base::debug::CrashKey> keys(std::begin(kFixedKeys),
#include <iterator> for std::begin and std::end
#include <vector> for std::vector

[grt (UTC plus 2), 2015-11-30 13:50:16]

Turns out there are some mismatched types causing compile failures (see
https://codereview.chromium.org/1478273003/#ps20001).

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
File components/crash/core/common/crash_keys_unittest.cc (right):

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:79: const int kMax =
crash_keys::kSwitchesMaxCount + 2;
int -> size_t for the sake of kSwitchesMaxCount

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:81: for (int i = 1; i <=
kMax; ++i)
int -> size_t here, too

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:125: for (int i = 2; i <=
kSwitchesMaxCount; ++i)
int -> size_t

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:132: for (int i = 1; i <=
kSwitchesMaxCount; ++i) {
int -> size_t

[jochen (gone - plz use gerrit), 2015-11-30 15:58:41]

I defer to rsesek

[Robert Sesek, 2015-11-30 16:08:12]

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys.cc:107: // The fixed_keys names are
string constants. Use static storage for
|fixed_keys| ?

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys.h:35: typedef bool
(*SwitchFilterFunction)(const std::string &flag);
using directive is preferred. Also, & goes with the type.

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys_unittest.cc (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:27:
crash_keys::kChunkMaxLength);
EXPECT that |keys| is not empty?

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:121:
command_line.AppendSwitch("--boring");
Should also have a test with more switches after --boring so that you ensure
that the keys are numbered correctly.

[Joe Mason, 2015-11-30 17:23:56]

https://codereview.chromium.org/1481933002/diff/40001/chrome/common/crash_keys.h
File chrome/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/40001/chrome/common/crash_key...
chrome/common/crash_keys.h:25: // Sets the kNumSwitches key and a set of keys
named using kSwitchFormat based
On 2015/11/28 02:04:30, grt wrote:
> comment nit to be addressed if you need to make other changes:
> "the set of keys named" rather than "a set of keys named" since the set of
names
> is fixed (there can be only one).

Done.

https://codereview.chromium.org/1481933002/diff/40001/chrome/installer/setup/...
File chrome/installer/setup/installer_crash_reporting.cc (right):

https://codereview.chromium.org/1481933002/diff/40001/chrome/installer/setup/...
chrome/installer/setup/installer_crash_reporting.cc:131:
std::vector<base::debug::CrashKey> keys(std::begin(kFixedKeys),
On 2015/11/28 02:04:30, grt wrote:
> #include <iterator> for std::begin and std::end
> #include <vector> for std::vector

Done.

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
File components/crash/core/common/crash_keys_unittest.cc (right):

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:79: const int kMax =
crash_keys::kSwitchesMaxCount + 2;
On 2015/11/30 13:50:16, grt wrote:
> int -> size_t for the sake of kSwitchesMaxCount

Done. I changed all the for loops in this file to use size_t, for consistency.

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:81: for (int i = 1; i <=
kMax; ++i)
On 2015/11/30 13:50:16, grt wrote:
> int -> size_t here, too

Done.

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:125: for (int i = 2; i <=
kSwitchesMaxCount; ++i)
On 2015/11/30 13:50:16, grt wrote:
> int -> size_t

Done.

https://codereview.chromium.org/1481933002/diff/40001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:132: for (int i = 1; i <=
kSwitchesMaxCount; ++i) {
On 2015/11/30 13:50:16, grt wrote:
> int -> size_t

Done.

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys.cc:107: // The fixed_keys names are
string constants. Use static storage for
On 2015/11/30 16:08:12, Robert Sesek wrote:
> |fixed_keys| ?

Actually the first part of the comment is obsolete since fixed_keys was part of
the function this was pulled out of. Reworded it.

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys_unittest.cc (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:27:
crash_keys::kChunkMaxLength);
On 2015/11/30 16:08:12, Robert Sesek wrote:
> EXPECT that |keys| is not empty?

Done.

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:121:
command_line.AppendSwitch("--boring");
On 2015/11/30 16:08:12, Robert Sesek wrote:
> Should also have a test with more switches after --boring so that you ensure
> that the keys are numbered correctly.

Not sure what you're asking for? This test already does have more switches after
--boring, and it does check that keys are numbered correctly.

[Joe Mason, 2015-11-30 17:27:36]

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys.h:35: typedef bool
(*SwitchFilterFunction)(const std::string &flag);
On 2015/11/30 16:08:12, Robert Sesek wrote:
> using directive is preferred. Also, & goes with the type.

Missed this comment. How would a using directive be used in this context?

[Robert Sesek, 2015-11-30 17:32:18]

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys.h:35: typedef bool
(*SwitchFilterFunction)(const std::string &flag);
On 2015/11/30 17:27:36, joenotcharles wrote:
> On 2015/11/30 16:08:12, Robert Sesek wrote:
> > using directive is preferred. Also, & goes with the type.
> 
> Missed this comment. How would a using directive be used in this context?

using SwitchFilterFunction = bool (*)(const std::string& flag);

[Robert Sesek, 2015-11-30 17:33:18]

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys_unittest.cc (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys_unittest.cc:121:
command_line.AppendSwitch("--boring");
On 2015/11/30 17:23:55, joenotcharles wrote:
> On 2015/11/30 16:08:12, Robert Sesek wrote:
> > Should also have a test with more switches after --boring so that you ensure
> > that the keys are numbered correctly.
> 
> Not sure what you're asking for? This test already does have more switches
after
> --boring, and it does check that keys are numbered correctly.

Sorry, I had deleted that comment, but I guess AppEngine didn't ack.

[Joe Mason, 2015-11-30 18:37:23]

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
File components/crash/core/common/crash_keys.h (right):

https://codereview.chromium.org/1481933002/diff/60001/components/crash/core/c...
components/crash/core/common/crash_keys.h:35: typedef bool
(*SwitchFilterFunction)(const std::string &flag);
On 2015/11/30 17:32:18, Robert Sesek wrote:
> On 2015/11/30 17:27:36, joenotcharles wrote:
> > On 2015/11/30 16:08:12, Robert Sesek wrote:
> > > using directive is preferred. Also, & goes with the type.
> > 
> > Missed this comment. How would a using directive be used in this context?
> 
> using SwitchFilterFunction = bool (*)(const std::string& flag);

Done.

[Robert Sesek, 2015-11-30 20:32:11]

LGTM

[jochen (gone - plz use gerrit), 2015-12-01 13:46:09]

lgtm

[Joe Mason, 2015-12-01 16:28:06]

The CQ bit was checked by joenotcharles@chromium.org

[Joe Mason, 2015-12-01 16:28:07]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1481933002/#ps160001
(title: "typedef -> using, %lu -> PRIuS")

[commit-bot: I haz the power, 2015-12-01 16:29:48]

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

[commit-bot: I haz the power, 2015-12-01 16:57:38]

Description was changed from

==========
Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552
==========

to

==========
Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552
==========

[commit-bot: I haz the power, 2015-12-01 16:57:41]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-01 16:58:24]

Description was changed from

==========
Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552
==========

to

==========
Move crash keys for command-line switches to components/crash so they can be set
from the installer as well as chrome, and set them on installer startup.

BUG=558552

Committed: https://crrev.com/0edd36dd4ac523ad6989fd2e387b15c68d22379e
Cr-Commit-Position: refs/heads/master@{#362437}
==========

[commit-bot: I haz the power, 2015-12-01 16:58:25]

Patchset 9 (id:??) landed as
https://crrev.com/0edd36dd4ac523ad6989fd2e387b15c68d22379e
Cr-Commit-Position: refs/heads/master@{#362437}