Cleanup: Pass std::string as const reference if possible

Cleanup: Pass std::string as const reference if possible

Passing std::string by reference can prevent extra copying of object.

BUG=367418
TEST=
R=limasdf@gmail.com,jam@chromium.org,stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[ki.stfu, 2015-09-20 17:11:42]

The CQ bit was checked by ki.stfu@gmail.com

[commit-bot: I haz the power, 2015-09-20 17:11:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1349783006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1349783006/20001

[commit-bot: I haz the power, 2015-09-20 17:11:53]

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

[commit-bot: I haz the power, 2015-09-20 17:11:55]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[ki.stfu, 2015-09-20 17:28:39]

The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-20 17:28:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1349783006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1349783006/20001

[commit-bot: I haz the power, 2015-09-20 17:28:51]

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

[commit-bot: I haz the power, 2015-09-20 17:28:53]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tfarina, 2015-09-20 19:08:59]

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

[tfarina, 2015-09-20 19:09:00]

Usually we do not send like this.

We split usually per top-level directory, like one patch for ash/, one for cc/,
one for chrome/, one for chromeos and so so. That way individual OWNERS of those
directories can look and review your patch more easily.

btw, while you can get a review from limasdf (he is a frequent contributor), he
is not an OWNER afaik.

Hope that helps,

Thanks,

[ki.stfu, 2015-09-20 20:19:20]

On 2015/09/20 19:09:00, tfarina wrote:
> Usually we do not send like this.
> 
> We split usually per top-level directory, like one patch for ash/, one for
cc/,
> one for chrome/, one for chromeos and so so. That way individual OWNERS of
those
> directories can look and review your patch more easily.
> 
> btw, while you can get a review from limasdf (he is a frequent contributor),
he
> is not an OWNER afaik.
> 
> Hope that helps,
> 
> Thanks,

Thanks for the advice. Here is CLs for each folder:
* https://codereview.chromium.org/1352613004 - ash & base
* https://codereview.chromium.org/1359593002 - cc
* https://codereview.chromium.org/1358783002 - chrome
* https://codereview.chromium.org/1348033003 - chromecast
* https://codereview.chromium.org/1352793003 - chrome_elf
* https://codereview.chromium.org/1353323002 - chromeos
* https://codereview.chromium.org/1355983002 - components
* https://codereview.chromium.org/1354363002 - content
* https://codereview.chromium.org/1353333002 - courgette
* https://codereview.chromium.org/1357173002 - device
* https://codereview.chromium.org/1340413004 - extensions
* https://codereview.chromium.org/1354373002 - google_apis
* https://codereview.chromium.org/1360513002 - gpu
* https://codereview.chromium.org/1354353003 - ios
* https://codereview.chromium.org/1350163006 - ipc
* https://codereview.chromium.org/1360523002 - media
* https://codereview.chromium.org/1355993002 - native_client_sdk
* https://codereview.chromium.org/1346943004 - net
* https://codereview.chromium.org/1345393005 - pdf
* https://codereview.chromium.org/1357843002 - remoting
* https://codereview.chromium.org/1353323003 - rlz
* https://codereview.chromium.org/1348313003 - sync
* https://codereview.chromium.org/1350043004 - testing & tools
* https://codereview.chromium.org/1352393002 - ui
* https://codereview.chromium.org/1360533002 - url

[Peter Kasting, 2015-09-20 21:58:18]

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

[Peter Kasting, 2015-09-20 21:58:20]

https://codereview.chromium.org/1349783006/diff/20001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1349783006/diff/20001/url/gurl.h#newcode51
url/gurl.h:51: // can convert from WebURL without copying the string. When we
call this
This sort of change is likely wrong.  The old code is very explicit about why
it's passing by value, and changing that in the new code probably does the
opposite of what you want, by forcing the compiler to generate an object whose
address can be taken.

Unless whoever wrote the old comment was flat-out wrong, which I very much
doubt, places like this should not be changed.

This was only the second file in this CL I looked at; it worries me that it took
2 files to find a potential red flag.  Reviewers are likely to rubber-stamp this
sort of change, so you need to be very careful it's really right.

[ki.stfu, 2015-09-21 05:26:06]

https://codereview.chromium.org/1349783006/diff/20001/url/gurl.h
File url/gurl.h (right):

https://codereview.chromium.org/1349783006/diff/20001/url/gurl.h#newcode51
url/gurl.h:51: // can convert from WebURL without copying the string. When we
call this
On 2015/09/20 21:58:20, Peter Kasting wrote:
> This sort of change is likely wrong.  The old code is very explicit about why
> it's passing by value, and changing that in the new code probably does the
> opposite of what you want, by forcing the compiler to generate an object whose
> address can be taken.
> 
> Unless whoever wrote the old comment was flat-out wrong, which I very much
> doubt, places like this should not be changed.
> 
> This was only the second file in this CL I looked at; it worries me that it
took
> 2 files to find a potential red flag.  Reviewers are likely to rubber-stamp
this
> sort of change, so you need to be very careful it's really right.

I'll copy and answer on it in https://codereview.chromium.org/1360533002.