Avoid calling strlen() where possible in base/trace_config

The main target of this CL is to avoid calling strlen() where possible.
It can happen if a raw |char*| literal or result of |c_str()| function is used
to construct a StringPiece or std::string objects.
In addition, StringPiece API like SplitStringPiece was used to avoid
unnecessary string copying and allocations.

BUG=679479
Review-Url: https://codereview.chromium.org/2777203004
Cr-Commit-Position: refs/heads/master@{#460203}
Committed: https://chromium.googlesource.com/chromium/src/+/b5d44bb49fd4dc5e544f4312de6b00b1a1eb625c

[Andrey Kraynov, 2017-03-28 13:04:57]

iceman@yandex-team.ru changed reviewers:
+ primiano@chromium.org

[Andrey Kraynov, 2017-03-28 13:04:58]

Hi!

This CL is a continuation of my previous patch,
https://codereview.chromium.org/2624583002/patch/20001/30003.
I had to split it for convenience.
Here I did a little bit more than in my first CL, hope this can be useful.
PTAL.

[Primiano Tucci (use gerrit), 2017-03-28 15:08:06]

LGTM with 1 comment

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
File base/trace_event/trace_config_category_filter.cc (left):

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config_category_filter.cc:93:
DCHECK(category_group_name);
can you keep this dcheck?

[Andrey Kraynov, 2017-03-28 16:00:11]

On 2017/03/28 15:08:06, Primiano Tucci wrote:
> LGTM with 1 comment
> 
>
https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
> File base/trace_event/trace_config_category_filter.cc (left):
> 
>
https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
> base/trace_event/trace_config_category_filter.cc:93:
> DCHECK(category_group_name);
> can you keep this dcheck?

Yes, of course.
Now it can look like
DCHECK(category_group_name.empty());

It will check for both NULL string and empty string(""), is that OK?

[Andrey Kraynov, 2017-03-28 16:14:13]

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
File base/trace_event/trace_config_category_filter.cc (right):

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config_category_filter.cc:92: CStringTokenizer
category_group_tokens(category_group_name.begin(),
And one question I forgot to mention.
It seems we can use something like this 

using StringPieceTokenizer =
      StringTokenizerT<StringPiece, StringPiece::const_iterator>;
StringPieceTokenizer category_group_tokens(category_group_name, ",");

to iterate over tokens without making a std::string object.
CStringTokenizer can help, but it requires a pair of iterators.
How do you think, is it a good idea to add this typedef into base/ ?

[Andrey Kraynov, 2017-03-28 16:55:45]

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
File base/trace_event/trace_config_category_filter.cc (left):

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config_category_filter.cc:93:
DCHECK(category_group_name);
On 2017/03/28 15:08:06, Primiano Tucci wrote:
> can you keep this dcheck?

Done.

[Andrey Kraynov, 2017-03-28 16:57:57]

iceman@yandex-team.ru changed reviewers:
+ yusukes@chromium.org

[Andrey Kraynov, 2017-03-28 16:57:57]

+ yusukes@ as OWNER for chrome/browser/chromeos/arc/tracing/
PTAL

[Primiano Tucci (use gerrit), 2017-03-28 16:59:56]

> It will check for both NULL string and empty string(""), is that OK?
ack

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
File base/trace_event/trace_config_category_filter.cc (right):

https://codereview.chromium.org/2777203004/diff/1/base/trace_event/trace_conf...
base/trace_event/trace_config_category_filter.cc:92: CStringTokenizer
category_group_tokens(category_group_name.begin(),
On 2017/03/28 16:14:13, Andrey Kraynov wrote:
> And one question I forgot to mention.
> It seems we can use something like this 
> 
> using StringPieceTokenizer =
>       StringTokenizerT<StringPiece, StringPiece::const_iterator>;
> StringPieceTokenizer category_group_tokens(category_group_name, ",");
> 
> to iterate over tokens without making a std::string object.
> CStringTokenizer can help, but it requires a pair of iterators.
> How do you think, is it a good idea to add this typedef into base/ ?
you have to discuss this with //base owners.

[Yusuke Sato, 2017-03-28 17:09:50]

arc/ lgtm

[Andrey Kraynov, 2017-03-28 17:25:34]

The CQ bit was checked by iceman@yandex-team.ru

[Andrey Kraynov, 2017-03-28 17:25:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2777203004/#ps20001
(title: "Keep DCHECKs.")

[commit-bot: I haz the power, 2017-03-28 17:26:08]

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-03-28 20:24:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490721934198560,
"parent_rev": "0b39e0b743641ffe37979c796c0ca0a1666d681f", "commit_rev":
"b5d44bb49fd4dc5e544f4312de6b00b1a1eb625c"}

[commit-bot: I haz the power, 2017-03-28 20:25:42]

Description was changed from

==========
The main target of this CL is to avoid calling strlen() where possible.
It can happen if a raw |char*| literal or result of |c_str()| function is used
to construct a StringPiece or std::string objects.
In addition, StringPiece API like SplitStringPiece was used to avoid
unnecessary string copying and allocations.

BUG=679479
==========

to

==========
The main target of this CL is to avoid calling strlen() where possible.
It can happen if a raw |char*| literal or result of |c_str()| function is used
to construct a StringPiece or std::string objects.
In addition, StringPiece API like SplitStringPiece was used to avoid
unnecessary string copying and allocations.

BUG=679479

Review-Url: https://codereview.chromium.org/2777203004
Cr-Commit-Position: refs/heads/master@{#460203}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5d44bb49fd4dc5e544f4312de6b...
==========

[commit-bot: I haz the power, 2017-03-28 20:25:43]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b5d44bb49fd4dc5e544f4312de6b...

[imcheng, 2017-03-28 22:13:45]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2776393004/ by imcheng@chromium.org.

The reason for reverting is: Broke WebKit Win x64 Builder (dbg) 
blink_platform_unittests:

WorkQueueTest.BlockedByFenceNewFenceUnblocks
WorkQueueTest.TakeTaskFromWorkQueue
WorkQueueTest.InsertFenceAfterEnqueueingNonBlocking
WorkQueueTest.RemoveFence
WorkQueueTest.TakeTaskFromWorkQueue_HitFence
WorkQueueTest.InsertNewFence
WorkQueueTest.BlockedByFencePop
WorkQueueTest.BlockedByFencePopBecomesEmpty

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit....

[Andrey Kraynov, 2017-03-28 22:41:14]

On 2017/03/28 22:13:45, imcheng wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2776393004/ by mailto:imcheng@chromium.org.
> 
> The reason for reverting is: Broke WebKit Win x64 Builder (dbg) 
> blink_platform_unittests:
> 
> WorkQueueTest.BlockedByFenceNewFenceUnblocks
> WorkQueueTest.TakeTaskFromWorkQueue
> WorkQueueTest.InsertFenceAfterEnqueueingNonBlocking
> WorkQueueTest.RemoveFence
> WorkQueueTest.TakeTaskFromWorkQueue_HitFence
> WorkQueueTest.InsertNewFence
> WorkQueueTest.BlockedByFencePop
> WorkQueueTest.BlockedByFencePopBecomesEmpty
> 
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit....

My new DCHECK was triggered:
FATAL:trace_event_etw_export_win.cc(352)] Check failed:
!category_group_name.empty().

It seems |category_group_name| can be empty during these tests.
I am not sure, is it a bug or expected behavior.

[Andrey Kraynov, 2017-03-29 12:26:06]

On 2017/03/28 16:59:56, Primiano Tucci wrote:
> > It will check for both NULL string and empty string(""), is that OK?
> ack
> 

I see that in Blink tests categoty_group_name is set to an empty string("")
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...

Then it will be used here
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...

primiano@, could you please say if this initialization is correct and
categoty_group_name can have value of "" ?
If yes, then my DCHECK was wrong. Otherwise the Blink test code should be fixed.

[Primiano Tucci (use gerrit), 2017-03-29 15:33:11]

On 2017/03/29 12:26:06, Andrey Kraynov wrote:
> On 2017/03/28 16:59:56, Primiano Tucci wrote:
> > > It will check for both NULL string and empty string(""), is that OK?
> > ack
> > 
> 
> I see that in Blink tests categoty_group_name is set to an empty string("")
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...
> 
> Then it will be used here
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...
> 
> primiano@, could you please say if this initialization is correct and
> categoty_group_name can have value of "" ?
> If yes, then my DCHECK was wrong. Otherwise the Blink test code should be
fixed.

That looks definitely a bug to me, shame that caused a revert of this :/
Filed crbug.com/706416

[Andrey Kraynov, 2017-03-29 16:02:34]

On 2017/03/29 15:33:11, Primiano Tucci wrote:
> 
> That looks definitely a bug to me, shame that caused a revert of this :/
> Filed crbug.com/706416

Thanks for your help!
Hope we can find a solution for the new bug 706416 so I will be able to reland
this CL.