Make compression optional in TraceUploader.

Make compression optional in TraceUploader.

In many instances the trace data is already compressed, and the TraceCrashServiceUploader is compressing it again.

BUG=504862
Committed: https://crrev.com/c93f6b8c13f493a54bdace70b66b643981b81043
Cr-Commit-Position: refs/heads/master@{#337828}

[shatch, 2015-07-07 15:28:41]

simonhatch@chromium.org changed reviewers:
+ dsinclair@chromium.org

[shatch, 2015-07-07 15:29:41]

Patchset #1 (id:1) has been deleted

[dsinclair, 2015-07-07 16:42:04]

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

https://codereview.chromium.org/1225923003/diff/20001/content/public/browser/...
content/public/browser/trace_uploader.h:30: bool compress_on_upload,
Can we switch the bool to an enum? COMPRESSED_UPLOAD and UNCOMPRESSED_UPLOAD ?

[shatch, 2015-07-07 18:25:48]

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

https://codereview.chromium.org/1225923003/diff/20001/content/public/browser/...
content/public/browser/trace_uploader.h:30: bool compress_on_upload,
On 2015/07/07 16:42:04, dsinclair wrote:
> Can we switch the bool to an enum? COMPRESSED_UPLOAD and UNCOMPRESSED_UPLOAD ?


Done.

[dsinclair, 2015-07-07 18:36:37]

lgtm w/ nit.

https://codereview.chromium.org/1225923003/diff/40001/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/1225923003/diff/40001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:319: DoUploadInternal(file_contents,
false);
Can these pass the enum as well?

[shatch, 2015-07-07 19:25:15]

simonhatch@chromium.org changed reviewers:
+ davidben@chromium.org

[shatch, 2015-07-07 19:26:00]

+davidben for content/public/browser/trace_uploader.h

https://codereview.chromium.org/1225923003/diff/40001/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/1225923003/diff/40001/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:319: DoUploadInternal(file_contents,
false);
On 2015/07/07 18:36:37, dsinclair wrote:
> Can these pass the enum as well?

Done.

[davidben, 2015-07-07 23:35:25]

https://codereview.chromium.org/1225923003/diff/20002/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/1225923003/diff/20002/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:319: DoUploadInternal(file_contents,
TraceUploader::UNCOMPRESSED_UPLOAD);
It's not obvious from reading this why base64 implies uncompressed. Looks like
its keyed on an:
   var isBinary = /[.]gz$/.test(filename) || /[.]zip$/.test(filename);

Could you add a comment like perhaps:

  // doUploadBase64 is used to upload binary data which is assumed to already be
compressed.

[shatch, 2015-07-08 14:04:11]

https://codereview.chromium.org/1225923003/diff/20002/content/browser/tracing...
File content/browser/tracing/tracing_ui.cc (right):

https://codereview.chromium.org/1225923003/diff/20002/content/browser/tracing...
content/browser/tracing/tracing_ui.cc:319: DoUploadInternal(file_contents,
TraceUploader::UNCOMPRESSED_UPLOAD);
On 2015/07/07 23:35:25, David Benjamin wrote:
> It's not obvious from reading this why base64 implies uncompressed. Looks like
> its keyed on an:
>    var isBinary = /[.]gz$/.test(filename) || /[.]zip$/.test(filename);
> 
> Could you add a comment like perhaps:
> 
>   // doUploadBase64 is used to upload binary data which is assumed to already
be
> compressed.

Done.

[davidben, 2015-07-08 16:02:50]

lgtm

[shatch, 2015-07-08 16:03:13]

The CQ bit was checked by simonhatch@chromium.org

[shatch, 2015-07-08 16:03:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/1225923003/#ps70001
(title: "Added comment for clarification.")

[commit-bot: I haz the power, 2015-07-08 16:03:53]

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

[commit-bot: I haz the power, 2015-07-08 16:09:15]

Committed patchset #4 (id:70001)

[commit-bot: I haz the power, 2015-07-08 16:10:08]

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