Cleanup PushMessagingAppIdentifier

chrome/browser/push_messaging/push_messaging_app_identifier.h

Index: chrome/browser/push_messaging/push_messaging_app_identifier.h
diff --git a/chrome/browser/push_messaging/push_messaging_app_identifier.h b/chrome/browser/push_messaging/push_messaging_app_identifier.h
index 11567ea2318d25bab2bbfd71fc7007787b0ab1d1..4a566b659cb161cc15569e4529399fbdd40850ed 100644
--- a/chrome/browser/push_messaging/push_messaging_app_identifier.h
+++ b/chrome/browser/push_messaging/push_messaging_app_identifier.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_PUSH_MESSAGING_PUSH_MESSAGING_APP_IDENTIFIER_H_
 #define CHROME_BROWSER_PUSH_MESSAGING_PUSH_MESSAGING_APP_IDENTIFIER_H_
 
+#include <stdint.h>
 #include <string>
 #include <vector>
 
@@ -20,9 +21,9 @@ class PrefRegistrySyncable;
 // The prefix used for all push messaging application ids.
 extern const char kPushMessagingAppIdentifierPrefix[];
 
-// Type used to identify a web app from a Push API perspective.
-// These can be persisted to disk, in a 1:1 mapping between app_id and
-// pair<origin, service_worker_registration_id>.
+// Type used to identify a web app (specifically, a Service Worker registration)

[Peter Beverloo, 2015/05/11 16:54:08]

+1 to the addition, but I would prefer to remove the "web app" terminology
completely if we can. Michael's been working to do the same with the W3C API.

[johnme, 2015/05/12 13:21:50]

Done.

+// from a Push API perspective. These can be persisted to disk, in a 1:1 mapping

[Peter Beverloo, 2015/05/11 16:54:07]

"to disk" -> "to prefs" to match the other changes.

[johnme, 2015/05/12 13:21:50]

Done.

+// between app_id and pair<origin, service_worker_registration_id>.
 class PushMessagingAppIdentifier {
  public:
   // Register profile-specific prefs.
@@ -31,17 +32,17 @@ class PushMessagingAppIdentifier {
   // Generates a new app identifier with random app_id.
   static PushMessagingAppIdentifier Generate(
       const GURL& origin,
-      int64 service_worker_registration_id);
+      int64_t service_worker_registration_id);
 
-  // Looks up an app identifier by app_id. Will be invalid if not found.
+  // Looks up an app identifier by app_id. Will be null if not found.

[Peter Beverloo, 2015/05/11 16:54:08]

What do you think about changing null -> is_null() in comments? If I read it,
I'd expect it to return a nullptr, while it returns an object.

[johnme, 2015/05/12 13:21:50]

Done.

   static PushMessagingAppIdentifier Get(Profile* profile,
                                         const std::string& app_id);
 
   // Looks up an app identifier by origin & service worker registration id.
-  // Will be invalid if not found.
+  // Will be null if not found.
   static PushMessagingAppIdentifier Get(Profile* profile,
                                         const GURL& origin,
-                                        int64 service_worker_registration_id);
+                                        int64_t service_worker_registration_id);
 
   // Returns all the PushMessagingAppIdentifiers currently registered for the
   // given |profile|.
@@ -49,17 +50,27 @@ class PushMessagingAppIdentifier {
 
   ~PushMessagingAppIdentifier();
 
-  // Persist this app identifier to disk.
-  void PersistToDisk(Profile* profile) const;
+  // Persist this app identifier to prefs.
+  void PersistToPrefs(Profile* profile) const;
 
-  // Delete this app identifier from disk.
-  void DeleteFromDisk(Profile* profile) const; // TODO: Does const make sense?
+  // Delete this app identifier from prefs.
+  void DeleteFromPrefs(Profile* profile) const;
 
-  bool IsValid() const;
+  // Returns true if this object has not been initialized.

[Peter Beverloo, 2015/05/11 16:54:07]

The object *will* have been initialized. What about:

// Returns true if this identifier does not represent an app.

[johnme, 2015/05/12 13:21:50]

Done ("Returns true if this identifier does not represent an app (i.e. this was
returned by a failed call to Get).")

+  bool is_null() const { return service_worker_registration_id_ < 0; }
 
+  // Perform a DCHECK that the origin, app_id and service_worker_registration_id

[Peter Beverloo, 2015/05/11 16:54:08]

nit: Make this private (per the changes in PushMessagingServiceImpl). I would
personally not mention the fields it checks, but rather just say it validates
that everything about this app identifier has valid values.

[johnme, 2015/05/12 13:21:50]

Done.

+  // all contain valid values.
+  void DCheckValid() const;
+
+  // String that should be passed to push services like GCM to identify a
+  // particular Service Worker (so we can route incoming messages). Example:
+  // wp:9CC55CCE-B8F9-4092-A364-3B0F73A3AB5F
   const std::string& app_id() const { return app_id_; }
+
   const GURL& origin() const { return origin_; }
-  int64 service_worker_registration_id() const {
+
+  int64_t service_worker_registration_id() const {
     return service_worker_registration_id_;
   }
 
@@ -71,11 +82,11 @@ class PushMessagingAppIdentifier {
   // Constructs a valid app identifier.
   PushMessagingAppIdentifier(const std::string& app_id,
                              const GURL& origin,
-                             int64 service_worker_registration_id);
+                             int64_t service_worker_registration_id);
 
   std::string app_id_;
   GURL origin_;
-  int64 service_worker_registration_id_;
+  int64_t service_worker_registration_id_;
 };
 
 #endif  // CHROME_BROWSER_PUSH_MESSAGING_PUSH_MESSAGING_APP_IDENTIFIER_H_