[arc-intents] Remove SSP matching; add unit tests.

[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=628788
Committed: https://crrev.com/5acc3b7599948290a46f7f9003e7a1cc5b466590
Cr-Commit-Position: refs/heads/master@{#406283}

[Ben Kwa, 2016-07-15 19:23:28]

Description was changed from

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=29248657
==========

to

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=29248657
==========

[Ben Kwa, 2016-07-15 19:23:28]

kenobi@chromium.org changed reviewers:
+ rickyz@chromium.org, yusukes@chromium.org

[Ben Kwa, 2016-07-15 22:40:08]

Description was changed from

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=29248657
==========

to

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=628788
==========

[rickyz (no longer on Chrome), 2016-07-15 23:32:01]

lgtm, thanks!

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/intent_filter_unittest.cc (right):

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:101: }
Would be nice to add one that tests/documents the behavior with default ports
that confused me earlier:

// A filter with a specified port should only match URLs
// that explicitly include that port.
IntentFilter filter_port_443 = BuildIntentFilter()
    .authority("authority1", 443);
EXPECT_FALSE(filter_port_443.match(GURL("https://authority1")));
EXPECT_TRUE(filter_port_443.match(GURL("https://authority1:443")));

[Ben Kwa, 2016-07-16 00:30:49]

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/intent_filter_unittest.cc (right):

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:101: }
On 2016/07/15 23:32:01, rickyz wrote:
> Would be nice to add one that tests/documents the behavior with default ports
> that confused me earlier:
> 
> // A filter with a specified port should only match URLs
> // that explicitly include that port.
> IntentFilter filter_port_443 = BuildIntentFilter()
>     .authority("authority1", 443);
> EXPECT_FALSE(filter_port_443.match(GURL("https://authority1")));
> EXPECT_TRUE(filter_port_443.match(GURL("https://authority1:443")));

That's the test starting on line 90, unless I'm misunderstanding you?

[rickyz (no longer on Chrome), 2016-07-16 00:32:35]

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/intent_filter_unittest.cc (right):

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:101: }
On 2016/07/16 at 00:30:49, Ben Kwa wrote:
> On 2016/07/15 23:32:01, rickyz wrote:
> > Would be nice to add one that tests/documents the behavior with default
ports
> > that confused me earlier:
> > 
> > // A filter with a specified port should only match URLs
> > // that explicitly include that port.
> > IntentFilter filter_port_443 = BuildIntentFilter()
> >     .authority("authority1", 443);
> > EXPECT_FALSE(filter_port_443.match(GURL("https://authority1")));
> > EXPECT_TRUE(filter_port_443.match(GURL("https://authority1:443")));
> 
> That's the test starting on line 90, unless I'm misunderstanding you?

Ah, this was referring specifically to the fact that authority1 with port 443
doesn't match https://authority1 even though 443 is the default port for https
(would probably be sufficient to just s/100/80 above too)

[Yusuke Sato, 2016-07-19 02:19:22]

lgtm with comments:

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/intent_filter_unittest.cc (right):

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:9: 
IWYU: #include <algorithm> for move(), please. It's not included in
inteht_filter.h.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:19: class
BuildIntentFilter {
optional: What about IntentFilterBuilder? A class name starting with a verb
seems a bit unusual in Chromium.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:75:
EXPECT_FALSE(filter.match(GURL("http://authority2")));
Testing with 'http://authority' (a substring of authotiry1) and
'http://authority11' (authority1 + an extra char) might be interesting too?
...and noticed line 117-124 does that :) Please disregard.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:77:
EXPECT_TRUE(filter.match(GURL("http://authority1")));
nit: test with https:// too? (you can do it in a separate CL if needed.)

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:147:
EXPECT_FALSE(filter.match(GURL("")));
nit: GURL("") is used here (and below) and GURL() is used in
TestAuthorityEntry_empty. Could you use only one of them?

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:191:
TEST(IntentFilterTest, TestDataPath_globInfix) {
Did you write the test from scratch? Or did you transcribe the CTS tests? If the
latter, can you add a code comment to refer to the tests?

[Ben Kwa, 2016-07-19 14:59:07]

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
File components/arc/intent_helper/intent_filter_unittest.cc (right):

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:9: 
On 2016/07/19 02:19:22, Yusuke Sato (ooo Jul 21-31) wrote:
> IWYU: #include <algorithm> for move(), please. It's not included in
> inteht_filter.h.

Done.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:19: class
BuildIntentFilter {
On 2016/07/19 02:19:22, Yusuke Sato (ooo Jul 21-31) wrote:
> optional: What about IntentFilterBuilder? A class name starting with a verb
> seems a bit unusual in Chromium.

Done.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:75:
EXPECT_FALSE(filter.match(GURL("http://authority2")));
On 2016/07/19 02:19:22, Yusuke Sato (ooo Jul 21-31) wrote:
> Testing with 'http://authority' (a substring of authotiry1) and
> 'http://authority11' (authority1 + an extra char) might be interesting too?
> ...and noticed line 117-124 does that :) Please disregard.

Acknowledged.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:77:
EXPECT_TRUE(filter.match(GURL("http://authority1")));
On 2016/07/19 02:19:22, Yusuke Sato (ooo Jul 21-31) wrote:
> nit: test with https:// too? (you can do it in a separate CL if needed.)

Done.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:101: }
On 2016/07/16 00:32:35, rickyz wrote:
> On 2016/07/16 at 00:30:49, Ben Kwa wrote:
> > On 2016/07/15 23:32:01, rickyz wrote:
> > > Would be nice to add one that tests/documents the behavior with default
> ports
> > > that confused me earlier:
> > > 
> > > // A filter with a specified port should only match URLs
> > > // that explicitly include that port.
> > > IntentFilter filter_port_443 = BuildIntentFilter()
> > >     .authority("authority1", 443);
> > > EXPECT_FALSE(filter_port_443.match(GURL("https://authority1")));
> > > EXPECT_TRUE(filter_port_443.match(GURL("https://authority1:443")));
> > 
> > That's the test starting on line 90, unless I'm misunderstanding you?
> 
> Ah, this was referring specifically to the fact that authority1 with port 443
> doesn't match https://authority1 even though 443 is the default port for https
> (would probably be sufficient to just s/100/80 above too)

I see, that makes sense.  Thanks for catching that.  It actually exposes more
behaviour mismatches between android and chrome authority matching, so I'll
address this in a follow-up CL.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:147:
EXPECT_FALSE(filter.match(GURL("")));
On 2016/07/19 02:19:22, Yusuke Sato (ooo Jul 21-31) wrote:
> nit: GURL("") is used here (and below) and GURL() is used in
> TestAuthorityEntry_empty. Could you use only one of them?

Done.

https://codereview.chromium.org/2156683002/diff/1/components/arc/intent_helpe...
components/arc/intent_helper/intent_filter_unittest.cc:191:
TEST(IntentFilterTest, TestDataPath_globInfix) {
On 2016/07/19 02:19:22, Yusuke Sato (ooo Jul 21-31) wrote:
> Did you write the test from scratch? Or did you transcribe the CTS tests? If
the
> latter, can you add a code comment to refer to the tests?

Done.

[Ben Kwa, 2016-07-19 14:59:21]

The CQ bit was checked by kenobi@chromium.org

[Ben Kwa, 2016-07-19 14:59:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rickyz@chromium.org,
yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2156683002/#ps20001
(title: "Apply CL feedback.")

[commit-bot: I haz the power, 2016-07-19 14:59:36]

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, 2016-07-19 15:49:59]

Description was changed from

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=628788
==========

to

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=628788
==========

[commit-bot: I haz the power, 2016-07-19 15:50:00]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-19 15:51:59]

Description was changed from

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=628788
==========

to

==========
[arc-intents] Remove SSP matching; add unit tests.

- As discussed with elijahtaylor@ and yusukes@, don't attempt matching
  for scheme-specific parts.  Extracting an SSP from a chrome URL is
  fraught, and SSPs don't really apply to http/https URLs anyway.

- Add unit tests for the IntentFilter class.

BUG=628788

Committed: https://crrev.com/5acc3b7599948290a46f7f9003e7a1cc5b466590
Cr-Commit-Position: refs/heads/master@{#406283}
==========

[commit-bot: I haz the power, 2016-07-19 15:52:00]

Patchset 2 (id:??) landed as
https://crrev.com/5acc3b7599948290a46f7f9003e7a1cc5b466590
Cr-Commit-Position: refs/heads/master@{#406283}