Skip to content

GitLab

T
tangled
Commit 5d908908 authored by Sébastien Marchand's avatar Sébastien Marchand Committed by Commit Bot
Browse files
Options

Revert "Media Engagement: Histograms for preloaded list." 

This reverts commit 3946a6ad.

Reason for revert: This is breaking the Windows build.

https://chromium-review.googlesource.com/c/chromium/src/+/803994 broke the Windows build and needs to be reverted, and as this CL is depending on it it need to be reverted too.

Original change's description:
> Media Engagement: Histograms for preloaded list.
> 
> This adds two histograms to MediaEngagementPreloadedList. The first is
> used when the list is loaded and will allow us to see if the loads are
> failing and why. The second is used when the list is checked and will
> allow us to see if the list has not been loaded or is empty, as well
> as how often the list is matching.
> 
> BUG=787464
> 
> Change-Id: I935405ef56aa197a2f32fbd85df7c9ae4851b12a
> Reviewed-on: https://chromium-review.googlesource.com/808305
> Commit-Queue: Becca Hughes <beccahughes@chromium.org>
> Reviewed-by: Jesse Doherty <jwd@chromium.org>
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#522085}

TBR=jwd@chromium.org,mlamouri@chromium.org,beccahughes@chromium.org

Change-Id: Id6f811e28c70063467a0ce1e97da59a2dc0e6c14
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 787464
Reviewed-on: https://chromium-review.googlesource.com/812044Reviewed-by: default avatarSébastien Marchand <sebmarchand@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522183}
parent 5161e612
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 253 deletions
chrome/browser/media/media_engagement_preloaded_list.cc
View file @ 5d908908
......@@ -5,49 +5,35 @@
#include "chrome/browser/media/media_engagement_preloaded_list.h"
#include "base/files/file_util.h"
#include "base/metrics/histogram_macros.h"
#include "base/path_service.h"
#include "chrome/browser/media/media_engagement_preload.pb.h"
#include "net/base/lookup_string_in_fixed_set.h"
#include "url/origin.h"
const char MediaEngagementPreloadedList::kHistogramCheckResultName[] =
"Media.Engagement.PreloadedList.CheckResult";
const char MediaEngagementPreloadedList::kHistogramLoadResultName[] =
"Media.Engagement.PreloadedList.LoadResult";
MediaEngagementPreloadedList::MediaEngagementPreloadedList() = default;
MediaEngagementPreloadedList::~MediaEngagementPreloadedList() = default;
bool MediaEngagementPreloadedList::LoadFromFile(const base::FilePath& path) {
// Check the file exists.
if (!base::PathExists(path)) {
RecordLoadResult(LoadResult::kFileNotFound);
if (!base::PathExists(path))
return false;
}
// Read the file to a string.
std::string file_data;
if (!base::ReadFileToString(path, &file_data)) {
RecordLoadResult(LoadResult::kFileReadFailed);
if (!base::ReadFileToString(path, &file_data))
return false;
}
// Load the preloaded list into a proto message.
chrome_browser_media::PreloadedData message;
if (!message.ParseFromString(file_data)) {
RecordLoadResult(LoadResult::kParseProtoFailed);
if (!message.ParseFromString(file_data))
return false;
}
// Copy data from the protobuf message.
dafsa_ = std::vector<unsigned char>(
message.dafsa().c_str(),
message.dafsa().c_str() + message.dafsa().length());
RecordLoadResult(LoadResult::kLoaded);
is_loaded_ = true;
return true;
}
......@@ -59,33 +45,7 @@ bool MediaEngagementPreloadedList::CheckOriginIsPresent(
bool MediaEngagementPreloadedList::CheckStringIsPresent(
const std::string& input) const {
// Check if we have loaded the data.
if (!loaded()) {
RecordCheckResult(CheckResult::kListNotLoaded);
return false;
}
// Check if the data is empty.
if (empty()) {
RecordCheckResult(CheckResult::kListEmpty);
return false;
}
bool result =
net::LookupStringInFixedSet(dafsa_.data(), dafsa_.size(), input.c_str(),
input.size()) == net::kDafsaFound;
// Record and return the result.
RecordCheckResult(result ? CheckResult::kFound : CheckResult::kNotFound);
return result;
}
void MediaEngagementPreloadedList::RecordLoadResult(LoadResult result) {
UMA_HISTOGRAM_ENUMERATION(kHistogramLoadResultName, result,
LoadResult::kCount);
}
void MediaEngagementPreloadedList::RecordCheckResult(CheckResult result) const {
UMA_HISTOGRAM_ENUMERATION(kHistogramCheckResultName, result,
CheckResult::kCount);
return net::LookupStringInFixedSet(dafsa_.data(), dafsa_.size(),
input.c_str(),
input.size()) == net::kDafsaFound;
}
chrome/browser/media/media_engagement_preloaded_list.h
View file @ 5d908908
......@@ -39,56 +39,9 @@ class MediaEngagementPreloadedList {
protected:
friend class MediaEngagementPreloadedListTest;
// The names of the CheckResult and LoadResult histograms.
static const char kHistogramCheckResultName[];
static const char kHistogramLoadResultName[];
// The result of the CheckStringIsPresent operation. This enum is used to
// record a histogram and should not be renumbered.
enum class CheckResult {
// The check succeeded and the string was found in the data.
kFound = 0,
// The check succeeded but the string was not found in the data.
kNotFound,
// The check failed because the list is empty.
kListEmpty,
// The check failed because the list has not been loaded.
kListNotLoaded,
kCount
};
// The result of the LoadFromFile operation. This enum is used to record
// a histogram and should not be renumbered.
enum class LoadResult {
// The list was loaded successfully.
kLoaded = 0,
// The list was not loaded because the file was not found.
kFileNotFound,
// The list was not loaded because the file could not be read.
kFileReadFailed,
// The list was not loaded because the proto stored in the file could not be
// parsed.
kParseProtoFailed,
kCount
};
// Checks if |input| is present in the preloaded data.
bool CheckStringIsPresent(const std::string& input) const;
// Records |result| to the LoadResult histogram.
void RecordLoadResult(LoadResult result);
// Records |result| to the CheckResult histogram.
void RecordCheckResult(CheckResult result) const;
private:
// The preloaded data in dafsa format.
std::vector<unsigned char> dafsa_;
......
chrome/browser/media/media_engagement_preloaded_list_unittest.cc
View file @ 5d908908
......@@ -9,7 +9,6 @@
#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/path_service.h"
#include "base/test/histogram_tester.h"
#include "build/build_config.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/test/base/testing_profile.h"
......@@ -32,9 +31,6 @@ const base::FilePath kBadFormatFilePath =
const base::FilePath kEmptyFilePath = kTestDataPath.AppendASCII("empty.pb");
const base::FilePath kFileReadFailedPath =
base::FilePath(FILE_PATH_LITERAL(".."));
base::FilePath GetModulePath() {
base::FilePath module_dir;
#if defined(OS_ANDROID)
......@@ -75,69 +71,8 @@ class MediaEngagementPreloadedListTest : public ::testing::Test {
bool IsEmpty() { return preloaded_list_->empty(); }
void ExpectCheckResultFoundCount(int count) {
ExpectCheckResultCount(MediaEngagementPreloadedList::CheckResult::kFound,
count);
}
void ExpectCheckResultNotFoundCount(int count) {
ExpectCheckResultCount(MediaEngagementPreloadedList::CheckResult::kNotFound,
count);
}
void ExpectCheckResultNotLoadedCount(int count) {
ExpectCheckResultCount(
MediaEngagementPreloadedList::CheckResult::kListNotLoaded, count);
}
void ExpectCheckResultListEmptyCount(int count) {
ExpectCheckResultCount(
MediaEngagementPreloadedList::CheckResult::kListEmpty, count);
}
void ExpectCheckResultTotal(int total) {
histogram_tester_.ExpectTotalCount(
MediaEngagementPreloadedList::kHistogramCheckResultName, total);
}
void ExpectLoadResultLoaded() {
ExpectLoadResult(MediaEngagementPreloadedList::LoadResult::kLoaded);
}
void ExpectLoadResultFileNotFound() {
ExpectLoadResult(MediaEngagementPreloadedList::LoadResult::kFileNotFound);
}
void ExpectLoadResultFileReadFailed() {
ExpectLoadResult(MediaEngagementPreloadedList::LoadResult::kFileReadFailed);
}
void ExpectLoadResultParseProtoFailed() {
ExpectLoadResult(
MediaEngagementPreloadedList::LoadResult::kParseProtoFailed);
}
protected:
void ExpectLoadResult(MediaEngagementPreloadedList::LoadResult result) {
histogram_tester_.ExpectBucketCount(
MediaEngagementPreloadedList::kHistogramLoadResultName,
static_cast<int>(result), 1);
// Ensure not other results were logged.
histogram_tester_.ExpectTotalCount(
MediaEngagementPreloadedList::kHistogramLoadResultName, 1);
}
void ExpectCheckResultCount(MediaEngagementPreloadedList::CheckResult result,
int count) {
histogram_tester_.ExpectBucketCount(
MediaEngagementPreloadedList::kHistogramCheckResultName,
static_cast<int>(result), count);
}
std::unique_ptr<MediaEngagementPreloadedList> preloaded_list_;
base::HistogramTester histogram_tester_;
};
TEST_F(MediaEngagementPreloadedListTest, CheckOriginIsPresent) {
......@@ -145,95 +80,38 @@ TEST_F(MediaEngagementPreloadedListTest, CheckOriginIsPresent) {
EXPECT_TRUE(IsLoaded());
EXPECT_FALSE(IsEmpty());
// Check the load result was recorded on the histogram.
ExpectLoadResultLoaded();
// Check some origins that are not in the list.
ExpectCheckResultTotal(0);
EXPECT_TRUE(CheckOriginIsPresent(GURL("https://example.com")));
EXPECT_TRUE(CheckOriginIsPresent(GURL("https://example.org:1234")));
EXPECT_TRUE(CheckOriginIsPresent(GURL("https://test--3ya.com")));
EXPECT_TRUE(CheckOriginIsPresent(GURL("http://123.123.123.123")));
// Check they were recorded on the histogram.
ExpectCheckResultTotal(4);
ExpectCheckResultFoundCount(4);
// Check some origins that are not in the list.
EXPECT_FALSE(CheckOriginIsPresent(GURL("https://example.org")));
EXPECT_FALSE(CheckOriginIsPresent(GURL("http://example.com")));
EXPECT_FALSE(CheckOriginIsPresent(GURL("http://123.123.123.124")));
// Check they were recorded on the histogram.
ExpectCheckResultTotal(7);
ExpectCheckResultNotFoundCount(3);
// Make sure only the full origin matches.
EXPECT_FALSE(CheckStringIsPresent("123"));
EXPECT_FALSE(CheckStringIsPresent("http"));
EXPECT_FALSE(CheckStringIsPresent("example.com"));
// Check they were recorded on the histogram.
ExpectCheckResultTotal(10);
ExpectCheckResultNotFoundCount(6);
}
TEST_F(MediaEngagementPreloadedListTest, LoadMissingFile) {
ASSERT_FALSE(LoadFromFile(GetFilePathRelativeToModule(kMissingFilePath)));
EXPECT_FALSE(IsLoaded());
EXPECT_TRUE(IsEmpty());
// Check the load result was recorded on the histogram.
ExpectLoadResultFileNotFound();
// Test checking an origin and make sure the result is recorded to the
// histogram.
EXPECT_FALSE(CheckOriginIsPresent(GURL("https://example.com")));
ExpectCheckResultTotal(1);
ExpectCheckResultNotLoadedCount(1);
}
TEST_F(MediaEngagementPreloadedListTest, LoadFileReadFailed) {
ASSERT_FALSE(LoadFromFile(kFileReadFailedPath));
EXPECT_FALSE(IsLoaded());
EXPECT_TRUE(IsEmpty());
// Check the load result was recorded on the histogram.
ExpectLoadResultFileReadFailed();
// Test checking an origin and make sure the result is recorded to the
// histogram.
EXPECT_FALSE(CheckOriginIsPresent(GURL("https://example.com")));
ExpectCheckResultTotal(1);
ExpectCheckResultNotLoadedCount(1);
}
TEST_F(MediaEngagementPreloadedListTest, LoadBadFormatFile) {
ASSERT_FALSE(LoadFromFile(GetFilePathRelativeToModule(kBadFormatFilePath)));
EXPECT_FALSE(IsLoaded());
EXPECT_TRUE(IsEmpty());
// Check the load result was recorded on the histogram.
ExpectLoadResultParseProtoFailed();
// Test checking an origin and make sure the result is recorded to the
// histogram.
EXPECT_FALSE(CheckOriginIsPresent(GURL("https://example.com")));
ExpectCheckResultTotal(1);
ExpectCheckResultNotLoadedCount(1);
}
TEST_F(MediaEngagementPreloadedListTest, LoadEmptyFile) {
ASSERT_TRUE(LoadFromFile(GetFilePathRelativeToModule(kEmptyFilePath)));
EXPECT_TRUE(IsLoaded());
EXPECT_TRUE(IsEmpty());
// Check the load result was recorded on the histogram.
ExpectLoadResultLoaded();
// Test checking an origin and make sure the result is recorded to the
// histogram.
EXPECT_FALSE(CheckOriginIsPresent(GURL("https://example.com")));
ExpectCheckResultTotal(1);
ExpectCheckResultListEmptyCount(1);
}
tools/metrics/histograms/enums.xml
View file @ 5d908908
......@@ -33859,20 +33859,6 @@ Called by update_net_trust_anchors.py.-->
<int value="2" label="VERSION_LATEST"/>
</enum>
<enum name="PreloadedListCheckResult">
<int value="0" label="string was found"/>
<int value="1" label="string was not found"/>
<int value="2" label="check failed because list was empty"/>
<int value="3" label="check failed because list was not loaded"/>
</enum>
<enum name="PreloadedListLoadResult">
<int value="0" label="loaded"/>
<int value="1" label="file not found"/>
<int value="2" label="file read failed"/>
<int value="3" label="parse proto failed"/>
</enum>
<enum name="PrerenderCookieSendType">
<obsolete>
Deprecated March 13 2015.
tools/metrics/histograms/histograms.xml
View file @ 5d908908
......@@ -33818,30 +33818,6 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
</summary>
</histogram>
<histogram name="Media.Engagement.PreloadedList.CheckResult"
enum="PreloadedListCheckResult">
<owner>beccahughes@chromium.org</owner>
<owner>media-dev@chromium.org</owner>
<summary>
Recorded when the Media Engagement Preloaded List is checked whether a
string is present on that list. If the check was successful then the result
of the check is recorded in this histogram. If the check was not successful
then the reason for the check failing is recorded.
</summary>
</histogram>
<histogram name="Media.Engagement.PreloadedList.LoadResult"
enum="PreloadedListLoadResult">
<owner>beccahughes@chromium.org</owner>
<owner>media-dev@chromium.org</owner>
<summary>
Recorded when data is loaded into the Media Engagement Preloaded List. If
the load is successful then &quot;loaded&quot; is recorded to this
histogram. If the load was not successful then the reason why is recorded to
this histogram.
</summary>
</histogram>
<histogram name="Media.Engagement.ScoreAtPlayback" units="%">
<owner>beccahughes@chromium.org</owner>
<owner>media-dev@chromium.org</owner>
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