components: add wifi_sync component

components/wifi_sync/wifi_credential_syncable_service.h

+// 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.
+
+#ifndef COMPONENTS_WIFI_SYNC_WIFI_CREDENTIAL_SYNCABLE_SERVICE_H_
+#define COMPONENTS_WIFI_SYNC_WIFI_CREDENTIAL_SYNCABLE_SERVICE_H_
+
+#include <stdint.h>
+
+#include <set>
+#include <string>

[erikwright (departed), 2014/11/17 20:18:58]

not used

[mukesh agrawal, 2014/11/18 17:17:44]

Done.

+#include <utility>  // for std::pair

[erikwright (departed), 2014/11/17 20:18:58]

This comment is unnecessary.

[mukesh agrawal, 2014/11/18 17:17:44]

Done.

+#include <vector>
+
+#include "base/macros.h"
+#include "base/threading/thread_checker.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/wifi_sync/wifi_security_class.h"
+#include "sync/api/sync_data.h"
+#include "sync/api/sync_error.h"

[erikwright (departed), 2014/11/17 20:18:57]

Not required (you only use it as part of declaring an override; we are
guaranteed that the base class declaration already had sufficient definition of
SyncError for this purpose).

[mukesh agrawal, 2014/11/18 17:17:43]

Done. (Also removed sync_data, for the same reason.)

+#include "sync/api/sync_error_factory.h"
+#include "sync/api/syncable_service.h"
+
+namespace content {
+class BrowserContext;
+}  // content
+
+namespace wifi_sync {
+
+// KeyedService that synchronizes WiFi credentials between local settings,
+// and Chrome Sync.
+//
+// This service does not necessarily own the storage for WiFi
+// credentials. In particular, on ChromeOS, WiFi credential storage is
+// managed by the ChromeOS connection manager ("Shill").
+class WifiCredentialSyncableService
+    : public syncer::SyncableService, public KeyedService {
+ public:
+  using SsidBytes = std::vector<uint8_t>;
+  using WifiSsidSecuritySet =
+      std::set<std::pair<SsidBytes, WifiSecurityClass>>;
+
+  // Constructs a service for the given |context|. Local changes will

[erikwright (departed), 2014/11/17 20:18:57]

either "for |context|" or "for the given BrowserContext."

[mukesh agrawal, 2014/11/18 17:17:44]

Done.

+  // be applied to the Chrome Sync data for the user associated with
+  // |context|. On ChromeOS, this class should only be instantiated
+  // for the primary user profile, as that is the only profile for
+  // which a Shill profile is loaded.
+  explicit WifiCredentialSyncableService(content::BrowserContext *context);
+  ~WifiCredentialSyncableService() override;
+
+  // syncer::SyncableService implementation.
+

[erikwright (departed), 2014/11/17 20:18:57]

remove the blank lines 52,58,60,65

[mukesh agrawal, 2014/11/18 17:17:44]

Done.

+  syncer::SyncMergeResult MergeDataAndStartSyncing(
+      syncer::ModelType type,
+      const syncer::SyncDataList& initial_sync_data,
+      scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
+      scoped_ptr<syncer::SyncErrorFactory> error_handler) override;
+
+  void StopSyncing(syncer::ModelType type) override;
+
+  // The implementation of GetAllSyncData may not actually return the

[erikwright (departed), 2014/11/17 20:18:57]

There should not be a reason to add comments to an override declaration. The
caller of this method is presumably using a base class and reading the base
class's documentation. They might never read this comment. Which means that
nothing important should be written here.

Of course, nothing _unimportant_ should be written here, either, so nothing
should be written here at all.

If clients of the derived class (i.e., the instantiator) need this information
it should go in the class comments.

If this comment is meant to be a warning to clients about the potential variety
of derived implementations it should go in the base class.

If this comment is targeted at future developers of
WifiCredentialSyncableService it should potentially go in the implementation
(.cc file).

[mukesh agrawal, 2014/11/18 17:17:44]

Done. (Removed, as I couldn't find a reason to keep it. I may it back to the
implementation file in a later CL.)

+  // complete data list. In particular, the ChromeOS implementation
+  // will return an empty list.
+  syncer::SyncDataList GetAllSyncData(syncer::ModelType type) const override;
+
+  syncer::SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const syncer::SyncChangeList& change_list) override;
+
+  // Test support.
+  WifiSsidSecuritySet GetWifiNetworksForTest() const;
+
+ private:
+  friend class WifiCredentialSyncableServiceTest;
+
+  static const enum syncer::ModelType kModelType;

[erikwright (departed), 2014/11/17 20:18:57]

I think you are using this include transitively. You should introduce a direct
include of this type.

[erikwright (departed), 2014/11/17 20:18:58]

I'm not used to seeing 'enum' somewhere like this. I would remove it unless you
see examples or have reasons to the contrary.

[mukesh agrawal, 2014/11/18 17:17:43]

Done.

[mukesh agrawal, 2014/11/18 17:17:44]

You are correct that this is included transitively. I believe this is the way
the since headers are intended to be used. In particular, the direct include
would require sync/internal_api/public/base/model_type.h. From the path
("internal_api"), I inferred that the file should not be directly used outside
of sync.

A search on the existing source seems to confirm that idea. In particular,
outside of the sync code, only one file includes model_type.h directly [1].
Nonetheless, if you still think model_type.h should be included directly, I can
make that change.

[1]
https://code.google.com/p/chromium/codesearch#search/&q=include.%2Bmodel_type...

+
+  // Const members.

[erikwright (departed), 2014/11/17 20:18:58]

I don't think the section headings here are required. The declarations already
include the 'const' attribute.

On the other hand, the Google C++ style guide expects comments on each member
declaration.

[mukesh agrawal, 2014/11/18 17:17:44]

Done.

+  const content::BrowserContext* const browser_context_;
+  const base::ThreadChecker thread_checker_;
+
+  // Non-const members.
+  scoped_ptr<syncer::SyncChangeProcessor> sync_processor_;

[erikwright (departed), 2014/11/17 20:18:57]

Requires an include?

[erikwright (departed), 2014/11/17 20:18:57]

include base/memory/scoped_ptr.h

[mukesh agrawal, 2014/11/18 17:17:43]

Done.

[mukesh agrawal, 2014/11/18 17:17:43]

Done.

+  scoped_ptr<syncer::SyncErrorFactory> sync_error_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(WifiCredentialSyncableService);
+};
+
+}  // namespace wifi_sync
+
+#endif  // COMPONENTS_WIFI_SYNC_WIFI_CREDENTIAL_SYNCABLE_SERVICE_H_