Renaming RMQStore to GCMStore and breaking out its interface

Renaming RMQStore to GCMStore and breaking out its interface

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

[fgorski, 2013-12-27 17:31:40]

Please take a look at the change renaming and breaking out the interface of GCM
Store.

[Nicolas Zea, 2013-12-27 20:03:05]

Remind me, were you planning to add a fake implementation as well? Or are we
exposing the gcm store at the chrome/browser level, hence the need for an
interface?

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store.cc (right):

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store.cc:21: void GCMStore::Load(const LoadCallback&
callback) {}
aside from the constructors, I don't think any of these are necessary.

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.h (right):

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.h:20: // A Reliable Message Queue store.
Update comment.

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

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:93: // Sends a message, with or without
reliable message queueing (GCM) support.
nit: leave as RMQ, here and below

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:209: // The reliable message queue
persistent store. Owned by the caller.
nit: update comment.

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:56: 'engine/gcm_store.cc',
nit: technically the cc should be ordered before the h file. I messed that up
when I originally added these, but we should probably go ahead and be consistent
from here forward.

[fgorski, 2013-12-27 22:20:39]

We are going to have a fake implementation so that we don't have to do actual
persistence every time we test something that has the store below. Possibly we
could also rewire some of the MCSClient tests.

As for external consumption, we will be injecting the actual implementation into
the GCMClient. Do you have concerns about any of the exports being unnecessary?

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store.cc (right):

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store.cc:21: void GCMStore::Load(const LoadCallback&
callback) {}
On 2013/12/27 20:03:06, Nicolas Zea wrote:
> aside from the constructors, I don't think any of these are necessary.

Done.

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.h (right):

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.h:20: // A Reliable Message Queue store.
On 2013/12/27 20:03:06, Nicolas Zea wrote:
> Update comment.

Done.

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

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:93: // Sends a message, with or without
reliable message queueing (GCM) support.
On 2013/12/27 20:03:06, Nicolas Zea wrote:
> nit: leave as RMQ, here and below

Done, but I'll leave the one below as GCM, given that the whole store is
destroyed and not just RMQ portion of it.

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client.h:209: // The reliable message queue
persistent store. Owned by the caller.
On 2013/12/27 20:03:06, Nicolas Zea wrote:
> nit: update comment.

Done.

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/gcm.gyp
File google_apis/gcm/gcm.gyp (right):

https://codereview.chromium.org/121743002/diff/30001/google_apis/gcm/gcm.gyp#...
google_apis/gcm/gcm.gyp:56: 'engine/gcm_store.cc',
On 2013/12/27 20:03:06, Nicolas Zea wrote:
> nit: technically the cc should be ordered before the h file. I messed that up
> when I originally added these, but we should probably go ahead and be
consistent
> from here forward.

Done.

[Nicolas Zea, 2013-12-28 00:52:13]

LGTM

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

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:209: // The GCM persistent store. Owned by
the caller.
nit: Owned by caller -> Not owned.

[jianli, 2014-01-02 19:02:53]

For new files, please change to use 2014.

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store.h:51: virtual ~GCMStore() = 0;
Why making it abstract? You implemented it in .cc file.

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:496: : GCMStore(),
nit: Calling default constructor is not needed

[jianli, 2014-01-02 19:03:06]

lgtm

[fgorski, 2014-01-02 19:46:33]

Thanks for the reviews. Applied all comments. Sending to CQ.

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store.h (right):

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store.h:51: virtual ~GCMStore() = 0;
On 2014/01/02 19:02:53, jianli wrote:
> Why making it abstract? You implemented it in .cc file.

Done.

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
google_apis/gcm/engine/gcm_store_impl.cc:496: : GCMStore(),
On 2014/01/02 19:02:53, jianli wrote:
> nit: Calling default constructor is not needed

Done.

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

https://codereview.chromium.org/121743002/diff/140001/google_apis/gcm/engine/...
google_apis/gcm/engine/mcs_client.h:209: // The GCM persistent store. Owned by
the caller.
On 2013/12/28 00:52:13, Nicolas Zea wrote:
> nit: Owned by caller -> Not owned.

Done.

[commit-bot: I haz the power, 2014-01-02 19:47:41]

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

[commit-bot: I haz the power, 2014-01-02 19:59:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/121743002/570001

[commit-bot: I haz the power, 2014-01-03 17:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/121743002/570001

[commit-bot: I haz the power, 2014-01-03 17:47:36]

Failed to apply patch for google_apis/gcm/engine/mcs_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file google_apis/gcm/engine/mcs_client.cc
  Hunk #2 succeeded at 82 (offset -3 lines).
  Hunk #3 FAILED at 95.
  Hunk #4 succeeded at 103 (offset -6 lines).
  Hunk #5 succeeded at 182 (offset -7 lines).
  Hunk #6 succeeded at 217 (offset -7 lines).
  Hunk #7 succeeded at 236 (offset -7 lines).
  Hunk #8 succeeded at 301 (offset -7 lines).
  Hunk #9 succeeded at 418 (offset -9 lines).
  Hunk #10 succeeded at 567 (offset -1 lines).
  Hunk #11 succeeded at 611 (offset -1 lines).
  Hunk #12 succeeded at 645 (offset -1 lines).
  1 out of 12 hunks FAILED -- saving rejects to file
google_apis/gcm/engine/mcs_client.cc.rej

Patch:       google_apis/gcm/engine/mcs_client.cc
Index: google_apis/gcm/engine/mcs_client.cc
diff --git a/google_apis/gcm/engine/mcs_client.cc
b/google_apis/gcm/engine/mcs_client.cc
index
07f1004af2eab3e4365b2019065e6b486d6f3591..8ffa9d1063808797cd27eecc2ac87b3226d10a9c
100644
--- a/google_apis/gcm/engine/mcs_client.cc
+++ b/google_apis/gcm/engine/mcs_client.cc
@@ -10,7 +10,6 @@
 #include "google_apis/gcm/base/mcs_util.h"
 #include "google_apis/gcm/base/socket_stream.h"
 #include "google_apis/gcm/engine/connection_factory.h"
-#include "google_apis/gcm/engine/rmq_store.h"
 
 using namespace google::protobuf::io;
 
@@ -86,7 +85,7 @@ ReliablePacketInfo::ReliablePacketInfo()
 }
 ReliablePacketInfo::~ReliablePacketInfo() {}
 
-MCSClient::MCSClient(ConnectionFactory* connection_factory, RMQStore*
rmq_store)
+MCSClient::MCSClient(ConnectionFactory* connection_factory, GCMStore*
gcm_store)
     : state_(UNINITIALIZED),
       android_id_(0),
       security_token_(0),
@@ -96,12 +95,11 @@ MCSClient::MCSClient(ConnectionFactory* connection_factory,
RMQStore* rmq_store)
       last_server_to_device_stream_id_received_(0),
       stream_id_out_(0),
       stream_id_in_(0),
-      rmq_store_(rmq_store),
+      gcm_store_(gcm_store),
       heartbeat_interval_(
           base::TimeDelta::FromSeconds(kHeartbeatDefaultSeconds)),
       heartbeat_timer_(true, true),
-      weak_ptr_factory_(this) {
-}
+      weak_ptr_factory_(this) {}
 
 MCSClient::~MCSClient() {
 }
@@ -110,7 +108,7 @@ void MCSClient::Initialize(
     const InitializationCompleteCallback& initialization_callback,
     const OnMessageReceivedCallback& message_received_callback,
     const OnMessageSentCallback& message_sent_callback,
-    const RMQStore::LoadResult& load_result) {
+    const GCMStore::LoadResult& load_result) {
   DCHECK_EQ(state_, UNINITIALIZED);
   initialization_callback_ = initialization_callback;
   message_received_callback_ = message_received_callback;
@@ -190,10 +188,10 @@ void MCSClient::Login(uint64 android_id, uint64
security_token) {
     DCHECK(restored_unackeds_server_ids_.empty());
     android_id_ = android_id;
     security_token_ = security_token;
-    rmq_store_->SetDeviceCredentials(
+    gcm_store_->SetDeviceCredentials(
         android_id_,
         security_token_,
-        base::Bind(&MCSClient::OnRMQUpdateFinished,
+        base::Bind(&MCSClient::OnGCMUpdateFinished,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
@@ -225,11 +223,11 @@ void MCSClient::SendMessage(const MCSMessage& message,
bool use_rmq) {
     packet_info->persistent_id = persistent_id;
     SetPersistentId(persistent_id,
                     packet_info->protobuf.get());
-    rmq_store_->AddOutgoingMessage(persistent_id,
-                                   MCSMessage(message.tag(),
-                                              *(packet_info->protobuf)),
-                                   base::Bind(&MCSClient::OnRMQUpdateFinished,
-                                              weak_ptr_factory_.GetWeakPtr()));
+    gcm_store_->AddOutgoingMessage(
+        persistent_id,
+        MCSMessage(message.tag(), *(packet_info->protobuf)),
+        base::Bind(&MCSClient::OnGCMUpdateFinished,
+                   weak_ptr_factory_.GetWeakPtr()));
   } else {
     // Check that there is an active connection to the endpoint.
     if (!connection_handler_->CanSendMessage()) {
@@ -244,7 +242,7 @@ void MCSClient::SendMessage(const MCSMessage& message, bool
use_rmq) {
 }
 
 void MCSClient::Destroy() {
-  rmq_store_->Destroy(base::Bind(&MCSClient::OnRMQUpdateFinished,
+  gcm_store_->Destroy(base::Bind(&MCSClient::OnGCMUpdateFinished,
                                  weak_ptr_factory_.GetWeakPtr()));
 }
 
@@ -309,7 +307,7 @@ void MCSClient::SendHeartbeat() {
               false);
 }
 
-void MCSClient::OnRMQUpdateFinished(bool success) {
+void MCSClient::OnGCMUpdateFinished(bool success) {
   LOG_IF(ERROR, !success) << "RMQ Update failed!";
   // TODO(zea): Rebuild the store from scratch in case of persistence failure?
 }
@@ -428,8 +426,8 @@ void MCSClient::HandlePacketFromWire(
   ++stream_id_in_;
   if (!persistent_id.empty()) {
     unacked_server_ids_[stream_id_in_] = persistent_id;
-    rmq_store_->AddIncomingMessage(persistent_id,
-                                   base::Bind(&MCSClient::OnRMQUpdateFinished,
+    gcm_store_->AddIncomingMessage(persistent_id,
+                                   base::Bind(&MCSClient::OnGCMUpdateFinished,
                                               weak_ptr_factory_.GetWeakPtr()));
   }
 
@@ -569,9 +567,9 @@ void MCSClient::HandleStreamAck(StreamId
last_stream_id_received) {
   DVLOG(1) << "Server acked " << acked_outgoing_persistent_ids.size()
            << " outgoing messages, " << to_resend_.size()
            << " remaining unacked";
-  rmq_store_->RemoveOutgoingMessages(
+  gcm_store_->RemoveOutgoingMessages(
       acked_outgoing_persistent_ids,
-      base::Bind(&MCSClient::OnRMQUpdateFinished,
+      base::Bind(&MCSClient::OnGCMUpdateFinished,
                  weak_ptr_factory_.GetWeakPtr()));
 
   HandleServerConfirmedReceipt(last_stream_id_received);
@@ -613,9 +611,9 @@ void MCSClient::HandleSelectiveAck(const PersistentIdList&
id_list) {
 
   DVLOG(1) << "Server acked " << id_list.size()
            << " messages, " << to_resend_.size() << " remaining unacked.";
-  rmq_store_->RemoveOutgoingMessages(
+  gcm_store_->RemoveOutgoingMessages(
       id_list,
-      base::Bind(&MCSClient::OnRMQUpdateFinished,
+      base::Bind(&MCSClient::OnGCMUpdateFinished,
                  weak_ptr_factory_.GetWeakPtr()));
 
   // Resend any remaining outgoing messages, as they were not received by the
@@ -647,9 +645,9 @@ void MCSClient::HandleServerConfirmedReceipt(StreamId
device_stream_id) {
 
   DVLOG(1) << "Server confirmed receipt of " << acked_incoming_ids.size()
            << " acknowledged server messages.";
-  rmq_store_->RemoveIncomingMessages(
+  gcm_store_->RemoveIncomingMessages(
       acked_incoming_ids,
-      base::Bind(&MCSClient::OnRMQUpdateFinished,
+      base::Bind(&MCSClient::OnGCMUpdateFinished,
                  weak_ptr_factory_.GetWeakPtr()));
 }

[commit-bot: I haz the power, 2014-01-06 15:47:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/121743002/770001

[commit-bot: I haz the power, 2014-01-06 16:33:46]

Retried try job too often on linux_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2014-01-06 19:02:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/121743002/1070001

[commit-bot: I haz the power, 2014-01-06 20:00:18]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-06 20:35:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/121743002/1220001

[commit-bot: I haz the power, 2014-01-06 23:09:59]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-07 00:19:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fgorski@chromium.org/121743002/1220001

[commit-bot: I haz the power, 2014-01-07 10:18:05]

Change committed as 243239