Skip to content

Commit

Permalink
Revert "Add experiment to exercise AffiliationService with dummy data…
Browse files Browse the repository at this point in the history 
…, plus add related UMA histograms."

This reverts commit c096020.
B=479841
TBR=engedy
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review URL: https://codereview.chromium.org/1065773006

Cr-Commit-Position: refs/heads/master@{#326373}
  • Loading branch information
oysteine authored and Commit bot committed Apr 22, 2015
1 parent 76e98b5 commit c53c10a
Show file tree
Hide file tree
Showing 9 changed files with 4 additions and 352 deletions.
98 changes: 0 additions & 98 deletions components/password_manager/core/browser/affiliated_match_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@

#include "base/bind.h"
#include "base/callback.h"
#include "base/command_line.h"
#include "base/metrics/histogram_macros.h"
#include "base/single_thread_task_runner.h"
#include "base/thread_task_runner_handle.h"
#include "components/autofill/core/common/password_form.h"
Expand All @@ -17,34 +15,6 @@ namespace password_manager {

namespace {

// Dummy Android facet URIs for which affiliations will be fetched as part of an
// experiment to exercise the AffiliationService code in the wild, before users
// would get a chance to have real Android credentials saved.
// Note: although somewhat redundant, the URLs are listed explicitly so that
// they are easy to find in code search if someone wonders why they are fetched.
const char* kDummyAndroidFacetURIs[] = {
"android://oEOFeXmqYvBlkpl3gJlItdIzb59KFnmFGuc1eHFQcIKpEWQuV2X4L7GYkRtdqTi_"
"g9YvgKFAXew3rMDjeAkWVA==@com.example.one",
"android://oEOFeXmqYvBlkpl3gJlItdIzb59KFnmFGuc1eHFQcIKpEWQuV2X4L7GYkRtdqTi_"
"g9YvgKFAXew3rMDjeAkWVA==@com.example.two",
"android://oEOFeXmqYvBlkpl3gJlItdIzb59KFnmFGuc1eHFQcIKpEWQuV2X4L7GYkRtdqTi_"
"g9YvgKFAXew3rMDjeAkWVA==@com.example.twoprime",
"android://oEOFeXmqYvBlkpl3gJlItdIzb59KFnmFGuc1eHFQcIKpEWQuV2X4L7GYkRtdqTi_"
"g9YvgKFAXew3rMDjeAkWVA==@com.example.three",
"android://oEOFeXmqYvBlkpl3gJlItdIzb59KFnmFGuc1eHFQcIKpEWQuV2X4L7GYkRtdqTi_"
"g9YvgKFAXew3rMDjeAkWVA==@com.example.four",
"android://oEOFeXmqYvBlkpl3gJlItdIzb59KFnmFGuc1eHFQcIKpEWQuV2X4L7GYkRtdqTi_"
"g9YvgKFAXew3rMDjeAkWVA==@com.example.fourprime"};

// Dummy Web facet URIs for the same purpose. The URIs with the same numbers are
// in the same equivalence class.
const char* kDummyWebFacetURIs[] = {"https://one.example.com",
"https://two.example.com",
"https://three.example.com",
"https://threeprime.example.com",
"https://four.example.com",
"https://fourprime.example.com"};

// Returns whether or not |form| represents a credential for an Android
// application, and if so, returns the |facet_uri| of that application.
bool IsAndroidApplicationCredential(const autofill::PasswordForm& form,
Expand Down Expand Up @@ -187,29 +157,6 @@ void AffiliatedMatchHelper::CompleteGetAffiliatedWebRealms(
result_callback.Run(affiliated_realms);
}

void AffiliatedMatchHelper::VerifyAffiliationsForDummyFacets(
VerificationTiming timing) {
DCHECK(affiliation_service_);
for (const char* web_facet_uri : kDummyWebFacetURIs) {
// If affiliation for the Android facets has successfully been prefetched,
// then cache-restricted queries into affiliated Web facets should succeed.
affiliation_service_->GetAffiliations(
FacetURI::FromCanonicalSpec(web_facet_uri),
AffiliationService::StrategyOnCacheMiss::FAIL,
base::Bind(&OnRetrievedAffiliationResultsForDummyWebFacets, timing));
}
}

void AffiliatedMatchHelper::ScheduleVerifyAffiliationsForDummyFacets(
base::Timer* timer,
base::TimeDelta delay,
VerificationTiming timing) {
timer->Start(
FROM_HERE, delay,
base::Bind(&AffiliatedMatchHelper::VerifyAffiliationsForDummyFacets,
base::Unretained(this), timing));
}

void AffiliatedMatchHelper::OnLoginsChanged(
const PasswordStoreChangeList& changes) {
for (const PasswordStoreChange& change : changes) {
Expand All @@ -231,51 +178,6 @@ void AffiliatedMatchHelper::OnGetPasswordStoreResults(
if (IsAndroidApplicationCredential(*form, &facet_uri))
affiliation_service_->Prefetch(facet_uri, base::Time::Max());
}

// If the respective experiment is enabled, test prefetching affiliation data
// for dummy Android facet URIs to discover potenial issues in the wild, even
// before users would get a chance to have real Android credentials saved.
if (password_manager::IsAffiliationRequestsForDummyFacetsEnabled(
*base::CommandLine::ForCurrentProcess())) {
for (const char* android_facet_uri : kDummyAndroidFacetURIs) {
affiliation_service_->Prefetch(
FacetURI::FromCanonicalSpec(android_facet_uri), base::Time::Max());
}
ScheduleVerifyAffiliationsForDummyFacets(&on_startup_verification_timer_,
base::TimeDelta::FromMinutes(1),
VerificationTiming::ON_STARTUP);
ScheduleVerifyAffiliationsForDummyFacets(&repeated_verification_timer_,
base::TimeDelta::FromHours(1),
VerificationTiming::PERIODIC);
}
}

// static
void AffiliatedMatchHelper::OnRetrievedAffiliationResultsForDummyWebFacets(
VerificationTiming timing,
const AffiliatedFacets& results,
bool success) {
if (timing == AffiliatedMatchHelper::VerificationTiming::ON_STARTUP) {
UMA_HISTOGRAM_BOOLEAN(
"PasswordManager.AffiliationDummyData.RequestSuccess.OnStartup",
success);
if (success) {
UMA_HISTOGRAM_COUNTS_100(
"PasswordManager.AffiliationDummyData.RequestResultCount.OnStartup",
results.size());
}
} else if (timing == AffiliatedMatchHelper::VerificationTiming::PERIODIC) {
UMA_HISTOGRAM_BOOLEAN(
"PasswordManager.AffiliationDummyData.RequestSuccess.Periodic",
success);
if (success) {
UMA_HISTOGRAM_COUNTS_100(
"PasswordManager.AffiliationDummyData.RequestResultCount.Periodic",
results.size());
}
} else {
NOTREACHED();
}
}

} // namespace password_manager
29 changes: 0 additions & 29 deletions components/password_manager/core/browser/affiliated_match_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/scoped_vector.h"
#include "base/timer/timer.h"
#include "components/password_manager/core/browser/affiliation_utils.h"
#include "components/password_manager/core/browser/password_store.h"
#include "components/password_manager/core/browser/password_store_consumer.h"
Expand Down Expand Up @@ -109,10 +108,6 @@ class AffiliatedMatchHelper : public PasswordStore::Observer,
static const int64 kInitializationDelayOnStartupInSeconds = 8;

private:
// Indicates the time at which verifying that affiliation information has been
// correctly prefetched for dummy Android applications takes place.
enum class VerificationTiming { ON_STARTUP, PERIODIC };

// Reads all autofillable credentials from the password store and starts
// observing the store for future changes.
void DoDeferredInitialization();
Expand All @@ -134,26 +129,6 @@ class AffiliatedMatchHelper : public PasswordStore::Observer,
const AffiliatedFacets& results,
bool success);

// Called either shortly after startup or periodically in the steady state (as
// indicated by |timing|) to verify that affiliation information has been
// correctly prefetched for the dummy Android applications. Will record UMA
// histograms using a appropriate suffix based on |timing|.
void VerifyAffiliationsForDummyFacets(VerificationTiming timing);

// Sets up the given |timer| to call VerifyAffiliationsForDummyFacets() with
// the specified |delay|, with the given |timing| designation.
void ScheduleVerifyAffiliationsForDummyFacets(base::Timer* timer,
base::TimeDelta delay,
VerificationTiming timing);

// Called back by the AffiliationService in response to requesting affiliation
// information for the dummy Web facets. The |timing| indicates if the check
// was performed shortly after startup or periodically in the steady state.
static void OnRetrievedAffiliationResultsForDummyWebFacets(
VerificationTiming timing,
const AffiliatedFacets& results,
bool success);

// PasswordStore::Observer:
void OnLoginsChanged(const PasswordStoreChangeList& changes) override;

Expand All @@ -167,10 +142,6 @@ class AffiliatedMatchHelper : public PasswordStore::Observer,
// Being the sole consumer of AffiliationService, |this| owns the service.
scoped_ptr<AffiliationService> affiliation_service_;

// Timers used to schedule VerifyAffiliationsPrefetchedForDummyFacets().
base::OneShotTimer<AffiliatedMatchHelper> on_startup_verification_timer_;
base::RepeatingTimer<AffiliatedMatchHelper> repeated_verification_timer_;

base::WeakPtrFactory<AffiliatedMatchHelper> weak_ptr_factory_;

DISALLOW_COPY_AND_ASSIGN(AffiliatedMatchHelper);
Expand Down
21 changes: 0 additions & 21 deletions components/password_manager/core/browser/affiliation_backend.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include "base/bind.h"
#include "base/location.h"
#include "base/metrics/histogram_macros.h"
#include "base/single_thread_task_runner.h"
#include "base/threading/thread_checker.h"
#include "base/time/clock.h"
Expand All @@ -31,7 +30,6 @@ AffiliationBackend::AffiliationBackend(
task_runner_(task_runner),
clock_(time_source.Pass()),
tick_clock_(time_tick_source.Pass()),
construction_time_(clock_->Now()),
weak_ptr_factory_(this) {
DCHECK_LT(base::Time(), clock_->Now());
}
Expand Down Expand Up @@ -227,28 +225,9 @@ bool AffiliationBackend::OnCanSendNetworkRequest() {
fetcher_.reset(AffiliationFetcher::Create(request_context_getter_.get(),
requested_facet_uris, this));
fetcher_->StartRequest();
ReportStatistics(requested_facet_uris.size());
return true;
}

void AffiliationBackend::ReportStatistics(size_t requested_facet_uri_count) {
UMA_HISTOGRAM_COUNTS_100("PasswordManager.AffiliationBackend.FetchSize",
requested_facet_uri_count);

if (last_request_time_.is_null()) {
base::TimeDelta delay = clock_->Now() - construction_time_;
UMA_HISTOGRAM_CUSTOM_TIMES(
"PasswordManager.AffiliationBackend.FirstFetchDelay", delay,
base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(3), 50);
} else {
base::TimeDelta delay = clock_->Now() - last_request_time_;
UMA_HISTOGRAM_CUSTOM_TIMES(
"PasswordManager.AffiliationBackend.SubsequentFetchDelay", delay,
base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(3), 50);
}
last_request_time_ = clock_->Now();
}

void AffiliationBackend::SetThrottlerForTesting(
scoped_ptr<AffiliationFetchThrottler> throttler) {
throttler_ = throttler.Pass();
Expand Down
7 changes: 0 additions & 7 deletions components/password_manager/core/browser/affiliation_backend.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,6 @@ class AffiliationBackend : public FacetManagerHost,
// Returns the number of in-memory FacetManagers. Used only for testing.
size_t facet_manager_count_for_testing() { return facet_managers_.size(); }

// Reports the |requested_facet_uri_count| in a single fetch; and the elapsed
// time before the first fetch, and in-between subsequent fetches.
void ReportStatistics(size_t requested_facet_uri_count);

// To be called after Initialize() to use |throttler| instead of the default
// one. Used only for testing.
void SetThrottlerForTesting(scoped_ptr<AffiliationFetchThrottler> throttler);
Expand All @@ -131,9 +127,6 @@ class AffiliationBackend : public FacetManagerHost,
scoped_ptr<AffiliationFetcher> fetcher_;
scoped_ptr<AffiliationFetchThrottler> throttler_;

base::Time construction_time_;
base::Time last_request_time_;

// Contains a FacetManager for each facet URI that need ongoing attention. To
// save memory, managers are discarded as soon as they become redundant.
base::ScopedPtrHashMap<FacetURI, FacetManager> facet_managers_;
Expand Down
47 changes: 4 additions & 43 deletions components/password_manager/core/browser/affiliation_fetcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

#include "components/password_manager/core/browser/affiliation_fetcher.h"

#include "base/metrics/histogram_macros.h"
#include "base/metrics/sparse_histogram.h"
#include "components/password_manager/core/browser/affiliation_api.pb.h"
#include "components/password_manager/core/browser/affiliation_utils.h"
#include "components/password_manager/core/browser/test_affiliation_fetcher_factory.h"
Expand All @@ -19,37 +17,6 @@

namespace password_manager {

namespace {

// Enumeration listing the possible outcomes of fetching affiliation information
// from the Affiliation API. This is used in UMA histograms, so do not change
// existing values, only add new values at the end.
enum AffiliationFetchResult {
AFFILIATION_FETCH_RESULT_SUCCESS,
AFFILIATION_FETCH_RESULT_FAILURE,
AFFILIATION_FETCH_RESULT_MALFORMED,
AFFILIATION_FETCH_RESULT_MAX
};

// Records the given fetch |result| into the respective UMA histogram, as well
// as the response and error codes of |fetcher| if it is non-null.
void ReportStatistics(AffiliationFetchResult result,
const net::URLFetcher* fetcher) {
UMA_HISTOGRAM_ENUMERATION("PasswordManager.AffiliationFetcher.FetchResult",
result, AFFILIATION_FETCH_RESULT_MAX);
if (fetcher) {
UMA_HISTOGRAM_SPARSE_SLOWLY(
"PasswordManager.AffiliationFetcher.FetchHttpResponseCode",
fetcher->GetResponseCode());
// Network error codes are negative. See: src/net/base/net_error_list.h.
UMA_HISTOGRAM_SPARSE_SLOWLY(
"PasswordManager.AffiliationFetcher.FetchErrorCode",
-fetcher->GetStatus().error());
}
}

} // namespace

static TestAffiliationFetcherFactory* g_testing_factory = nullptr;

AffiliationFetcher::AffiliationFetcher(
Expand Down Expand Up @@ -194,21 +161,15 @@ bool AffiliationFetcher::ParseResponse(
void AffiliationFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
DCHECK_EQ(source, fetcher_.get());

// Note that invoking the |delegate_| may destroy |this| synchronously, so the
// invocation must happen last.
scoped_ptr<AffiliationFetcherDelegate::Result> result_data(
scoped_ptr<AffiliationFetcherDelegate::Result> result(
new AffiliationFetcherDelegate::Result);
if (fetcher_->GetStatus().status() == net::URLRequestStatus::SUCCESS &&
fetcher_->GetResponseCode() == net::HTTP_OK) {
if (ParseResponse(result_data.get())) {
ReportStatistics(AFFILIATION_FETCH_RESULT_SUCCESS, nullptr);
delegate_->OnFetchSucceeded(result_data.Pass());
} else {
ReportStatistics(AFFILIATION_FETCH_RESULT_MALFORMED, nullptr);
if (ParseResponse(result.get()))
delegate_->OnFetchSucceeded(result.Pass());
else
delegate_->OnMalformedResponse();
}
} else {
ReportStatistics(AFFILIATION_FETCH_RESULT_FAILURE, fetcher_.get());
delegate_->OnFetchFailed();
}
}
Expand Down
11 changes: 0 additions & 11 deletions components/password_manager/core/browser/affiliation_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,17 +320,6 @@ bool IsPropagatingPasswordChangesToWebCredentialsEnabled(
return LowerCaseEqualsASCII(update_enabled, "enabled");
}

bool IsAffiliationRequestsForDummyFacetsEnabled(
const base::CommandLine& command_line) {
const std::string synthesizing_enabled = variations::GetVariationParamValue(
kFieldTrialName, "affiliation_requests_for_dummy_facets");
if (command_line.HasSwitch(switches::kDisableAffiliationBasedMatching))
return false;
if (command_line.HasSwitch(switches::kEnableAffiliationBasedMatching))
return true;
return LowerCaseEqualsASCII(synthesizing_enabled, "enabled");
}

bool IsValidAndroidFacetURI(const std::string& url) {
FacetURI facet = FacetURI::FromPotentiallyInvalidSpec(url);
return facet.IsValidAndroidFacetURI();
Expand Down
8 changes: 0 additions & 8 deletions components/password_manager/core/browser/affiliation_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,6 @@ bool IsAffiliationBasedMatchingEnabled(const base::CommandLine& command_line);
bool IsPropagatingPasswordChangesToWebCredentialsEnabled(
const base::CommandLine& command_line);

// Returns whether or not affiliation requests for dummy facets should be
// triggered as part of an experiment to exercise AffiliationService code before
// users would get a chance to have any real Android-based credentials. If the
// main feature is forced enabled/disabled via the command line, the experiment
// is force enabled/disabled correspondingly.
bool IsAffiliationRequestsForDummyFacetsEnabled(
const base::CommandLine& command_line);

// For logging use only.
std::ostream& operator<<(std::ostream& os, const FacetURI& facet_uri);

Expand Down
Loading

0 comments on commit c53c10a

Please sign in to comment.