wifi_sync: implement ACTION_ADD in WifiCredentialSyncableService

components/wifi_sync/wifi_credential_syncable_service.cc

Index: components/wifi_sync/wifi_credential_syncable_service.cc
diff --git a/components/wifi_sync/wifi_credential_syncable_service.cc b/components/wifi_sync/wifi_credential_syncable_service.cc
index a78b92a686dc2b0092dd1f00ff86b8ee13ec5035..b0735f6ab0b86a356c18dbb0ba3401657449f78e 100644
--- a/components/wifi_sync/wifi_credential_syncable_service.cc
+++ b/components/wifi_sync/wifi_credential_syncable_service.cc
@@ -4,18 +4,111 @@
 
 #include "components/wifi_sync/wifi_credential_syncable_service.h"
 
+#include <stdint.h>
+#include <vector>
+
 #include "base/logging.h"
+#include "components/wifi_sync/wifi_credential.h"
+#include "components/wifi_sync/wifi_security_class.h"
 #include "sync/api/sync_change.h"
+#include "sync/api/sync_data.h"
 #include "sync/api/sync_error.h"
 #include "sync/api/sync_error_factory.h"
 #include "sync/api/sync_merge_result.h"
+#include "sync/protocol/sync.pb.h"
 
 namespace wifi_sync {
 
+namespace {
+
+struct raw_credential_data {

[erikwright (departed), 2015/01/22 15:59:09]

This should still be named in camel case unless there is some convention I'm
unaware of.

[stevenjb, 2015/01/22 17:44:56]

+1

[mukesh agrawal, 2015/01/22 23:36:03]

Done. (Sorry -- when I think about structs, my old-school C habits kick in.)

+  std::vector<uint8_t> ssid;
+  WifiSecurityClass security_class;
+  std::string passphrase;
+};
+
+void BuildSpecifics(const WifiCredential& credential,
+                    sync_pb::EntitySpecifics* out_buffer) {
+  DCHECK(out_buffer);
+  sync_pb::WifiCredentialSpecifics* credential_specifics =
+      out_buffer->mutable_wifi_credential();
+  DCHECK(credential_specifics);
+  credential_specifics->set_ssid(credential.ssid().data(),
+                                 credential.ssid().size());
+  credential_specifics->set_security_class(
+      WifiSecurityClassToSyncSecurityClass(credential.security_class()));
+  if (WifiSecurityClassSupportsPassphrases(credential.security_class())) {
+    credential_specifics->set_passphrase(credential.passphrase().data(),
+                                         credential.passphrase().size());
+  }
+}
+
+bool ParseSpecifics(const sync_pb::EntitySpecifics& specifics,
+                    struct raw_credential_data* raw_credential) {

[erikwright (departed), 2015/01/22 15:59:09]

you don't need 'struct' here.

[mukesh agrawal, 2015/01/22 23:36:02]

Done.

+  DCHECK(raw_credential);
+  if (!specifics.has_wifi_credential()) {
+    LOG(ERROR) << "Specifics with missing wifi_credential; skipping";
+    return false;
+  }
+
+  const sync_pb::WifiCredentialSpecifics& credential_specifics =
+      specifics.wifi_credential();
+  if (!credential_specifics.has_ssid()) {
+    LOG(ERROR) << "Specifics with missing SSID; skipping";
+    return false;
+  }
+  if (!credential_specifics.has_security_class()) {
+    LOG(ERROR) << "Specifics with missing security class; skipping";
+    return false;
+  }
+
+  const WifiSecurityClass security_class =
+      WifiSecurityClassFromSyncSecurityClass(
+          credential_specifics.security_class());
+  if (WifiSecurityClassSupportsPassphrases(security_class) &&
+      !credential_specifics.has_passphrase()) {
+    LOG(ERROR) << "Specifics for security class "
+               << credential_specifics.security_class()
+               << " is missing passphrase; skipping";
+    return false;
+  }
+
+  raw_credential->ssid.assign(credential_specifics.ssid().begin(),
+                              credential_specifics.ssid().end());
+  raw_credential->security_class = security_class;
+  raw_credential->passphrase = credential_specifics.passphrase();
+  return true;
+}
+
+// TODO(quiche): Separate SyncData validation from parsing of
+// WifiCredentialSpecifics.
+bool ParseSyncData(const syncer::SyncData& sync_data,
+                   struct raw_credential_data* raw_credential) {

[erikwright (departed), 2015/01/22 15:59:09]

- struct

[mukesh agrawal, 2015/01/22 23:36:02]

Done.

+  DCHECK(raw_credential);
+  if (!sync_data.IsValid()) {
+    LOG(WARNING) << "Invalid SyncData; skipping item";
+    return false;
+  }
+
+  if (sync_data.GetDataType() != syncer::WIFI_CREDENTIALS) {
+    LOG(WARNING) << "Unexpected SyncData of type "
+                 << syncer::ModelTypeToString(sync_data.GetDataType())
+                 << "; skipping item";
+    return false;
+  }
+
+  return ParseSpecifics(sync_data.GetSpecifics(), raw_credential);
+}
+
+}  // namespace
+
 const syncer::ModelType WifiCredentialSyncableService::kModelType =
     syncer::WIFI_CREDENTIALS;
 
-WifiCredentialSyncableService::WifiCredentialSyncableService() {
+WifiCredentialSyncableService::WifiCredentialSyncableService(
+    scoped_ptr<WifiConfigDelegate> network_config_delegate)
+    : network_config_delegate_(network_config_delegate.Pass()) {
+  DCHECK(network_config_delegate_);
 }
 
 WifiCredentialSyncableService::~WifiCredentialSyncableService() {
@@ -32,7 +125,7 @@ syncer::SyncMergeResult WifiCredentialSyncableService::MergeDataAndStartSyncing(
 
   sync_processor_ = sync_processor.Pass();
 
-  // TODO(quiche): Update local WiFi configuration.
+  // TODO(quiche): Update local WiFi configuration from |initial_sync_data|.
   // TODO(quiche): Notify upper layers that sync is ready.
   NOTIMPLEMENTED();
 
@@ -54,8 +147,76 @@ syncer::SyncDataList WifiCredentialSyncableService::GetAllSyncData(
 syncer::SyncError WifiCredentialSyncableService::ProcessSyncChanges(
     const tracked_objects::Location& /* caller_location */,
     const syncer::SyncChangeList& change_list) {
-  NOTIMPLEMENTED();
+  if (!sync_processor_.get()) {
+    return syncer::SyncError(
+        FROM_HERE, syncer::SyncError::UNREADY_ERROR,
+        "ProcessSyncChanges called before MergeDataAndStartSyncing",
+        kModelType);
+  }
+
+  for (const auto& sync_change : change_list) {

[erikwright (departed), 2015/01/22 15:59:09]

I'm not sure what type this is. Is it particularly verbose?

The auto keyword here makes it that much harder for me to understand the code
(now I need to figure out where SyncChangeList is declared, to figure out what
type it returns, etc.).

[stevenjb, 2015/01/22 17:44:56]

We use this pattern pretty widely now in for loops, especially with STL
containers, which are usually typedefed. While I agree that we shouldn't overuse
auto, this seems like a pretty straightforward use to me. SyncChangeList is
pretty much what it is named, vector<SyncChange>.

[mukesh agrawal, 2015/01/22 23:36:02]

Thanks, Steven. I think that explains my thinking when wrote this with auto.
That said, since it proved to be confusing, I've changed the type to be
explicit.

[stevenjb, 2015/01/22 23:58:00]

Huh, I didn't realize you could use for ( : ) without auto; good to know, I do
think this is a bit more clear.

+    DCHECK(sync_change.IsValid());
+    struct raw_credential_data raw_credential;

[erikwright (departed), 2015/01/22 15:59:09]

-struct

[mukesh agrawal, 2015/01/22 23:36:02]

Done.

+    if (!ParseSyncData(sync_change.sync_data(), &raw_credential)) {
+      LOG(WARNING) << "Failed to parse item; skipping "
+                   << syncer::SyncChange::ChangeTypeToString(
+                          sync_change.change_type());
+      continue;
+    }
+
+    scoped_ptr<WifiCredential> credential;
+    switch (sync_change.change_type()) {
+      case syncer::SyncChange::ACTION_ADD:
+        credential = WifiCredential::Create(raw_credential.ssid,
+                                            raw_credential.security_class,
+                                            raw_credential.passphrase);
+        if (!credential)
+          LOG(WARNING) << "Failed to create credential; skipping";
+        else
+          network_config_delegate_->AddToLocalNetworks(*credential);
+        break;
+      case syncer::SyncChange::ACTION_UPDATE:
+        // TODO(quiche): Implement update, and add appropriate tests.
+        NOTIMPLEMENTED();
+        break;
+      case syncer::SyncChange::ACTION_DELETE:
+        // TODO(quiche): Implement delete, and add appropriate tests.
+        NOTIMPLEMENTED();
+        break;
+      default:
+        return syncer::SyncError(
+            FROM_HERE, syncer::SyncError::DATATYPE_ERROR,
+            "ProcessSyncChanges given invalid SyncChangeType", kModelType);
+    }
+  }
+
   return syncer::SyncError();
 }
 
+bool WifiCredentialSyncableService::AddToSyncedNetworks(
+    const std::string& item_id,
+    const WifiCredential& credential) {
+  if (!sync_processor_.get()) {
+    LOG(WARNING) << "WifiCredentials syncable service is not started.";

[erikwright (departed), 2015/01/22 15:59:09]

Is this standard (as opposed to queueing data for subsequent syncing?

[stevenjb, 2015/01/22 17:44:56]

Generally when sync is started it would query for any new changes. In this case
we can't because we don't store credentials in Chrome, but at the same time we
don't really want to queue the credentials locally here either. This is pretty
unlikely (but possible if sync is slow to start for some reason).  I think it is
OK not to handle that, but we should add a comment to that effect here.

[mukesh agrawal, 2015/01/22 23:36:02]

==stevenjb.

For most datatypes, the local data is managed by Chrome. So changes can
effectively be queued by 1) updating the persistent copy of that data, and 2)
consulting the persistent data to identify new items during
MergeDataAndStartSyncing.

The WifiCredential data type is odd, in that the local data is managed out of
process, by a ChromeOS daemon (called "shill"). Furthermore, shill doesn't
provide read access to the passwords. So we can't use the same approach to merge
new items during MergeDataAndStartSyncing.

That said, we could, as you suggest, queue the credentials inside the
SyncableService. But I agree with Steven that we're better off not doing so.

The reason I argue against a queue in this SyncableService is that the caller of
AddToSyncedNetworks will need to cope with the possibility that the
SyncableService doesn't even exist. So the calling code needs to have a queue
anyway. Given that, it seems simpler to ask the caller to also use that queue
when the SyncableService exists, but isn't ready.

(Why might the SyncableService not exist? The SyncableService can't exist at the
ChromeOS login screen, because there is no Google account authenticated. But
we'd like a network that was configured at the login screen to get synced into
the account of the first user who logs in. Hence the need for a queue in the
calling code.)

You might also be concerned about how the calling code knows when it is safe to
call AddToSyncedNetworks. E.g., does it have to periodically retry its queue?
The answer to that is in MergeDataAndStartSyncing method: "// TODO(quiche):
Notify upper layers that sync is ready." So no, the calling code won't have to
poll/retry to see when the SyncableService is ready.

+    return false;
+  }
+
+  // TODO(quiche): Handle case where network already exists.
+  syncer::SyncChangeList change_list;
+  syncer::SyncError sync_error;
+  sync_pb::EntitySpecifics wifi_credential_specifics;
+  BuildSpecifics(credential, &wifi_credential_specifics);
+  change_list.push_back(
+      syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD,
+                         syncer::SyncData::CreateLocalData(
+                             item_id, item_id, wifi_credential_specifics)));
+  sync_error = sync_processor_->ProcessSyncChanges(FROM_HERE, change_list);
+  if (sync_error.IsSet()) {
+    LOG(ERROR) << sync_error.ToString();
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace wifi_sync