Add feedback uploader to upload reports in a sequenced manner with retries.

Add feedback uploader to upload reports in a sequenced manner with retries.
This is the first part of the CL that will enable re-sending feedback reports that were sent when the user was offline, across sessions.

R=jcivelli@chromium.org, zork@chromium.org
BUG=249853
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241962

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242069

[Zachary Kuznia, 2013-12-17 01:12:57]

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/OWNERS
File chrome/browser/feedback/OWNERS (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/OWNE...
chrome/browser/feedback/OWNERS:2: zork@chromium.org
This change should probably be in a separate CL.

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_uploader.cc (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader.cc:28: const base::TimeDelta
kRetryDelay = base::TimeDelta::FromMinutes(60);
Is there a reason you don't use a shorter delay and an exponential backoff?

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_uploader.h (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader.h:25: struct FeedbackReport {
I think this class could be implemented in the .cc file.

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader.h:34: FeedbackReport& operator=(
I think this doesn't need a line break.

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_uploader_delegate.cc (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader_delegate.cc:60: delete source;
If you put this in a scoped_ptr at the beginning of the function, you can
protect against a memory leak if an early return gets added later.

[rkc, 2013-12-17 23:26:00]

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/OWNERS
File chrome/browser/feedback/OWNERS (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/OWNE...
chrome/browser/feedback/OWNERS:2: zork@chromium.org
On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> This change should probably be in a separate CL.

Done.

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_uploader.cc (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader.cc:28: const base::TimeDelta
kRetryDelay = base::TimeDelta::FromMinutes(60);
On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> Is there a reason you don't use a shorter delay and an exponential backoff?

Most of the reason behind the exponential backoff was the shorter delay. The
reason for the shorter delay was that we weren't going to be persisting reports
between Chrome sessions. Since that reason doesn't exist, we can afford a longer
delay. With a 1 hour delay, there didn't seem any particular need for a backoff
(the previous backoff also capped at 4 hours anyway, which seemed excessive).

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_uploader.h (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader.h:25: struct FeedbackReport {
On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> I think this class could be implemented in the .cc file.

The struct implementation itself is needed in this .h file, since we define the
reports comparator struct defined below, which accesses this struct. Having just
the declaration here won't work.

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader.h:34: FeedbackReport& operator=(
On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> I think this doesn't need a line break.

Done.

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
File chrome/browser/feedback/feedback_uploader_delegate.cc (right):

https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
chrome/browser/feedback/feedback_uploader_delegate.cc:60: delete source;
On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> If you put this in a scoped_ptr at the beginning of the function, you can
> protect against a memory leak if an early return gets added later.

Done.

[Zachary Kuznia, 2013-12-17 23:31:01]

On 2013/12/17 23:26:00, Rahul Chaturvedi wrote:
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/OWNERS
> File chrome/browser/feedback/OWNERS (right):
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/OWNE...
> chrome/browser/feedback/OWNERS:2: mailto:zork@chromium.org
> On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> > This change should probably be in a separate CL.
> 
> Done.
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> File chrome/browser/feedback/feedback_uploader.cc (right):
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> chrome/browser/feedback/feedback_uploader.cc:28: const base::TimeDelta
> kRetryDelay = base::TimeDelta::FromMinutes(60);
> On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> > Is there a reason you don't use a shorter delay and an exponential backoff?
> 
> Most of the reason behind the exponential backoff was the shorter delay. The
> reason for the shorter delay was that we weren't going to be persisting
reports
> between Chrome sessions. Since that reason doesn't exist, we can afford a
longer
> delay. With a 1 hour delay, there didn't seem any particular need for a
backoff
> (the previous backoff also capped at 4 hours anyway, which seemed excessive).
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> File chrome/browser/feedback/feedback_uploader.h (right):
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> chrome/browser/feedback/feedback_uploader.h:25: struct FeedbackReport {
> On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> > I think this class could be implemented in the .cc file.
> 
> The struct implementation itself is needed in this .h file, since we define
the
> reports comparator struct defined below, which accesses this struct. Having
just
> the declaration here won't work.
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> chrome/browser/feedback/feedback_uploader.h:34: FeedbackReport& operator=(
> On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> > I think this doesn't need a line break.
> 
> Done.
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> File chrome/browser/feedback/feedback_uploader_delegate.cc (right):
> 
>
https://codereview.chromium.org/116863002/diff/1/chrome/browser/feedback/feed...
> chrome/browser/feedback/feedback_uploader_delegate.cc:60: delete source;
> On 2013/12/17 01:12:57, Zachary Kuznia wrote:
> > If you put this in a scoped_ptr at the beginning of the function, you can
> > protect against a memory leak if an early return gets added later.
> 
> Done.

LGTM once struct is in cc.

[Jay Civelli, 2013-12-18 00:20:10]

LGTM for DEPS change

[commit-bot: I haz the power, 2013-12-18 00:23:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/116863002/40001

[commit-bot: I haz the power, 2013-12-18 01:24:38]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-18 23:10:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/116863002/60001

[commit-bot: I haz the power, 2013-12-19 00:19:12]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-19 19:34:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/116863002/60001

[rkc, 2013-12-19 22:46:10]

Committed patchset #5 manually as r241962 (presubmit successful).

[rkc, 2013-12-19 23:40:34]

A revert of this CL has been created in
https://codereview.chromium.org/119603003/ by rkc@chromium.org.

The reason for reverting is: Broke Static Initializers.

[commit-bot: I haz the power, 2013-12-20 00:07:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/116863002/100001

[commit-bot: I haz the power, 2013-12-20 08:55:40]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-20 09:16:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/116863002/100001

[commit-bot: I haz the power, 2013-12-20 12:18:05]

Change committed as 242069