Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=663729
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/88b3b717eaf289cad99f4ed87bd75e94f15ca6e5
Cr-Commit-Position: refs/heads/master@{#431952}

[Charlie Harrison, 2016-11-04 21:27:48]

Description was changed from

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

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

to

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Harrison, 2016-11-04 21:29:39]

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

[commit-bot: I haz the power, 2016-11-04 21:30:14]

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-11-04 23:21:45]

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

[commit-bot: I haz the power, 2016-11-04 23:21:46]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-05 00:01:02]

csharrison@chromium.org changed reviewers:
+ thestig@chromium.org

[Charlie Harrison, 2016-11-05 00:01:03]

thestig: is this a worthwhile patch? I don't have data that any of these places
are hot spots, but seeing as sometimes URLs are rather big, this seems like it
could be a perf win in some cases.

[Lei Zhang, 2016-11-05 00:08:29]

Sure. Since you already spent the time to do this.

I'm wondering can we replace host/path with host_piece/path_piece completely?
Then if the caller needs a string, they can call as_string().

[Charlie Harrison, 2016-11-05 00:16:38]

On 2016/11/05 00:08:29, Lei Zhang wrote:
> Sure. Since you already spent the time to do this.
> 
> I'm wondering can we replace host/path with host_piece/path_piece completely?
> Then if the caller needs a string, they can call as_string().

I suspect a big percentage of host()/path() will need as_string, but it would be
nice to have single StringPiece interface to GURL / Origin, so consumers will be
efficient by default.

Let me look into it!

[vabr (Chromium), 2016-11-07 08:43:50]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-11-07 08:43:51]

autofill and password_manager components LGTM, thanks for the improvement!

Cheers,
Vaclav

[Charlie Harrison, 2016-11-07 20:27:00]

On 2016/11/07 08:43:51, vabr (Chromium) wrote:
> autofill and password_manager components LGTM, thanks for the improvement!
> 
> Cheers,
> Vaclav

Thanks!

FYI thestig: I have https://codereview.chromium.org/2481923002 running which
tries to make path() return a StringPiece outright. It compiles 'all' on my
workstation but that's all I've verified :D. Converting host() will probably be
~2x as much work judging from codesearch.

[Charlie Harrison, 2016-11-08 14:43:00]

csharrison@chromium.org changed reviewers:
+ mmenke@chromium.org, nick@chromium.org, rdevlin.cronin@chromium.org

[Charlie Harrison, 2016-11-08 14:43:00]

The linked CL is proving to be a bit difficult to get bots green, let's just
move forward with this CL for now.

+devlin for //extensions and webui/extensions
+nick for //content
+mmenke for //net

Thanks, everyone

[mmenke, 2016-11-08 15:02:32]

On 2016/11/08 14:43:00, Charlie Harrison wrote:
> The linked CL is proving to be a bit difficult to get bots green, let's just
> move forward with this CL for now.
> 
> +devlin for //extensions and webui/extensions
> +nick for //content
> +mmenke for //net
> 
> Thanks, everyone

net LGTM

[mmenke, 2016-11-08 15:05:34]

On 2016/11/08 15:02:32, mmenke wrote:
> On 2016/11/08 14:43:00, Charlie Harrison wrote:
> > The linked CL is proving to be a bit difficult to get bots green, let's just
> > move forward with this CL for now.
> > 
> > +devlin for //extensions and webui/extensions
> > +nick for //content
> > +mmenke for //net
> > 
> > Thanks, everyone
> 
> net LGTM

Would it make sense to mass replace GURL::path() with GURL::path().as_string(),
and make GURL::path() return a StringPiece, and get rid of the string version? 
path().as_string() may be marginally higher overhead than the current path(),
but it would also discourage using i.  Or could even rename the current path()
to path_as_string().

[Charlie Harrison, 2016-11-08 15:07:25]

On 2016/11/08 15:05:34, mmenke wrote:
> On 2016/11/08 15:02:32, mmenke wrote:
> > On 2016/11/08 14:43:00, Charlie Harrison wrote:
> > > The linked CL is proving to be a bit difficult to get bots green, let's
just
> > > move forward with this CL for now.
> > > 
> > > +devlin for //extensions and webui/extensions
> > > +nick for //content
> > > +mmenke for //net
> > > 
> > > Thanks, everyone
> > 
> > net LGTM
> 
> Would it make sense to mass replace GURL::path() with
GURL::path().as_string(),
> and make GURL::path() return a StringPiece, and get rid of the string version?

> path().as_string() may be marginally higher overhead than the current path(),
> but it would also discourage using i.  Or could even rename the current path()
> to path_as_string().

[Charlie Harrison, 2016-11-08 15:13:00]

On 2016/11/08 15:07:25, Charlie Harrison wrote:
> On 2016/11/08 15:05:34, mmenke wrote:
> > On 2016/11/08 15:02:32, mmenke wrote:
> > > On 2016/11/08 14:43:00, Charlie Harrison wrote:
> > > > The linked CL is proving to be a bit difficult to get bots green, let's
> just
> > > > move forward with this CL for now.
> > > > 
> > > > +devlin for //extensions and webui/extensions
> > > > +nick for //content
> > > > +mmenke for //net
> > > > 
> > > > Thanks, everyone
> > > 
> > > net LGTM
> > 
> > Would it make sense to mass replace GURL::path() with
> GURL::path().as_string(),
> > and make GURL::path() return a StringPiece, and get rid of the string
version?
> 
> > path().as_string() may be marginally higher overhead than the current
path(),
> > but it would also discourage using i.  Or could even rename the current
path()
> > to path_as_string().

That might work. I think it will make things easier to grep and audit callsites
to ensure they actually need the as_string() call. Let's do that after this one
lands (assuming url owners approval).

[Devlin, 2016-11-08 15:55:49]

> +devlin for //extensions and webui/extensions
I don't see a webui/extensions?

But extension stuff lgtm % small nit

https://codereview.chromium.org/2478573004/diff/1/extensions/common/extension...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2478573004/diff/1/extensions/common/extension...
extensions/common/extension_urls.cc:86: } else {
Totally not your code, but since you're here... :)

Would you mind taking out this else (no need for else after a return) and
removing the parens around the conditions on the line below?

i.e.
if (update_url == store_url)
  return true;
return update_url.host_piece() == store_url.host_piece() &&
       update_url.path_piece() == store_url.path_piece();

[Charlie Harrison, 2016-11-08 16:00:55]

Oops sorry I meant chrome/browser/ui/extensions

https://codereview.chromium.org/2478573004/diff/1/extensions/common/extension...
File extensions/common/extension_urls.cc (right):

https://codereview.chromium.org/2478573004/diff/1/extensions/common/extension...
extensions/common/extension_urls.cc:86: } else {
On 2016/11/08 15:55:49, Devlin (slow) wrote:
> Totally not your code, but since you're here... :)
> 
> Would you mind taking out this else (no need for else after a return) and
> removing the parens around the conditions on the line below?
> 
> i.e.
> if (update_url == store_url)
>   return true;
> return update_url.host_piece() == store_url.host_piece() &&
>        update_url.path_piece() == store_url.path_piece();

No problem. Done!

[Lei Zhang, 2016-11-08 19:26:59]

Non-extension chrome/ bits lgtm

[Charlie Harrison, 2016-11-08 19:30:37]

csharrison@chromium.org changed reviewers:
+ blundell@chromium.org

[Charlie Harrison, 2016-11-08 19:30:38]

Thanks!!

+blundell could you please take a look at //components?

[ncarter (slow), 2016-11-08 19:33:18]

content lgtm

Thanks for this

[Charlie Harrison, 2016-11-08 22:06:45]

csharrison@chromium.org changed reviewers:
+ dbeam@chromium.org

[Charlie Harrison, 2016-11-08 22:06:46]

+dbeam for webui/options

[Charlie Harrison, 2016-11-08 22:16:46]

csharrison@chromium.org changed reviewers:
+ stevenjb@chromium.org
- dbeam@chromium.org

[Charlie Harrison, 2016-11-08 22:16:47]

Alrighty let's -dbeam and +stevenjb for webui/options

[blundell, 2016-11-09 10:27:25]

//components lgtm

[Charlie Harrison, 2016-11-09 14:07:25]

Description was changed from

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=663729
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Harrison, 2016-11-09 14:09:16]

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

[commit-bot: I haz the power, 2016-11-09 14:09:31]

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-11-09 15:11:06]

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

[commit-bot: I haz the power, 2016-11-09 15:11:07]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-11-14 13:13:47]

stevenjb, friendly ping

[stevenjb, 2016-11-14 18:23:18]

chrome/browser/ui/webui/options/ lgtm

[Charlie Harrison, 2016-11-14 21:05:08]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-14 21:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
rdevlin.cronin@chromium.org, mmenke@chromium.org, nick@chromium.org,
thestig@chromium.org, stevenjb@chromium.org, blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2478573004/#ps40001
(title: "rebase to #431874")

[commit-bot: I haz the power, 2016-11-14 21:06:02]

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-11-14 23:13:00]

Description was changed from

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=663729
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=663729
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-14 23:13:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-14 23:22:44]

Description was changed from

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=663729
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert GURL::{host,path} to GURL::{host_piece,path_piece} for ==.

host(), path() and friends copy data into a std::string. For comparisons
this is unnecessary.

BUG=663729
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/88b3b717eaf289cad99f4ed87bd75e94f15ca6e5
Cr-Commit-Position: refs/heads/master@{#431952}
==========

[commit-bot: I haz the power, 2016-11-14 23:22:46]

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