[GCM] Adding GCMAccountMapper to link signed in profile to accounts.

components/gcm_driver/gcm_account_mapper.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/gcm_driver/gcm_account_mapper.h"
+
+#include "base/bind.h"
+#include "base/guid.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
+#include "components/gcm_driver/gcm_driver_desktop.h"
+#include "google_apis/gcm/engine/gcm_store.h"
+
+namespace gcm {
+
+namespace {
+
+const char kGCMAccountMapperSenderId[] = "745476177629";
+const char kGCMAccountMapperAppId[] = "com.google.android.gms";
+const int kGCMAccountMapperMessageTTL = 24 * 60 * 60;  // 1 day in seconds.
+const int kGCMUpdateInterval = 24;                     // hours.

[jianli, 2014/08/27 18:12:00]

Add the Hour suffix to the constant name.

[fgorski, 2014/08/27 21:47:17]

Done.

+const char kRegistrationIdMessgaeKey[] = "id";
+const char kTokenMessageKey[] = "t";
+const char kAccountMessageKey[] = "a";
+const char kRemoveAccountMessageKey[] = "r";
+const char kRemoveAccountMessageValue[] = "1";
+
+std::string GenerateMessageID() {
+  return base::GenerateGUID();
+}
+
+}  // namespace
+
+GCMAccountMapper::GCMAccountMapper(GCMDriver* gcm_driver)
+    : gcm_driver_(gcm_driver),
+      clock_(new base::DefaultClock),
+      initialized_(false),
+      weak_ptr_factory_(this) {
+}
+
+GCMAccountMapper::~GCMAccountMapper() {
+}
+
+void GCMAccountMapper::Initialize(
+    const std::vector<AccountMapping>& account_mappings,
+    const std::string& registration_id) {
+  initialized_ = true;

[jianli, 2014/08/27 18:12:00]

nit: add DCHECK for !initialized_.

[fgorski, 2014/08/27 21:47:17]

Done.

+  registration_id_ = registration_id;
+
+  accounts_.insert(
+      accounts_.end(), account_mappings.begin(), account_mappings.end());

[jianli, 2014/08/27 18:12:01]

nit: simpler to say accounts_ = account_mappings

[fgorski, 2014/08/27 21:47:16]

Done.

+
+  gcm_driver_->AddAppHandler(kGCMAccountMapperAppId, this);
+
+  // TODO(fgorski): if no registration ID, get registration ID.
+}
+
+void GCMAccountMapper::SetAccountTokens(
+    const std::vector<GCMClient::AccountTokenInfo> account_tokens) {
+  DCHECK(initialized_);
+
+  // Start from removing the old tokens, from all of the known accounts.
+  for (AccountMappings::iterator iter = accounts_.begin();
+       iter != accounts_.end();
+       ++iter) {
+    iter->access_token.clear();
+    // Sending a message triggers a status change, so we can change if the
+    // message is perhaps expired (and event was missed).

[jianli, 2014/08/27 18:12:00]

is perhaps expired => has expired

[fgorski, 2014/08/27 21:47:16]

Done.

+    if (IsMessageExpired(iter->status_change_timestamp)) {

[jianli, 2014/08/27 18:12:00]

I found out that it was hard to understand using a single timestamp
|status_change_timestamp| for two different purposes:  checking message
expiration and checking update due. How about using 2 separate timestamps:
last_update_time_ and message_sending_time_?

[fgorski, 2014/08/27 21:47:17]

I didn't want to store too much. It is not necessary to have both timestamps.
I renamed the method to make name more self-explanatory, and added a check for
specific status that is relevant to the problem.

+      iter->last_message_type = AccountMapping::MSG_NONE;
+      iter->last_message_id.clear();
+    }
+  }
+
+  // Update the internal collection of mappings with the new tokens.
+  for (std::vector<GCMClient::AccountTokenInfo>::const_iterator token_iter =
+           account_tokens.begin();
+       token_iter != account_tokens.end();
+       ++token_iter) {
+    AccountMapping* account_mapping =
+        FindMappingByAccountId(token_iter->account_id);
+    if (!account_mapping) {
+      AccountMapping new_mapping;
+      new_mapping.status = AccountMapping::NEW;
+      new_mapping.account_id = token_iter->account_id;
+      new_mapping.access_token = token_iter->access_token;
+      new_mapping.email = token_iter->email;
+      new_mapping.last_message_type = AccountMapping::MSG_NONE;
+      accounts_.push_back(new_mapping);
+    } else {
+      // Since we got a token for an account, drop the remove message and treat
+      // it as new.

[jianli, 2014/08/27 18:12:00]

When we get a new token for an account, do we need to fill in the new token info
and set the status back to NEW regardless the current status?

Say, if the current status is ADDING, it means an adding message has been sent.
When the new token is retrieved, do we need to trigger sending a new adding
message?

[fgorski, 2014/08/27 21:47:16]

Getting form AccountMapping::REMOVING to AccountMapping::MAPPED is intended, in
order to make sure the account is properly mapped.

AccountMapping::NEW is deleted, if the mapping fails, AccountMapping::MAPPED is
still tracked and if no more tokens show up, it will be going to state
AccountMapping::REMOVING.

This means we won't lose track of it.

As for tokens: They only have one hour of validity, that is why it makes sense
to dispose of them when a new batch arrives and "start fresh".

+      if (account_mapping->last_message_type == AccountMapping::MSG_REMOVE) {
+        account_mapping->status = AccountMapping::MAPPED;
+        account_mapping->status_change_timestamp = base::Time();
+        account_mapping->last_message_type = AccountMapping::MSG_NONE;
+        account_mapping->last_message_id.clear();
+      }
+
+      account_mapping->email = token_iter->email;
+      account_mapping->access_token = token_iter->access_token;
+    }
+  }
+
+  // Decide what to do with each account (either start mapping, or start
+  // removing).
+  for (AccountMappings::iterator mappings_iter = accounts_.begin();
+       mappings_iter != accounts_.end();
+       ++mappings_iter) {
+    if (mappings_iter->access_token.empty()) {
+      if (mappings_iter->last_message_id.empty() ||
+          mappings_iter->last_message_type != AccountMapping::MSG_REMOVE) {
+        SendRemoveMappingMessage(*mappings_iter);
+      }
+    } else {
+      if (IsUpdateDue(mappings_iter->status_change_timestamp) ||

[jianli, 2014/08/27 18:12:00]

Should || be &&, per our discussion? Please also comment on this.

[fgorski, 2014/08/27 21:47:17]

Done. I've added account status checks to make it more readable.

+          mappings_iter->status == AccountMapping::NEW) {
+        SendAddMappingMessage(*mappings_iter);
+      }
+    }
+  }
+}
+
+void GCMAccountMapper::ShutdownHandler() {
+  gcm_driver_->RemoveAppHandler(kGCMAccountMapperAppId);
+}
+
+void GCMAccountMapper::OnMessage(const std::string& app_id,
+                                 const GCMClient::IncomingMessage& message) {
+  // Account message does not expect messages right now.
+}
+
+void GCMAccountMapper::OnMessagesDeleted(const std::string& app_id) {
+  // Account message does not expect messages right now.
+}
+
+void GCMAccountMapper::OnSendError(
+    const std::string& app_id,
+    const GCMClient::SendErrorDetails& send_error_details) {
+  DCHECK_EQ(app_id, kGCMAccountMapperAppId);
+
+  AccountMappings::iterator account_mapping_it =
+      FindMappingByMessageId(send_error_details.message_id);
+
+  if (account_mapping_it == accounts_.end())
+    return;
+
+  // TODO(fgorski): What other status can we get here if any? What should we do
+  // about that. (likely TTL_EXCEEDED is the only one)
+  DCHECK_EQ(send_error_details.result, GCMClient::TTL_EXCEEDED);
+
+  if (account_mapping_it->last_message_type == AccountMapping::MSG_REMOVE) {
+    SendRemoveMappingMessage(*account_mapping_it);
+  } else {
+    DCHECK_EQ(account_mapping_it->last_message_type, AccountMapping::MSG_ADD);
+    if (account_mapping_it->status == AccountMapping::ADDING) {
+      // There is no mapping established, so we can remove the entry.
+      // Getting a fresh token will trigger a new attempt.
+      gcm_driver_->RemoveAccountMapping(account_mapping_it->account_id);
+      accounts_.erase(account_mapping_it);
+    } else {
+      // Account is already MAPPED, we have to wait for another token.
+      account_mapping_it->last_message_id.clear();
+      account_mapping_it->last_message_type = AccountMapping::MSG_NONE;
+      gcm_driver_->UpdateAccountMapping(*account_mapping_it);
+    }
+  }
+}
+
+void GCMAccountMapper::OnSendAcknowledged(const std::string& app_id,
+                                          const std::string& message_id) {
+  DCHECK_EQ(app_id, kGCMAccountMapperAppId);
+  AccountMappings::iterator account_mapping_it =
+      FindMappingByMessageId(message_id);
+
+  DVLOG(1) << "OnSendAcknowledged with message ID: " << message_id;
+
+  if (account_mapping_it == accounts_.end())
+    return;
+
+  // here is where we advance a status of a mapping and persist or remove.

[jianli, 2014/08/27 18:12:00]

nit: make 1st letter capitalized

[fgorski, 2014/08/27 21:47:17]

Done.

+  if (account_mapping_it->last_message_type == AccountMapping::MSG_REMOVE) {
+    DVLOG(1) << "Removal message found, account_id "
+             << account_mapping_it->account_id;
+    // Message removing the account has been confirmed by the GCM, we can remove
+    // all the information related to the account (from memory and store).
+    gcm_driver_->RemoveAccountMapping(account_mapping_it->account_id);
+    accounts_.erase(account_mapping_it);
+  } else {
+    DVLOG(1) << "Add message found, account_id "
+             << account_mapping_it->account_id;
+    // Message adding the account has been confirmed by the GCM, we can now

[jianli, 2014/08/27 18:12:00]

The comment is a bit hard to follow since it goes into too much detail. Could
you please rephrase it in high-level summary, like?
 // With the ACK from the GCM, we can update the local tracking state.

Indeed this comment seems to apply to both if and else nicely.

[fgorski, 2014/08/27 21:47:17]

Done. Restructured, rephrased and added a DCHECK.

+    // ensure the status and status change timestamp is updated, while message
+    // information is cleared.
+    DCHECK_EQ(account_mapping_it->last_message_type, AccountMapping::MSG_ADD);
+    if (account_mapping_it->status == AccountMapping::ADDING)
+      account_mapping_it->status = AccountMapping::MAPPED;
+
+    account_mapping_it->status_change_timestamp = clock_->Now();
+    account_mapping_it->last_message_type = AccountMapping::MSG_NONE;
+    account_mapping_it->last_message_id.clear();
+
+    gcm_driver_->UpdateAccountMapping(*account_mapping_it);
+  }
+}
+
+void GCMAccountMapper::OnConnected(const net::IPEndPoint& ip_endpoint) {
+  // Account mapper ignores connection status.
+}
+
+void GCMAccountMapper::OnDisconnected() {
+  // Account mapper ignores connection status.
+}
+
+bool GCMAccountMapper::CanHandle(const std::string& app_id) const {
+  return app_id.compare(kGCMAccountMapperAppId) == 0;
+}
+
+void GCMAccountMapper::SendAddMappingMessage(AccountMapping& account_mapping) {
+  // Account mapping is only written to the store once a message was
+  // successfully sent.
+  if (account_mapping.status == AccountMapping::NEW)
+    account_mapping.status = AccountMapping::ADDING;

[jianli, 2014/08/27 18:12:00]

Why do we not update account_mapping.status_change_timestamp on status change
here?

[fgorski, 2014/08/27 21:47:17]

This is by design.
I want the state of ADDING only to be persisted once the message is actually
queued for sending.
With REMOVING I am not dependent on the message so I do that before I send it.

You are right however about those two things happening together.
I moved the status update to OnSendFinished, so that it happens together with
time-stamping.

+
+  GCMClient::OutgoingMessage outgoing_message;
+  outgoing_message.id = GenerateMessageID();
+  outgoing_message.time_to_live = kGCMAccountMapperMessageTTL;
+  outgoing_message.data[kRegistrationIdMessgaeKey] = registration_id_;
+  outgoing_message.data[kTokenMessageKey] = account_mapping.access_token;
+  outgoing_message.data[kAccountMessageKey] = account_mapping.email;
+
+  gcm_driver_->Send(kGCMAccountMapperAppId,
+                    kGCMAccountMapperSenderId,
+                    outgoing_message,
+                    base::Bind(&GCMAccountMapper::OnSendFinished,
+                               weak_ptr_factory_.GetWeakPtr(),
+                               account_mapping.account_id,
+                               AccountMapping::MSG_ADD));
+}
+
+void GCMAccountMapper::SendRemoveMappingMessage(
+    AccountMapping& account_mapping) {
+  if (account_mapping.status != AccountMapping::REMOVING) {
+    account_mapping.status = AccountMapping::REMOVING;
+    account_mapping.status_change_timestamp = clock_->Now();
+  }
+
+  account_mapping.last_message_id.clear();
+  account_mapping.last_message_type = AccountMapping::MSG_NONE;
+  gcm_driver_->UpdateAccountMapping(account_mapping);
+
+  GCMClient::OutgoingMessage outgoing_message;
+  outgoing_message.id = GenerateMessageID();
+  outgoing_message.time_to_live = kGCMAccountMapperMessageTTL;
+  outgoing_message.data[kRegistrationIdMessgaeKey] = registration_id_;
+  outgoing_message.data[kAccountMessageKey] = account_mapping.email;
+  outgoing_message.data[kRemoveAccountMessageKey] = kRemoveAccountMessageValue;
+
+  gcm_driver_->Send(kGCMAccountMapperAppId,
+                    kGCMAccountMapperSenderId,
+                    outgoing_message,
+                    base::Bind(&GCMAccountMapper::OnSendFinished,
+                               weak_ptr_factory_.GetWeakPtr(),
+                               account_mapping.account_id,
+                               AccountMapping::MSG_REMOVE));
+}
+
+void GCMAccountMapper::OnSendFinished(const std::string& account_id,
+                                      AccountMapping::MessageType message_type,
+                                      const std::string& message_id,
+                                      GCMClient::Result result) {
+  if (result == GCMClient::SUCCESS) {

[jianli, 2014/08/27 18:12:01]

What is expected when result is not SUCCESS?

If we don't need to do anything on error, it would be better to do early return
on error.

[fgorski, 2014/08/27 21:47:17]

Done.

+    AccountMapping* account_mapping_it = FindMappingByAccountId(account_id);
+    DCHECK(account_mapping_it);
+
+    account_mapping_it->last_message_type = message_type;
+    account_mapping_it->last_message_id = message_id;
+
+    // Update of the status when removing the account happens before a message

[jianli, 2014/08/27 18:12:00]

Not sure I understand "removing the account happens before...". It seems not to
match the code below.

[fgorski, 2014/08/27 21:47:17]

Done together with the work on the comment above.

+    // is sent.
+    if (message_type == AccountMapping::MSG_ADD)
+      account_mapping_it->status_change_timestamp = clock_->Now();
+
+    gcm_driver_->UpdateAccountMapping(*account_mapping_it);
+  }
+}
+
+bool GCMAccountMapper::IsUpdateDue(const base::Time& last_update_time) {
+  return last_update_time + base::TimeDelta::FromHours(kGCMUpdateInterval) <

[jianli, 2014/08/27 18:12:00]

Should "<" be ">="?

[fgorski, 2014/08/27 21:47:17]

< is ok.

the question is, should I update now or wait. 
start time + interval < now  --- means now I am overdue
start time + interval >= now --- means I don't have to run an update yet

Based on hour discussion I added a 6 hour early update period, in case some of
the tokens show up earlier. 

+         clock_->Now();
+}
+
+bool GCMAccountMapper::IsMessageExpired(const base::Time& estimated_send_time) {
+  return estimated_send_time +
+             base::TimeDelta::FromSeconds(kGCMAccountMapperMessageTTL) <
+         clock_->Now();
+}
+
+AccountMapping* GCMAccountMapper::FindMappingByAccountId(
+    const std::string& account_id) {
+  for (AccountMappings::iterator iter = accounts_.begin();
+       iter != accounts_.end();
+       ++iter) {
+    if (iter->account_id == account_id)
+      return &*iter;
+  }
+
+  return NULL;
+}
+
+GCMAccountMapper::AccountMappings::iterator
+GCMAccountMapper::FindMappingByMessageId(const std::string& message_id) {
+  for (std::vector<AccountMapping>::iterator iter = accounts_.begin();
+       iter != accounts_.end();
+       ++iter) {
+    if (iter->last_message_id == message_id)
+      return iter;
+  }
+
+  return accounts_.end();
+}
+
+void GCMAccountMapper::SetClockForTesting(scoped_ptr<base::Clock> clock) {
+  clock_ = clock.Pass();
+}
+
+}  // namespace gcm