Skip to content

GitLab

T
tangled
Commit 57c29857 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot
Browse files
Options

favicons: Remove metric Favicons.LargeIconService.BlacklistedURLMismatch 

The histogram expired long ago so let's clean up the code.

Fixed: 975436
Change-Id: Idff2ed49e4b4f1e11badd781ab88c3866a009916
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2440665
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: default avatarPeter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: default avatarRobert Kaplow <rkaplow@chromium.org>
Commit-Queue: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813234}
parent 22e02ba6
Show whitespace changes
Inline Side-by-side
Showing with 6 additions and 253 deletions
components/favicon/core/large_icon_service_impl.cc
View file @ 57c29857
......@@ -8,11 +8,9 @@
#include <string>
#include "base/bind.h"
#include "base/containers/flat_map.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/memory/ref_counted.h"
#include "base/memory/singleton.h"
#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "base/strings/stringprintf.h"
......@@ -25,7 +23,6 @@
#include "components/favicon_base/favicon_util.h"
#include "components/image_fetcher/core/request_metadata.h"
#include "net/base/network_change_notifier.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "skia/ext/image_operations.h"
#include "ui/gfx/codec/png_codec.h"
#include "ui/gfx/geometry/size.h"
......@@ -53,8 +50,6 @@ const double kGoogleServerV2DesiredToMaxSizeFactor = 2.0;
const int kGoogleServerV2MinimumMaxSizeInPixel = 256;
const int kInvalidOrganizationId = -1;
GURL TrimPageUrlForGoogleServer(const GURL& page_url,
bool should_trim_page_url_path) {
if (!page_url.SchemeIsHTTPOrHTTPS() || page_url.HostIsIPAddress())
......@@ -178,77 +173,6 @@ void FinishServerRequestAsynchronously(
FROM_HERE, base::BindOnce(std::move(callback), status));
}
// Singleton map keyed by organization-identifying domain (excludes registrar
// portion, e.g. final ".com") and a value that represents an ID for the
// organization.
class DomainToOrganizationIdMap {
public:
// Returns singleton instance.
static const DomainToOrganizationIdMap* GetInstance();
// Returns a unique ID representing a known organization, or
// |kInvalidOrganizationId| in case the URL is not known.
int GetCanonicalOrganizationId(const GURL& url) const;
private:
friend struct base::DefaultSingletonTraits<const DomainToOrganizationIdMap>;
DomainToOrganizationIdMap();
~DomainToOrganizationIdMap();
// Helper function to populate the data during construction.
static base::flat_map<std::string, int> BuildData();
// Actual data.
const base::flat_map<std::string, int> data_;
DISALLOW_COPY_AND_ASSIGN(DomainToOrganizationIdMap);
};
// static
const DomainToOrganizationIdMap* DomainToOrganizationIdMap::GetInstance() {
return base::Singleton<const DomainToOrganizationIdMap>::get();
}
int DomainToOrganizationIdMap::GetCanonicalOrganizationId(
const GURL& url) const {
auto it = data_.find(LargeIconServiceImpl::GetOrganizationNameForUma(url));
return it == data_.end() ? kInvalidOrganizationId : it->second;
}
DomainToOrganizationIdMap::DomainToOrganizationIdMap() : data_(BuildData()) {}
DomainToOrganizationIdMap::~DomainToOrganizationIdMap() {}
// static
base::flat_map<std::string, int> DomainToOrganizationIdMap::BuildData() {
// Each row in the matrix below represents an organization and lists some
// known domains (not necessarily all), for the purpose of logging UMA
// metrics. The idea is that <pageUrl, iconUrl> pairs should not mix different
// rows (otherwise there is likely a bug).
const std::vector<std::vector<std::string>> kOrganizationTable = {
{"amazon", "ssl-images-amazon"},
{"cnn"},
{"espn", "espncdn"},
{"facebook", "fbcdn"},
{"google", "gstatic"},
{"live", "gfx"},
{"nytimes"},
{"twitter", "twimg"},
{"washingtonpost"},
{"wikipedia"},
{"yahoo", "yimg"},
{"youtube"},
};
base::flat_map<std::string, int> result;
for (int row = 0; row < static_cast<int>(kOrganizationTable.size()); ++row) {
for (const std::string& organization : kOrganizationTable[row]) {
result[organization] = row + 1;
}
}
return result;
}
// Processes the bitmap data returned from the FaviconService as part of a
// LargeIconService request.
class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
......@@ -265,7 +189,6 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
// ProcessIconOnBackgroundThread() so we do not perform complex image
// operations on the UI thread.
void OnIconLookupComplete(
const GURL& page_url,
const favicon_base::FaviconRawBitmapResult& db_result);
private:
......@@ -277,12 +200,6 @@ class LargeIconWorker : public base::RefCountedThreadSafe<LargeIconWorker> {
// Invoked when ProcessIconOnBackgroundThread() is done.
void OnIconProcessingComplete();
// Logs UMA metrics that reflect suspicious page-URL / icon-URL pairs, because
// we know they shouldn't be hosting their favicons in each other.
void LogSuspiciousURLMismatches(
const GURL& page_url,
const favicon_base::FaviconRawBitmapResult& db_result);
int min_source_size_in_pixel_;
int desired_size_in_pixel_;
favicon_base::LargeIconCallback raw_bitmap_callback_;
......@@ -318,9 +235,7 @@ LargeIconWorker::LargeIconWorker(
LargeIconWorker::~LargeIconWorker() {}
void LargeIconWorker::OnIconLookupComplete(
const GURL& page_url_for_uma,
const favicon_base::FaviconRawBitmapResult& db_result) {
LogSuspiciousURLMismatches(page_url_for_uma, db_result);
tracker_->PostTaskAndReply(
background_task_runner_.get(), FROM_HERE,
base::BindOnce(&ProcessIconOnBackgroundThread, db_result,
......@@ -355,26 +270,6 @@ void LargeIconWorker::OnIconProcessingComplete() {
.Run(favicon_base::LargeIconImageResult(fallback_icon_style_.release()));
}
void LargeIconWorker::LogSuspiciousURLMismatches(
const GURL& page_url_for_uma,
const favicon_base::FaviconRawBitmapResult& db_result) {
const int page_organization_id =
DomainToOrganizationIdMap::GetInstance()->GetCanonicalOrganizationId(
page_url_for_uma);
// Ignore trivial cases.
if (!db_result.is_valid() || page_organization_id == kInvalidOrganizationId)
return;
const int icon_organization_id =
DomainToOrganizationIdMap::GetInstance()->GetCanonicalOrganizationId(
db_result.icon_url);
const bool mismatch_found = page_organization_id != icon_organization_id &&
icon_organization_id != kInvalidOrganizationId;
UMA_HISTOGRAM_BOOLEAN("Favicons.LargeIconService.BlacklistedURLMismatch",
mismatch_found);
}
void ReportDownloadedSize(int size) {
UMA_HISTOGRAM_COUNTS_1000("Favicons.LargeIconService.DownloadedSize", size);
}
......@@ -502,9 +397,7 @@ LargeIconServiceImpl::GetLargeIconRawBitmapOrFallbackStyleForIconUrl(
std::max(desired_size_in_pixel, min_source_size_in_pixel);
return favicon_service_->GetRawFavicon(
icon_url, favicon_base::IconType::kFavicon, max_size_in_pixel,
base::BindOnce(&LargeIconWorker::OnIconLookupComplete, worker,
/*page_url_for_uma=*/GURL()),
tracker);
base::BindOnce(&LargeIconWorker::OnIconLookupComplete, worker), tracker);
}
base::CancelableTaskTracker::TaskId
......@@ -522,9 +415,7 @@ LargeIconServiceImpl::GetIconRawBitmapOrFallbackStyleForPageUrl(
return favicon_service_->GetRawFaviconForPageURL(
page_url, {favicon_base::IconType::kFavicon}, desired_size_in_pixel,
/*fallback_to_host=*/true,
base::BindOnce(&LargeIconWorker::OnIconLookupComplete, worker,
/*page_url_for_uma=*/GURL()),
tracker);
base::BindOnce(&LargeIconWorker::OnIconLookupComplete, worker), tracker);
}
void LargeIconServiceImpl::
......@@ -595,25 +486,6 @@ void LargeIconServiceImpl::SetServerUrlForTesting(
server_url_ = server_url_for_testing;
}
// static
std::string LargeIconServiceImpl::GetOrganizationNameForUma(const GURL& url) {
const size_t registry_length =
net::registry_controlled_domains::GetRegistryLength(
url, net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
std::string organization =
net::registry_controlled_domains::GetDomainAndRegistry(
url, net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
if (registry_length == 0 || registry_length == std::string::npos ||
registry_length >= organization.size()) {
return std::string();
}
// Strip final registry as well as the preceding dot.
organization.resize(organization.size() - registry_length - 1);
return organization;
}
base::CancelableTaskTracker::TaskId
LargeIconServiceImpl::GetLargeIconOrFallbackStyleImpl(
const GURL& page_url,
......@@ -637,8 +509,7 @@ LargeIconServiceImpl::GetLargeIconOrFallbackStyleImpl(
// URL of a large icon is known but its bitmap is not available.
return favicon_service_->GetLargestRawFaviconForPageURL(
page_url, large_icon_types_, max_size_in_pixel,
base::BindOnce(&LargeIconWorker::OnIconLookupComplete, worker, page_url),
tracker);
base::BindOnce(&LargeIconWorker::OnIconLookupComplete, worker), tracker);
}
void LargeIconServiceImpl::OnCanSetOnDemandFaviconComplete(
......
components/favicon/core/large_icon_service_impl.h
View file @ 57c29857
......@@ -79,11 +79,6 @@ class LargeIconServiceImpl : public LargeIconService {
// testing.
void SetServerUrlForTesting(const GURL& server_url_for_testing);
// Extracts the organization-identifying domain from |url| which excludes
// registrar portion (e.g. final ".com"). Used for logging UMA metrics.
// Exposed publicly for testing.
static std::string GetOrganizationNameForUma(const GURL& url);
private:
base::CancelableTaskTracker::TaskId GetLargeIconOrFallbackStyleImpl(
const GURL& page_url,
......
components/favicon/core/large_icon_service_impl_unittest.cc
View file @ 57c29857
......@@ -42,7 +42,6 @@ namespace {
using image_fetcher::MockImageFetcher;
using testing::_;
using testing::ElementsAre;
using testing::Eq;
using testing::HasSubstr;
using testing::IsEmpty;
......@@ -96,14 +95,6 @@ favicon_base::FaviconRawBitmapResult CreateTestBitmapResult(int w,
return result;
}
favicon_base::FaviconRawBitmapResult CreateTestBitmapResultWithIconUrl(
const GURL& icon_url) {
favicon_base::FaviconRawBitmapResult result =
CreateTestBitmapResult(64, 64, kTestColor);
result.icon_url = icon_url;
return result;
}
bool HasBackgroundColor(
const favicon_base::FallbackIconStyle& fallback_icon_style,
SkColor color) {
......@@ -584,96 +575,6 @@ TEST_P(LargeIconServiceGetterTest, FallbackSinceTooPicky) {
24, /*expected_count=*/1);
}
// Tests UMA metric BlacklistedURLMismatch ignores unknown page URLs.
TEST_P(LargeIconServiceGetterTest,
ShouldNotRecordUrlMismatchesForUnknownPages) {
const std::string kUmaMetricName =
"Favicons.LargeIconService.BlacklistedURLMismatch";
const GURL kUnknownPageUrl1("http://www.foo.com/path");
const GURL kUnknownPageUrl2("http://www.bar.com/path");
const GURL kUnknownPageUrl3("http://com/path");
const GURL kUnknownIconUrl1("http://www.foo.com/favicon.ico");
const GURL kUnknownIconUrl2("http://www.bar.com/favicon.ico");
const GURL kUnknownIconUrl3("http://com/favicon.ico");
const GURL kKnownIconUrl("http://www.google.com/favicon.ico");
// Only URLs in the list of known organizations contribute to the histogram,
// so neither of the sites below should be logged.
InjectMockResult(kUnknownPageUrl1,
CreateTestBitmapResultWithIconUrl(kUnknownIconUrl1));
InjectMockResult(kUnknownPageUrl3,
CreateTestBitmapResultWithIconUrl(kUnknownIconUrl3));
GetLargeIconOrFallbackStyleAndWaitForCallback(kUnknownPageUrl1, 1, 0);
GetLargeIconOrFallbackStyleAndWaitForCallback(kUnknownPageUrl3, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName), IsEmpty());
// Even if there is a mismatch, it's irrelevant if none of the URLs are known.
InjectMockResult(kUnknownPageUrl1,
CreateTestBitmapResultWithIconUrl(kUnknownIconUrl2));
GetLargeIconOrFallbackStyleAndWaitForCallback(kUnknownPageUrl1, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName), IsEmpty());
// If a unknown site uses a known icon, it's still ignored.
InjectMockResult(kUnknownPageUrl1,
CreateTestBitmapResultWithIconUrl(kKnownIconUrl));
GetLargeIconOrFallbackStyleAndWaitForCallback(kUnknownPageUrl1, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName), IsEmpty());
}
// Tests UMA metric BlacklistedURLMismatch emits records for known page URLs.
TEST_P(LargeIconServiceGetterTest, ShouldRecordUrlMismatchesForKnownPages) {
const std::string kUmaMetricName =
"Favicons.LargeIconService.BlacklistedURLMismatch";
const GURL kKnownPageUrl1("http://www.google.com/path");
const GURL kKnownPageUrl2("http://www.youtube.com/path");
const GURL kKnownIconUrl1("http://www.google.com/favicon.ico");
const GURL kKnownIconUrl2("http://www.youtube.com/favicon.ico");
const GURL kUnknownIconUrl("http://www.foo.com/favicon.ico");
// Mismatch between a known organization and an unknown one should contribute
// to bucket 0, although we're unsure if it's legit (false positives ok).
InjectMockResult(kKnownPageUrl1,
CreateTestBitmapResultWithIconUrl(kUnknownIconUrl));
GetLargeIconOrFallbackStyleAndWaitForCallback(kKnownPageUrl1, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName),
ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
// Matching pairs within known organizations should contribute to bucket 0.
InjectMockResult(kKnownPageUrl1,
CreateTestBitmapResultWithIconUrl(kKnownIconUrl1));
InjectMockResult(kKnownPageUrl2,
CreateTestBitmapResultWithIconUrl(kKnownIconUrl2));
GetLargeIconOrFallbackStyleAndWaitForCallback(kKnownPageUrl1, 1, 0);
GetLargeIconOrFallbackStyleAndWaitForCallback(kKnownPageUrl2, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName),
ElementsAre(base::Bucket(/*min=*/0, /*count=*/3)));
// Mismatch between a known organization and another known one should
// contribute to bucket 1.
InjectMockResult(kKnownPageUrl1,
CreateTestBitmapResultWithIconUrl(kKnownIconUrl2));
GetLargeIconOrFallbackStyleAndWaitForCallback(kKnownPageUrl1, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName),
ElementsAre(base::Bucket(/*min=*/0, /*count=*/3),
base::Bucket(/*min=*/1, /*count=*/1)));
}
// Tests UMA metric BlacklistedURLMismatch treats different URLs corresponding
// to the same organization as matches.
TEST_P(LargeIconServiceGetterTest, ShouldRecordMatchesDespiteDifferentUrls) {
const std::string kUmaMetricName =
"Favicons.LargeIconService.BlacklistedURLMismatch";
const GURL kKnownPageUrl("http://www.google.de/path");
const GURL kKnownIconUrl("http://www.google.com/favicon.ico");
// Matching pairs within known organizations should contribute to bucket 0.
InjectMockResult(kKnownPageUrl,
CreateTestBitmapResultWithIconUrl(kKnownIconUrl));
GetLargeIconOrFallbackStyleAndWaitForCallback(kKnownPageUrl, 1, 0);
EXPECT_THAT(histogram_tester_.GetAllSamples(kUmaMetricName),
ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
}
// Every test will appear with suffix /0 (param false) and /1 (param true), e.g.
// LargeIconServiceGetterTest.FallbackSinceTooPicky/0: get image.
// LargeIconServiceGetterTest.FallbackSinceTooPicky/1: get raw bitmap.
......@@ -681,22 +582,5 @@ INSTANTIATE_TEST_SUITE_P(All, // Empty instatiation name.
LargeIconServiceGetterTest,
::testing::Values(false, true));
TEST(LargeIconServiceOrganizationNameTest, ShouldGetOrganizationNameForUma) {
EXPECT_EQ("", LargeIconServiceImpl::GetOrganizationNameForUma(GURL()));
EXPECT_EQ("",
LargeIconServiceImpl::GetOrganizationNameForUma(GURL("http://")));
EXPECT_EQ("", LargeIconServiceImpl::GetOrganizationNameForUma(GURL("com")));
EXPECT_EQ(
"", LargeIconServiceImpl::GetOrganizationNameForUma(GURL("http://com")));
EXPECT_EQ("", LargeIconServiceImpl::GetOrganizationNameForUma(
GURL("http://google")));
EXPECT_EQ("google", LargeIconServiceImpl::GetOrganizationNameForUma(
GURL("http://google.com")));
EXPECT_EQ("google", LargeIconServiceImpl::GetOrganizationNameForUma(
GURL("http://google.de")));
EXPECT_EQ("google", LargeIconServiceImpl::GetOrganizationNameForUma(
GURL("http://foo.google.com")));
}
} // namespace
} // namespace favicon
tools/metrics/histograms/histograms_xml/others/histograms.xml
View file @ 57c29857
......@@ -4764,6 +4764,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
<histogram name="Favicons.LargeIconService.BlacklistedURLMismatch"
units="BooleanError" expires_after="M77">
<obsolete>
Removed in M88.
</obsolete>
<owner>mastiz@chromium.org</owner>
<summary>
Records the number of large icons that were fetched from the local cache
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment