Move UrlPatternIndex-specific tests from indexed_ruleset_unittest.

Move UrlPatternIndex-specific tests from indexed_ruleset_unittest.

BUG=713774
Review-Url: https://codereview.chromium.org/2862853002
Cr-Commit-Position: refs/heads/master@{#469653}
Committed: https://chromium.googlesource.com/chromium/src/+/09721eaab9e4d8dd62796f991da68dc2b89a325c

[pkalinnikov, 2017-05-04 14:12:20]

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

[commit-bot: I haz the power, 2017-05-04 14:12:38]

Dry run: 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-05-04 14:16:24]

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

[commit-bot: I haz the power, 2017-05-04 14:16:25]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[pkalinnikov, 2017-05-05 11:52:07]

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

[commit-bot: I haz the power, 2017-05-05 11:52:26]

Dry run: CQ is trying da patch.

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

[pkalinnikov, 2017-05-05 11:54:12]

Patchset #1 (id:1) has been deleted

[pkalinnikov, 2017-05-05 12:02:37]

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

[pkalinnikov, 2017-05-05 12:02:42]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-05 12:02:54]

Dry run: CQ is trying da patch.

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

[pkalinnikov, 2017-05-05 12:04:32]

pkalinnikov@chromium.org changed reviewers:
+ engedy@chromium.org

[pkalinnikov, 2017-05-05 12:04:33]

Balazs, PTAL. This is mostly a mechanical change.

[engedy, 2017-05-05 12:55:24]

LGTM % comments.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_index_unittest.cc
(left):

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:12:
#include "base/macros.h"
nit: We use this here for reasons unrelated to the header, so probably good to
leave this.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:13:
#include "base/strings/string_piece.h"
Let's include "base/strings/string_piece_forward.h" in the header and keep this
here.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_index_unittest.cc
(right):

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:37:
const auto* flat_index =
nit: Is this also an offset? Can we call this |index_offset|, and the one above
|temporary_index_offset|?

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:72:
EXPECT_FALSE(FindMatch(nullptr));
nit: How about:

base::StringPiece() /* url */

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:201:
EXPECT_EQ(test_case.expect_match, !!FindMatch(test_case.url));
Can we somehow check that it returns the correct rule too? (Especially in the
tests below where there are multiple rules.)

[commit-bot: I haz the power, 2017-05-05 12:58:17]

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

[commit-bot: I haz the power, 2017-05-05 12:58:18]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-05-05 13:20:36]

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

[commit-bot: I haz the power, 2017-05-05 13:20:45]

Dry run: CQ is trying da patch.

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

[pkalinnikov, 2017-05-05 13:21:01]

Done. Ship it?

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_index_unittest.cc
(left):

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:12:
#include "base/macros.h"
On 2017/05/05 12:55:24, engedy wrote:
> nit: We use this here for reasons unrelated to the header, so probably good to
> leave this.

Done.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:13:
#include "base/strings/string_piece.h"
On 2017/05/05 12:55:24, engedy wrote:
> Let's include "base/strings/string_piece_forward.h" in the header and keep
this
> here.

Done.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_index_unittest.cc
(right):

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:37:
const auto* flat_index =
On 2017/05/05 12:55:24, engedy wrote:
> nit: Is this also an offset? Can we call this |index_offset|, and the one
above
> |temporary_index_offset|?

It is not an offset, but a pointer to a FlatBuffers object. Does the explicitly
spelled type help?

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:72:
EXPECT_FALSE(FindMatch(nullptr));
On 2017/05/05 12:55:24, engedy wrote:
> nit: How about:
> 
> base::StringPiece() /* url */

Done.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:201:
EXPECT_EQ(test_case.expect_match, !!FindMatch(test_case.url));
On 2017/05/05 12:55:24, engedy wrote:
> Can we somehow check that it returns the correct rule too? (Especially in the
> tests below where there are multiple rules.)

Let us move this question to https://codereview.chromium.org/2861063004/

[engedy, 2017-05-05 13:23:50]

Ship it.

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
File components/subresource_filter/core/common/url_pattern_index_unittest.cc
(right):

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:37:
const auto* flat_index =
On 2017/05/05 13:21:00, pkalinnikov wrote:
> On 2017/05/05 12:55:24, engedy wrote:
> > nit: Is this also an offset? Can we call this |index_offset|, and the one
> above
> > |temporary_index_offset|?
> 
> It is not an offset, but a pointer to a FlatBuffers object. Does the
explicitly
> spelled type help?

Yep, much better. Thanks!

https://codereview.chromium.org/2862853002/diff/40001/components/subresource_...
components/subresource_filter/core/common/url_pattern_index_unittest.cc:201:
EXPECT_EQ(test_case.expect_match, !!FindMatch(test_case.url));
On 2017/05/05 13:21:00, pkalinnikov wrote:
> On 2017/05/05 12:55:24, engedy wrote:
> > Can we somehow check that it returns the correct rule too? (Especially in
the
> > tests below where there are multiple rules.)
> 
> Let us move this question to https://codereview.chromium.org/2861063004/

Acknowledged.

[engedy, 2017-05-05 13:24:14]

(Still LGTM.)

[commit-bot: I haz the power, 2017-05-05 14:41:30]

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

[commit-bot: I haz the power, 2017-05-05 14:41:31]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-05-05 15:11:44]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2017-05-05 15:12:12]

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-05-05 15:15:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493997104226280,
"parent_rev": "4c04c9f3e2510d5b49aee9612a77a93d159206b6", "commit_rev":
"09721eaab9e4d8dd62796f991da68dc2b89a325c"}

[commit-bot: I haz the power, 2017-05-05 15:16:10]

Description was changed from

==========
Move UrlPatternIndex-specific tests from indexed_ruleset_unittest.

BUG=713774
==========

to

==========
Move UrlPatternIndex-specific tests from indexed_ruleset_unittest.

BUG=713774

Review-Url: https://codereview.chromium.org/2862853002
Cr-Commit-Position: refs/heads/master@{#469653}
Committed:
https://chromium.googlesource.com/chromium/src/+/09721eaab9e4d8dd62796f991da6...
==========

[commit-bot: I haz the power, 2017-05-05 15:16:11]

Committed patchset #2 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/09721eaab9e4d8dd62796f991da6...