Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(42)

Unified Diff: components/ntp_tiles/most_visited_sites.cc

Issue 2105933002: NTP: Fix metrics recording crash by plumbing the necessary data to Java. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Address treib@ comments. Created 4 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « components/ntp_tiles/most_visited_sites.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: components/ntp_tiles/most_visited_sites.cc
diff --git a/components/ntp_tiles/most_visited_sites.cc b/components/ntp_tiles/most_visited_sites.cc
index eb9246d59e5a714b58ef57e1d4bea5d4285afed1..b2ffaacc93ba0457003a2be5a2a0fc70af22570a 100644
--- a/components/ntp_tiles/most_visited_sites.cc
+++ b/components/ntp_tiles/most_visited_sites.cc
@@ -49,25 +49,6 @@ const char kPopularSitesFieldTrialName[] = "NTPPopularSites";
const base::Feature kDisplaySuggestionsServiceTiles{
"DisplaySuggestionsServiceTiles", base::FEATURE_ENABLED_BY_DEFAULT};
-// The visual type of a most visited tile.
-//
-// These values must stay in sync with the MostVisitedTileType enum
-// in histograms.xml.
-//
-// A Java counterpart will be generated for this enum.
-// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp
-enum MostVisitedTileType {
- // The icon or thumbnail hasn't loaded yet.
- NONE,
- // The item displays a site's actual favicon or touch icon.
- ICON_REAL,
- // The item displays a color derived from the site's favicon or touch icon.
- ICON_COLOR,
- // The item displays a default gray box in place of an icon.
- ICON_DEFAULT,
- NUM_TILE_TYPES,
-};
-
// Log an event for a given |histogram| at a given element |position|. This
// routine exists because regular histogram macros are cached thus can't be used
// if the name of the histogram will change at a given call site.
@@ -133,9 +114,8 @@ bool AreURLsEquivalent(const GURL& url1, const GURL& url2) {
return url1.host() == url2.host() && url1.path() == url2.path();
}
-std::string GetSourceHistogramName(
- const MostVisitedSites::Suggestion& suggestion) {
- switch (suggestion.source) {
+std::string GetSourceHistogramName(int source, int provider_index) {
+ switch (source) {
case MostVisitedSites::TOP_SITES:
return kHistogramClientName;
case MostVisitedSites::POPULAR:
@@ -143,15 +123,19 @@ std::string GetSourceHistogramName(
case MostVisitedSites::WHITELIST:
return kHistogramWhitelistName;
case MostVisitedSites::SUGGESTIONS_SERVICE:
- return suggestion.provider_index >= 0
- ? base::StringPrintf(kHistogramServerFormat,
- suggestion.provider_index)
+ return provider_index >= 0
+ ? base::StringPrintf(kHistogramServerFormat, provider_index)
: kHistogramServerName;
}
NOTREACHED();
return std::string();
}
+std::string GetSourceHistogramNameFromSuggestion(
+ const MostVisitedSites::Suggestion& suggestion) {
+ return GetSourceHistogramName(suggestion.source, suggestion.provider_index);
+}
+
void AppendSuggestions(MostVisitedSites::SuggestionsVector src,
MostVisitedSites::SuggestionsVector* dst) {
dst->insert(dst->end(),
@@ -265,15 +249,16 @@ void MostVisitedSites::AddOrRemoveBlacklistedUrl(const GURL& url,
}
void MostVisitedSites::RecordTileTypeMetrics(
- const std::vector<int>& tile_types) {
- DCHECK_EQ(current_suggestions_.size(), tile_types.size());
+ const std::vector<int>& tile_types,
+ const std::vector<int>& sources,
+ const std::vector<int>& provider_indices) {
int counts_per_type[NUM_TILE_TYPES] = {0};
for (size_t i = 0; i < tile_types.size(); ++i) {
int tile_type = tile_types[i];
++counts_per_type[tile_type];
std::string histogram = base::StringPrintf(
"NewTabPage.TileType.%s",
- GetSourceHistogramName(current_suggestions_[i]).c_str());
+ GetSourceHistogramName(sources[i], provider_indices[i]).c_str());
LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
}
@@ -286,16 +271,21 @@ void MostVisitedSites::RecordTileTypeMetrics(
}
void MostVisitedSites::RecordOpenedMostVisitedItem(int index, int tile_type) {
+ // TODO(treib): |current_suggestions_| could be updated before this function
+ // is called, leading to DCHECKs and/or memory corruption. Adjust this
+ // function to work with asynchronous UI.
DCHECK_GE(index, 0);
DCHECK_LT(index, static_cast<int>(current_suggestions_.size()));
std::string histogram = base::StringPrintf(
"NewTabPage.MostVisited.%s",
- GetSourceHistogramName(current_suggestions_[index]).c_str());
+ GetSourceHistogramNameFromSuggestion(current_suggestions_[index])
+ .c_str());
LogHistogramEvent(histogram, index, num_sites_);
histogram = base::StringPrintf(
"NewTabPage.TileTypeClicked.%s",
- GetSourceHistogramName(current_suggestions_[index]).c_str());
+ GetSourceHistogramNameFromSuggestion(current_suggestions_[index])
+ .c_str());
LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
}
@@ -570,7 +560,7 @@ void MostVisitedSites::RecordImpressionUMAMetrics() {
for (size_t i = 0; i < current_suggestions_.size(); i++) {
std::string histogram = base::StringPrintf(
"NewTabPage.SuggestionsImpression.%s",
- GetSourceHistogramName(current_suggestions_[i]).c_str());
+ GetSourceHistogramNameFromSuggestion(current_suggestions_[i]).c_str());
LogHistogramEvent(histogram, static_cast<int>(i), num_sites_);
}
}
« no previous file with comments | « components/ntp_tiles/most_visited_sites.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698