Moved the TraceUploader to chrome/ and abstracted it for alternative new upload destinations.

Moved the TraceUploader from content/ to chrome/ and did some groundwork for other upload destinations to come.

The TraceUploader is using Google APIs and should not be under content/.

R=nduca,dsinclair,creis,thestig
BUG=457323
Committed: https://crrev.com/c563c582a8fe77aa2204c3164cae28161d5bcea6
Cr-Commit-Position: refs/heads/master@{#316258}

[oystein (OOO til 10th of July), 2015-02-10 19:25:26]

oysteine@chromium.org changed reviewers:
+ creis@chromium.org, dsinclair@chromium.org, nduca@chromium.org,
thestig@chromium.org

[oystein (OOO til 10th of July), 2015-02-10 19:26:40]

Patchset #1 (id:1) has been deleted

[oystein (OOO til 10th of July), 2015-02-10 19:30:52]

creis/thestig: Does this look like a reasonable way to interact with the trace
upload chrome/ stuff from content/? Suggestions on better abstractions welcome.

[oystein (OOO til 10th of July), 2015-02-10 19:31:56]

(content/browser/tracing/tracing_ui.cc is a good starting point to follow the
flow)

[Lei Zhang, 2015-02-10 20:32:33]

copy the OWNERS file over to chrome/browser/tracing ?

https://codereview.chromium.org/911153002/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/911153002/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2780: 'chrome_browser_tracing_sources': [
You need to include this set of files in the GN build too.

https://codereview.chromium.org/911153002/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:3028: '<@(chrome_browser_tracing_sources)',
This is in a !IOS block. Does Android need this code?

[oystein (OOO til 10th of July), 2015-02-10 21:08:13]

On 2015/02/10 20:32:33, Lei Zhang wrote:
> copy the OWNERS file over to chrome/browser/tracing ?
> 

Done.

https://codereview.chromium.org/911153002/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/911153002/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:3028: '<@(chrome_browser_tracing_sources)',
On 2015/02/10 20:32:33, Lei Zhang wrote:
> This is in a !IOS block. Does Android need this code?

Good point; we hope to eventually reuse parts of this on Android, but I moved
this to chrome_browser_non_mobile_sources for now.

[Lei Zhang, 2015-02-10 22:28:16]

chrome/ lgtm

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
File chrome/browser/tracing/chrome_tracing_delegate.h (right):

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
chrome/browser/tracing/chrome_tracing_delegate.h:12: content::TraceUploader*
GetTraceUploader(
Is it possible to change the interface to return a scoped_ptr so it's obvious
that the callers takes ownership of the TraceUploader?

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
File chrome/browser/tracing/crash_service_uploader.h (right):

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
chrome/browser/tracing/crash_service_uploader.h:49: UploadProgressCallback
progress_callback,
General feedback, feel free to address this in a separate CL: Callbacks can be
passed const-ref.

[oystein (OOO til 10th of July), 2015-02-10 23:38:57]

New patchsets have been uploaded after l-g-t-m from thestig@chromium.org

[oystein (OOO til 10th of July), 2015-02-10 23:40:15]

Thanks thestig!

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
File chrome/browser/tracing/chrome_tracing_delegate.h (right):

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
chrome/browser/tracing/chrome_tracing_delegate.h:12: content::TraceUploader*
GetTraceUploader(
On 2015/02/10 22:28:16, Lei Zhang wrote:
> Is it possible to change the interface to return a scoped_ptr so it's obvious
> that the callers takes ownership of the TraceUploader?

Done. Looks like the original code here was leaking this object each time.

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
File chrome/browser/tracing/crash_service_uploader.h (right):

https://codereview.chromium.org/911153002/diff/20001/chrome/browser/tracing/c...
chrome/browser/tracing/crash_service_uploader.h:49: UploadProgressCallback
progress_callback,
On 2015/02/10 22:28:16, Lei Zhang wrote:
> General feedback, feel free to address this in a separate CL: Callbacks can be
> passed const-ref.

Done.

[oystein (OOO til 10th of July), 2015-02-11 00:11:25]

Patchset #4 (id:80001) has been deleted

[oystein (OOO til 10th of July), 2015-02-12 16:15:00]

dsinclair: ping :)

[Charlie Reis, 2015-02-12 17:11:32]

Seems reasonable to me.  content/ LGTM with nits.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:107: class TracingDelegate;
nit: Alphabetize.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
File content/public/browser/trace_uploader.h (right):

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/trace_uploader.h:12: class TraceUploader {
Please add a class-level comment, since this is the public API.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/trace_uploader.h:14: typedef base::Callback<void(bool,
const std::string&, const std::string&)>
Let's document these callbacks so that embedders know what they're for, and what
the parameters are.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/tracing_delegate.h:17: class TracingDelegate {
Please add a class-level comment.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/tracing_delegate.h:21: virtual scoped_ptr<TraceUploader>
GetTraceUploader(
Please add a comment.

[dsinclair, 2015-02-12 20:57:35]

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2597: #if !defined(OS_ANDROID)
We don't allow uploading from android?

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.cc (right):

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:68: content::BrowserThread::UI,
FROM_HERE,
nit: This looks short enough to fit on one line?

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:113: if
(!GURL(upload_url).is_valid()) {
nit: Don't need {} on single line statement.

[oystein (OOO til 10th of July), 2015-02-13 16:57:59]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[oystein (OOO til 10th of July), 2015-02-13 17:02:45]

Thanks guys!

dsinclair: looks OK?

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2597: #if !defined(OS_ANDROID)
On 2015/02/12 20:57:35, dsinclair wrote:
> We don't allow uploading from android?

There didn't seem to be any point right now, since about://tracing doesn't work
on Android. I figured I'd just change the build rules when we have an
alternative way to trigger uploads here.

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.cc (right):

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:68: content::BrowserThread::UI,
FROM_HERE,
On 2015/02/12 20:57:35, dsinclair wrote:
> nit: This looks short enough to fit on one line?

The two first arguments yes, but the base::Bind() call is too long to align with
the start parenthesis of PostTask(, so all the arguments should be at +4
independation from the start (this is a retroactive investigative justification
of what 'git cl format' did :P).

https://codereview.chromium.org/911153002/diff/100001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:113: if
(!GURL(upload_url).is_valid()) {
On 2015/02/12 20:57:35, dsinclair wrote:
> nit: Don't need {} on single line statement.

Done.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/content_browser_client.h:107: class TracingDelegate;
On 2015/02/12 17:11:32, Charlie Reis wrote:
> nit: Alphabetize.

Done.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
File content/public/browser/trace_uploader.h (right):

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/trace_uploader.h:12: class TraceUploader {
On 2015/02/12 17:11:32, Charlie Reis wrote:
> Please add a class-level comment, since this is the public API.

Done.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/trace_uploader.h:14: typedef base::Callback<void(bool,
const std::string&, const std::string&)>
On 2015/02/12 17:11:32, Charlie Reis wrote:
> Let's document these callbacks so that embedders know what they're for, and
what
> the parameters are.

Done.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
File content/public/browser/tracing_delegate.h (right):

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/tracing_delegate.h:17: class TracingDelegate {
On 2015/02/12 17:11:32, Charlie Reis wrote:
> Please add a class-level comment.

Done.

https://codereview.chromium.org/911153002/diff/100001/content/public/browser/...
content/public/browser/tracing_delegate.h:21: virtual scoped_ptr<TraceUploader>
GetTraceUploader(
On 2015/02/12 17:11:32, Charlie Reis wrote:
> Please add a comment.

Done.

[Charlie Reis, 2015-02-13 17:19:16]

Thanks.  content/public changes LGTM.

[dsinclair, 2015-02-13 17:43:15]

lgtm

[oystein (OOO til 10th of July), 2015-02-13 17:43:41]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2015-02-13 17:44:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/911153002/140001

[commit-bot: I haz the power, 2015-02-13 18:37:30]

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

[commit-bot: I haz the power, 2015-02-13 18:37:31]

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

[oystein (OOO til 10th of July), 2015-02-13 18:38:25]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2015-02-13 18:39:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/911153002/140001

[commit-bot: I haz the power, 2015-02-13 19:20:04]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-02-13 19:20:47]

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