[BackgroundSync] Add max sync event duration to BackgroundSyncParameters

[BackgroundSync] Add max sync event duration to BackgroundSyncParameters

Adds max sync event duration as a parameter to
BackgroundSyncParameters and allows it to be overwritten via field
trials.

BUG=569001
Committed: https://crrev.com/f056423ad06d75eb45425e8e066a4150287cf568
Cr-Commit-Position: refs/heads/master@{#366987}

[jkarlin, 2015-12-18 14:58:21]

jkarlin@chromium.org changed reviewers:
+ davidben@chromium.org, iclelland@chromium.org

[jkarlin, 2015-12-18 14:58:22]

iclelland@ PTAL at everything

davidben@ PTAL at content/public/

Thanks!

[davidben, 2015-12-19 00:28:25]

lgtm

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
File chrome/browser/background_sync/background_sync_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
chrome/browser/background_sync/background_sync_controller_impl_unittest.cc:127:
"8000000000000";  // something larger than int32.
Did you mean to lose that part of the test?

[iclelland, 2015-12-21 19:50:15]

lgtm with one issue.

My only meta-comment is that we now have three finch-definable time values,
using three different units (ms, sec, and min.) I don't know what the other side
of the finch UI looks like, but is it worth it to define all of these in the
same scale, to avoid any possible confusion when setting them?

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
File chrome/browser/background_sync/background_sync_controller_impl.cc (right):

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
chrome/browser/background_sync/background_sync_controller_impl.cc:88: if
(base::StringToInt64(field_params[kMaxSyncEventDuration],
I think this should be StringToInt (and `int max_sync_event_duration_sec` rather
than int64_t), to match the processing of initial_retry_delay_min, and the int
constant defined in background_sync_parameters.cc.

[jkarlin, 2015-12-28 15:51:04]

> My only meta-comment is that we now have three finch-definable time values,
> using three different units (ms, sec, and min.) I don't know what the other
side
> of the finch UI looks like, but is it worth it to define all of these in the
> same scale, to avoid any possible confusion when setting them?

I don't think it's a big deal either way but since they could all reasonably be
seconds I changed them to seconds.

[jkarlin, 2015-12-28 15:51:29]

Thanks all.

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
File chrome/browser/background_sync/background_sync_controller_impl.cc (right):

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
chrome/browser/background_sync/background_sync_controller_impl.cc:88: if
(base::StringToInt64(field_params[kMaxSyncEventDuration],
On 2015/12/21 19:50:15, iclelland wrote:
> I think this should be StringToInt (and `int max_sync_event_duration_sec`
rather
> than int64_t), to match the processing of initial_retry_delay_min, and the int
> constant defined in background_sync_parameters.cc.

Done.

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
File chrome/browser/background_sync/background_sync_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/1536023002/diff/1/chrome/browser/background_s...
chrome/browser/background_sync/background_sync_controller_impl_unittest.cc:127:
"8000000000000";  // something larger than int32.
On 2015/12/19 00:28:25, davidben wrote:
> Did you mean to lose that part of the test?

Yeah, there is no need for it to be an int64. Also changed to int in the field
trial parsing code.

[jkarlin, 2015-12-28 16:51:09]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-12-28 16:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1536023002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-28 16:51:30]

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

[jkarlin, 2015-12-28 16:51:51]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2015-12-28 16:56:06]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-12-28 16:56:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1536023002/#ps80001
(title: "Rename a constant to match the others")

[commit-bot: I haz the power, 2015-12-28 16:56:12]

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

[commit-bot: I haz the power, 2015-12-28 17:53:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-28 17:54:20]

Description was changed from

==========
[BackgroundSync] Add max sync event duration to BackgroundSyncParameters

Adds max sync event duration as a parameter to
BackgroundSyncParameters and allows it to be overwritten via field
trials.

BUG=569001
==========

to

==========
[BackgroundSync] Add max sync event duration to BackgroundSyncParameters

Adds max sync event duration as a parameter to
BackgroundSyncParameters and allows it to be overwritten via field
trials.

BUG=569001

Committed: https://crrev.com/f056423ad06d75eb45425e8e066a4150287cf568
Cr-Commit-Position: refs/heads/master@{#366987}
==========

[commit-bot: I haz the power, 2015-12-28 17:54:21]

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