I would be inclined to use the same separator for tests ':', which may be platform specific.

@sky - you're suggesting that we use the same separate for files as we do for tests? 

I wouldn't do that. I'd prefer it if we supported specifying the same flag multiple times, e.g. `--test-launcher-filter-file=foo --test-launcher-filter-file=bar`, which would be consistent with how argument parsing works in python. I don't think base::switches has built-in support for this so you'd probably have to do a bit of extra work to make it work.

Why not support multiple files, just like --gtest_filter supports multiple patterns.

Thanks for the suggestions!

Re #c2: Do we have any resolutions or discussions on "specifying the same flag multiple times" is the style we want to go?
IIUC currently |base::CommandLine| stores switches in a one-to-one map, which means one switch overrides the other if they share the same key[1]. (+msw@ who worked on this before)
This behavior was here since at least 2009[2], and I believe we'd need a strong reason to change it due to the potential wider impact.

Or am I misunderstanding something?

Thanks!

[1] https://cs.chromium.org/chromium/src/base/command_line.cc?l=340&rcl=207efcc65ae99a235e88cc6b8208e8da19f4c23c
[2] https://chromium.googlesource.com/chromium/src/+blame/87ba2e6056bfe771ddc6ce540cf3d4e413d4e6f4/base/command_line.cc#138

I believe your characterization of base::CommandLine is correct. Personally I consider this just to be a missing feature, since most command-line-parsing libraries support using the same flag multiple times, and that's certainly a more common convention than trying to use separators inside the arg value.

@sky - along the same line of reasoning, I'd would normally encourage you to have to specify --gtest_filter multiple times. I do agree that it gets more cumbersome as the filter gets even longer, and I don't mind using delimiters inside the value too much for that field. 

Offhand, I can't think of a case in other executables where you'd do that for multiple filenames, though, so that feels much more unusual.

Re #c5: I did some experiments and noticed a few test failures[1] if we change override => append.

Most notably it feels to me that "Display all previews related flag info on chrome://interventions-internals." (1d9c78282fd1ed10842efea09878255fbdc5fc3d) is explicitly relying on the override behavior to force a switch.

I'd argue that we couldn't simply mock python or other executables's arg style since Chrome may need to modify switches at multiple layers.

Does that make sense?

Thanks!

[1] https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/182687

No, I'm afraid you lost me here. 

Are you talking about changing the flag behavior so that if a flag is specified twice, you append (forming a list) rather that overriding?

If so, I would believe some flags want to be a list, and some want to be a single value, and so I wouldn't expect you to be able to safely use one or the other. 

I would also expect that if we were actually doing something that relied on overriding flags then that'd be a bug, though, i.e., someone's probably doing something wrong. But I wouldn't want to fight that fight for the sake of this bug.

Re #c7:

> Are you talking about changing the flag behavior so that if a flag is specified twice, you append (forming a list) rather that overriding?

Yes, my original assumption was that you want to change the switch for ALL keys from:
```
std::map<std::string, StringType> switches_;
```
To
```
std::map<std::string, std::list<StringType>> switches_;
```


> If so, I would believe some flags want to be a list, and some want to be a single value, and so I wouldn't expect you to be able to safely use one or the other. 

IIUC are you suggesting something like:
```
const std::vector<std::string> kListValueSwitches;

std::map<std::string, StringType> single_value_switches_;
std::map<std::string, std::list<StringType>> list_value_switches_;
```

I don't see other ways of telling if a key is a list-value-switch other than baking |kListValueSwitches| into source code.
This approach feels really complex for me and I'm still not convinced why we want to change the style while other Chrome switches are either using ',' or ':'. 

> I would also expect that if we were actually doing something that relied on overriding flags then that'd be a bug, though, i.e., someone's probably doing something wrong. But I wouldn't want to fight that fight for the sake of this bug.

I'm not sure what we want to do here, but I agree this should be considered as a separate issue.

Are there switches *besides* --gtest_filter that take a list of values?

Yes, here is a few found under //src/testing/:
https://cs.chromium.org/search/?q=%5C-%5C-%5Ba-z-%5D%2B%3D%5Ba-zA-Z0-9-._/%5C%5C%5D%2B%5B,%5D%5Ba-zA-Z0-9-._/%5C%5C%5D+pcre:yes+file:%5Esrc/testing/+package:%5Echromium$&type=cs

More specifically they are:

--benchmarks:
https://cs.chromium.org/chromium/src/testing/scripts/run_performance_tests.py?l=185&rcl=82ef3030f829530ac4ac9ff7c34798c2d2ce84f9

--enable-features:
--disable-features:
https://cs.chromium.org/chromium/src/base/feature_list.h?l=78&rcl=82ef3030f829530ac4ac9ff7c34798c2d2ce84f9

--replace-system-package:
https://cs.chromium.org/chromium/src/testing/buildbot/chromium.fyi.json?l=14&rcl=82ef3030f829530ac4ac9ff7c34798c2d2ce84f9

If you use a ':' separator as I suggested above you don't need to change CommandLine at all.

Re #c11: Thanks for the suggestion and sorry I didn't emphasize it before.

I did prefer the ':' separator approached you've suggested, but I was under the impression that dpranke@ is objecting to that idea, and I want to make sure we are on the same page before moving forward.

Here is my sample ':' separator patch FYI:
https://crrev.com/c/1213866

--benchmarks in run_performance_tests.py isn't a great example, because that's a python script :). The others are good examples, though.

I remembered that we actually have this same problem in the isolatedscripttest interface, and there we use "::" as the delimiter. For perf tests, we can't use ":" as the delimiter because some tests have URLs in the name and use ":". I think there are other tests (in the layout tests, maybe) that have "," in the name.

+kbr might remember.

> I was under the impression that dpranke@ is objecting to that idea, 
> and I want to make sure we are on the same page before moving forward.

You are correct, I was, and I appreciate you wanting to get us all on the same page.

Sorry, I was confused. That's not really the same problem, because one is a list of test names, and one is a list of files. 

I prefer ','. I think both '::' and ':' are worse, but I don't want to drag this out indefinitely. Maybe kbr@ can be a tie-breaker between sky and myself, though if he votes for '::' then I've just made things worse :).

":" seems fine to me. "::" was only needed for the isolated script test interface because some test names included URLs, including the scheme.

Re #15: Thanks for the additional input! I will go with ':' for this issue then (unless we found evidence that we are using drive letters in Windows).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b1210256f6b9af46bb2cf3e1e6c90feae0c8107

commit 2b1210256f6b9af46bb2cf3e1e6c90feae0c8107
Author: Chong Zhang <chongz@chromium.org>
Date: Tue Sep 11 22:51:25 2018

TestLauncher: Support multiple filter files

Make --test-launcher-filter-file support multiple filter files separated
by ';'.

Bug:  881438 
Change-Id: Ibe17d1e8443cbb490a2d1b8942bb003c96a79525
Reviewed-on: https://chromium-review.googlesource.com/1213866
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590513}
[modify] https://crrev.com/2b1210256f6b9af46bb2cf3e1e6c90feae0c8107/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/2b1210256f6b9af46bb2cf3e1e6c90feae0c8107/base/test/launcher/unit_test_launcher.cc
[modify] https://crrev.com/2b1210256f6b9af46bb2cf3e1e6c90feae0c8107/base/test/test_switches.cc
[modify] https://crrev.com/2b1210256f6b9af46bb2cf3e1e6c90feae0c8107/testing/buildbot/filters/README.md

We ended up with using ';' due to the concern about Windows. See https://crrev.com/c/1213866/5/base/test/launcher/test_launcher.cc#954