Make MetricsService upload logs through an interface.

Make MetricsService upload logs through an interface.

This allows targets that do not wish to depend on net
to use the MetricsService code.

Also, moves compression code to be in the net/ directory
of the metrics component.

BUG=375771, 374295
R=isherman@chromium.org,blundell@chromium.org,davidben@chromium.org
TBR=agl@chromium.org

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

[Alexei Svitkine (slow), 2014-05-23 15:25:02]

Mr. Sherman and Dr. Blundell, please have a look.

[blundell, 2014-05-26 07:56:55]

LGTM

https://codereview.chromium.org/291153013/diff/130001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/291153013/diff/130001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1180: // Compression failed, and log
discarded :-/.
is this comment a little out of date now?

https://codereview.chromium.org/291153013/diff/130001/components/metrics/DEPS
File components/metrics/DEPS (left):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/DEPS...
components/metrics/DEPS:2: "-net",
This should still be there, right, and then there should be an inclusion in the
net/ subdir?

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
components/metrics/metrics_service_client.h:63: const base::Callback<void(int)>
on_upload_complete) = 0;
It seems like all these parameters belong as constructor parameters on
MetricsLogUploader, and the second sentence of the comment on this function
could then move to be a comment on MetricsLogUploader as well.

https://codereview.chromium.org/291153013/diff/130001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.cc:68:
scoped_ptr<net::URLFetcher> fetch(current_fetch_.Pass());
nit: This is pretty bizarre code. Why not just do a current_fetch_.reset();
after line 73?

[Alexei Svitkine (slow), 2014-05-26 20:04:09]

https://codereview.chromium.org/291153013/diff/130001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/291153013/diff/130001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1180: // Compression failed, and log
discarded :-/.
On 2014/05/26 07:56:56, blundell wrote:
> is this comment a little out of date now?

Done.

https://codereview.chromium.org/291153013/diff/130001/components/metrics/DEPS
File components/metrics/DEPS (left):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/DEPS...
components/metrics/DEPS:2: "-net",
On 2014/05/26 07:56:56, blundell wrote:
> This should still be there, right, and then there should be an inclusion in
the
> net/ subdir?

Done.

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
components/metrics/metrics_service_client.h:63: const base::Callback<void(int)>
on_upload_complete) = 0;
On 2014/05/26 07:56:56, blundell wrote:
> It seems like all these parameters belong as constructor parameters on
> MetricsLogUploader, and the second sentence of the comment on this function
> could then move to be a comment on MetricsLogUploader as well.

Just to clarify, do you mean changing MetricsLogUploader to no longer be just a
pure virtual interface but instead have a ctor and protected members
server_url_, mime_type_ and on_upload_complete_ that can then be used by its
sub-classes?

I'm okay doing this, just wanted to make sure this is what you're suggesting
since this will involve adding a metrics_log_uploader.cc (and this function
would still take the same params).

https://codereview.chromium.org/291153013/diff/130001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.cc:68:
scoped_ptr<net::URLFetcher> fetch(current_fetch_.Pass());
On 2014/05/26 07:56:56, blundell wrote:
> nit: This is pretty bizarre code. Why not just do a current_fetch_.reset();
> after line 73?

Done. I had copied the pattern from the previous code, but I agree that a
.reset() at the end is simpler.

[blundell, 2014-05-26 20:22:15]

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
components/metrics/metrics_service_client.h:63: const base::Callback<void(int)>
on_upload_complete) = 0;
I was actually just suggesting adding them as parameters to the
MetricsLogUploader constructor, as it seems intuitive to me to have these params
on that constructor (where they can be described in detail in the place where
someone would look when trying to figure out how to implement that interface in
a reasonable way). The extra step that you're suggesting of having them be ivars
of the MetricsLogUploader base class sounds good though.

On 2014/05/26 20:04:10, Alexei Svitkine wrote:
> On 2014/05/26 07:56:56, blundell wrote:
> > It seems like all these parameters belong as constructor parameters on
> > MetricsLogUploader, and the second sentence of the comment on this function
> > could then move to be a comment on MetricsLogUploader as well.
> 
> Just to clarify, do you mean changing MetricsLogUploader to no longer be just
a
> pure virtual interface but instead have a ctor and protected members
> server_url_, mime_type_ and on_upload_complete_ that can then be used by its
> sub-classes?
> 
> I'm okay doing this, just wanted to make sure this is what you're suggesting
> since this will involve adding a metrics_log_uploader.cc (and this function
> would still take the same params).

[Alexei Svitkine (slow), 2014-05-27 14:36:39]

Thanks, PTAL.

I also now moved compression_utils_unittest.cc to the component as well in the
latest patch set.

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/130001/components/metrics/metr...
components/metrics/metrics_service_client.h:63: const base::Callback<void(int)>
on_upload_complete) = 0;
On 2014/05/26 20:22:15, blundell wrote:
> I was actually just suggesting adding them as parameters to the
> MetricsLogUploader constructor, as it seems intuitive to me to have these
params
> on that constructor (where they can be described in detail in the place where
> someone would look when trying to figure out how to implement that interface
in
> a reasonable way). The extra step that you're suggesting of having them be
ivars
> of the MetricsLogUploader base class sounds good though.
> 
> On 2014/05/26 20:04:10, Alexei Svitkine wrote:
> > On 2014/05/26 07:56:56, blundell wrote:
> > > It seems like all these parameters belong as constructor parameters on
> > > MetricsLogUploader, and the second sentence of the comment on this
function
> > > could then move to be a comment on MetricsLogUploader as well.
> > 
> > Just to clarify, do you mean changing MetricsLogUploader to no longer be
just
> a
> > pure virtual interface but instead have a ctor and protected members
> > server_url_, mime_type_ and on_upload_complete_ that can then be used by its
> > sub-classes?
> > 
> > I'm okay doing this, just wanted to make sure this is what you're suggesting
> > since this will involve adding a metrics_log_uploader.cc (and this function
> > would still take the same params).
> 

Done. I've now added those params/ivars to the base class.

[blundell, 2014-05-27 14:41:27]

Did you mean to add metrics_log_uploader.cc?

[Alexei Svitkine (slow), 2014-05-27 14:56:05]

Oops, added now. Thanks!

[blundell, 2014-05-27 15:01:01]

lgtm

[Ilya Sherman, 2014-05-28 06:36:38]

Apologies for not getting to this review today -- my flight got majorly delayed,
and the airplane didn't even have the decency to have WiFi.  I'll definitely be
able to take a look tomorrow.

[Ilya Sherman, 2014-05-29 03:54:48]

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:44: const
base::Callback<void(int)> on_upload_complete) OVERRIDE;
nit: Pass the callback by const-reference?

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:386:
waiting_for_asynchronous_reporting_step_(false) {
Did you mean to remove this instance variable entirely?  If not, you should
probably keep the initialization for it.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:201: #include
"components/metrics/net/compression_utils.h"
Hmm, are you planning to remove this dependency in a follow-up CL?

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:997: // If somehow there is a fetch in
progress, we return and hope things work
nit: "fetch" -> "log upload"

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1157: kServerUrl, kMimeType,
Do we expect clients to actually customize the server URL or MIME type?

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1165: bool success =
log_uploader_->UploadLog(log_manager_.staged_log(), hash);
Technically this is a behavioral change, as the code previously incorrectly
checked that the current_fetch_ existed at all, rather than that it had some
upload data set.  I think this change is fine, as documented by DCHECKs, but
please double-check this.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1166:
UMA_HISTOGRAM_BOOLEAN("UMA.UploadCreation", success);
The meaning of this histogram is definitely changing.  We should probably rename
it, and deprecate the old one.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1168: // Skip this upload and hope
things work out next time.
log_upload_in_progress_ should be reset here, right?

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
File components/metrics/metrics_log_uploader.h (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
components/metrics/metrics_log_uploader.h:28: // Uploads a log with the
specified |log_data| and |log_hash|.
nit: Please document the |log_data| and |log_hash| expectations more.  For
example, the |log_hash| is expected to be hex-encoded.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
components/metrics/metrics_log_uploader.h:32: protected:
Hmm, generally protected instance variables are discouraged.  However, since
these are all const, I guess it's not a big issue.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
components/metrics/metrics_service_client.h:13: #include
"components/metrics/metrics_log_uploader.h"
nit: Perhaps forward-declare this instead?

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.cc:29: std::string
compressed_log_text;
nit: Should this perhaps be renamed to "compressed_log_data", to match the
variable name "log_data"?  (Or, alternately, should "log_data" be renamed back
to "log_text"?)

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.h (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.h:8: #include
"components/metrics/metrics_log_uploader.h"
nit: I'm pretty sure that this should be sorted in the normal order, rather than
promoted to the top of the file.  Is my memory of the style guide out of date on
this?

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.h:12: #include "base/callback.h"
nit: I think callback-forward suffices, right?  Though, honestly, since the only
use of base::Callback is for an overridden method, I wouldn't bother to specify
this #include at all.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.h:28: // other params (see
comments on MetricsLogUploader for details).
nit: Please document lifetime expectations for the |request_context_getter|.

[Alexei Svitkine (slow), 2014-05-29 14:32:48]

+davidben from net/OWNERS to review components/metrics/net/DEPS, per presubmit

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.h:44: const
base::Callback<void(int)> on_upload_complete) OVERRIDE;
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: Pass the callback by const-reference?

Done.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:386:
waiting_for_asynchronous_reporting_step_(false) {
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> Did you mean to remove this instance variable entirely?  If not, you should
> probably keep the initialization for it.

Removed now - good catch!

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:201: #include
"components/metrics/net/compression_utils.h"
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> Hmm, are you planning to remove this dependency in a follow-up CL?

Removed now. Thanks!

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:997: // If somehow there is a fetch in
progress, we return and hope things work
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: "fetch" -> "log upload"

Done.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1157: kServerUrl, kMimeType,
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> Do we expect clients to actually customize the server URL or MIME type?

I passed it as a parameter so that we don't hardcode it in the net uploader,
since it will be needed by other uploader implementations.

I suppose it's possible to instead have them as static constants on
MetricsLogUploader - however I'm not sure that's so great either. For example, I
think it's quite possible that RapporService may want to use the same interface
for uploading logs and in that case, these would need to be different.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1165: bool success =
log_uploader_->UploadLog(log_manager_.staged_log(), hash);
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> Technically this is a behavioral change, as the code previously incorrectly
> checked that the current_fetch_ existed at all, rather than that it had some
> upload data set.  I think this change is fine, as documented by DCHECKs, but
> please double-check this.

Right. I believe the early return below was previously unreachable since the
fetch would always be non-NULL by the time it got there.

Now, we may reach it if compression failed which previously would actually
resulted in a weird state (i.e. the fetch existing but no upload data set).

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1166:
UMA_HISTOGRAM_BOOLEAN("UMA.UploadCreation", success);
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> The meaning of this histogram is definitely changing.  We should probably
rename
> it, and deprecate the old one.

IMHO, the existing histogram's meaning still matches this. Ultimately, it's an
implementation detail of the specific uploader instance why the upload failed to
be created (which is what a false return value indicates here - which is e.g.
different then callback being called with an error value), so the examples given
in that histogram could still be the case. Though perhaps we should explicitly
clarify it and mention compression failure rather than serialization failure.

Anyway, let me know if you still prefer to make a separate histogram given the
above or not.

https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1168: // Skip this upload and hope
things work out next time.
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> log_upload_in_progress_ should be reset here, right?

Indeed. Done!

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
File components/metrics/metrics_log_uploader.h (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
components/metrics/metrics_log_uploader.h:28: // Uploads a log with the
specified |log_data| and |log_hash|.
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: Please document the |log_data| and |log_hash| expectations more.  For
> example, the |log_hash| is expected to be hex-encoded.

Done.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
components/metrics/metrics_log_uploader.h:32: protected:
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> Hmm, generally protected instance variables are discouraged.  However, since
> these are all const, I guess it's not a big issue.

Yeah, providing const getters for them seems like unnecessary boilerplate.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/metr...
components/metrics/metrics_service_client.h:13: #include
"components/metrics/metrics_log_uploader.h"
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: Perhaps forward-declare this instead?

Done.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.cc:29: std::string
compressed_log_text;
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: Should this perhaps be renamed to "compressed_log_data", to match the
> variable name "log_data"?  (Or, alternately, should "log_data" be renamed back
> to "log_text"?)

Done.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.h (right):

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.h:8: #include
"components/metrics/metrics_log_uploader.h"
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: I'm pretty sure that this should be sorted in the normal order, rather
than
> promoted to the top of the file.  Is my memory of the style guide out of date
on
> this?

Yep, my bad. Done.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.h:12: #include "base/callback.h"
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: I think callback-forward suffices, right?  Though, honestly, since the
only
> use of base::Callback is for an overridden method, I wouldn't bother to
specify
> this #include at all.

Done.

https://codereview.chromium.org/291153013/diff/220001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.h:28: // other params (see
comments on MetricsLogUploader for details).
On 2014/05/29 03:54:48, Ilya Sherman wrote:
> nit: Please document lifetime expectations for the |request_context_getter|.

Done.

[Ilya Sherman, 2014-05-29 20:29:47]

LGTM, thanks.

Hmm, did you (perhaps accidentally?) delete the patch set that I commented on?

On 2014/05/29 14:32:48, Alexei Svitkine wrote:
>
https://codereview.chromium.org/291153013/diff/220001/chrome/browser/metrics/...
> chrome/browser/metrics/metrics_service.cc:1166:
> UMA_HISTOGRAM_BOOLEAN("UMA.UploadCreation", success);
> On 2014/05/29 03:54:48, Ilya Sherman wrote:
> > The meaning of this histogram is definitely changing.  We should probably
> rename
> > it, and deprecate the old one.
> 
> IMHO, the existing histogram's meaning still matches this. Ultimately, it's an
> implementation detail of the specific uploader instance why the upload failed
to
> be created (which is what a false return value indicates here - which is e.g.
> different then callback being called with an error value), so the examples
given
> in that histogram could still be the case. Though perhaps we should explicitly
> clarify it and mention compression failure rather than serialization failure.
> 
> Anyway, let me know if you still prefer to make a separate histogram given the
> above or not.

In theory, the DCHECKs in the code indicate that the log creation shouldn't be
able to fail.  Therefore, the histogram is effectively measuring whether the
DCHECKs are wrong.  It's now measuring a different DCHECK than it had before,
which is why I'd slightly lean toward renaming it.  However, I don't feel
strongly about it.

[Alexei Svitkine (slow), 2014-05-29 20:33:08]

On 2014/05/29 20:29:47, Ilya Sherman wrote:
> LGTM, thanks.
> 
> Hmm, did you (perhaps accidentally?) delete the patch set that I commented on?

Sigh, it looks that way. :\ Sorry, if that's what happened....

[davidben, 2014-05-29 20:38:02]

components/metrics/net/DEPS LGTM. I didn't do more than briefly skim the rest of
it.

[Alexei Svitkine (slow), 2014-05-29 20:38:44]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:43:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/260001

[commit-bot: I haz the power, 2014-05-29 21:12:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10646)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13720)

[Alexei Svitkine (slow), 2014-05-29 21:17:32]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-29 21:21:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10668)

[commit-bot: I haz the power, 2014-05-29 21:21:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/260002

[commit-bot: I haz the power, 2014-05-29 22:26:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10676)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13750)

[commit-bot: I haz the power, 2014-05-29 22:37:43]

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

[commit-bot: I haz the power, 2014-05-29 22:37:44]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10713)

[Alexei Svitkine (slow), 2014-05-30 14:32:24]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-30 14:33:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/280001

[Alexei Svitkine (slow), 2014-05-30 17:02:09]

The CQ bit was unchecked by asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-05-30 17:12:43]

Oops, forgot to list "+third_party/zlib" in components/metrics/net/DEPS for
compression_utils.cc.

+agl from third_party/zlib/OWNERS for including it in the DEPS file.

[Alexei Svitkine (slow), 2014-05-30 17:17:53]

agl@ is OOO, so TBRing

[Alexei Svitkine (slow), 2014-05-30 17:18:07]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:20:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/300001

[Alexei Svitkine (slow), 2014-05-30 17:34:19]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:38:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/320001

[Alexei Svitkine (slow), 2014-05-30 18:56:33]

Hmm, looks like it's still hitting presubmit errors on the bot:

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  components/metrics/net/compression_utils.cc
      Illegal include: "third_party/zlib/zlib.h"
    Because of no rule applying. \
  components/metrics/net/net_metrics_log_uploader.cc
      Illegal include: "net/base/load_flags.h"
    Because of "-net" from components/metrics's include_rules. \
  components/metrics/net/net_metrics_log_uploader.cc
      Illegal include: "net/url_request/url_fetcher.h"
    Because of "-net" from components/metrics's include_rules. \
  components/metrics/net/net_metrics_log_uploader.h
      Illegal include: "net/url_request/url_fetcher_delegate.h"
    Because of "-net" from components/metrics's include_rules.

However, I have net and third_party/zlib both listed in
components/metrics/net/DEPS. Anyone know what's wrong?

[blundell, 2014-05-30 19:44:25]

https://codereview.chromium.org/291153013/diff/320001/components/metrics/net/...
File components/metrics/net/DEPS (right):

https://codereview.chromium.org/291153013/diff/320001/components/metrics/net/...
components/metrics/net/DEPS:2: "+net"
I believe you need commas at the end of this line and the next one.

[Alexei Svitkine (slow), 2014-05-30 19:51:26]

https://codereview.chromium.org/291153013/diff/320001/components/metrics/net/...
File components/metrics/net/DEPS (right):

https://codereview.chromium.org/291153013/diff/320001/components/metrics/net/...
components/metrics/net/DEPS:2: "+net"
On 2014/05/30 19:44:26, blundell wrote:
> I believe you need commas at the end of this line and the next one.

Ah, thanks! Done.

[Alexei Svitkine (slow), 2014-05-30 19:51:36]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-30 19:55:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/340001

[Alexei Svitkine (slow), 2014-05-30 20:52:39]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:55:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/360001

[Alexei Svitkine (slow), 2014-05-31 01:06:52]

On 2014/05/30 20:55:34, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> 
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/360001

Looks like the moved files are hitting some warnings on the win64 compile bot.
It seems strange that compile warnings are different between chrome/ and
components/. Is that intentional?

[commit-bot: I haz the power, 2014-05-31 03:40:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-31 05:07:16]

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

[commit-bot: I haz the power, 2014-05-31 05:07:17]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Alexei Svitkine (slow), 2014-05-31 13:58:45]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-05-31 13:58:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/291153013/380001

[commit-bot: I haz the power, 2014-05-31 15:38:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-31 22:28:40]

Change committed as 274074