Cleanup PushMessagingAppIdentifier

In particular, the IsValid method had a confusing dual purpose - it was
used both to sanity check the inputs, and to check whether the instances
returned by the Get methods were null (i.e. Get failed). It has been
split into is_null() and DCheckValid(). Note that having DCheckValid as
a method also gives better stacktraces than the old DCHECK(IsValid())
style, since stacktraces will indicate which check failed.

BUG=458592

Review URL: https://codereview.chromium.org/1131303002

Cr-Commit-Position: refs/heads/master@{#329406}

chrome/browser/push_messaging/push_messaging_app_identifier.cc

@@ -30,7 +30,7 @@ void PushMessagingAppIdentifier::RegisterProfilePrefs(
 
 // static
 PushMessagingAppIdentifier PushMessagingAppIdentifier::Generate(
-    const GURL& origin, int64 service_worker_registration_id)
+    const GURL& origin, int64_t service_worker_registration_id)
 {
   std::string guid = base::GenerateGUID();
   CHECK(!guid.empty());
@@ -38,7 +38,7 @@ PushMessagingAppIdentifier PushMessagingAppIdentifier::Generate(
 
   PushMessagingAppIdentifier app_identifier(app_id, origin,
                                             service_worker_registration_id);
-  DCHECK(app_identifier.IsValid());
+  app_identifier.DCheckValid();
   return app_identifier;
 }
 
@@ -68,19 +68,20 @@ PushMessagingAppIdentifier PushMessagingAppIdentifier::Get(
 
   GURL origin = GURL(parts[0]);
 
-  int64 service_worker_registration_id;
+  int64_t service_worker_registration_id;
   if (!base::StringToInt64(parts[1], &service_worker_registration_id))
     return PushMessagingAppIdentifier();
 
   PushMessagingAppIdentifier app_identifier(uppercase_app_id, origin,
                                             service_worker_registration_id);
-  DCHECK(app_identifier.IsValid());
+  app_identifier.DCheckValid();
   return app_identifier;
 }
 
 // static
 PushMessagingAppIdentifier PushMessagingAppIdentifier::Get(
-    Profile* profile, const GURL& origin, int64 service_worker_registration_id)
+    Profile* profile, const GURL& origin,
+    int64_t service_worker_registration_id)
 {
   base::StringValue origin_and_sw_id = base::StringValue(origin.spec() +
       kSeparator + base::Int64ToString(service_worker_registration_id));
@@ -110,8 +111,25 @@ std::vector<PushMessagingAppIdentifier> PushMessagingAppIdentifier::GetAll(
   return result;
 }
 
-void PushMessagingAppIdentifier::PersistToDisk(Profile* profile) const {
-  DCHECK(IsValid());
+PushMessagingAppIdentifier::PushMessagingAppIdentifier()
+    : origin_(GURL::EmptyGURL()),
+      service_worker_registration_id_(-1) {
+}
+
+PushMessagingAppIdentifier::PushMessagingAppIdentifier(
+    const std::string& app_id,
+    const GURL& origin,
+    int64_t service_worker_registration_id)
+    : app_id_(app_id),
+      origin_(origin),
+      service_worker_registration_id_(service_worker_registration_id) {
+}
+
+PushMessagingAppIdentifier::~PushMessagingAppIdentifier() {
+}
+
+void PushMessagingAppIdentifier::PersistToPrefs(Profile* profile) const {
+  DCheckValid();
 
   DictionaryPrefUpdate update(profile->GetPrefs(),
                               prefs::kPushMessagingAppIdentifierMap);
@@ -121,7 +139,7 @@ void PushMessagingAppIdentifier::PersistToDisk(Profile* profile) const {
   // registration id (hence we ensure there is a 1:1 not 1:many mapping).
   PushMessagingAppIdentifier old = Get(profile, origin_,
                                        service_worker_registration_id_);
-  if (old.IsValid())
+  if (!old.is_null())
     map->RemoveWithoutPathExpansion(old.app_id_, nullptr);
 
   std::string origin_and_sw_id = origin_.spec() + kSeparator +
@@ -129,8 +147,8 @@ void PushMessagingAppIdentifier::PersistToDisk(Profile* profile) const {
   map->SetStringWithoutPathExpansion(app_id_, origin_and_sw_id);
 }
 
-void PushMessagingAppIdentifier::DeleteFromDisk(Profile* profile) const {
-  DCHECK(IsValid());
+void PushMessagingAppIdentifier::DeleteFromPrefs(Profile* profile) const {
+  DCheckValid();
 
   DictionaryPrefUpdate update(profile->GetPrefs(),
                               prefs::kPushMessagingAppIdentifierMap);
@@ -138,27 +156,11 @@ void PushMessagingAppIdentifier::DeleteFromDisk(Profile* profile) const {
   map->RemoveWithoutPathExpansion(app_id_, nullptr);
 }
 
-PushMessagingAppIdentifier::PushMessagingAppIdentifier()
-    : origin_(GURL::EmptyGURL()),
-      service_worker_registration_id_(-1) {
-}
-
-PushMessagingAppIdentifier::PushMessagingAppIdentifier(
-    const std::string& app_id,
-    const GURL& origin,
-    int64 service_worker_registration_id)
-    : app_id_(app_id),
-      origin_(origin),
-      service_worker_registration_id_(service_worker_registration_id) {
-}
-
-PushMessagingAppIdentifier::~PushMessagingAppIdentifier() {
-}
-
-bool PushMessagingAppIdentifier::IsValid() const {
+void PushMessagingAppIdentifier::DCheckValid() const {
   const size_t prefix_len = strlen(kPushMessagingAppIdentifierPrefix);
-  return origin_.is_valid() && origin_.GetOrigin() == origin_
-      && service_worker_registration_id_ >= 0
-      && !app_id_.compare(0, prefix_len, kPushMessagingAppIdentifierPrefix)
-      && base::IsValidGUID(app_id_.substr(prefix_len, std::string::npos));
+  DCHECK_GE(service_worker_registration_id_, 0);
+  DCHECK(origin_.is_valid());
+  DCHECK_EQ(origin_.GetOrigin(), origin_);
+  DCHECK_EQ(app_id_.substr(0, prefix_len), kPushMessagingAppIdentifierPrefix);
+  DCHECK(base::IsValidGUID(app_id_.substr(prefix_len, std::string::npos)));
 }

chrome/browser/push_messaging/push_messaging_app_identifier.h

@@ -5,10 +5,12 @@
 #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>
 
 #include "base/basictypes.h"
+#include "base/logging.h"
 #include "url/gurl.h"
 
 class Profile;
@@ -20,9 +22,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 Service Worker registration from a Push API
+// perspective. These can be persisted to prefs, in a 1:1 mapping between
+// app_id and pair<origin, service_worker_registration_id>.
 class PushMessagingAppIdentifier {
  public:
   // Register profile-specific prefs.
@@ -31,17 +33,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. If not found, is_null() will be true.
   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.
+  // If not found, is_null() will be true.
   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 +51,31 @@ 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 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; }
 
-  const std::string& app_id() const { return app_id_; }
-  const GURL& origin() const { return origin_; }
-  int64 service_worker_registration_id() 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 {
+    DCHECK(!is_null());
+    return app_id_;
+  }
+
+  const GURL& origin() const {
+    DCHECK(!is_null());
+    return origin_;
+  }
+
+  int64_t service_worker_registration_id() const {
+    DCHECK(!is_null());
     return service_worker_registration_id_;
   }
 
@@ -71,11 +87,14 @@ 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);
+
+  // Validates that all the fields contain valid values.
+  void DCheckValid() const;
 
   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_

chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc

@@ -9,7 +9,7 @@ class PushMessagingAppIdentifierTest : public testing::Test {
  protected:
   PushMessagingAppIdentifier GenerateId(
       const GURL& origin,
-      int64 service_worker_registration_id) {
+      int64_t service_worker_registration_id) {
     // To bypass DCHECK in PushMessagingAppIdentifier::Generate, we just use it
     // to generate app_id, and then use private constructor.
     std::string app_id = PushMessagingAppIdentifier::Generate(
@@ -20,13 +20,16 @@ class PushMessagingAppIdentifierTest : public testing::Test {
 };
 
 TEST_F(PushMessagingAppIdentifierTest, ConstructorValidity) {
-  EXPECT_TRUE(GenerateId(GURL("https://www.example.com/"), 1).IsValid());
-  EXPECT_TRUE(GenerateId(GURL("https://www.example.com"), 1).IsValid());
-  EXPECT_FALSE(GenerateId(GURL(""), 1).IsValid());
-  EXPECT_FALSE(GenerateId(GURL("foo"), 1).IsValid());
-  EXPECT_FALSE(GenerateId(GURL("https://www.example.com/foo"), 1).IsValid());
-  EXPECT_FALSE(GenerateId(GURL("https://www.example.com/#foo"), 1).IsValid());
-  EXPECT_FALSE(GenerateId(GURL("https://www.example.com/"), -1).IsValid());
+  // The following two are valid:
+  EXPECT_FALSE(GenerateId(GURL("https://www.example.com/"), 1).is_null());
+  EXPECT_FALSE(GenerateId(GURL("https://www.example.com"), 1).is_null());
+  // The following four are invalid and will DCHECK in Generate:
+  EXPECT_FALSE(GenerateId(GURL(""), 1).is_null());
+  EXPECT_FALSE(GenerateId(GURL("foo"), 1).is_null());
+  EXPECT_FALSE(GenerateId(GURL("https://www.example.com/foo"), 1).is_null());
+  EXPECT_FALSE(GenerateId(GURL("https://www.example.com/#foo"), 1).is_null());
+  // The following one is invalid and will DCHECK in Generate and be null:
+  EXPECT_TRUE(GenerateId(GURL("https://www.example.com/"), -1).is_null());
 }
 
 TEST_F(PushMessagingAppIdentifierTest, UniqueGuids) {

chrome/browser/push_messaging/push_messaging_browsertest.cc

@@ -248,7 +248,7 @@ PushMessagingBrowserTest::GetAppIdentifierForServiceWorkerRegistration(
   GURL origin = https_server()->GetURL(std::string()).GetOrigin();
   PushMessagingAppIdentifier app_identifier = PushMessagingAppIdentifier::Get(
       GetBrowser()->profile(), origin, service_worker_registration_id);
-  EXPECT_TRUE(app_identifier.IsValid());
+  EXPECT_FALSE(app_identifier.is_null());
   return app_identifier;
 }
 
@@ -1038,7 +1038,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
   PushMessagingAppIdentifier stored_app_identifier =
       PushMessagingAppIdentifier::Get(GetBrowser()->profile(),
                                       app_identifier.app_id());
-  EXPECT_TRUE(stored_app_identifier.IsValid());
+  EXPECT_FALSE(stored_app_identifier.is_null());
 
   // Simulate a user clearing site data (including Service Workers, crucially).
   BrowsingDataRemover* remover =
@@ -1065,7 +1065,7 @@ IN_PROC_BROWSER_TEST_F(PushMessagingBrowserTest,
   PushMessagingAppIdentifier stored_app_identifier2 =
       PushMessagingAppIdentifier::Get(GetBrowser()->profile(),
                                       app_identifier.app_id());
-  EXPECT_FALSE(stored_app_identifier2.IsValid());
+  EXPECT_TRUE(stored_app_identifier2.is_null());
 }
 
 class PushMessagingIncognitoBrowserTest : public PushMessagingBrowserTest {

chrome/browser/push_messaging/push_messaging_service_impl.cc

@@ -145,7 +145,7 @@ void PushMessagingServiceImpl::DecreasePushRegistrationCount(int subtract,
 }
 
 bool PushMessagingServiceImpl::CanHandle(const std::string& app_id) const {
-  return PushMessagingAppIdentifier::Get(profile_, app_id).IsValid();
+  return !PushMessagingAppIdentifier::Get(profile_, app_id).is_null();
 }
 
 void PushMessagingServiceImpl::ShutdownHandler() {
@@ -165,7 +165,7 @@ void PushMessagingServiceImpl::OnMessage(
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::Get(profile_, app_id);
   // Drop message and unregister if app_id was unknown (maybe recently deleted).
-  if (!app_identifier.IsValid()) {
+  if (app_identifier.is_null()) {
     DeliverMessageCallback(app_id, GURL::EmptyGURL(), -1, message,
                            message_handled_closure,
                            content::PUSH_DELIVERY_STATUS_UNKNOWN_APP_ID);
@@ -304,7 +304,6 @@ void PushMessagingServiceImpl::RegisterFromDocument(
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::Generate(requesting_origin,
                                            service_worker_registration_id);
-  DCHECK(app_identifier.IsValid());
 
   if (push_registration_count_ + pending_push_registration_count_
       >= kMaxRegistrations) {
@@ -362,7 +361,6 @@ void PushMessagingServiceImpl::RegisterFromWorker(
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::Generate(requesting_origin,
                                            service_worker_registration_id);
-  DCHECK(app_identifier.IsValid());
 
   if (profile_->GetPrefs()->GetInteger(
           prefs::kPushMessagingRegistrationCount) >= kMaxRegistrations) {
@@ -423,7 +421,7 @@ void PushMessagingServiceImpl::DidRegister(
   switch (result) {
     case gcm::GCMClient::SUCCESS:
       status = content::PUSH_REGISTRATION_STATUS_SUCCESS_FROM_PUSH_SERVICE;
-      app_identifier.PersistToDisk(profile_);
+      app_identifier.PersistToPrefs(profile_);
       IncreasePushRegistrationCount(1, false /* is_pending */);
       break;
     case gcm::GCMClient::INVALID_PARAMETER:
@@ -473,7 +471,7 @@ void PushMessagingServiceImpl::Unregister(
     const content::PushMessagingService::UnregisterCallback& callback) {
   PushMessagingAppIdentifier app_identifier = PushMessagingAppIdentifier::Get(
       profile_, requesting_origin, service_worker_registration_id);
-  if (!app_identifier.IsValid()) {
+  if (app_identifier.is_null()) {
     if (!callback.is_null()) {
       callback.Run(
           content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
@@ -494,9 +492,9 @@ void PushMessagingServiceImpl::Unregister(
   // retry unregistration if it fails due to network errors (crbug.com/465399).
   PushMessagingAppIdentifier app_identifier =
       PushMessagingAppIdentifier::Get(profile_, app_id);
-  bool was_registered = app_identifier.IsValid();
+  bool was_registered = !app_identifier.is_null();
   if (was_registered)
-    app_identifier.DeleteFromDisk(profile_);
+    app_identifier.DeleteFromPrefs(profile_);
 
   const auto& unregister_callback =
       base::Bind(&PushMessagingServiceImpl::DidUnregister,