[GCM] Fix gcm login to properly identify chrome

[GCM] Fix gcm login to properly identify chrome

Use version 41 and put the chrome version in the id field.

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

[Nicolas Zea, 2014-02-18 23:59:28]

PTAL.

[jianli, 2014-02-19 00:08:25]

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/base/mcs...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:72: login_request->set_id(kLoginId +
version_string);
If version_string is empty, we will set id to "chrome-", right? Just want to
confirm it.

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_handler_impl.cc (right):

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl.cc:266: if (version < kMCSVersion
&& version != 38) {
Why do we need to check for this? What's current version returned from the
server? It seems that we will let through version 38. What if version 39?

[Nicolas Zea, 2014-02-19 18:40:28]

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/base/mcs...
File google_apis/gcm/base/mcs_util.cc (right):

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/base/mcs...
google_apis/gcm/base/mcs_util.cc:72: login_request->set_id(kLoginId +
version_string);
On 2014/02/19 00:08:26, jianli wrote:
> If version_string is empty, we will set id to "chrome-", right? Just want to
> confirm it.

Yep

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/engine/c...
File google_apis/gcm/engine/connection_handler_impl.cc (right):

https://codereview.chromium.org/171683002/diff/30001/google_apis/gcm/engine/c...
google_apis/gcm/engine/connection_handler_impl.cc:266: if (version < kMCSVersion
&& version != 38) {
On 2014/02/19 00:08:26, jianli wrote:
> Why do we need to check for this? What's current version returned from the
> server? It seems that we will let through version 38. What if version 39?

A server bug that hasn't been fixed yet results in returning 38. It should in
fact return 41 or newer.

[jianli, 2014-02-19 18:42:12]

lgtm

[Nicolas Zea, 2014-02-19 18:43:18]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-19 18:43:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/30001

[commit-bot: I haz the power, 2014-02-19 18:43:37]

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

[commit-bot: I haz the power, 2014-02-19 18:43:37]

Failed to apply patch for google_apis/gcm/base/mcs_message_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file google_apis/gcm/base/mcs_message_unittest.cc
  Hunk #1 FAILED at 37.
  Hunk #2 FAILED at 56.
  Hunk #3 FAILED at 75.
  3 out of 3 hunks FAILED -- saving rejects to file
google_apis/gcm/base/mcs_message_unittest.cc.rej

Patch:       google_apis/gcm/base/mcs_message_unittest.cc
Index: google_apis/gcm/base/mcs_message_unittest.cc
diff --git a/google_apis/gcm/base/mcs_message_unittest.cc
b/google_apis/gcm/base/mcs_message_unittest.cc
index
07f1d7da24c9a4cb616dbaad919820e68449c6ff..0dbebc7cc45cf55183dce9261cfb6f40d82d99de
100644
--- a/google_apis/gcm/base/mcs_message_unittest.cc
+++ b/google_apis/gcm/base/mcs_message_unittest.cc
@@ -37,7 +37,7 @@ TEST_F(MCSMessageTest, Invalid) {
 TEST_F(MCSMessageTest, InitInferTag) {
   std::vector<int64> user_serial_numbers;
   scoped_ptr<mcs_proto::LoginRequest> login_request(
-      BuildLoginRequest(kAndroidId, kSecret, user_serial_numbers));
+      BuildLoginRequest(kAndroidId, kSecret, "", user_serial_numbers));
   scoped_ptr<google::protobuf::MessageLite> login_copy(
       new mcs_proto::LoginRequest(*login_request));
   MCSMessage message(*login_copy);
@@ -56,7 +56,7 @@ TEST_F(MCSMessageTest, InitInferTag) {
 TEST_F(MCSMessageTest, InitWithTag) {
   std::vector<int64> user_serial_numbers;
   scoped_ptr<mcs_proto::LoginRequest> login_request(
-      BuildLoginRequest(kAndroidId, kSecret, user_serial_numbers));
+      BuildLoginRequest(kAndroidId, kSecret, "", user_serial_numbers));
   scoped_ptr<google::protobuf::MessageLite> login_copy(
       new mcs_proto::LoginRequest(*login_request));
   MCSMessage message(kLoginRequestTag, *login_copy);
@@ -75,7 +75,7 @@ TEST_F(MCSMessageTest, InitWithTag) {
 TEST_F(MCSMessageTest, InitPassOwnership) {
   std::vector<int64> user_serial_numbers;
   scoped_ptr<mcs_proto::LoginRequest> login_request(
-      BuildLoginRequest(kAndroidId, kSecret, user_serial_numbers));
+      BuildLoginRequest(kAndroidId, kSecret, "", user_serial_numbers));
   scoped_ptr<google::protobuf::MessageLite> login_copy(
       new mcs_proto::LoginRequest(*login_request));
   MCSMessage message(kLoginRequestTag,

[Nicolas Zea, 2014-02-19 22:03:48]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-19 22:13:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/180001

[commit-bot: I haz the power, 2014-02-20 00:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/180001

[commit-bot: I haz the power, 2014-02-20 01:31:11]

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

[commit-bot: I haz the power, 2014-02-20 01:31:12]

Try jobs failed on following builders: linux_chromium_rel

[Nicolas Zea, 2014-02-20 01:32:09]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-20 01:42:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/180001

[commit-bot: I haz the power, 2014-02-20 02:05:03]

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

[commit-bot: I haz the power, 2014-02-20 02:05:03]

Try jobs failed on following builders: linux_chromium_rel

[Nicolas Zea, 2014-02-20 03:17:13]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-02-20 05:12:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/180001

[commit-bot: I haz the power, 2014-02-20 09:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/180001

[commit-bot: I haz the power, 2014-02-20 12:29:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/171683002/180001

[commit-bot: I haz the power, 2014-02-20 16:04:50]

Change committed as 252255