Index: components/doodle/doodle_service.cc |
diff --git a/components/doodle/doodle_service.cc b/components/doodle/doodle_service.cc |
index 12a56200bb613a7a9dae0f583c444d2082489220..4fe7deaa11d9960aa2f95b8ace94b9de6e98e6d4 100644 |
--- a/components/doodle/doodle_service.cc |
+++ b/components/doodle/doodle_service.cc |
@@ -7,6 +7,7 @@ |
#include <utility> |
#include "base/bind.h" |
+#include "base/metrics/histogram_macros.h" |
#include "base/time/time.h" |
#include "base/values.h" |
#include "components/doodle/pref_names.h" |
@@ -16,6 +17,41 @@ |
namespace doodle { |
+namespace { |
+ |
+// Note: Keep in sync with the corresponding enum in histograms.xml. Never |
+// remove values, and only insert new values at the end. |
+enum DoodleConfigDownloadOutcome { |
+ OUTCOME_NEW_DOODLE, |
+ OUTCOME_REVALIDATED_DOODLE, |
+ OUTCOME_CHANGED_DOODLE, |
+ OUTCOME_NO_DOODLE, |
+ OUTCOME_EXPIRED, |
+ OUTCOME_DOWNLOAD_ERROR, |
+ OUTCOME_PARSING_ERROR, |
+ // Insert new values here! |
+ OUTCOME_COUNT |
+}; |
+ |
+bool DownloadOutcomeIsSuccess(DoodleConfigDownloadOutcome outcome) { |
+ switch (outcome) { |
+ case OUTCOME_NEW_DOODLE: |
+ case OUTCOME_REVALIDATED_DOODLE: |
+ case OUTCOME_CHANGED_DOODLE: |
+ case OUTCOME_NO_DOODLE: |
+ return true; |
+ case OUTCOME_EXPIRED: |
+ case OUTCOME_DOWNLOAD_ERROR: |
+ case OUTCOME_PARSING_ERROR: |
+ return false; |
+ case OUTCOME_COUNT: |
+ NOTREACHED(); |
+ } |
+ return false; |
+} |
+ |
+} // namespace |
+ |
// static |
void DoodleService::RegisterProfilePrefs(PrefRegistrySimple* pref_registry) { |
pref_registry->RegisterDictionaryPref(prefs::kCachedConfig, |
@@ -28,21 +64,27 @@ void DoodleService::RegisterProfilePrefs(PrefRegistrySimple* pref_registry) { |
DoodleService::DoodleService(PrefService* pref_service, |
std::unique_ptr<DoodleFetcher> fetcher, |
std::unique_ptr<base::OneShotTimer> expiry_timer, |
- std::unique_ptr<base::Clock> clock) |
+ std::unique_ptr<base::Clock> clock, |
+ std::unique_ptr<base::TickClock> tick_clock) |
: pref_service_(pref_service), |
fetcher_(std::move(fetcher)), |
expiry_timer_(std::move(expiry_timer)), |
- clock_(std::move(clock)) { |
+ clock_(std::move(clock)), |
+ tick_clock_(std::move(tick_clock)) { |
DCHECK(pref_service_); |
DCHECK(fetcher_); |
DCHECK(expiry_timer_); |
DCHECK(clock_); |
+ DCHECK(tick_clock_); |
base::Time expiry_date = base::Time::FromInternalValue( |
pref_service_->GetInt64(prefs::kCachedConfigExpiry)); |
base::Optional<DoodleConfig> config = DoodleConfig::FromDictionary( |
*pref_service_->GetDictionary(prefs::kCachedConfig), base::nullopt); |
- UpdateCachedConfig(expiry_date - clock_->Now(), config); |
+ DoodleState state = |
+ config.has_value() ? DoodleState::AVAILABLE : DoodleState::NO_DOODLE; |
+ UpdateCachedConfig(/*download_time=*/base::nullopt, state, |
+ expiry_date - clock_->Now(), config); |
} |
DoodleService::~DoodleService() = default; |
@@ -58,18 +100,23 @@ void DoodleService::RemoveObserver(Observer* observer) { |
} |
void DoodleService::Refresh() { |
- fetcher_->FetchDoodle( |
- base::BindOnce(&DoodleService::DoodleFetched, base::Unretained(this))); |
+ fetcher_->FetchDoodle(base::BindOnce(&DoodleService::DoodleFetched, |
+ base::Unretained(this), |
+ tick_clock_->NowTicks())); |
} |
void DoodleService::DoodleFetched( |
+ base::TimeTicks start_time, |
DoodleState state, |
base::TimeDelta time_to_live, |
const base::Optional<DoodleConfig>& doodle_config) { |
- UpdateCachedConfig(time_to_live, doodle_config); |
+ base::TimeDelta download_time = tick_clock_->NowTicks() - start_time; |
+ UpdateCachedConfig(download_time, state, time_to_live, doodle_config); |
} |
void DoodleService::UpdateCachedConfig( |
+ base::Optional<base::TimeDelta> download_time, |
+ DoodleState state, |
base::TimeDelta time_to_live, |
const base::Optional<DoodleConfig>& doodle_config) { |
fhorschig
2017/03/21 15:26:27
80 lines and nested ifs make this really hard to r
|
// Handle the case where the new config is already expired. |
@@ -77,18 +124,26 @@ void DoodleService::UpdateCachedConfig( |
const base::Optional<DoodleConfig>& new_config = |
expired ? base::nullopt : doodle_config; |
+ DoodleConfigDownloadOutcome outcome = OUTCOME_COUNT; |
+ |
// If the config changed, update our cache and notify observers. |
// Note that this checks both for existence changes as well as changes of the |
// configs themselves. |
if (cached_config_ != new_config) { |
fhorschig
2017/03/21 15:26:27
Can we consider putting this branch into a new fun
Marc Treib
2017/03/21 16:10:45
Hm. Maaaybe there's a sensible way to split this u
|
+ bool had_config_before = cached_config_.has_value(); |
+ |
cached_config_ = new_config; |
if (cached_config_.has_value()) { |
+ outcome = had_config_before ? OUTCOME_CHANGED_DOODLE : OUTCOME_NEW_DOODLE; |
+ |
pref_service_->Set(prefs::kCachedConfig, *cached_config_->ToDictionary()); |
base::Time expiry_date = clock_->Now() + time_to_live; |
pref_service_->SetInt64(prefs::kCachedConfigExpiry, |
expiry_date.ToInternalValue()); |
} else { |
+ outcome = OUTCOME_NO_DOODLE; |
+ |
pref_service_->ClearPref(prefs::kCachedConfig); |
pref_service_->ClearPref(prefs::kCachedConfigExpiry); |
} |
@@ -96,6 +151,39 @@ void DoodleService::UpdateCachedConfig( |
for (auto& observer : observers_) { |
observer.OnDoodleConfigUpdated(cached_config_); |
} |
+ } else { |
+ outcome = cached_config_.has_value() ? OUTCOME_REVALIDATED_DOODLE |
+ : OUTCOME_NO_DOODLE; |
+ } |
+ |
+ // If the update came from the network, record metrics. |
+ DCHECK(outcome != OUTCOME_COUNT); |
fhorschig
2017/03/21 15:26:27
Is there any chance this couldn't have happened?
Marc Treib
2017/03/21 16:10:46
Nope, hence the DCHECK.
|
+ if (download_time.has_value()) { |
+ // Determine the final outcome. ERROR or EXPIRED override NO_DOODLE. |
fhorschig
2017/03/21 15:26:27
So this complete branch is basically dead for one
Marc Treib
2017/03/21 16:10:45
Good point.
fhorschig
2017/03/21 16:41:34
Not dead but very unused for one call.
that's onl
Marc Treib
2017/03/21 16:48:59
Pulled it all out into a separate DetermineDownloa
|
+ switch (state) { |
+ case DoodleState::AVAILABLE: |
+ case DoodleState::NO_DOODLE: |
+ if (expired) { |
+ outcome = OUTCOME_EXPIRED; |
+ } |
+ break; |
+ |
+ case DoodleState::DOWNLOAD_ERROR: |
+ outcome = OUTCOME_DOWNLOAD_ERROR; |
+ break; |
+ |
+ case DoodleState::PARSING_ERROR: |
+ outcome = OUTCOME_PARSING_ERROR; |
+ break; |
+ } |
+ |
+ UMA_HISTOGRAM_ENUMERATION("Doodle.ConfigDownloadOutcome", outcome, |
+ OUTCOME_COUNT); |
+ |
+ if (DownloadOutcomeIsSuccess(outcome)) { |
+ UMA_HISTOGRAM_MEDIUM_TIMES("Doodle.ConfigDownloadTime", |
+ download_time.value()); |
+ } |
} |
// Even if the configs are identical, the time-to-live might have changed. |
@@ -111,7 +199,8 @@ void DoodleService::UpdateCachedConfig( |
void DoodleService::DoodleExpired() { |
DCHECK(cached_config_.has_value()); |
- UpdateCachedConfig(base::TimeDelta(), base::nullopt); |
+ UpdateCachedConfig(/*download_time=*/base::nullopt, DoodleState::NO_DOODLE, |
+ base::TimeDelta(), base::nullopt); |
} |
} // namespace doodle |