Cleanup: Pass std::string as const reference from google_apis/ and components/gcm_driver/

Cleanup: Pass std::string as const reference from google_apis/ and components/gcm_driver/

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

BUG=367418
TEST=
R=rogerta@chromium.org,sdefresne@chromium.org

Committed: https://crrev.com/07be839f30d97158b803a2cc6dc6509b06c834e1
Cr-Commit-Position: refs/heads/master@{#351035}

[Roger Tawa OOO till Jul 10th, 2015-09-20 20:40:08]

https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
File google_apis/gaia/account_tracker.cc (right):

https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
google_apis/gaia/account_tracker.cc:218: void
AccountTracker::StopTrackingAccount(const std::string& account_key) {
I don't think this one can be changed without further cleanup.  When line 235
calls StopTrackingAccount(), the key it passes is from the |accounts_| map. 
When the entry is removed at line 226, the |account_key| argument is deleted. 
The behaviour at lines 229 and 230 becomes undefined.

One solution is to make a copy of string at line 235.  A better solution would
be to make a copy of key inside this function and then only use the copy. 
Either way please add a comment to explain what is happening.

[ki.stfu, 2015-09-21 06:15:40]

https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
File google_apis/gaia/account_tracker.cc (right):

https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
google_apis/gaia/account_tracker.cc:218: void
AccountTracker::StopTrackingAccount(const std::string& account_key) {
On 2015/09/20 20:40:08, Roger Tawa wrote:
> I don't think this one can be changed without further cleanup.  When line 235
> calls StopTrackingAccount(), the key it passes is from the |accounts_| map. 
> When the entry is removed at line 226, the |account_key| argument is deleted. 
> The behaviour at lines 229 and 230 becomes undefined.
Thanks for catching it!

> One solution is to make a copy of string at line 235.  A better solution would
> be to make a copy of key inside this function and then only use the copy. 
> Either way please add a comment to explain what is happening.
I think we can leave it as is, instead of passing the account_key by reference
and then copying it to a local variable.

[Roger Tawa OOO till Jul 10th, 2015-09-21 16:29:19]

On 2015/09/21 06:15:40, ki.stfu wrote:
>
https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
> File google_apis/gaia/account_tracker.cc (right):
> 
>
https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
> google_apis/gaia/account_tracker.cc:218: void
> AccountTracker::StopTrackingAccount(const std::string& account_key) {
> On 2015/09/20 20:40:08, Roger Tawa wrote:
> > I don't think this one can be changed without further cleanup.  When line
235
> > calls StopTrackingAccount(), the key it passes is from the |accounts_| map. 
> > When the entry is removed at line 226, the |account_key| argument is
deleted. 
> > The behaviour at lines 229 and 230 becomes undefined.
> Thanks for catching it!
> 
> > One solution is to make a copy of string at line 235.  A better solution
would
> > be to make a copy of key inside this function and then only use the copy. 
> > Either way please add a comment to explain what is happening.
> I think we can leave it as is, instead of passing the account_key by reference
> and then copying it to a local variable.

Sounds fine.  Please just add a comment to explain why this one is different
from the rest.

[ki.stfu, 2015-09-21 17:54:12]

On 2015/09/21 16:29:19, Roger Tawa wrote:
> On 2015/09/21 06:15:40, ki.stfu wrote:
> >
>
https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
> > File google_apis/gaia/account_tracker.cc (right):
> > 
> >
>
https://codereview.chromium.org/1354373002/diff/1/google_apis/gaia/account_tr...
> > google_apis/gaia/account_tracker.cc:218: void
> > AccountTracker::StopTrackingAccount(const std::string& account_key) {
> > On 2015/09/20 20:40:08, Roger Tawa wrote:
> > > I don't think this one can be changed without further cleanup.  When line
> 235
> > > calls StopTrackingAccount(), the key it passes is from the |accounts_|
map. 
> > > When the entry is removed at line 226, the |account_key| argument is
> deleted. 
> > > The behaviour at lines 229 and 230 becomes undefined.
> > Thanks for catching it!
> > 
> > > One solution is to make a copy of string at line 235.  A better solution
> would
> > > be to make a copy of key inside this function and then only use the copy. 
> > > Either way please add a comment to explain what is happening.
> > I think we can leave it as is, instead of passing the account_key by
reference
> > and then copying it to a local variable.
> 
> Sounds fine.  Please just add a comment to explain why this one is different
> from the rest.

Done.

[Roger Tawa OOO till Jul 10th, 2015-09-21 20:47:40]

lgtm

Sorry, meant to lgtm the previous comment.

https://codereview.chromium.org/1354373002/diff/20001/google_apis/gaia/accoun...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/1354373002/diff/20001/google_apis/gaia/accoun...
google_apis/gaia/account_tracker.h:105: // object is stored in |accounts_| and
it will be destroyed after
nit: is --> may be

[ki.stfu, 2015-09-21 21:34:54]

https://codereview.chromium.org/1354373002/diff/20001/google_apis/gaia/accoun...
File google_apis/gaia/account_tracker.h (right):

https://codereview.chromium.org/1354373002/diff/20001/google_apis/gaia/accoun...
google_apis/gaia/account_tracker.h:105: // object is stored in |accounts_| and
it will be destroyed after
On 2015/09/21 20:47:40, Roger Tawa wrote:
> nit: is --> may be

Done.

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

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

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

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1354373002/#ps40001
(title: "Fix a comment about usage of const reference in
AccountTracker::StopTrackingAccount")

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

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

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

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

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

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

On 2015/09/21 21:50:11, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

This CL depends on https://codereview.chromium.org/1355983002

[ki.stfu, 2015-09-24 19:23:25]

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

[commit-bot: I haz the power, 2015-09-24 19:25:19]

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

[commit-bot: I haz the power, 2015-09-24 20:15:33]

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

[commit-bot: I haz the power, 2015-09-24 20:15:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ki.stfu, 2015-09-27 06:17:11]

On 2015/09/24 20:15:34, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I can't reproduce this failure on my side:
```
$ ./browser_tests --gtest_filter=ExtensionPreferenceApiTest.DataReductionProxy
IMPORTANT DEBUGGING NOTE: each test is run inside its own process.
For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with either
--single_process (to run the test in one launcher/browser process) or
--single-process (to do the above, and also run Chrome in single-process mode).
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = ExtensionPreferenceApiTest.DataReductionProxy
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExtensionPreferenceApiTest, where TypeParam = 
[ RUN      ] ExtensionPreferenceApiTest.DataReductionProxy
[15632:15632:0927/091410:WARNING:password_store_factory.cc(334)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[15632:15632:0927/091411:INFO:CONSOLE(0)] "[SUCCESS] getDrpPrefs", source:
chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[15632:15632:0927/091411:INFO:CONSOLE(0)] "[SUCCESS] updateDailyLengths",
source: chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[15632:15632:0927/091411:INFO:CONSOLE(0)] "[SUCCESS] clearDataSavings", source:
chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[       OK ] ExtensionPreferenceApiTest.DataReductionProxy (1116 ms)
[----------] 1 test from ExtensionPreferenceApiTest (1116 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1116 ms total)
[  PASSED  ] 1 test.
[1/1] ExtensionPreferenceApiTest.DataReductionProxy (1580 ms)
SUCCESS: all tests passed.
```

[ki.stfu, 2015-09-27 06:17:12]

On 2015/09/24 20:15:34, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I can't reproduce this failure on my side:
```
$ ./browser_tests --gtest_filter=ExtensionPreferenceApiTest.DataReductionProxy
IMPORTANT DEBUGGING NOTE: each test is run inside its own process.
For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with either
--single_process (to run the test in one launcher/browser process) or
--single-process (to do the above, and also run Chrome in single-process mode).
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = ExtensionPreferenceApiTest.DataReductionProxy
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExtensionPreferenceApiTest, where TypeParam = 
[ RUN      ] ExtensionPreferenceApiTest.DataReductionProxy
[15632:15632:0927/091410:WARNING:password_store_factory.cc(334)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[15632:15632:0927/091411:INFO:CONSOLE(0)] "[SUCCESS] getDrpPrefs", source:
chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[15632:15632:0927/091411:INFO:CONSOLE(0)] "[SUCCESS] updateDailyLengths",
source: chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[15632:15632:0927/091411:INFO:CONSOLE(0)] "[SUCCESS] clearDataSavings", source:
chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[       OK ] ExtensionPreferenceApiTest.DataReductionProxy (1116 ms)
[----------] 1 test from ExtensionPreferenceApiTest (1116 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1116 ms total)
[  PASSED  ] 1 test.
[1/1] ExtensionPreferenceApiTest.DataReductionProxy (1580 ms)
SUCCESS: all tests passed.
```

[ki.stfu, 2015-09-27 06:17:26]

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

[ki.stfu, 2015-09-27 06:17:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1354373002/#ps60001
(title: "Fix gcm::GCMStatsRecorderImpl::RecordCheckinFailure()'s signature")

[commit-bot: I haz the power, 2015-09-27 06:17:42]

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

[commit-bot: I haz the power, 2015-09-27 06:24:28]

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

[commit-bot: I haz the power, 2015-09-27 06:24:28]

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...)

[ki.stfu, 2015-09-27 08:42:58]

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

[ki.stfu, 2015-09-27 08:42:59]

sdefresne@: please review components/gcm_driver/gcm_stats_recorder_impl.[cc|h]

[ki.stfu, 2015-09-27 08:59:55]

I see a few failures on linux/mac buildbots, but it works for me on Linux:
```
$ ./browser_tests --gtest_filter=SessionRestoreTest.WindowWithOneTab
IMPORTANT DEBUGGING NOTE: each test is run inside its own process.
For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with either
--single_process (to run the test in one launcher/browser process) or
--single-process (to do the above, and also run Chrome in single-process mode).
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = SessionRestoreTest.WindowWithOneTab
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from SessionRestoreTest, where TypeParam = 
[ RUN      ] SessionRestoreTest.WindowWithOneTab
[13910:13910:0927/115348:WARNING:password_store_factory.cc(334)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[13910:13936:0927/115349:ERROR:browser_gpu_channel_host_factory.cc(144)] Failed
to create channel.
[13910:13936:0927/115349:ERROR:channel.cc(340)] RawChannel read error
(connection broken)
[13910:13930:0927/115349:WARNING:sqlite_persistent_cookie_store.cc(1309)] Failed
to post task from
FinishedLoadingCookies@../../net/extras/sqlite/sqlite_persistent_cookie_store.cc:1317
to client_task_runner_.
[       OK ] SessionRestoreTest.WindowWithOneTab (1321 ms)
[----------] 1 test from SessionRestoreTest (1321 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1321 ms total)
[  PASSED  ] 1 test.
[1/1] SessionRestoreTest.WindowWithOneTab (1832 ms)
SUCCESS: all tests passed.
$ ./browser_tests --gtest_filter=ExtensionPreferenceApiTest.DataReductionProxy
IMPORTANT DEBUGGING NOTE: each test is run inside its own process.
For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with either
--single_process (to run the test in one launcher/browser process) or
--single-process (to do the above, and also run Chrome in single-process mode).
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = ExtensionPreferenceApiTest.DataReductionProxy
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ExtensionPreferenceApiTest, where TypeParam = 
[ RUN      ] ExtensionPreferenceApiTest.DataReductionProxy
[13984:13984:0927/115354:WARNING:password_store_factory.cc(334)] Using basic
(unencrypted) store for password storage. See
http://code.google.com/p/chromium/wiki/LinuxPasswordStorage for more information
about password storage options.
[13984:14009:0927/115354:ERROR:browser_gpu_channel_host_factory.cc(144)] Failed
to create channel.
[13984:13984:0927/115355:INFO:CONSOLE(0)] "[SUCCESS] getDrpPrefs", source:
chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[13984:13984:0927/115355:INFO:CONSOLE(0)] "[SUCCESS] updateDailyLengths",
source: chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[13984:13984:0927/115355:INFO:CONSOLE(0)] "[SUCCESS] clearDataSavings", source:
chrome-extension://pfmgfdlgomnbgkofeojodiodmgpgmkac/test.html (0)
[       OK ] ExtensionPreferenceApiTest.DataReductionProxy (1665 ms)
[----------] 1 test from ExtensionPreferenceApiTest (1665 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1665 ms total)
[  PASSED  ] 1 test.
[1/1] ExtensionPreferenceApiTest.DataReductionProxy (2101 ms)
SUCCESS: all tests passed.
```

Does anybody know what is the problem?

[ki.stfu, 2015-09-27 19:56:42]

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

[ki.stfu, 2015-09-27 19:56:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1354373002/#ps80001
(title: "Rebase against ToT")

[commit-bot: I haz the power, 2015-09-27 19:56:55]

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

[commit-bot: I haz the power, 2015-09-27 20:05:21]

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

[commit-bot: I haz the power, 2015-09-27 20:05:22]

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...)

[sdefresne, 2015-09-28 07:29:54]

components/gcm_driver lgtm

[ki.stfu, 2015-09-28 07:51:22]

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

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

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

[commit-bot: I haz the power, 2015-09-28 08:25:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-28 08:26:38]

Patchset 5 (id:??) landed as
https://crrev.com/07be839f30d97158b803a2cc6dc6509b06c834e1
Cr-Commit-Position: refs/heads/master@{#351035}