From 7eff1739fa276d09e649e0bde69580abff32f197 Mon Sep 17 00:00:00 2001 From: Robbie Gibson Date: Tue, 13 Jun 2023 18:22:24 +0000 Subject: [PATCH] [iOS] Convert promos CHECK to a DWC and add necessary data as crash key This includes which promo failed to be dismissed as crash data, to help fixes. (cherry picked from commit 8f84dd1fab4ce2deadd629a8e05fa10bdaa2eb0b) Bug: 1443982 Change-Id: Ica32e3154f7fbddaf9c67308ec8e787caabddc41 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4572516 Reviewed-by: Justin Cohen Reviewed-by: Rohit Rao Commit-Queue: Robbie Gibson Cr-Original-Commit-Position: refs/heads/main@{#1154898} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4605926 Reviewed-by: Benjamin Williams Auto-Submit: Robbie Gibson Cr-Commit-Position: refs/branch-heads/5790@{#701} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../browser/promos_manager/constants.cc | 21 ++++++++++++------- ios/chrome/browser/promos_manager/constants.h | 5 ++++- .../promos_manager/promos_manager_impl.mm | 6 +++--- ios/chrome/browser/ui/promos_manager/BUILD.gn | 1 + .../promos_manager_coordinator.mm | 14 +++++++++++-- 5 files changed, 33 insertions(+), 14 deletions(-) diff --git a/ios/chrome/browser/promos_manager/constants.cc b/ios/chrome/browser/promos_manager/constants.cc index 381391452e304..393d02565f42c 100644 --- a/ios/chrome/browser/promos_manager/constants.cc +++ b/ios/chrome/browser/promos_manager/constants.cc @@ -5,6 +5,7 @@ #include "ios/chrome/browser/promos_manager/constants.h" #include "base/notreached.h" +#include "base/strings/strcat.h" #import "third_party/abseil-cpp/absl/types/optional.h" namespace promos_manager { @@ -42,22 +43,26 @@ absl::optional PromoForName(base::StringPiece promo) { return absl::nullopt; } -base::StringPiece NameForPromo(Promo promo) { +std::string NameForPromo(Promo promo) { + return base::StrCat({"promos_manager::Promo::", ShortNameForPromo(promo)}); +} + +base::StringPiece ShortNameForPromo(Promo promo) { switch (promo) { case promos_manager::Promo::Test: - return "promos_manager::Promo::Test"; + return "Test"; case promos_manager::Promo::DefaultBrowser: - return "promos_manager::Promo::DefaultBrowser"; + return "DefaultBrowser"; case promos_manager::Promo::AppStoreRating: - return "promos_manager::Promo::AppStoreRating"; + return "AppStoreRating"; case promos_manager::Promo::CredentialProviderExtension: - return "promos_manager::Promo::CredentialProviderExtension"; + return "CredentialProviderExtension"; case promos_manager::Promo::PostRestoreSignInFullscreen: - return "promos_manager::Promo::PostRestoreSignInFullscreen"; + return "PostRestoreSignInFullscreen"; case promos_manager::Promo::PostRestoreSignInAlert: - return "promos_manager::Promo::PostRestoreSignInAlert"; + return "PostRestoreSignInAlert"; case promos_manager::Promo::WhatsNew: - return "promos_manager::Promo::WhatsNew"; + return "WhatsNew"; } } diff --git a/ios/chrome/browser/promos_manager/constants.h b/ios/chrome/browser/promos_manager/constants.h index 8aa30c6702a23..06278a5214227 100644 --- a/ios/chrome/browser/promos_manager/constants.h +++ b/ios/chrome/browser/promos_manager/constants.h @@ -71,7 +71,10 @@ struct Impression { }; // Returns string representation of promos_manager::Promo `promo`. -base::StringPiece NameForPromo(Promo promo); +std::string NameForPromo(Promo promo); + +// Returns a string representation of the short name for the provided `promo`. +base::StringPiece ShortNameForPromo(Promo promo); // Returns promos_manager::Promo for string `promo`. absl::optional PromoForName(base::StringPiece promo); diff --git a/ios/chrome/browser/promos_manager/promos_manager_impl.mm b/ios/chrome/browser/promos_manager/promos_manager_impl.mm index d25e70fede57c..4ffb9be589701 100644 --- a/ios/chrome/browser/promos_manager/promos_manager_impl.mm +++ b/ios/chrome/browser/promos_manager/promos_manager_impl.mm @@ -52,7 +52,7 @@ void ConditionallyAppendPromoToPrefList(promos_manager::Promo promo, ScopedListPrefUpdate update(local_state, pref_path); - base::StringPiece promo_name = promos_manager::NameForPromo(promo); + std::string promo_name = promos_manager::NameForPromo(promo); // Erase `promo_name` if it already exists in `active_promos`; avoid polluting // `active_promos` with duplicate `promo_name` entries. @@ -170,7 +170,7 @@ ScopedListPrefUpdate update(local_state_, // update the pending promos saved in pref. ScopedDictPrefUpdate pending_promos_update( local_state_, prefs::kIosPromosManagerSingleDisplayPendingPromos); - base::StringPiece promo_name = promos_manager::NameForPromo(promo); + std::string promo_name = promos_manager::NameForPromo(promo); base::Time becomes_active_time = clock_->Now() + becomes_active_after_period; pending_promos_update->Set(promo_name, base::TimeToValue(becomes_active_time)); @@ -190,7 +190,7 @@ ScopedListPrefUpdate single_display_promos_update( ScopedDictPrefUpdate pending_promos_update( local_state_, prefs::kIosPromosManagerSingleDisplayPendingPromos); - base::StringPiece promo_name = promos_manager::NameForPromo(promo); + std::string promo_name = promos_manager::NameForPromo(promo); // Erase `promo_name` from the single-display and continuous-display active // promos lists. diff --git a/ios/chrome/browser/ui/promos_manager/BUILD.gn b/ios/chrome/browser/ui/promos_manager/BUILD.gn index e9aa5babd1eab..0e173a712ed35 100644 --- a/ios/chrome/browser/ui/promos_manager/BUILD.gn +++ b/ios/chrome/browser/ui/promos_manager/BUILD.gn @@ -65,6 +65,7 @@ source_set("promos_manager") { ] deps = [ ":promos_manager_scene_agent", + "//components/crash/core/common:crash_key", "//ios/chrome/app/strings", "//ios/chrome/browser/credential_provider_promo:features", "//ios/chrome/browser/default_browser:utils", diff --git a/ios/chrome/browser/ui/promos_manager/promos_manager_coordinator.mm b/ios/chrome/browser/ui/promos_manager/promos_manager_coordinator.mm index 024cf3697ddc6..38119718b9795 100644 --- a/ios/chrome/browser/ui/promos_manager/promos_manager_coordinator.mm +++ b/ios/chrome/browser/ui/promos_manager/promos_manager_coordinator.mm @@ -9,9 +9,11 @@ #import "base/check.h" #import "base/containers/small_map.h" +#import "base/debug/dump_without_crashing.h" #import "base/metrics/histogram_functions.h" #import "base/notreached.h" #import "base/strings/sys_string_conversions.h" +#import "components/crash/core/common/crash_key.h" #import "components/feature_engagement/public/tracker.h" #import "ios/chrome/app/tests_hook.h" #import "ios/chrome/browser/credential_provider_promo/features.h" @@ -207,8 +209,16 @@ - (void)displayPromo:(promos_manager::Promo)promo { return; } - CHECK(!current_promo.has_value()) << "Current promo is already set: " - << NameForPromo(current_promo.value()); + // Trying to display a promo while the previous dismissal was not communicated + // back to the promos manager. + // TODO(crbug.com/1452233): Remove once all promos dismiss themselves. + if (current_promo.has_value()) { + static crash_reporter::CrashKeyString<40> key("current-promo"); + crash_reporter::ScopedCrashKeyString crashKey( + &key, ShortNameForPromo(current_promo.value())); + base::debug::DumpWithoutCrashing(); + } + current_promo = promo; auto handler_it = _displayHandlerPromos.find(promo);