Add activity recording capability to gcm internals page. User can refresh, start/stop recording, an…

Add activity recording capability to gcm internals page. User can refresh, start/stop recording, and clear recording logs.
Also added more information to the device info section and fixed a bug there.

arv: owner review of chrome/browser/resources/*.
jianli & fgorski please review the rest of the code, and zea for owner review of these code. 

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

[fgorski, 2014-03-18 21:28:36]

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.html (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:118: <h2>Checkin Log</h2>
Last checkin information should be a part of device info.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:132: <h2>Registration Log</h2>
Make sure it covers registration and unregistration events. Perhaps
"Registration Events Log is a better heading.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:140: <th
class="info-heading">Details</th>
I suggest the following:
Time, App ID, Sender IDs (this might be a list of senders), Event (in place of
Type and Details)

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:147: <h2>Receive Message Log</h2>
Received Events Log
it should cover OnMessage and OnMessagesDeleted

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:156: <th
class="info-heading">Details</th>
Time, App Id, Sender Id, Msg Type, Event, Details (e.g. collapse_key if present,
size of message)

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:172: <th
class="info-heading">Details</th>
Time, App Id, Receiver Id, Msg Id, Event, Details (I would put the message size
over here, and possibly some other information)

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:44: * @param {HTMLElement} ele A HTML
element.
rename ele to element.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:62: for (var j = 0; j <
list[i].length; ++j) {
Why did we choose to have a list here instead of a JSON object?

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:65: if (j == 0) {
in that case you should probably have a simple loop starting at 1 and a couple
of lines before the loop for element 0

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:213: if (!profile_service) {
this is the place we identified as a problem.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:67: std::string GetMessageSendStatusString(
This should happen closer to UI. MCS client should not care about what gets
printed.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:156: return to_send_.size();
Could we pass these values to the recorder every time we complete processing the
queues? It seems like we are adding to the interface of different classes when
we could centralize around the log collector/recorder.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:57: std::string
GetStatusString(RegistrationRequest::Status status) {
Same here, I think it is better to pass the RegistrationRequest::Status to the
Stats recorder...

Also perhaps UMA loggin could be moved to the GCMStatsRecorder, per our
conversation (probably other patch).

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

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:222: virtual void SetRecording(bool recording) = 0;
Did you consider have SetRecording directly on the recorder?

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.cc:158: uint64 msg_len,
you are not using msg_len

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:27: class GCM_EXPORT GCMStatsRecorder {
Tests for recorder?

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:112: void RecordRegisterWithStatus(const
std::string& app_id,
For this and most of the other methods. Consider passing the original status
enums into Recorder, and even decision about failure/success can be moved to the
recorder. The only think that you may want to pass in is the information whether
a retry was attempted.

[jianli, 2014-03-18 23:53:23]

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:69: } else
nit: add brackets for else to be symmetric

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:33: it < checkins.end();
nit: !=

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:136: int GetSendQueueSize() const ;
nit: space betwee ';'.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:139: int GetUnackedQueueSize() const ;
ditto

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.h:13: #include "base/logging.h"
Why is this needed?

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:470: = recorder_.CheckinActivities().begin();
nit: '=' should be moved to the end of previous line and there should be 4
spaces from '(' char of the previous line.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.h:39: class GCMStatsRecorder;
Not needed.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.cc:108:
InsertCircularBuffer(&checkin_activities_, data);
It might be a bit more efficient by doing something like:
  CheckinActivity* data = InsertCircularBuffer(&checkin_activities);
  data->type = type;
  ...

InsertCircularBuffer will return the pointer to the inserted data.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:9: #include <map>
not used

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:16: #include "base/stl_util.h"
not used?

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:95: bool IsRecording() const {
nit: this can be exposed as getter: bool is_recording() const

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:109: void RecordRegister(const std::string&
app_id,
RecordRegistration?

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:115: std::string status);
It would be better to combine both RecordRegister***. Also use "const
std::string&" for status.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:118: void RecordSend(const std::string&
app_id,
RecordSending?

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:139: const std::deque<CheckinActivity>&
CheckinActivities() const {
ditto for all these 4 methods

[juyik, 2014-03-20 01:09:52]

Per Filip's suggestion, I have split my original big patch. This patch only
records sending activities now. It also fixed the empty page issue when
username_ is empty.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.html (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:118: <h2>Checkin Log</h2>
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Last checkin information should be a part of device info.

Done. Removed it for now. Will do it in my next patch.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:132: <h2>Registration Log</h2>
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Make sure it covers registration and unregistration events. Perhaps
> "Registration Events Log is a better heading.

Remove it for now. Will do it in future patch.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:140: <th
class="info-heading">Details</th>
On 2014/03/18 21:28:37, Filip Gorski wrote:
> I suggest the following:
> Time, App ID, Sender IDs (this might be a list of senders), Event (in place of
> Type and Details)

Remove it for now. Will do it in future patch.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:147: <h2>Receive Message Log</h2>
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Received Events Log
> it should cover OnMessage and OnMessagesDeleted

Remove it for now. Will do it in future patch.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:156: <th
class="info-heading">Details</th>
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Time, App Id, Sender Id, Msg Type, Event, Details (e.g. collapse_key if
present,
> size of message)

Remove it for now. Will do it in future patch.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:172: <th
class="info-heading">Details</th>
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Time, App Id, Receiver Id, Msg Id, Event, Details (I would put the message
size
> over here, and possibly some other information)

Done.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:44: * @param {HTMLElement} ele A HTML
element.
On 2014/03/18 21:28:37, Filip Gorski wrote:
> rename ele to element.

Done.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:62: for (var j = 0; j <
list[i].length; ++j) {
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Why did we choose to have a list here instead of a JSON object?

Do you mean a dictionary kind of object? I thought using list would simplify the
code since it's generally applicable to all activities. If I use dictionary,
there would be lots of code that create cells for every different property names
of different activities.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:65: if (j == 0) {
On 2014/03/18 21:28:37, Filip Gorski wrote:
> in that case you should probably have a simple loop starting at 1 and a couple
> of lines before the loop for element 0

Done.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:69: } else
On 2014/03/18 23:53:23, jianli wrote:
> nit: add brackets for else to be symmetric

N/A anymore after I refactored the code.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:33: it < checkins.end();
On 2014/03/18 23:53:23, jianli wrote:
> nit: !=

Done.

https://codereview.chromium.org/202083005/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:213: if (!profile_service) {
On 2014/03/18 21:28:37, Filip Gorski wrote:
> this is the place we identified as a problem.

I looked into it deeper and now it doesn't seemed to be the cause. For example
when I opened an incognito window the information does show, while the profile
service was null. Then Jian and I found out that  username_ being empty is
causing the problem.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:67: std::string GetMessageSendStatusString(
On 2014/03/18 21:28:37, Filip Gorski wrote:
> This should happen closer to UI. MCS client should not care about what gets
> printed.

Following my comments about SoC principle in gcm_stats_recorder.h, these are
string representations of MCS client states so they should stay with the MCS
logic owner. And if the enum ever changes, it would be easier to change this
function since it stays closer to the enum definition.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.cc:156: return to_send_.size();
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Could we pass these values to the recorder every time we complete processing
the
> queues? It seems like we are adding to the interface of different classes when
> we could centralize around the log collector/recorder.

It seems that having a snapshot of these items (like queue size, username,
android id, etc) is sufficiently useful; it is not necessary to build a change
history/log for them. So if we're not building a history, then notify the
recorder on every change is not very efficient and costs a lot of execution
cycles. Besides, some of the items might get changed from multiple code paths,
so adding recording code to all those paths clutters the code. And if a new code
path is added to change the item, it is easy to miss the recording step.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:136: int GetSendQueueSize() const ;
On 2014/03/18 23:53:23, jianli wrote:
> nit: space betwee ';'.

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:139: int GetUnackedQueueSize() const ;
On 2014/03/18 23:53:23, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.cc:57: std::string
GetStatusString(RegistrationRequest::Status status) {
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Same here, I think it is better to pass the RegistrationRequest::Status to the
> Stats recorder...
> 
> Also perhaps UMA loggin could be moved to the GCMStatsRecorder, per our
> conversation (probably other patch).

Removed changes for now. Will do this in a future patch.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/engine/r...
google_apis/gcm/engine/registration_request.h:13: #include "base/logging.h"
On 2014/03/18 23:53:23, jianli wrote:
> Why is this needed?

Done.

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

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:222: virtual void SetRecording(bool recording) = 0;
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Did you consider have SetRecording directly on the recorder?

It is already on the recorder. I felt that recorder is embedded insode the
client instance and we should not expose recorder instance externally. Therefore
this method will call recorder_.SetRecording() on behalf of the caller..

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:470: = recorder_.CheckinActivities().begin();
On 2014/03/18 23:53:23, jianli wrote:
> nit: '=' should be moved to the end of previous line and there should be 4
> spaces from '(' char of the previous line.

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.h:39: class GCMStatsRecorder;
On 2014/03/18 23:53:23, jianli wrote:
> Not needed.

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.cc:108:
InsertCircularBuffer(&checkin_activities_, data);
On 2014/03/18 23:53:23, jianli wrote:
> It might be a bit more efficient by doing something like:
>   CheckinActivity* data = InsertCircularBuffer(&checkin_activities);
>   data->type = type;
>   ...
> 
> InsertCircularBuffer will return the pointer to the inserted data.

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.cc:158: uint64 msg_len,
On 2014/03/18 21:28:37, Filip Gorski wrote:
> you are not using msg_len

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:9: #include <map>
On 2014/03/18 23:53:23, jianli wrote:
> not used

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:16: #include "base/stl_util.h"
On 2014/03/18 23:53:23, jianli wrote:
> not used?

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:27: class GCM_EXPORT GCMStatsRecorder {
On 2014/03/18 21:28:37, Filip Gorski wrote:
> Tests for recorder?

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:95: bool IsRecording() const {
On 2014/03/18 23:53:23, jianli wrote:
> nit: this can be exposed as getter: bool is_recording() const

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:109: void RecordRegister(const std::string&
app_id,
On 2014/03/18 23:53:23, jianli wrote:
> RecordRegistration?

Will do in my future patch. Splitting this part of the code to later patches.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:112: void RecordRegisterWithStatus(const
std::string& app_id,
On 2014/03/18 21:28:37, Filip Gorski wrote:
> For this and most of the other methods. Consider passing the original status
> enums into Recorder, and even decision about failure/success can be moved to
the
> recorder. The only think that you may want to pass in is the information
whether
> a retry was attempted.

I thought about it a little and I felt that the SoC (Separation of Concerns)
principle is a good design pattern here. Basically a class should be an expert
and owner of its one responsibility; for example the registration class should
own all registration logic, including registration error logic. And the recorder
class should just do the recording and should not know too much about checkin,
registration, send, receive, etc. Hence it should just take a few generic const
string descriptions. This makes it cleaner because domain specific logic will
stay with its owner as close as possible.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:115: std::string status);
On 2014/03/18 23:53:23, jianli wrote:
> It would be better to combine both RecordRegister***. Also use "const
> std::string&" for status.

Will do in my future patch. Splitting this part of the code to later patches.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:118: void RecordSend(const std::string&
app_id,
On 2014/03/18 23:53:23, jianli wrote:
> RecordSending?

Done.

https://codereview.chromium.org/202083005/diff/40001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:139: const std::deque<CheckinActivity>&
CheckinActivities() const {
On 2014/03/18 23:53:23, jianli wrote:
> ditto for all these 4 methods

Done for sending. Will do the rest in my future patch. Splitting this part of
the code to later patches.

[jianli, 2014-03-21 18:25:26]

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:1232: for
(RegistrationInfoMap::const_iterator it = registration_info_map_.begin();
FYI, the registration info map will be moved to GCMClientImpl in my other patch
https://codereview.chromium.org/207443002/.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:30: base::ListValue* sendInfo) {
nit: send_info

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:120: base::ListValue* sendInfo = new
base::ListValue();
nit: send_info

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:170:
gcm::GCMProfileServiceFactory::GetForProfile(profile);
nit: indentation

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:204: base::Unretained(this)));
Please use weak_ptr_factory_.

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

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:104: std::string app_ids_cached;
These are indeed ids of apps that call register to receive registration ID. How
about registered_app_ids_?

Can we use std::vector? If not convenient, please comment that it is comma
separated list.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:7: #include <deque>
nit: not needed since it is included in gcm_stats_recorder.h.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.cc:86: UMA_HISTOGRAM_ENUMERATION(uma_name,
uma_value, uma_value_bound);
It seems to me the only difference among these 3 RecordSending methods is the
addition of UMA_HISTOGRAM_* call. Why not calling UMA_HISTOGRAM_* directly in
the caller? I do not think adding 2 more functions will bring any benefit but
making RecordSending cumbersome.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:13: #include "base/memory/ref_counted.h"
nit: not needed

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:14: #include "base/memory/weak_ptr.h"
ditto

[arv (Not doing code reviews), 2014-03-24 18:47:05]

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.html (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:16: <table cellpadding="5"
cellspacing="0">
Move to css

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:16: <table cellpadding="5"
cellspacing="0">
Why the table? Maybe display: flex would allow you to skip the table for layout?

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:17: <tr valign="center">
move to css

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:46: <tr class="odd-number-row">
Can you use CSS instead? 

:nth-child(odd)

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:122: <th
class="info-heading">Time</th>
Why do all of these need to repeat the class? Can you use descendant selectors
instead?

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:49: while (element.firstChild) {
element.textContent = '';

[juyik, 2014-03-26 04:06:03]

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.html (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:16: <table cellpadding="5"
cellspacing="0">
On 2014/03/24 18:47:05, arv wrote:
> Why the table? Maybe display: flex would allow you to skip the table for
layout?

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:16: <table cellpadding="5"
cellspacing="0">
On 2014/03/24 18:47:05, arv wrote:
> Move to css

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:17: <tr valign="center">
On 2014/03/24 18:47:05, arv wrote:
> move to css

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:46: <tr class="odd-number-row">
On 2014/03/24 18:47:05, arv wrote:
> Can you use CSS instead? 
> 
> :nth-child(odd)

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.html:122: <th
class="info-heading">Time</th>
On 2014/03/24 18:47:05, arv wrote:
> Why do all of these need to repeat the class? Can you use descendant selectors
> instead?

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/resources...
chrome/browser/resources/gcm_internals.js:49: while (element.firstChild) {
On 2014/03/24 18:47:05, arv wrote:
> element.textContent = '';

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:1232: for
(RegistrationInfoMap::const_iterator it = registration_info_map_.begin();
On 2014/03/21 18:25:27, jianli wrote:
> FYI, the registration info map will be moved to GCMClientImpl in my other
patch
> https://codereview.chromium.org/207443002/.

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:30: base::ListValue* sendInfo) {
On 2014/03/21 18:25:27, jianli wrote:
> nit: send_info

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:120: base::ListValue* sendInfo = new
base::ListValue();
On 2014/03/21 18:25:27, jianli wrote:
> nit: send_info

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:170:
gcm::GCMProfileServiceFactory::GetForProfile(profile);
On 2014/03/21 18:25:27, jianli wrote:
> nit: indentation

Done.

https://codereview.chromium.org/202083005/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/gcm_internals_ui.cc:204: base::Unretained(this)));
On 2014/03/21 18:25:27, jianli wrote:
> Please use weak_ptr_factory_.

Done.

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

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:104: std::string app_ids_cached;
On 2014/03/21 18:25:27, jianli wrote:
> These are indeed ids of apps that call register to receive registration ID.
How
> about registered_app_ids_?
> 
> Can we use std::vector? If not convenient, please comment that it is comma
> separated list.

Done.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.cc:7: #include <deque>
On 2014/03/21 18:25:27, jianli wrote:
> nit: not needed since it is included in gcm_stats_recorder.h.

Done.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.cc:86: UMA_HISTOGRAM_ENUMERATION(uma_name,
uma_value, uma_value_bound);
On 2014/03/21 18:25:27, jianli wrote:
> It seems to me the only difference among these 3 RecordSending methods is the
> addition of UMA_HISTOGRAM_* call. Why not calling UMA_HISTOGRAM_* directly in
> the caller? I do not think adding 2 more functions will bring any benefit but
> making RecordSending cumbersome.

According to team consensus, I have changed the class to use more verbose
function interfaces. The UMA decision will be made inside these functions.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:13: #include "base/memory/ref_counted.h"
On 2014/03/21 18:25:27, jianli wrote:
> nit: not needed

Done.

https://codereview.chromium.org/202083005/diff/60001/google_apis/gcm/gcm_stat...
google_apis/gcm/gcm_stats_recorder.h:14: #include "base/memory/weak_ptr.h"
On 2014/03/21 18:25:27, jianli wrote:
> ditto

Done.

[jianli, 2014-03-26 20:17:17]

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:134: // Enables/disables GCM
activity recording, and then returns the stats.
When recording is stopped, do we need to return the stats?

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:117:
device_info->SetInteger("sendQueueSize", stats->send_queue_size);
Do we need to show 0 when size is 0?

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:174: if (!profile_service) {
nit: it would be better for readability if we do early return, like:
  if (!profile_service) {
    ReturnResults(profile, NULL, NULL);
    return;
  }

  if (profile_service->SignedInUserName().empty()) {
    ReturnResults(profile, profile_service, NULL);
    return;
  }

  ...

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:13: #include "base/strings/stringprintf.h"
nit: is this needed?

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:509: recorder_.RecordSendCalled(
"SendCalled" in the method name sounds a bit strange. Since we're going to call
MCSClient::SendMessage, can we move the recording logic to this method? Also,
probably better name like RecordMessageQueued?

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.h (right):

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.h:42: class GCMStatsRecorder;
nit: not needed since header file is already included

[juyik, 2014-03-26 20:57:31]

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:134: // Enables/disables GCM
activity recording, and then returns the stats.
On 2014/03/26 20:17:18, jianli wrote:
> When recording is stopped, do we need to return the stats?

The stats also contains snapshots (registered app ids, etc), so even if the
recording stops, these snapshots may still have changed. Another thing I'm
trying to accomplish is: whenever the user took an action (start/stop recording,
clearing the log, etc) the screen will always be refreshed with new information
(snapshot and logs). I think that would be a good user experience.

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/ui/webui...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:117:
device_info->SetInteger("sendQueueSize", stats->send_queue_size);
On 2014/03/26 20:17:18, jianli wrote:
> Do we need to show 0 when size is 0?

Done.

https://codereview.chromium.org/202083005/diff/140002/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:174: if (!profile_service) {
On 2014/03/26 20:17:18, jianli wrote:
> nit: it would be better for readability if we do early return, like:
>   if (!profile_service) {
>     ReturnResults(profile, NULL, NULL);
>     return;
>   }
> 
>   if (profile_service->SignedInUserName().empty()) {
>     ReturnResults(profile, profile_service, NULL);
>     return;
>   }
> 
>   ...

Done.

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:13: #include "base/strings/stringprintf.h"
On 2014/03/26 20:17:18, jianli wrote:
> nit: is this needed?

Done. Not anymore.

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:509: recorder_.RecordSendCalled(
On 2014/03/26 20:17:18, jianli wrote:
> "SendCalled" in the method name sounds a bit strange. Since we're going to
call
> MCSClient::SendMessage, can we move the recording logic to this method? Also,
> probably better name like RecordMessageQueued?

OK, in that case this recording seems pretty redundant so I'll just remove it,
because in MCSClient::SendMessage I have already recorded a whole bunch of
status including queued.

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.h (right):

https://codereview.chromium.org/202083005/diff/140002/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.h:42: class GCMStatsRecorder;
On 2014/03/26 20:17:18, jianli wrote:
> nit: not needed since header file is already included

Done.

[juyik, 2014-03-26 22:46:25]

ping.

[jianli, 2014-03-26 23:25:18]

lgtm

https://codereview.chromium.org/202083005/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:117: if (stats->unacked_queue_size >
0)
You need to do the same thing here.

https://codereview.chromium.org/202083005/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:120: if (stats->sending.size() > 0)
{
ditto.

[juyik, 2014-03-26 23:49:58]

https://codereview.chromium.org/202083005/diff/190001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/gcm_internals_ui.cc (right):

https://codereview.chromium.org/202083005/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:117: if (stats->unacked_queue_size >
0)
On 2014/03/26 23:25:18, jianli wrote:
> You need to do the same thing here.

Done.

https://codereview.chromium.org/202083005/diff/190001/chrome/browser/ui/webui...
chrome/browser/ui/webui/gcm_internals_ui.cc:120: if (stats->sending.size() > 0)
{
On 2014/03/26 23:25:18, jianli wrote:
> ditto.

This is actually different. If there is no recorded SEND events, then we don't
need to create a send info list, and the javascript won't need to create rows in
the table.

[arv (Not doing code reviews), 2014-03-27 18:17:03]

js/html/css LGTM

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gcm_internals.html (right):

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
chrome/browser/resources/gcm_internals.html:26: <td class="row-caption">
Instead of putting row-caption on all of these you can use :first-child

table tr :first-child {
  font-weight: bold;
  ...
}

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
chrome/browser/resources/gcm_internals.js:66: cell.textContent = d.toString();
no need for toString here

[juyik, 2014-03-27 18:40:11]

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gcm_internals.html (right):

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
chrome/browser/resources/gcm_internals.html:26: <td class="row-caption">
On 2014/03/27 18:17:03, arv wrote:
> Instead of putting row-caption on all of these you can use :first-child
> 
> table tr :first-child {
>   font-weight: bold;
>   ...
> }

Done.

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
File chrome/browser/resources/gcm_internals.js (right):

https://codereview.chromium.org/202083005/diff/210001/chrome/browser/resource...
chrome/browser/resources/gcm_internals.js:66: cell.textContent = d.toString();
On 2014/03/27 18:17:03, arv wrote:
> no need for toString here

Done.

[Nicolas Zea, 2014-03-27 18:55:41]

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:509: recorder_->RecordSentToWire(
This won't record login requests or other non-data messages. That might be okay
from a debug perspective, just making sure that's the intent.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:143: int GetUnackedQueueSize() const;
Unacked is ambiguous, as incoming messages can be unacked too. I think calling
this ResendQueueSize is clearer

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:106: int unacked_queue_size;
resend_queue here_size too

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.cc:21: T* InsertCircularBuffer(std::deque<T>*
q, const T& item) {
Comment for this method

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:92: std::deque<SendingActivity>
sending_activities_;
If this is a circular buffer, why use a dequeue rather than a vector of fixed
size?

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder_unittest.cc (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:34: 
remove extra newline

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:100: 
remove extra newlines

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:102: TEST_F(GCMStatsRecorderTest,
RecordSendingTest) {
Could you split this test up into three different tests that check the different
behavior?

[juyik, 2014-03-27 20:05:10]

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:509: recorder_->RecordSentToWire(
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> This won't record login requests or other non-data messages. That might be
okay
> from a debug perspective, just making sure that's the intent.

Yes, that's the intent. I have changed the method name to reflect that.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.h (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:143: int GetUnackedQueueSize() const;
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> Unacked is ambiguous, as incoming messages can be unacked too. I think calling
> this ResendQueueSize is clearer

Done.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:106: int unacked_queue_size;
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> resend_queue here_size too

Done.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.cc:21: T* InsertCircularBuffer(std::deque<T>*
q, const T& item) {
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> Comment for this method

Done.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:92: std::deque<SendingActivity>
sending_activities_;
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> If this is a circular buffer, why use a dequeue rather than a vector of fixed
> size?

My intention is to display events in reverse chronological order. So I thought
using a deque and push_front() for every new event would make it cleaner and
explicit.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder_unittest.cc (right):

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:34: 
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> remove extra newline

Done.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:100: 
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> remove extra newlines

Done.

https://codereview.chromium.org/202083005/diff/230001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:102: TEST_F(GCMStatsRecorderTest,
RecordSendingTest) {
On 2014/03/27 18:55:42, Nicolas Zea wrote:
> Could you split this test up into three different tests that check the
different
> behavior?

Done.

[fgorski, 2014-03-29 05:09:28]

Sorry for a last review, this was a really busy week.

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:382: stats.gcm_client_created
= true;
with all of the stuff that is happening inside of GetStatistics, you should
probably move this line there as well. Who better to tell you that GCM_client_
is create then itself?

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:131: void
RequestGCMStatistics(RequestGCMStatisticsCallback callback,
nit: GetGCMStatistics

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:509: recorder_->RecordDataSentToWire(
This is queueing I don't think this is equivalent to sending to wire.
We should work on the naming of methods to capture events correctly in the code.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm.gyp...
google_apis/gcm/gcm.gyp:75: 'gcm_stats_recorder.cc',
zea@ does it make sense to have gcm_stats_recorder and related stuff in a
subfolder?
e.g. tools/ or monitoring/ ?

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:552: recorder_.sending_activities().begin();
stats.sending.insert(
    stats.sending.being(),
    recorder_.sending_activities().being(),
    recorder_.sending_activities().end())
and that applies to the registrations below as well.

2. please document that you are doing newest first.

3. Would it make sense for the recorder/stats to do that work? GCMClientImpl
should probably not be responsible for copying monitoring data from one
structure to another. (Unless it has people skills. :) )

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:693: data_message_stanza.ByteSize(),
I am not getting it. How is byte size of an error message relevant here?
Same goes for TTL.

I think the important part here is the origin of SendError, e.g. server vs. gcm
Client (doing a TTL expiration of the message).

Here is my suggestion:
Extract calls to delegate_->OnMessageSendError to a separate method with
parameters being (app_id, send_error_details)  ==> there is another call you are
missing
Do your best by logging only information from the parameters above.
Make sure to capture the result (it will be either SERVER_ERROR or TTL_EXCEEDED,
which will be a good indication of where the error originated)

Update:
I see that we are getting the local TTL EXCEEDED  as part of send message
status, so maybe there is a way to make the method more generic and add send
error as additional case.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:10: #include <vector>
nit: not needed

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:12: #include "base/compiler_specific.h"
what is that for?

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder_unittest.cc (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:40:
std::deque<gcm::GCMStatsRecorder::SendingActivity>* sending_activities() {
given that you are not changing the sending_activities in tests, the signature
should be:

const std::deque<gcm::GCMStatsRecorder::SendingActivity>& sending_activties()
const {


+ it seems that GCMStatsRecorder already has that, so you probably don't even
need that class.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:58: void VerifyDataSentToWire(){
This is a great way of reducing the amount of code you write, but it will
backfire when the unit tests fail, as there will be no stack trace. That means
that if you make that call multiple times during the test you have to provide a
way to distinguish that, otherwise you will have hard time chasing it.
Oh, and these verify methods add another layer of indirection by calling
VerifyData.

A way to solve he problem is to pass a string to every method, and then follow
every with that string, like:
EXPEC_XX() << that_string;

This way you can trace it back to the line in the test that failed.

[juyik, 2014-03-31 22:19:23]

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:382: stats.gcm_client_created
= true;
On 2014/03/29 05:09:29, Filip Gorski wrote:
> with all of the stuff that is happening inside of GetStatistics, you should
> probably move this line there as well. Who better to tell you that GCM_client_
> is create then itself?

Done.

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/202083005/diff/250001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:131: void
RequestGCMStatistics(RequestGCMStatisticsCallback callback,
On 2014/03/29 05:09:29, Filip Gorski wrote:
> nit: GetGCMStatistics

Done.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/engine/...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.cc:509: recorder_->RecordDataSentToWire(
On 2014/03/29 05:09:29, Filip Gorski wrote:
> This is queueing I don't think this is equivalent to sending to wire.
> We should work on the naming of methods to capture events correctly in the
code.

I named it this way because the method itself is called SendPacketToWire(). It
sends the packet to connection manager which just flush it down the socket. The
QUEUED state has already happened and recorded inside the SendMessage()
function. So from MCS point of view, at this point it has detected that the end
point is reachable, it has  popped the message from the to_send_ queue, and it
is ready to send the packet to wire.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm.gyp...
google_apis/gcm/gcm.gyp:75: 'gcm_stats_recorder.cc',
On 2014/03/29 05:09:29, Filip Gorski wrote:
> zea@ does it make sense to have gcm_stats_recorder and related stuff in a
> subfolder?
> e.g. tools/ or monitoring/ ?

I'll let zea@ comment on this.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client_impl.cc (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:552: recorder_.sending_activities().begin();
On 2014/03/29 05:09:29, Filip Gorski wrote:
> stats.sending.insert(
>     stats.sending.being(),
>     recorder_.sending_activities().being(),
>     recorder_.sending_activities().end())
> and that applies to the registrations below as well.
> 
> 2. please document that you are doing newest first.
> 
> 3. Would it make sense for the recorder/stats to do that work? GCMClientImpl
> should probably not be responsible for copying monitoring data from one
> structure to another. (Unless it has people skills. :) )

Done.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client_impl.cc:693: data_message_stanza.ByteSize(),
On 2014/03/29 05:09:29, Filip Gorski wrote:
> I am not getting it. How is byte size of an error message relevant here?
> Same goes for TTL.
> 
> I think the important part here is the origin of SendError, e.g. server vs.
gcm
> Client (doing a TTL expiration of the message).
> 
> Here is my suggestion:
> Extract calls to delegate_->OnMessageSendError to a separate method with
> parameters being (app_id, send_error_details)  ==> there is another call you
are
> missing
> Do your best by logging only information from the parameters above.
> Make sure to capture the result (it will be either SERVER_ERROR or
TTL_EXCEEDED,
> which will be a good indication of where the error originated)
> 
> Update:
> I see that we are getting the local TTL EXCEEDED  as part of send message
> status, so maybe there is a way to make the method more generic and add send
> error as additional case. 

I have removed byte size and ttl. This call site and this recorder method is
exclusively used to record server errors. GCM client side send error is recorded
elsewhere.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:10: #include <vector>
On 2014/03/29 05:09:29, Filip Gorski wrote:
> nit: not needed

Done.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:12: #include "base/compiler_specific.h"
On 2014/03/29 05:09:29, Filip Gorski wrote:
> what is that for?

Done.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder_unittest.cc (right):

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:40:
std::deque<gcm::GCMStatsRecorder::SendingActivity>* sending_activities() {
On 2014/03/29 05:09:29, Filip Gorski wrote:
> given that you are not changing the sending_activities in tests, the signature
> should be:
> 
> const std::deque<gcm::GCMStatsRecorder::SendingActivity>& sending_activties()
> const {
> 
> 
> + it seems that GCMStatsRecorder already has that, so you probably don't even
> need that class.

Done.

https://codereview.chromium.org/202083005/diff/250001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder_unittest.cc:58: void VerifyDataSentToWire(){
On 2014/03/29 05:09:29, Filip Gorski wrote:
> This is a great way of reducing the amount of code you write, but it will
> backfire when the unit tests fail, as there will be no stack trace. That means
> that if you make that call multiple times during the test you have to provide
a
> way to distinguish that, otherwise you will have hard time chasing it.
> Oh, and these verify methods add another layer of indirection by calling
> VerifyData.
> 
> A way to solve he problem is to pass a string to every method, and then follow
> every with that string, like:
> EXPEC_XX() << that_string;
> 
> This way you can trace it back to the line in the test that failed.

Done.

[fgorski, 2014-04-01 03:36:28]

lgtm.

Please make sure Nicolas comments on the directory where the recorder should be
placed.

Address the comments below.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.cc:88: for
(std::deque<GCMStatsRecorder::SendingActivity>::const_iterator it =
nit: you could still use:
activities->insert(
    activities->being(),
    sending_activities_.being(),
    sending_activities_.end())

Your call though.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.cc:89: sending_activities_.begin();
nit: indentation

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:76: void
CollectSendingActivities(std::vector<SendingActivity>* activities) const;
Add documentation, including information about the order of entries.

[Nicolas Zea, 2014-04-01 18:53:21]

LGTM with some nits. Also, agree with Filip that it makes sense to move the
recorder logic/tests to a monitoring folder.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:108: std::vector<GCMStatsRecorder::SendingActivity>
sending;
nit: sending_activity seems like a better name

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:31: std::string event;  // A short
description of the event.
nit: align comments

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:38: virtual ~SendingActivity() OVERRIDE;
nit: override not needed

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:59: // Records when the outgoing data
message is sent to wire.
nit: rather than say "Records when.." in these comments, "Records that" is a bit
clearer (and use past tense). So, in this case, it would be "Record that an
outgoing message was sent over the wire".

[juyik, 2014-04-01 22:40:35]

Moved the recorder files to a monitoring sub folder.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_cli...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_cli...
google_apis/gcm/gcm_client.h:108: std::vector<GCMStatsRecorder::SendingActivity>
sending;
On 2014/04/01 18:53:21, Nicolas Zea wrote:
> nit: sending_activity seems like a better name

Done.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.cc (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.cc:88: for
(std::deque<GCMStatsRecorder::SendingActivity>::const_iterator it =
On 2014/04/01 03:36:29, Filip Gorski wrote:
> nit: you could still use:
> activities->insert(
>     activities->being(),
>     sending_activities_.being(),
>     sending_activities_.end())
> 
> Your call though.

Done.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.cc:89: sending_activities_.begin();
On 2014/04/01 03:36:29, Filip Gorski wrote:
> nit: indentation

Done.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
File google_apis/gcm/gcm_stats_recorder.h (right):

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:31: std::string event;  // A short
description of the event.
On 2014/04/01 18:53:21, Nicolas Zea wrote:
> nit: align comments

Done.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:38: virtual ~SendingActivity() OVERRIDE;
On 2014/04/01 18:53:21, Nicolas Zea wrote:
> nit: override not needed

Done.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:59: // Records when the outgoing data
message is sent to wire.
On 2014/04/01 18:53:21, Nicolas Zea wrote:
> nit: rather than say "Records when.." in these comments, "Records that" is a
bit
> clearer (and use past tense). So, in this case, it would be "Record that an
> outgoing message was sent over the wire".

Done.

https://codereview.chromium.org/202083005/diff/270001/google_apis/gcm/gcm_sta...
google_apis/gcm/gcm_stats_recorder.h:76: void
CollectSendingActivities(std::vector<SendingActivity>* activities) const;
On 2014/04/01 03:36:29, Filip Gorski wrote:
> Add documentation, including information about the order of entries.

Done.

[juyik, 2014-04-02 06:53:39]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-04-02 06:54:21]

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

[commit-bot: I haz the power, 2014-04-02 07:26:35]

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

[commit-bot: I haz the power, 2014-04-02 07:26:36]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, components_unittests, content_unittests, crypto_unittests,
net_unittests, sql_unittests, ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[juyik, 2014-04-15 23:21:06]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-04-15 23:21:51]

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

[commit-bot: I haz the power, 2014-04-16 01:32:46]

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

[commit-bot: I haz the power, 2014-04-16 06:11:28]

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

[commit-bot: I haz the power, 2014-04-16 06:11:29]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[juyik, 2014-04-16 18:06:29]

The CQ bit was checked by juyik@chromium.org

[commit-bot: I haz the power, 2014-04-16 18:07:20]

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

[commit-bot: I haz the power, 2014-04-16 21:41:47]

Change committed as 264313