Check the SRT logs upload time for an entire queue of SRT invocations at once.

Check the SRT logs upload time for an entire queue of SRT invocations at once.

BUG=688430
Review-Url: https://codereview.chromium.org/2669393003
Cr-Commit-Position: refs/heads/master@{#448842}
Committed: https://chromium.googlesource.com/chromium/src/+/918826c74118b237c236b8af6709bc51ccb9b766

[Joe Mason, 2017-02-03 18:04:00]

Description was changed from

==========
Check the SRT logs upload time for an entire queue of SRT invocations at once.

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

to

==========
Check the SRT logs upload time for an entire queue of SRT invocations at once.

BUG=688430
==========

[Joe Mason, 2017-02-03 18:04:00]

joenotcharles@chromium.org changed reviewers:
+ ftirelo@chromium.org, grt@chromium.org

[Joe Mason, 2017-02-03 18:04:11]

ptal

[grt (UTC plus 2), 2017-02-06 08:25:35]

lgtm w/ a nit

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/srt_fetcher_win.cc:1016: bool
in_logs_upload_period_ = false;
please document this new member.

[ftirelo, 2017-02-06 14:13:24]

lgtm with a small nit.

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/srt_fetcher_win.cc:916: const base::Time now =
base::Time::Now();
Please reuse |now| from line 905.

[Joe Mason, 2017-02-06 18:07:09]

joenotcharles@chromium.org changed reviewers:
+ sorin@chromium.org

[Joe Mason, 2017-02-06 18:07:09]

+sorin for owners review

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/srt_fetcher_win.cc (right):

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/srt_fetcher_win.cc:916: const base::Time now =
base::Time::Now();
On 2017/02/06 14:13:24, ftirelo wrote:
> Please reuse |now| from line 905.

Good catch. Done.

https://codereview.chromium.org/2669393003/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/srt_fetcher_win.cc:1016: bool
in_logs_upload_period_ = false;
On 2017/02/06 08:25:35, grt (UTC plus 1) wrote:
> please document this new member.

Done.

[Joe Mason, 2017-02-06 18:08:33]

joenotcharles@chromium.org changed reviewers:
+ jialiul@chromium.org
- sorin@chromium.org

[Joe Mason, 2017-02-06 18:08:33]

Sorry, got mixed up which patch this was. Meant to add jialiul for OWNERS
review.

[Sorin Jianu, 2017-02-06 18:31:44]

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

[Sorin Jianu, 2017-02-06 18:31:45]

thank you, fyi small comment below.

https://codereview.chromium.org/2669393003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right):

https://codereview.chromium.org/2669393003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:609: for (auto path
: {path1, path2}) {
do we want to use auto& to avoid a copy here?

[Joe Mason, 2017-02-06 18:50:52]

https://codereview.chromium.org/2669393003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc (right):

https://codereview.chromium.org/2669393003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/srt_fetcher_browsertest_win.cc:609: for (auto path
: {path1, path2}) {
On 2017/02/06 18:31:44, Sorin Jianu wrote:
> do we want to use auto& to avoid a copy here?

Good idea. Done.

[Sorin Jianu, 2017-02-07 22:58:28]

lgtm

thank you

[Jialiu Lin, 2017-02-07 23:06:55]

lgtm

[Joe Mason, 2017-02-07 23:36:10]

The CQ bit was checked by joenotcharles@chromium.org

[Joe Mason, 2017-02-07 23:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
ftirelo@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2669393003/#ps40001
(title: "Use const ref in loop index")

[commit-bot: I haz the power, 2017-02-07 23:36:55]

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, 2017-02-08 01:42:29]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486510570398480,
"parent_rev": "ee5808847dbb29e56ff7b0ed2dafdc81291e53a2", "commit_rev":
"918826c74118b237c236b8af6709bc51ccb9b766"}

[commit-bot: I haz the power, 2017-02-08 01:43:07]

Description was changed from

==========
Check the SRT logs upload time for an entire queue of SRT invocations at once.

BUG=688430
==========

to

==========
Check the SRT logs upload time for an entire queue of SRT invocations at once.

BUG=688430

Review-Url: https://codereview.chromium.org/2669393003
Cr-Commit-Position: refs/heads/master@{#448842}
Committed:
https://chromium.googlesource.com/chromium/src/+/918826c74118b237c236b8af6709...
==========

[commit-bot: I haz the power, 2017-02-08 01:43:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/918826c74118b237c236b8af6709...