Enable auto-refreshing of the gcm-internals page whenever a GCM activity is recorded.

Enable auto-refreshing of the gcm-internals page whenever a GCM activity is recorded.

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

[jianli, 2014-05-06 22:54:52]

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_service.cc:307: DoGetGCMStatistics();
Why not just calleding GetGCMStatistics with false value?

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_service.cc:357: if (gcm_client_.get()) {
It seems that there is behavior change when gcm_client_ is empty:
GCMService::GetGCMStatisticsFinished is not called.

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_service.cc:367: if (gcm_client_.get()) {
nit: brackets not needed

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client.h
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client.h...
google_apis/gcm/gcm_client.h:160: // Called when the GCMClient is recording
activities and a new activity has
Should we mention "the GCMClient is recording activities"?

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
File google_apis/gcm/gcm_client_impl.h (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
google_apis/gcm/gcm_client_impl.h:77: : public GCMClient,
GCMStatsRecorder::RecorderDelegate {
nit: add public

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
google_apis/gcm/gcm_client_impl_unittest.cc:254: }
nit: '}' should be appended to previous line.

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/monitoring/g...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/monitoring/g...
google_apis/gcm/monitoring/gcm_stats_recorder.h:100: class RecorderDelegate {
Simpler to name it as Delegate since it is already defined within the recorder
class.

[juyik, 2014-05-07 00:07:48]

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_service.cc:307: DoGetGCMStatistics();
On 2014/05/06 22:54:53, jianli wrote:
> Why not just calleding GetGCMStatistics with false value?

Done.

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_service.cc:357: if (gcm_client_.get()) {
On 2014/05/06 22:54:53, jianli wrote:
> It seems that there is behavior change when gcm_client_ is empty:
> GCMService::GetGCMStatisticsFinished is not called.

Done.

https://codereview.chromium.org/266913015/diff/1/chrome/browser/services/gcm/...
chrome/browser/services/gcm/gcm_service.cc:367: if (gcm_client_.get()) {
On 2014/05/06 22:54:53, jianli wrote:
> nit: brackets not needed

Done.

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client.h
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client.h...
google_apis/gcm/gcm_client.h:160: // Called when the GCMClient is recording
activities and a new activity has
On 2014/05/06 22:54:53, jianli wrote:
> Should we mention "the GCMClient is recording activities"?

Done.

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
File google_apis/gcm/gcm_client_impl.h (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
google_apis/gcm/gcm_client_impl.h:77: : public GCMClient,
GCMStatsRecorder::RecorderDelegate {
On 2014/05/06 22:54:53, jianli wrote:
> nit: add public

Done.

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
File google_apis/gcm/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/gcm_client_i...
google_apis/gcm/gcm_client_impl_unittest.cc:254: }
On 2014/05/06 22:54:53, jianli wrote:
> nit: '}' should be appended to previous line.

Done.

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/monitoring/g...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/266913015/diff/1/google_apis/gcm/monitoring/g...
google_apis/gcm/monitoring/gcm_stats_recorder.h:100: class RecorderDelegate {
On 2014/05/06 22:54:53, jianli wrote:
> Simpler to name it as Delegate since it is already defined within the recorder
> class.

Done.

[jianli, 2014-05-07 00:22:20]

lgtm

[Nicolas Zea, 2014-05-07 17:15:07]

lgtm

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:160: // Called when activities are being recording
and a new activity has just
being recording -> being recorded

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:118: // Set a delegate to
receive async callback from the recorder.
nit: Is async accurate? It seems you're invoking the callback synchronously

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:236: void
NotifyActivityRecorded();
nit: newline after

[juyik, 2014-05-07 18:45:51]

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:160: // Called when activities are being recording
and a new activity has just
On 2014/05/07 17:15:07, Nicolas Zea wrote:
> being recording -> being recorded

Done.

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/monitori...
File google_apis/gcm/monitoring/gcm_stats_recorder.h (right):

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:118: // Set a delegate to
receive async callback from the recorder.
On 2014/05/07 17:15:07, Nicolas Zea wrote:
> nit: Is async accurate? It seems you're invoking the callback synchronously

Done.

https://codereview.chromium.org/266913015/diff/20001/google_apis/gcm/monitori...
google_apis/gcm/monitoring/gcm_stats_recorder.h:236: void
NotifyActivityRecorded();
On 2014/05/07 17:15:07, Nicolas Zea wrote:
> nit: newline after

Done.

[juyik, 2014-05-07 18:52:27]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-05-07 18:55:46]

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

[commit-bot: I haz the power, 2014-05-07 22:19:15]

Change committed as 268976

[Nikita (slow), 2014-05-08 12:17:43]

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

The reason for reverting is: Introduces crash on sign in on Chrome OS.

https://code.google.com/p/chromium/issues/detail?id=371297.

[jianli, 2014-05-08 19:00:46]

We should consider adding a test to cover this.


On Thu, May 8, 2014 at 5:17 AM, <nkostylev@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/273753005/ by nkostylev@chromium.org.
>
> The reason for reverting is: Introduces crash on sign in on Chrome OS.
>
> https://code.google.com/p/chromium/issues/detail?id=371297.
>
> https://codereview.chromium.org/266913015/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[locote, 2014-05-15 07:45:48]

lgtm