From db9cb0e5848645420e439277974b5aa379d83553 Mon Sep 17 00:00:00 2001 From: johnme Date: Tue, 12 May 2015 07:55:42 -0700 Subject: [PATCH] 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} --- .../push_messaging_app_identifier.cc | 66 +++++++++++----------- .../push_messaging/push_messaging_app_identifier.h | 53 +++++++++++------ .../push_messaging_app_identifier_unittest.cc | 19 ++++--- .../push_messaging/push_messaging_browsertest.cc | 6 +- .../push_messaging/push_messaging_service_impl.cc | 14 ++--- 5 files changed, 90 insertions(+), 68 deletions(-) diff --git a/chrome/browser/push_messaging/push_messaging_app_identifier.cc b/chrome/browser/push_messaging/push_messaging_app_identifier.cc index 58d8263b9b39..feb069a006cd 100644 --- a/chrome/browser/push_messaging/push_messaging_app_identifier.cc +++ b/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::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))); } diff --git a/chrome/browser/push_messaging/push_messaging_app_identifier.h b/chrome/browser/push_messaging/push_messaging_app_identifier.h index 11567ea2318d..6d64fc2be5f4 100644 --- a/chrome/browser/push_messaging/push_messaging_app_identifier.h +++ b/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 #include #include #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. +// 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. 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_ diff --git a/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc b/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc index ee22a00d298d..e79364d22d2b 100644 --- a/chrome/browser/push_messaging/push_messaging_app_identifier_unittest.cc +++ b/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) { diff --git a/chrome/browser/push_messaging/push_messaging_browsertest.cc b/chrome/browser/push_messaging/push_messaging_browsertest.cc index fe531d2b46a5..6dab03aee9ac 100644 --- a/chrome/browser/push_messaging/push_messaging_browsertest.cc +++ b/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 { diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.cc b/chrome/browser/push_messaging/push_messaging_service_impl.cc index 2cf40858917e..06debd70ae59 100644 --- a/chrome/browser/push_messaging/push_messaging_service_impl.cc +++ b/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, -- 2.11.4.GIT