Provide the equivalent of GURL::DomainIs for url::Origin.

Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
Committed: https://crrev.com/054f40334067cafe53af8518accbfcf3b0f803d9
Cr-Commit-Position: refs/heads/master@{#415606}

[pkalinnikov, 2016-08-26 12:52:02]

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

[pkalinnikov, 2016-08-26 12:52:03]

Balazs, please take an initial look.

[pkalinnikov, 2016-08-26 13:10:24]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.
==========

to

==========
Factor DomainIs out and add it to url::Origin.

BUG=517560
==========

[engedy, 2016-08-26 16:48:14]

LGTM % comments

Could you please elaborate a bit more in the CL description on what the change
is about and reason that reconstructing an url::Origin into a URL has
significant performance impact?

Also, I wonder if there is any good way we could avoid replicating the same core
set of test cases three times.

https://codereview.chromium.org/2287483002/diff/1/url/origin_unittest.cc
File url/origin_unittest.cc (right):

https://codereview.chromium.org/2287483002/diff/1/url/origin_unittest.cc#newc...
url/origin_unittest.cc:255: TEST(OriginTest, DomainIs) {
Could you please add an explicit test for a file: URL, and also for a unique
origin?

https://codereview.chromium.org/2287483002/diff/1/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/2287483002/diff/1/url/url_util.cc#newcode621
url/url_util.cc:621: DCHECK(!canonicalized_host.empty());
I think it would be safer if this shared stuff did the checking, and returned
false if either argument is the empty string.

Then we could get rid of the checking in the two places that call into this. 

Don't forget to add the new unittests in URLUtils.

https://codereview.chromium.org/2287483002/diff/1/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2287483002/diff/1/url/url_util.h#newcode221
url/url_util.h:221: // Returns true if the |canonicalized_host| matches or is in
the same domain as
I'd argue this should go into a new section before line 136.

// Domains --

[pkalinnikov, 2016-08-29 09:48:37]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.

BUG=517560
==========

to

==========
Factor DomainIs out and add it to url::Origin.

Put a DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Before the
origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

[pkalinnikov, 2016-08-29 09:49:06]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.

Put a DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Before the
origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

to

==========
Factor DomainIs out and add it to url::Origin.

Put a DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Before the
origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

[pkalinnikov, 2016-08-29 09:49:30]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.

Put a DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Before the
origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

to

==========
Factor DomainIs out and add it to url::Origin.

Put DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Before the
origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

[pkalinnikov, 2016-08-29 09:50:25]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.

Put DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Before the
origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

to

==========
Factor DomainIs out and add it to url::Origin.

Put DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Until now
the origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

[pkalinnikov, 2016-08-29 09:50:54]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.

Put DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Until now
the origins had to be converted to GURLs, which involves string copying, and,
therefore, allocations.

BUG=517560
==========

to

==========
Factor DomainIs out and add it to url::Origin.

Put DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Until now
the origins had to be converted to GURLs, which involved string copying, and,
therefore, allocations.

BUG=517560
==========

[pkalinnikov, 2016-08-29 09:53:00]

https://codereview.chromium.org/2287483002/diff/1/url/origin_unittest.cc
File url/origin_unittest.cc (right):

https://codereview.chromium.org/2287483002/diff/1/url/origin_unittest.cc#newc...
url/origin_unittest.cc:255: TEST(OriginTest, DomainIs) {
On 2016/08/26 16:48:14, engedy wrote:
> Could you please add an explicit test for a file: URL, and also for a unique
> origin?

Done.

https://codereview.chromium.org/2287483002/diff/1/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/2287483002/diff/1/url/url_util.cc#newcode621
url/url_util.cc:621: DCHECK(!canonicalized_host.empty());
On 2016/08/26 16:48:14, engedy wrote:
> I think it would be safer if this shared stuff did the checking, and returned
> false if either argument is the empty string.
> 
> Then we could get rid of the checking in the two places that call into this. 
> 
> Don't forget to add the new unittests in URLUtils.

Done.

https://codereview.chromium.org/2287483002/diff/1/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2287483002/diff/1/url/url_util.h#newcode221
url/url_util.h:221: // Returns true if the |canonicalized_host| matches or is in
the same domain as
On 2016/08/26 16:48:14, engedy wrote:
> I'd argue this should go into a new section before line 136.
> 
> // Domains --

Done.

[pkalinnikov, 2016-08-29 09:56:59]

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

[pkalinnikov, 2016-08-29 09:56:59]

Mike, can you please review this?

[engedy, 2016-08-29 12:49:59]

Description was changed from

==========
Factor DomainIs out and add it to url::Origin.

Put DomainIs function to url_util, which is used both by GURL and url::Origin.

The reason for this refactoring is significant performance gain. Until now
the origins had to be converted to GURLs, which involved string copying, and,
therefore, allocations.

BUG=517560
==========

to

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

[engedy, 2016-08-29 12:51:19]

Description was changed from

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

to

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

This CL moves most of the implementation of GURL::DomainIs into a shared
function in url_util/, to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

[pkalinnikov, 2016-08-29 12:51:54]

Description was changed from

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

This CL moves most of the implementation of GURL::DomainIs into a shared
function in url_util/, to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

to

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

[Mike West, 2016-08-30 09:32:13]

LGTM

[engedy, 2016-08-30 17:13:13]

Still, LGTM, just two small nits. Thanks!

https://codereview.chromium.org/2287483002/diff/20001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/2287483002/diff/20001/url/gurl.cc#newcode494
url/gurl.cc:494: if (!is_valid_ || lower_ascii_domain.empty())
nit: The second argument of this OR condition is no longer needed, right?

https://codereview.chromium.org/2287483002/diff/20001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2287483002/diff/20001/url/url_util.h#newcode135
url/url_util.h:135: // Domains
---------------------------------------------------------------------
nit: Blank line before.

[pkalinnikov, 2016-08-31 08:43:10]

https://codereview.chromium.org/2287483002/diff/20001/url/gurl.cc
File url/gurl.cc (right):

https://codereview.chromium.org/2287483002/diff/20001/url/gurl.cc#newcode494
url/gurl.cc:494: if (!is_valid_ || lower_ascii_domain.empty())
On 2016/08/30 17:13:13, engedy wrote:
> nit: The second argument of this OR condition is no longer needed, right?

Done.

https://codereview.chromium.org/2287483002/diff/20001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2287483002/diff/20001/url/url_util.h#newcode135
url/url_util.h:135: // Domains
---------------------------------------------------------------------
On 2016/08/30 17:13:13, engedy wrote:
> nit: Blank line before.

Done.

[pkalinnikov, 2016-08-31 08:45:13]

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

[commit-bot: I haz the power, 2016-08-31 08:45:35]

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, 2016-08-31 10:45:17]

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

[commit-bot: I haz the power, 2016-08-31 10:45:18]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2016-08-31 10:50:17]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2016-08-31 10:50:18]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2287483002/#ps40001
(title: "Minor changes.")

[commit-bot: I haz the power, 2016-08-31 10:50:34]

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-08-31 10:54:34]

Description was changed from

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

to

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

[commit-bot: I haz the power, 2016-08-31 10:54:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-31 10:58:12]

Description was changed from

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560
==========

to

==========
Provide the equivalent of GURL::DomainIs for url::Origin.

Move the implementation of GURL::DomainIs into a shared function in url_util/,
to be used both by GURL and url::Origin.

The reason for this refactoring is to reap significant performance gains in
scenarios where previously url::Origins had to be converted to GURLs for
DomainIs checks. This involved string copying, allocations, and parsing the
entire URL once again.

BUG=517560

Committed: https://crrev.com/054f40334067cafe53af8518accbfcf3b0f803d9
Cr-Commit-Position: refs/heads/master@{#415606}
==========

[commit-bot: I haz the power, 2016-08-31 10:58:13]

Patchset 3 (id:??) landed as
https://crrev.com/054f40334067cafe53af8518accbfcf3b0f803d9
Cr-Commit-Position: refs/heads/master@{#415606}