Added anonymization patterns for URLs and email addresses

Added anonymization patterns for URLs and email addresses

BUG=567870
Committed: https://crrev.com/03910b4689274c8f5a806b1cfda6d8aab5aea9a0
Cr-Commit-Position: refs/heads/master@{#368573}

[battre, 2015-12-21 17:14:27]

battre@chromium.org changed reviewers:
+ vasilii@chromium.org

[battre, 2015-12-21 17:14:27]

Hi Vasilii,

could you please review this CL?

Thank you,
Dominic

[battre, 2015-12-21 17:15:09]

CC rkc (FYI)

[vasilii, 2015-12-22 12:31:42]

lgtm

As soon as the tests are green

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
components/feedback/anonymizer_tool.cc:60: #define RESERVED NGC(GEN_DELIMS "|"
SUB_DELIMS)
I never used a macros in a macros. Does it work as expected?

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
components/feedback/anonymizer_tool.h:16: struct CustomPatternWithoutContext {
A comment.

[battre, 2016-01-08 14:14:32]

Hi Vasilii,

could you please take another look?

I have done some performance optimizations:

Patch 5 introduces the caching of regular expressions so that RE2 does not need
to recompile them every time.

Patch 6 replaces a the strategy to capture skipped bytes via a (.*) prefix in
the regular expression. This provides significant performance gains. (Before the
tests would time out on a 10MB file, not is it down to 1.7 seconds).

Thanks,
Dominic

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
components/feedback/anonymizer_tool.cc:60: #define RESERVED NGC(GEN_DELIMS "|"
SUB_DELIMS)
On 2015/12/22 12:31:42, vasilii wrote:
> I never used a macros in a macros. Does it work as expected?

Yes, this is just string replacements.

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1543633003/diff/20001/components/feedback/ano...
components/feedback/anonymizer_tool.h:16: struct CustomPatternWithoutContext {
On 2015/12/22 12:31:42, vasilii wrote:
> A comment.

Done.

[vasilii, 2016-01-08 17:02:26]

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
components/feedback/anonymizer_tool.h:21: struct CustomPatternWithoutContext {
Does moving to cc work for you?

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
components/feedback/anonymizer_tool.h:40: re2::RE2* GetRegExp(const std::string&
pattern);
friend declaration first, then methods.

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
components/feedback/anonymizer_tool.h:69: std::map<std::string,
scoped_ptr<re2::RE2>> regexp_cache_;
Comment

https://codereview.chromium.org/1543633003/diff/120001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/120001/components/feedback/an...
components/feedback/anonymizer_tool.cc:202: return Func(p0, p1, p2, args, 3);
I think you can collapse these definition using a variadic template.

template <typename Result, typename Param0, typename Param1, typename Param2,
typename ...Arg>
Result FindAndConsumeAndGetSkippedInternal(Param0 p0, Param1 p1, Param2 p2,
Arg... args) {
  auto* args_array = {args...};
  return FindAndConsumeAndGetSkippedN(p0, p1, p2, args,  arraysize(args));
}

using FindAndConsumeAndGetSkipped = FindAndConsumeAndGetSkippedInternal<your
parameters>;

[battre, 2016-01-11 09:02:20]

Thank you. PTAL.

Best regards,
Dominic

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
File components/feedback/anonymizer_tool.h (right):

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
components/feedback/anonymizer_tool.h:21: struct CustomPatternWithoutContext {
On 2016/01/08 17:02:26, vasilii wrote:
> Does moving to cc work for you?

This is used in the unit test.

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
components/feedback/anonymizer_tool.h:40: re2::RE2* GetRegExp(const std::string&
pattern);
On 2016/01/08 17:02:26, vasilii wrote:
> friend declaration first, then methods.

Done.

https://codereview.chromium.org/1543633003/diff/100001/components/feedback/an...
components/feedback/anonymizer_tool.h:69: std::map<std::string,
scoped_ptr<re2::RE2>> regexp_cache_;
On 2016/01/08 17:02:26, vasilii wrote:
> Comment

Done.

https://codereview.chromium.org/1543633003/diff/120001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/120001/components/feedback/an...
components/feedback/anonymizer_tool.cc:202: return Func(p0, p1, p2, args, 3);
On 2016/01/08 17:02:26, vasilii wrote:
> I think you can collapse these definition using a variadic template.
> 
> template <typename Result, typename Param0, typename Param1, typename Param2,
> typename ...Arg>
> Result FindAndConsumeAndGetSkippedInternal(Param0 p0, Param1 p1, Param2 p2,
> Arg... args) {
>   auto* args_array = {args...};
>   return FindAndConsumeAndGetSkippedN(p0, p1, p2, args,  arraysize(args));
> }
> 
> using FindAndConsumeAndGetSkipped = FindAndConsumeAndGetSkippedInternal<your
> parameters>;

I don't even need this anymore once I use a variadic template.

Done.

[vasilii, 2016-01-11 10:54:29]

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
components/feedback/anonymizer_tool.cc:180: // in |skipped_input|.
I think it's misleading because if you pass no arguments you get nothing in
|skipped_input|. Is there any difference between args[0] and |input| after the
call?

[battre, 2016-01-11 10:59:24]

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
components/feedback/anonymizer_tool.cc:180: // in |skipped_input|.
On 2016/01/11 10:54:29, vasilii wrote:
> I think it's misleading because if you pass no arguments you get nothing in
> |skipped_input|. Is there any difference between args[0] and |input| after the
> call?

I have added an example. Is it clearer now?

[vasilii, 2016-01-11 11:58:32]

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
components/feedback/anonymizer_tool.cc:180: // in |skipped_input|.
On 2016/01/11 10:59:23, battre wrote:
> On 2016/01/11 10:54:29, vasilii wrote:
> > I think it's misleading because if you pass no arguments you get nothing in
> > |skipped_input|. Is there any difference between args[0] and |input| after
the
> > call?
> 
> I have added an example. Is it clearer now?

The comment is false. Something meaningful is stored in |skipped_input| only
when |args| isn't empty.
The example does mention that |input| is modified.

https://codereview.chromium.org/1543633003/diff/180001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/180001/components/feedback/an...
components/feedback/anonymizer_tool.cc:197: *skipped_input = *input;
This worsens my concern. Now if |args| is empty then |skipped_input| contains
wrong value upon exit.

[battre, 2016-01-11 12:33:08]

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/160001/components/feedback/an...
components/feedback/anonymizer_tool.cc:180: // in |skipped_input|.
On 2016/01/11 11:58:32, vasilii wrote:
> On 2016/01/11 10:59:23, battre wrote:
> > On 2016/01/11 10:54:29, vasilii wrote:
> > > I think it's misleading because if you pass no arguments you get nothing
in
> > > |skipped_input|. Is there any difference between args[0] and |input| after
> the
> > > call?
> > 
> > I have added an example. Is it clearer now?
> 
> The comment is false. Something meaningful is stored in |skipped_input| only
> when |args| isn't empty.
> The example does mention that |input| is modified.

Done.

https://codereview.chromium.org/1543633003/diff/180001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/180001/components/feedback/an...
components/feedback/anonymizer_tool.cc:197: *skipped_input = *input;
On 2016/01/11 11:58:32, vasilii wrote:
> This worsens my concern. Now if |args| is empty then |skipped_input| contains
> wrong value upon exit.

Done.

[vasilii, 2016-01-11 12:56:42]

lgtm

[battre, 2016-01-11 12:57:37]

The CQ bit was checked by battre@chromium.org

[commit-bot: I haz the power, 2016-01-11 12:57:43]

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

[commit-bot: I haz the power, 2016-01-11 13:42:41]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-01-11 13:43:39]

Description was changed from

==========
Added anonymization patterns for URLs and email addresses

BUG=567870
==========

to

==========
Added anonymization patterns for URLs and email addresses

BUG=567870

Committed: https://crrev.com/03910b4689274c8f5a806b1cfda6d8aab5aea9a0
Cr-Commit-Position: refs/heads/master@{#368573}
==========

[commit-bot: I haz the power, 2016-01-11 13:43:39]

Patchset 12 (id:??) landed as
https://crrev.com/03910b4689274c8f5a806b1cfda6d8aab5aea9a0
Cr-Commit-Position: refs/heads/master@{#368573}

[tfarina, 2016-01-11 15:25:31]

https://codereview.chromium.org/1543633003/diff/220001/components/feedback/an...
File components/feedback/anonymizer_tool.cc (right):

https://codereview.chromium.org/1543633003/diff/220001/components/feedback/an...
components/feedback/anonymizer_tool.cc:12: 
forgot to remove this blank line.

[tfarina, 2016-01-11 15:25:44]

Description was changed from

==========
Added anonymization patterns for URLs and email addresses

BUG=567870

Committed: https://crrev.com/03910b4689274c8f5a806b1cfda6d8aab5aea9a0
Cr-Commit-Position: refs/heads/master@{#368573}
==========

to

==========
Added anonymization patterns for URLs and email addresses

BUG=567870

Committed: https://crrev.com/03910b4689274c8f5a806b1cfda6d8aab5aea9a0
Cr-Commit-Position: refs/heads/master@{#368573}
==========