Avoid unnecessary std::string allocations.

Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=655187
Committed: https://crrev.com/e0a1c1bd1456928a05a582c78210a81f8fe30859
Cr-Commit-Position: refs/heads/master@{#425111}

[pkalinnikov, 2016-09-28 16:16:38]

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

[commit-bot: I haz the power, 2016-09-28 16:16:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2016-09-28 16:20:06]

Description was changed from

==========
Avoid unnecessary std::string allocations.

BUG=
==========

to

==========
Avoid unnecessary std::string allocations.

BUG=
==========

[pkalinnikov, 2016-09-28 16:20:06]

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

[pkalinnikov, 2016-09-28 16:34:17]

Balazs, can you take a first look?

[commit-bot: I haz the power, 2016-09-28 17:13:28]

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

[commit-bot: I haz the power, 2016-09-28 17:13:28]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-09-28 17:22:53]

LGTM % comments. Thanks for looking into this!

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
net/base/registry_controlled_domains/registry_controlled_domain.h:185: // Same
as above, but returns a part of |gurl|'s host in a StringPiece.
Phrasing nit: ... returns the domain and registry as a StringPiece that
references the underlying string of the passed-in |gurl|.

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
net/base/registry_controlled_domains/registry_controlled_domain.h:186: //
TODO(pkalinnikov): Rename this to GetDomainAndRegistry and get rid of the
As discussed, please run some quick tests to see how often this is called during
normal browsing, e.g. by adding TRACE_EVENT inside.

If called often, I agree that we should file a bug documenting that fact,
reference that bug here in this CL, then update all existing call sites in a
follow-up CL and remove this TODO.

[pkalinnikov, 2016-10-05 11:51:35]

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
net/base/registry_controlled_domains/registry_controlled_domain.h:185: // Same
as above, but returns a part of |gurl|'s host in a StringPiece.
On 2016/09/28 17:22:52, engedy wrote:
> Phrasing nit: ... returns the domain and registry as a StringPiece that
> references the underlying string of the passed-in |gurl|.

Done.

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
net/base/registry_controlled_domains/registry_controlled_domain.h:186: //
TODO(pkalinnikov): Rename this to GetDomainAndRegistry and get rid of the
On 2016/09/28 17:22:52, engedy wrote:
> As discussed, please run some quick tests to see how often this is called
during
> normal browsing, e.g. by adding TRACE_EVENT inside.
> 
> If called often, I agree that we should file a bug documenting that fact,
> reference that bug here in this CL, then update all existing call sites in a
> follow-up CL and remove this TODO.

It is used not frequently during normal browsing. In a small experiment I
collected 9 calls, which was roughly the number of times I clicked some
hyperlink.

Can I leave the TODO?

[pkalinnikov, 2016-10-05 11:53:05]

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

[pkalinnikov, 2016-10-05 11:54:34]

Hi Peter,

Can you please review this? Please also advise if I can attach this to any
existing bug, or if I should create a new one.

Thanks,
Pavel

[pkalinnikov, 2016-10-05 11:57:47]

Description was changed from

==========
Avoid unnecessary std::string allocations.

BUG=
==========

to

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=
==========

[engedy, 2016-10-05 11:58:14]

Still LGTM.

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
File net/base/registry_controlled_domains/registry_controlled_domain.h (right):

https://codereview.chromium.org/2380523004/diff/1/net/base/registry_controlle...
net/base/registry_controlled_domains/registry_controlled_domain.h:186: //
TODO(pkalinnikov): Rename this to GetDomainAndRegistry and get rid of the
On 2016/10/05 11:51:35, pkalinnikov wrote:
> On 2016/09/28 17:22:52, engedy wrote:
> > As discussed, please run some quick tests to see how often this is called
> during
> > normal browsing, e.g. by adding TRACE_EVENT inside.
> > 
> > If called often, I agree that we should file a bug documenting that fact,
> > reference that bug here in this CL, then update all existing call sites in a
> > follow-up CL and remove this TODO.
> 
> It is used not frequently during normal browsing. In a small experiment I
> collected 9 calls, which was roughly the number of times I clicked some
> hyperlink.
> 
> Can I leave the TODO?

Usually it's recommended to leave a TODO only if we are actually planning to do
this refactor in the near future, and in that case we should file a bug to track
it.

I'd leave it up to Peter to decide if the refactor is worth it in light of how
rarely this is called.

[Peter Kasting, 2016-10-06 06:21:35]

LGTM with comment

https://codereview.chromium.org/2380523004/diff/20001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/2380523004/diff/20001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:187:
base::StringPiece GetDomainAndRegistryAsStringPiece(
For now, I would move this function into the anonymous namespace above and
remove it from the .h, since it's not called outside the class.

I'm not opposed to changing the public interface of the class to use
StringPieces if it makes sense.  (That would probably be better than have string
and StringPiece variants of some of the functions there.)  However, let's not
touch the header until we're actually going to do that.

If you want to pursue it, I suggest filing a bug against yourself about this and
adding a TODO on this helper to eliminate it by exposing StringPiece as the
interface type for all these APIs.  However, StringPiece can be viral, and it
would be sad to add 50 .as_string() calls in the callers if that didn't end up
being a timesave.

[pkalinnikov, 2016-10-06 10:51:36]

https://codereview.chromium.org/2380523004/diff/20001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/2380523004/diff/20001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:187:
base::StringPiece GetDomainAndRegistryAsStringPiece(
On 2016/10/06 06:21:35, Peter Kasting (OOO Oct. 6-7) wrote:
> For now, I would move this function into the anonymous namespace above and
> remove it from the .h, since it's not called outside the class.
> 
> I'm not opposed to changing the public interface of the class to use
> StringPieces if it makes sense.  (That would probably be better than have
string
> and StringPiece variants of some of the functions there.)  However, let's not
> touch the header until we're actually going to do that.
> 
> If you want to pursue it, I suggest filing a bug against yourself about this
and
> adding a TODO on this helper to eliminate it by exposing StringPiece as the
> interface type for all these APIs.  However, StringPiece can be viral, and it
> would be sad to add 50 .as_string() calls in the callers if that didn't end up
> being a timesave.

Done.

[pkalinnikov, 2016-10-12 16:31:09]

Description was changed from

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=
==========

to

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=655187
==========

[pkalinnikov, 2016-10-13 13:06:19]

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

[commit-bot: I haz the power, 2016-10-13 13:06:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2016-10-13 13:06:28]

Hi Peter,

Does this look good? Can I submit?

[commit-bot: I haz the power, 2016-10-13 14:24:33]

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

[commit-bot: I haz the power, 2016-10-13 14:24:34]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-10-13 16:58:58]

Yep, LGTM

[pkalinnikov, 2016-10-13 18:49:08]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2016-10-13 18:49:09]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2380523004/#ps40001
(title: "Remove function from the header.")

[commit-bot: I haz the power, 2016-10-13 18:49:27]

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-10-13 18:55:31]

Description was changed from

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=655187
==========

to

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=655187
==========

[commit-bot: I haz the power, 2016-10-13 18:55:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-13 18:58:43]

Description was changed from

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=655187
==========

to

==========
Avoid unnecessary std::string allocations.

When checking SameDomainOrHost, it is unnecessary to allocate std::string's
when retrieving DomainAndRegistry from URLs. It is actually unnecessary
elsewhere, but this can be addressed in a different CL.

BUG=655187

Committed: https://crrev.com/e0a1c1bd1456928a05a582c78210a81f8fe30859
Cr-Commit-Position: refs/heads/master@{#425111}
==========

[commit-bot: I haz the power, 2016-10-13 18:58:44]

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