Enable chrome://gcm-internals on Android

Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 7.5KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179
Committed: https://crrev.com/1ed82985c317377bff21e3f66a20d4ef87ec69b1
Cr-Commit-Position: refs/heads/master@{#366402}

[Peter Beverloo, 2015-12-11 20:24:57]

peter@chromium.org changed reviewers:
+ jianli@chromium.org

[Peter Beverloo, 2015-12-11 20:24:57]

+jianli, you seem to have done most work on chrome://gcm-internals. Mind taking
a look?

[Peter Beverloo, 2015-12-11 21:08:59]

peter@chromium.org changed reviewers:
+ zea@chromium.org

[Peter Beverloo, 2015-12-11 21:09:00]

Hmm, also +zea

While most of the failures are due to the tree state,
android_chromium_gn_compile_rel correctly points out that Android shouldn't
include //google_apis/gcm/engine/.

In this case it's fine because we only use values of the headers, but it's not a
safe, future-proof construct. I also have little hope of convincing GN of this
fact.

Any ideas on how to address this? I could factor out the enum values it uses to
a common, header-only target in //google_apis/gcm/ if this works for you?

[Nicolas Zea, 2015-12-11 23:41:44]

Which header values need to be shared now? Also I'm kind of surprised this adds
12KB to the apk size. Seems like a lot.

[Peter Beverloo, 2015-12-12 00:06:21]

Basically, Android has an implementation of the GCMDriver, but it interacts with
Java (then Google Play Services) rather than the GCMClient -> GCM Engine.

In order to be able to use the GCMStatsRecorder(Impl), the included files from
//google_apis/gcm/ in gcm_stats_recorder_impl.h need to be in a target available
to Android.

The following types are used:
    ConnectionFactory::ConnectionResetReason
    MCSClient::MessageSendStatus
    RegistrationRequest::Status
    UnregistrationRequest::Status

As well as the GCMStatsRecorder base class (which depends on gcm_export.h, but I
don't think it needs that). All of these are header-only dependencies.

I think we have a few options:

    (1) Make a package out of //google_apis/gcm/monitoring/ and move the enums
there.
    (2) Have a stats recorder specific to Android. It would duplicate some
logic, but nothing major. We'd lose histogram recording.
    (3) Don't have chrome://gcm-internals/ on Android.

My main concern for (2) is that it might cause minor inconsistencies between the
platforms as the implementations evolve. As for (3), I'm in favor, but want to
be clear that I'm happy to discuss alternatives.

Regarding the binary size increase, I just realized that I compared debug
builds. Release will be smaller, let me compare again on Monday.

[Nicolas Zea, 2015-12-12 00:22:41]

Hmm. Yeah, it's been nice to be able to say that basically all of
google_apis/gcm was being excluded from Android. If possible, I'd like to be
able to keep that guarantee.

Regarding #2, why would you lose histogram recording? You could just duplicate
it right? Although I agree that as much as possible it would be good to share
code between them.

Perhaps we should refactor GCMStatsRecorder in such a way that the shared logic
all platforms have is encapsulated within GCMStatsRecorderImpl, and there's a
platform specific GCMStatsRecorder that wraps the impl?

[Peter Beverloo, 2015-12-16 17:34:56]

Nicolas: PTAL, this removes the dependency on the GCM Engine entirely at the
cost of three new files, which turned out to be surprisingly simple.

[Peter Beverloo, 2015-12-16 17:39:22]

Description was changed from

==========
Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 12KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179
==========

to

==========
Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 7.5KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179
==========

[Nicolas Zea, 2015-12-16 22:24:32]

Most LG, mainly nits

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/B...
File components/gcm_driver/BUILD.gn (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/B...
components/gcm_driver/BUILD.gn:175: sources += [
"gcm_stats_recorder_android_unittest.cc" ]
nit: prefer not having a negative condition or adding to sources, and instead
subtracting from sources (similar to the gcm_driver and test_support targets)
and swapping condition.

if (is_android) {
sources -= non-android files
} else {
sources -= android files
}

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:225:
GetGCMStatistics(get_gcm_statistics_callback_, false /* clear_logs */);
style nit: you might want to consider making these boolean arguments into enums.
They're far more readable and less error prone than needing to add a comment to
specify that e.g. false == !clear_logs

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.h (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.h:14: #include
"base/memory/scoped_ptr.h"
is this needed?

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.cc (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.cc:11: const size_t
MAX_LOGGED_ACTIVITY_COUNT = 100;
nit: put this in an anonymous namespace?

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.cc:53: app_id, "Registration
response received", success ? "SUCCESS"
nit: consider defining these as const char[] in an anon namespace at top of file
so you can reuse them.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.h (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:35: void SetRecording(bool
recording) { is_recording_ = recording; }
nit: Generally if you're going to inline this method, it should be named
set_is_recording. Otherwise consider not inlining.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:44: void
RecordRegistrationSent(const std::string& app_id);
nit: would be good to have comments for these methods.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/r...
File components/gcm_driver/resources/gcm_internals.html (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/r...
components/gcm_driver/resources/gcm_internals.html:171: <if expr="not
is_android">
Should we be recording sent messages for Android too?

[Peter Beverloo, 2015-12-17 17:08:23]

peter@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Peter Beverloo, 2015-12-17 17:08:24]

+asvitkine

Mostly done, PTAL. I included the histograms in histograms.xml since we were
counting them, but actually never added the label (so can't see the stats).

There is some duplication here, since the total count of
GCM.RegistrationRequestStatus will be equal to GCM.RegistrationRequest, but
since (a) it already exists, and (b) we can't record the former on Android
because of layering this should be OK.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/B...
File components/gcm_driver/BUILD.gn (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/B...
components/gcm_driver/BUILD.gn:175: sources += [
"gcm_stats_recorder_android_unittest.cc" ]
On 2015/12/16 22:24:31, Nicolas Zea wrote:
> nit: prefer not having a negative condition or adding to sources, and instead
> subtracting from sources (similar to the gcm_driver and test_support targets)
> and swapping condition.
> 
> if (is_android) {
> sources -= non-android files
> } else {
> sources -= android files
> }

This was done as part of https://codereview.chromium.org/1418543006. Either way,
agreed & changed.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:225:
GetGCMStatistics(get_gcm_statistics_callback_, false /* clear_logs */);
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> style nit: you might want to consider making these boolean arguments into
enums.
> They're far more readable and less error prone than needing to add a comment
to
> specify that e.g. false == !clear_logs

Mm, I do agree, but I can't come up with a good name.

|enum class StatisticsLogs { CLEAR, KEEP }| w/ |StatisticsLogs logs_behavior|
could work, but I'm not happy about the difference in type/arg name.

Ideas?

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.h (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.h:14: #include
"base/memory/scoped_ptr.h"
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> is this needed?

No. Removed.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.cc (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.cc:11: const size_t
MAX_LOGGED_ACTIVITY_COUNT = 100;
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> nit: put this in an anonymous namespace?

No need, constants have internal linkage by default.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.cc:53: app_id, "Registration
response received", success ? "SUCCESS"
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> nit: consider defining these as const char[] in an anon namespace at top of
file
> so you can reuse them.

Moved them to constants, but same reply re: the namespace.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.h (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:35: void SetRecording(bool
recording) { is_recording_ = recording; }
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> nit: Generally if you're going to inline this method, it should be named
> set_is_recording. Otherwise consider not inlining.

Done.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:44: void
RecordRegistrationSent(const std::string& app_id);
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> nit: would be good to have comments for these methods.

Done.

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/r...
File components/gcm_driver/resources/gcm_internals.html (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/r...
components/gcm_driver/resources/gcm_internals.html:171: <if expr="not
is_android">
On 2015/12/16 22:24:32, Nicolas Zea wrote:
> Should we be recording sent messages for Android too?

We don't support sending messages on Android yet. This is something we'll look
into early next year to support delivery receipts in the Web Push protocol
(which behave slightly differently than the existing delivery receipts), but for
now there won't be any data.

[Alexei Svitkine (slow), 2015-12-17 18:20:52]

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14374: +<histogram
name="GCM.DataMessageReceived" units="messages">
Looks like this is logged via:
 UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);

That's inefficient, since it allocated 50 buckets to ever use one of them. It
would be better to use UMA_HISTOGRAM_BOOLEAN().

However, given that these histograms were never in the xml suggests that maybe
no one ever used them. In that case, perhaps it's better to simply remove them?

[Nicolas Zea, 2015-12-17 23:52:14]

LGTM

[Nicolas Zea, 2015-12-17 23:55:14]

Forgot to respond to your question. Still LGTM modulo the rename (and Alexei's
comments)

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver_android.cc (right):

https://codereview.chromium.org/1515153003/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_driver_android.cc:225:
GetGCMStatistics(get_gcm_statistics_callback_, false /* clear_logs */);
On 2015/12/17 17:08:24, Peter Beverloo wrote:
> On 2015/12/16 22:24:32, Nicolas Zea wrote:
> > style nit: you might want to consider making these boolean arguments into
> enums.
> > They're far more readable and less error prone than needing to add a comment
> to
> > specify that e.g. false == !clear_logs
> 
> Mm, I do agree, but I can't come up with a good name.
> 
> |enum class StatisticsLogs { CLEAR, KEEP }| w/ |StatisticsLogs logs_behavior|
> could work, but I'm not happy about the difference in type/arg name.
> 
> Ideas?

CLEAR_LOGS vs KEEP_LOGS would be enough in my opinion (basically replace your
comment)

[jianli, 2015-12-18 00:10:00]

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.h (right):

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:33:
~GCMStatsRecorderAndroid();
nit: add virtual

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:35: bool is_recording() const
{ return is_recording_; }
nit: move this getter and the setter below after method declarations.

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:38: void Clear();
nit: comment this

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:40: // Collect all recorded
activities into |*recorder_activities|.
nit: Collect => Collects

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:41: void
CollectActivities(RecordedActivities* recorder_activities) const;
recorder => recorded

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android_unittest.cc (right):

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android_unittest.cc:121: }  //
namespace
Why wrapping all test code inside anonymous namespace?

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14464: +  <summary>Total number of GCM
registration requests.</summary>
Mark this Android only if this is only intended for Android. Otherwise, please
also add it to the desktop version.

[Peter Beverloo, 2015-12-18 17:19:16]

peter@chromium.org changed reviewers:
+ bauerb@chromium.org

[Peter Beverloo, 2015-12-18 17:19:17]

All done. PTAL.

+bauerb for the //chrome/browser/ui/webui/ changes.

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.h (right):

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:33:
~GCMStatsRecorderAndroid();
On 2015/12/18 00:10:00, jianli wrote:
> nit: add virtual

Why? This class neither has a virtual base class, nor has derived classes.

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:35: bool is_recording() const
{ return is_recording_; }
On 2015/12/18 00:09:59, jianli wrote:
> nit: move this getter and the setter below after method declarations.

Done. (Also for GCMStatsRecorderImpl, + Nicolas' SetRecording comment.)

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:38: void Clear();
On 2015/12/18 00:09:59, jianli wrote:
> nit: comment this

Done.

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:40: // Collect all recorded
activities into |*recorder_activities|.
On 2015/12/18 00:10:00, jianli wrote:
> nit: Collect => Collects

Done.

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:41: void
CollectActivities(RecordedActivities* recorder_activities) const;
On 2015/12/18 00:09:59, jianli wrote:
> recorder => recorded

Done. (Also for GCMStatsRecorderImpl.)

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android_unittest.cc (right):

https://codereview.chromium.org/1515153003/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android_unittest.cc:121: }  //
namespace
On 2015/12/18 00:10:00, jianli wrote:
> Why wrapping all test code inside anonymous namespace?

It became a force of habit per the following thread:
   
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/pKY2G4GHcu0

There's plenty of code doing this, and since no friend-declarations are
necessary it doesn't have any downsides.

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14374: +<histogram
name="GCM.DataMessageReceived" units="messages">
On 2015/12/17 18:20:52, Alexei Svitkine (slow) wrote:
> Looks like this is logged via:
>  UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);
> 
> That's inefficient, since it allocated 50 buckets to ever use one of them. It
> would be better to use UMA_HISTOGRAM_BOOLEAN().

Ah, got it! Changed.

> However, given that these histograms were never in the xml suggests that maybe
> no one ever used them. In that case, perhaps it's better to simply remove
them?

At least for Android, there's value in them because I'd like to know how many
messages we receive as Chrome, and how many of those we can deliver using the
Push API. I suspect that we have a bug here (unsubscription behaviour is odd),
but this is hard to validate locally.

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14464: +  <summary>Total number of GCM
registration requests.</summary>
On 2015/12/18 00:10:00, jianli wrote:
> Mark this Android only if this is only intended for Android. Otherwise, please
> also add it to the desktop version.

The desktop version already records these.

[Bernhard Bauer, 2015-12-18 19:07:07]

LGTM w/ a nit, and a potential followup:

https://codereview.chromium.org/1515153003/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.h (right):

https://codereview.chromium.org/1515153003/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:25: class Delegate {
Maybe something for a followup, but consider replacing single-method interfaces
with base::Callback's.

https://codereview.chromium.org/1515153003/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:32: explicit
GCMStatsRecorderAndroid(Delegate* delegate);
Can you specify the ownership of |delegate| in a comment?

[Alexei Svitkine (slow), 2015-12-18 19:18:42]

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1515153003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14374: +<histogram
name="GCM.DataMessageReceived" units="messages">
On 2015/12/18 17:19:17, Peter Beverloo wrote:
> On 2015/12/17 18:20:52, Alexei Svitkine (slow) wrote:
> > Looks like this is logged via:
> >  UMA_HISTOGRAM_COUNTS("GCM.DataMessageReceived", 1);
> > 
> > That's inefficient, since it allocated 50 buckets to ever use one of them.
It
> > would be better to use UMA_HISTOGRAM_BOOLEAN().
> 
> Ah, got it! Changed.
> 
> > However, given that these histograms were never in the xml suggests that
maybe
> > no one ever used them. In that case, perhaps it's better to simply remove
> them?
> 
> At least for Android, there's value in them because I'd like to know how many
> messages we receive as Chrome, and how many of those we can deliver using the
> Push API. I suspect that we have a bug here (unsubscription behaviour is odd),
> but this is hard to validate locally.

Okay. My other issue with these histogram is they only log a value of "1" and
never anything else. This usually makes histograms have limited value, because
without other info it doesn't tell you much. e.g. what does a value like 33443
tell you? It would depend on other factors, like how many people are on that
channel/platform, how many people are using this feature, etc.

Usually, it's better to also log false in addition to true, which tells you at
least what percentage of messages (are received?).

From your description, it also sounds like you want something similar ("messages
we receive as Chrome, and how many of those we can deliver using the Push API")
- i.e. it sounds this should be one enum histogram with two buckets - one for
message receivedand one for message delivered.

[Peter Beverloo, 2015-12-21 13:09:45]

Thank you for the reviews all!

I have removed the UMA changes from this patch as it has become a separate
discussion now. Let's tackle this in its own CL.

https://codereview.chromium.org/1515153003/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_stats_recorder_android.h (right):

https://codereview.chromium.org/1515153003/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:25: class Delegate {
On 2015/12/18 19:07:07, Bernhard -- OOO until Jan 4 wrote:
> Maybe something for a followup, but consider replacing single-method
interfaces
> with base::Callback's.

Ack, I'll follow up for both implementations.

https://codereview.chromium.org/1515153003/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_stats_recorder_android.h:32: explicit
GCMStatsRecorderAndroid(Delegate* delegate);
On 2015/12/18 19:07:07, Bernhard -- OOO until Jan 4 wrote:
> Can you specify the ownership of |delegate| in a comment?

Done.

[Peter Beverloo, 2015-12-21 13:11:11]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-12-21 13:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1515153003/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-12-21 13:11:23]

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

[commit-bot: I haz the power, 2015-12-21 13:14:18]

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

[commit-bot: I haz the power, 2015-12-21 13:14:19]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Peter Beverloo, 2015-12-21 14:12:33]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-12-21 14:12:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1515153003/#ps100001
(title: "gn silliness")

[commit-bot: I haz the power, 2015-12-21 14:12:55]

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

[commit-bot: I haz the power, 2015-12-21 14:25:41]

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

[commit-bot: I haz the power, 2015-12-21 14:25:42]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Peter Beverloo, 2015-12-21 14:33:07]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-12-21 14:33:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1515153003/#ps120001
(title: "iOS changes")

[commit-bot: I haz the power, 2015-12-21 14:33:21]

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

[commit-bot: I haz the power, 2015-12-21 14:49:34]

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

[commit-bot: I haz the power, 2015-12-21 14:49:36]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Peter Beverloo, 2015-12-21 15:05:57]

(All these random failures that don't reproduce in my local builds are annoying,
even if they're completely straightforward.)

[Peter Beverloo, 2015-12-21 15:06:04]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-12-21 15:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1515153003/#ps140001
(title: "..")

[commit-bot: I haz the power, 2015-12-21 15:06:31]

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

[Alexei Svitkine (slow), 2015-12-21 15:38:13]

Are you planning to follow-up with another CL for the histograms.xml
change? I think adding them to histograms.xml is still better than the
status quo and it would be unfortunate if there's no follow up at all on
them after this change.

On Mon, Dec 21, 2015 at 10:06 AM, commit-bot@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1515153003/140001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1515153003/140001
>
>
> https://codereview.chromium.org/1515153003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Beverloo, 2015-12-21 15:40:14]

On 2015/12/21 15:38:13, Alexei Svitkine (slow) wrote:
> Are you planning to follow-up with another CL for the histograms.xml
> change? I think adding them to histograms.xml is still better than the
> status quo and it would be unfortunate if there's no follow up at all on
> them after this change.

Certainly - the current situation is broken, so it has to be fixed regardless,
but you raise good points about what exactly we want to fix.

I plan to have the follow up out today as well.

[Alexei Svitkine (slow), 2015-12-21 15:43:54]

In that case, lgtm % a nit. Thanks!

https://codereview.chromium.org/1515153003/diff/140001/components/gcm_driver/...
File components/gcm_driver/gcm_stats_recorder_android.cc (right):

https://codereview.chromium.org/1515153003/diff/140001/components/gcm_driver/...
components/gcm_driver/gcm_stats_recorder_android.cc:11: const char kSuccess[] =
"SUCCESS";
Nit: Add an anonymous namespace around these.

[commit-bot: I haz the power, 2015-12-21 15:49:38]

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

[commit-bot: I haz the power, 2015-12-21 15:49:39]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Peter Beverloo, 2015-12-21 16:18:58]

Done.

https://codereview.chromium.org/1515153003/diff/140001/components/gcm_driver/...
File components/gcm_driver/gcm_stats_recorder_android.cc (right):

https://codereview.chromium.org/1515153003/diff/140001/components/gcm_driver/...
components/gcm_driver/gcm_stats_recorder_android.cc:11: const char kSuccess[] =
"SUCCESS";
On 2015/12/21 15:43:54, Alexei Svitkine (slow) wrote:
> Nit: Add an anonymous namespace around these.

I have added an anonymous namespace per your request, but there is no need for
one and whether or not it should be there seems to be depending on the reviewer.

I've asked the CXX team for guidance here:
https://groups.google.com/a/chromium.org/forum/#!topic/cxx/VzzBZW4bwh4

[Peter Beverloo, 2015-12-21 16:19:09]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-12-21 16:19:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
zea@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1515153003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-12-21 16:19:18]

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

[commit-bot: I haz the power, 2015-12-21 17:22:56]

Description was changed from

==========
Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 7.5KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179
==========

to

==========
Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 7.5KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179
==========

[commit-bot: I haz the power, 2015-12-21 17:22:57]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-21 17:24:00]

Description was changed from

==========
Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 7.5KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179
==========

to

==========
Enable chrome://gcm-internals on Android

The page provides super valuable information to developers who are
wondering why their messages aren't arriving, which can be due to many
reasons. The page gives insight on whether Chrome received them.

This CL enables the page on Android. It doesn't display nearly as much
information as the desktop page, but will display (un)registrations
and received data messages.

It adds 7.5KB to the ChromePublic.apk size.

I've also added back some padding around the buttons since I found
an open issue about that by accident.

BUG=569127, 499179

Committed: https://crrev.com/1ed82985c317377bff21e3f66a20d4ef87ec69b1
Cr-Commit-Position: refs/heads/master@{#366402}
==========

[commit-bot: I haz the power, 2015-12-21 17:24:02]

Patchset 9 (id:??) landed as
https://crrev.com/1ed82985c317377bff21e3f66a20d4ef87ec69b1
Cr-Commit-Position: refs/heads/master@{#366402}