Remove no- prefix from command line flags in rtc_event_log2text and rtc_event_log2rtp_dump and nega…

Remove no- prefix from command line flags in rtc_event_log2text and rtc_event_log2rtp_dump and negate their meaning.

BUG=webrtc:8202
Review-Url: https://codereview.webrtc.org/3008113002
Cr-Commit-Position: refs/heads/master@{#19798}
Committed: https://chromium.googlesource.com/external/webrtc/+/c3d2bfd2443a55f22d84f15f5f64e2ea80daba19

[terelius, 2017-09-04 14:42:11]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-04 14:42:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[terelius, 2017-09-04 14:45:56]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-04 14:46:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[terelius, 2017-09-04 14:47:10]

terelius@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, oprypin@webrtc.org

[terelius, 2017-09-04 14:47:11]

PTAL

[oprypin_webrtc, 2017-09-04 14:48:22]

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config, true,
"Excludes stream configurations.");
It seems like the descriptions are misleading now.

Perhaps change all descriptions from
    DEFINE_bool\((\w+), true, "Excludes
to
    DEFINE_bool(\1, true, "Use --no\1 to exclude

[commit-bot: I haz the power, 2017-09-04 16:01:31]

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

[commit-bot: I haz the power, 2017-09-04 16:01:32]

Dry run: This issue passed the CQ dry run.

[oprypin_webrtc, 2017-09-05 07:07:04]

This change is not necessary with https://codereview.webrtc.org/3004363002/

[terelius, 2017-09-05 07:54:43]

On 2017/09/05 07:07:04, oprypin_webrtc wrote:
> This change is not necessary with https://codereview.webrtc.org/3004363002/

I still think it makes sense to land this. The code is slightly shorter, and the
old flags (e.g. --nortp) still works.

[oprypin_webrtc, 2017-09-05 07:57:21]

On 2017/09/04 14:48:22, oprypin_webrtc wrote:
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):
> 
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config,
true,
> "Excludes stream configurations.");
> It seems like the descriptions are misleading now.
> 
> Perhaps change all descriptions from
>     DEFINE_bool\((\w+), true, "Excludes
> to
>     DEFINE_bool(\1, true, "Use --no\1 to exclude

My previous comment still applies

[hlundin-webrtc, 2017-09-05 10:25:13]

henrik.lundin@webrtc.org changed reviewers:
+ ivoc@webrtc.org
- henrik.lundin@webrtc.org

[hlundin-webrtc, 2017-09-05 10:25:14]

Replacing self with ivoc@.

[ivoc, 2017-09-05 10:59:20]

lgtm, but please have a look at my comment.

https://codereview.webrtc.org/3008113002/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log2rtp_dump.cc (right):

https://codereview.webrtc.org/3008113002/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log2rtp_dump.cc:32: "Excludes audio
packets from the converted RTPdump file.");
Please update the descriptions.

[terelius, 2017-09-05 11:24:12]

The CQ bit was checked by terelius@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-05 11:24:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[terelius, 2017-09-05 11:26:28]

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config, true,
"Excludes stream configurations.");
On 2017/09/04 14:48:21, oprypin_webrtc wrote:
> It seems like the descriptions are misleading now.
> 
> Perhaps change all descriptions from
>     DEFINE_bool\((\w+), true, "Excludes
> to
>     DEFINE_bool(\1, true, "Use --no\1 to exclude

Changed to "Includes/excludes \1" instead. Is this acceptable to you?

https://codereview.webrtc.org/3008113002/diff/20001/webrtc/logging/rtc_event_...
File webrtc/logging/rtc_event_log/rtc_event_log2rtp_dump.cc (right):

https://codereview.webrtc.org/3008113002/diff/20001/webrtc/logging/rtc_event_...
webrtc/logging/rtc_event_log/rtc_event_log2rtp_dump.cc:32: "Excludes audio
packets from the converted RTPdump file.");
On 2017/09/05 10:59:20, ivoc wrote:
> Please update the descriptions.

Done.

[oprypin_webrtc, 2017-09-05 11:31:52]

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config, true,
"Excludes stream configurations.");
On 2017/09/05 11:26:28, terelius wrote:
> On 2017/09/04 14:48:21, oprypin_webrtc wrote:
> > It seems like the descriptions are misleading now.
> > 
> > Perhaps change all descriptions from
> >     DEFINE_bool\((\w+), true, "Excludes
> > to
> >     DEFINE_bool(\1, true, "Use --no\1 to exclude
> 
> Changed to "Includes/excludes \1" instead. Is this acceptable to you?

The problem is that people will not know how to use this. I don't think it's
clear enough.
--help lists the flag as --config but passing --config has no effect, and it's a
really obscure detail that --noconfig is what you actually need to pass.

[commit-bot: I haz the power, 2017-09-05 11:58:09]

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

[commit-bot: I haz the power, 2017-09-05 11:58:10]

Dry run: This issue passed the CQ dry run.

[terelius, 2017-09-05 12:07:15]

On 2017/09/05 11:31:52, oprypin_webrtc wrote:
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):
> 
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config,
true,
> "Excludes stream configurations.");
> On 2017/09/05 11:26:28, terelius wrote:
> > On 2017/09/04 14:48:21, oprypin_webrtc wrote:
> > > It seems like the descriptions are misleading now.
> > > 
> > > Perhaps change all descriptions from
> > >     DEFINE_bool\((\w+), true, "Excludes
> > > to
> > >     DEFINE_bool(\1, true, "Use --no\1 to exclude
> > 
> > Changed to "Includes/excludes \1" instead. Is this acceptable to you?
> 
> The problem is that people will not know how to use this. I don't think it's
> clear enough.
> --help lists the flag as --config but passing --config has no effect, and it's
a
> really obscure detail that --noconfig is what you actually need to pass.

But don't you think it is confusing that the flag is called --config and then
the documentation is about --noconfig?

Also, I believe the help text states that it is a bool and that the default
value is true, no?

[oprypin_webrtc, 2017-09-05 12:15:34]

On 2017/09/05 12:07:15, terelius wrote:
> On 2017/09/05 11:31:52, oprypin_webrtc wrote:
> >
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> > File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):
> > 
> >
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> > webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config,
> true,
> > "Excludes stream configurations.");
> > On 2017/09/05 11:26:28, terelius wrote:
> > > On 2017/09/04 14:48:21, oprypin_webrtc wrote:
> > > > It seems like the descriptions are misleading now.
> > > > 
> > > > Perhaps change all descriptions from
> > > >     DEFINE_bool\((\w+), true, "Excludes
> > > > to
> > > >     DEFINE_bool(\1, true, "Use --no\1 to exclude
> > > 
> > > Changed to "Includes/excludes \1" instead. Is this acceptable to you?
> > 
> > The problem is that people will not know how to use this. I don't think it's
> > clear enough.
> > --help lists the flag as --config but passing --config has no effect, and
it's
> a
> > really obscure detail that --noconfig is what you actually need to pass.
> 
> But don't you think it is confusing that the flag is called --config and then
> the documentation is about --noconfig?
> 
> Also, I believe the help text states that it is a bool and that the default
> value is true, no?

The default value is true, and yes, it's specified. Passing a flag has the same
effect as not passing it. There is no way to find out how to set it to false. I
don't know what can be more confusing than that.

[terelius, 2017-09-12 11:16:55]

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):

https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config, true,
"Excludes stream configurations.");
On 2017/09/05 11:31:52, oprypin_webrtc wrote:
> On 2017/09/05 11:26:28, terelius wrote:
> > On 2017/09/04 14:48:21, oprypin_webrtc wrote:
> > > It seems like the descriptions are misleading now.
> > > 
> > > Perhaps change all descriptions from
> > >     DEFINE_bool\((\w+), true, "Excludes
> > > to
> > >     DEFINE_bool(\1, true, "Use --no\1 to exclude
> > 
> > Changed to "Includes/excludes \1" instead. Is this acceptable to you?
> 
> The problem is that people will not know how to use this. I don't think it's
> clear enough.
> --help lists the flag as --config but passing --config has no effect, and it's
a
> really obscure detail that --noconfig is what you actually need to pass.

Oh, I see; the (intuitive) --config=false no longer works. Was there a reason
for removing it? Will we add it back?

Updated the help text according to your preference.

[oprypin_webrtc, 2017-09-12 11:23:18]

On 2017/09/12 11:16:55, terelius wrote:
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> File webrtc/logging/rtc_event_log/rtc_event_log2text.cc (right):
> 
>
https://codereview.webrtc.org/3008113002/diff/1/webrtc/logging/rtc_event_log/...
> webrtc/logging/rtc_event_log/rtc_event_log2text.cc:43: DEFINE_bool(config,
true,
> "Excludes stream configurations.");
> On 2017/09/05 11:31:52, oprypin_webrtc wrote:
> > On 2017/09/05 11:26:28, terelius wrote:
> > > On 2017/09/04 14:48:21, oprypin_webrtc wrote:
> > > > It seems like the descriptions are misleading now.
> > > > 
> > > > Perhaps change all descriptions from
> > > >     DEFINE_bool\((\w+), true, "Excludes
> > > > to
> > > >     DEFINE_bool(\1, true, "Use --no\1 to exclude
> > > 
> > > Changed to "Includes/excludes \1" instead. Is this acceptable to you?
> > 
> > The problem is that people will not know how to use this. I don't think it's
> > clear enough.
> > --help lists the flag as --config but passing --config has no effect, and
it's
> a
> > really obscure detail that --noconfig is what you actually need to pass.
> 
> Oh, I see; the (intuitive) --config=false no longer works. Was there a reason
> for removing it? Will we add it back?
> 
> Updated the help text according to your preference.

LGTM

Syntax like "--config=false" was part of gflags, which was removed. The reason
is that this dependency caused interoperability problems with both Chrome's and
Google's infrastructure.
It will not be re-added in the same form, but sure, expanding rtc_base/flags
with new functionality is not out of the question.

[terelius, 2017-09-12 12:21:55]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2017-09-12 12:21:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3008113002/#ps60001 (title:
"Nit")

[commit-bot: I haz the power, 2017-09-12 12:22:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-09-12 12:57:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1505218915462150,
"parent_rev": "661d94996b0301cee460d0e486e8371a05977aca", "commit_rev":
"c3d2bfd2443a55f22d84f15f5f64e2ea80daba19"}

[commit-bot: I haz the power, 2017-09-12 12:57:42]

Description was changed from

==========
Remove no- prefix from command line flags in rtc_event_log2text and
rtc_event_log2rtp_dump and negate their meaning.

BUG=webrtc:8202
==========

to

==========
Remove no- prefix from command line flags in rtc_event_log2text and
rtc_event_log2rtp_dump and negate their meaning.

BUG=webrtc:8202

Review-Url: https://codereview.webrtc.org/3008113002
Cr-Commit-Position: refs/heads/master@{#19798}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c3d2bfd2443a55f22d84f15f5...
==========

[commit-bot: I haz the power, 2017-09-12 12:57:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/c3d2bfd2443a55f22d84f15f5...