Cleanup: Pass std::string as const reference from chromeos/

Cleanup: Pass std::string as const reference from chromeos/

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

BUG=367418
TEST=
R=zelidrag@chromium.org,derat@chromium.org,gauravsh@chromium.org

Committed: https://crrev.com/d3709b5f91fe5d0d4accd7386da5a61743824b9e
Cr-Commit-Position: refs/heads/master@{#350078}

[ki.stfu, 2015-09-20 20:05:51]

ki.stfu@gmail.com changed reviewers:
+ gauravsh@chromium.org

[Daniel Erat, 2015-09-21 14:06:52]

thanks! please also add your name and email address to the top-level AUTHORS
file in this change and sign the CLA, per
http://www.chromium.org/developers/contributing-code/external-contributor-che....

[ki.stfu, 2015-09-21 14:32:19]

On 2015/09/21 14:06:52, Daniel Erat wrote:
> thanks! please also add your name and email address to the top-level AUTHORS
> file in this change and sign the CLA, per
>
http://www.chromium.org/developers/contributing-code/external-contributor-che....

Already done. It's not my first commit.

[Daniel Erat, 2015-09-21 14:49:50]

lgtm

ah, thanks! sorry, i was looking at a stale checkout.

[Daniel Erat, 2015-09-21 14:50:28]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2015-09-21 14:50:34]

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

[commit-bot: I haz the power, 2015-09-21 14:58:37]

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

[commit-bot: I haz the power, 2015-09-21 14:58:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2015-09-21 15:14:37]

ah, chromeos/network/OWNERS has "set noparent". i'll wait for gaurav.

[gauravsh, 2015-09-21 16:38:44]

lgtm

I probably shouldn't be in chromeos/network/OWNERS any more.

[Daniel Erat, 2015-09-21 16:40:20]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2015-09-21 16:40:48]

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

[commit-bot: I haz the power, 2015-09-21 17:00:21]

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

[commit-bot: I haz the power, 2015-09-21 17:00:22]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ki.stfu, 2015-09-21 20:35:20]

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

[commit-bot: I haz the power, 2015-09-21 20:36:23]

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

[ki.stfu, 2015-09-21 21:01:45]

@derat: Please review PS2.

[commit-bot: I haz the power, 2015-09-21 21:07:10]

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

[commit-bot: I haz the power, 2015-09-21 21:07:11]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2015-09-21 21:47:04]

derat@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Daniel Erat, 2015-09-21 21:47:05]

steven should be reviewing this.

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:1172: type_arg != "eth" && // synonym
for KTypeEthernet
this is different behavior than what was there before. the old code mapped "eth"
to kTypeEthernet, but you're not doing that now, so the next if condition will
no longer trigger for "eth".

[ki.stfu, 2015-09-21 22:20:41]

Review again please.

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:1172: type_arg != "eth" && // synonym
for KTypeEthernet
On 2015/09/21 21:47:05, Daniel Erat wrote:
> this is different behavior than what was there before. the old code mapped
"eth"
> to kTypeEthernet, but you're not doing that now, so the next if condition will
> no longer trigger for "eth".

Done.

[stevenjb, 2015-09-21 22:23:39]

NOT LGTM

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:1182: if (type_arg ==
shill::kTypeEthernet && state == kNetworkDisabled)
Need to update this condition also. I think it would be easier to leave this as
it was or copy type_arg = type_arg_in if there is some auditing reason why
changing this is important?

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:710:
InvokeConnectErrorCallback(service_path, error_callback, error_name);
We *REALLY* need to be careful about this sort of change. PLEASE audit any non
trivial cases more carefully. The minor optimizations are not (IMHO) worth
introducing crash bugs.

This line will crash if we do not copy service_path, that is why it was written
this way in the first place. CheckPendingRequest() may get called with
pending_request_[]->service_path, so erasing the request in the line above can
make |service_path| invalid if it is a reference.

In fairness a comment in the header is in order here, so please do add a comment
for this methods, thanks!

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:126: void
AddCertificateFromFile(std::string filename,
const&?

[ki.stfu, 2015-09-21 22:55:58]

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_manager_client.cc:1182: if (type_arg ==
shill::kTypeEthernet && state == kNetworkDisabled)
On 2015/09/21 22:23:39, stevenjb wrote:
> Need to update this condition also. I think it would be easier to leave this
as
> it was or copy type_arg = type_arg_in if there is some auditing reason why
> changing this is important?

I have already reverted it back.

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:710:
InvokeConnectErrorCallback(service_path, error_callback, error_name);
On 2015/09/21 22:23:39, stevenjb wrote:
> We *REALLY* need to be careful about this sort of change. PLEASE audit any non
> trivial cases more carefully. The minor optimizations are not (IMHO) worth
> introducing crash bugs.
> 
> This line will crash if we do not copy service_path, that is why it was
written
> this way in the first place. CheckPendingRequest() may get called with
> pending_request_[]->service_path, so erasing the request in the line above can
> make |service_path| invalid if it is a reference.
> 
> In fairness a comment in the header is in order here, so please do add a
comment
> for this methods, thanks!

Agreed. Done.

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right):

https://codereview.chromium.org/1353323002/diff/20001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:126: void
AddCertificateFromFile(std::string filename,
On 2015/09/21 22:23:39, stevenjb wrote:
> const&?

Done.

[stevenjb, 2015-09-21 22:57:49]

lgtm

[ki.stfu, 2015-09-21 23:02:13]

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

[ki.stfu, 2015-09-21 23:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
gauravsh@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1353323002/#ps60001
(title: "Don't use const& for NetworkConnectionHandler::CheckPendingRequest; Use
one more const& in unittests")

[commit-bot: I haz the power, 2015-09-21 23:03:04]

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

[commit-bot: I haz the power, 2015-09-22 00:56:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-22 00:57:32]

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