Skip to content

Commit

Permalink
Revert of PaswordFormManager should fetch the site stats. (patchset #5
Browse files Browse the repository at this point in the history
…id:80001 of https://codereview.chromium.org/1106153002/)

Reason for revert:
The smart bubble isn't gonna happen in M44. There is no reason to make these unnecessary requests.

Original issue's description:
> PaswordFormManager should fetch the site stats.
>
> ManagePasswordsUIController will be extended in the future to write the stats.
>
> BUG=431739
>
> Committed: https://crrev.com/0ff4c141dab88f9cca01a21b790b2a767b462ff2
> Cr-Commit-Position: refs/heads/master@{#327973}

TBR=vabr@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=431739

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

Cr-Commit-Position: refs/heads/master@{#328862}
  • Loading branch information
vasilii authored and Commit bot committed May 7, 2015
1 parent 72f602a commit 0a8e9de
Show file tree
Hide file tree
Showing 6 changed files with 12 additions and 66 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ class MockPasswordStore : public PasswordStore {
MOCK_METHOD1(FillBlacklistLogins,
bool(ScopedVector<autofill::PasswordForm>*));
MOCK_METHOD1(NotifyLoginsChanged, void(const PasswordStoreChangeList&));
MOCK_METHOD2(GetSiteStats,
void(const GURL& origin_domain,
PasswordStoreConsumer* consumer));
void AddSiteStatsImpl(const InteractionsStats& stats) override {}
void RemoveSiteStatsImpl(const GURL& origin_domain) override {}
scoped_ptr<InteractionsStats> GetSiteStatsImpl(
Expand Down
11 changes: 0 additions & 11 deletions components/password_manager/core/browser/password_form_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,6 @@ void PasswordFormManager::FetchMatchingLoginsFromPasswordStore(
return;
}
password_store->GetLogins(observed_form_, prompt_policy, this);

// The statistics is needed for the "Save password?" bubble.
password_store->GetSiteStats(observed_form_.origin.GetOrigin(), this);
}

bool PasswordFormManager::HasCompletedMatching() const {
Expand Down Expand Up @@ -534,14 +531,6 @@ void PasswordFormManager::OnGetPasswordStoreResults(
drivers_.clear();
}

void PasswordFormManager::OnGetSiteStatistics(
scoped_ptr<InteractionsStats> stats) {
// On Windows the password request may be resolved after the statistics due to
// importing from IE.
DCHECK(state_ == MATCHING_PHASE || state_ == POST_MATCHING_PHASE) << state_;
interactions_stats_.swap(stats);
}

bool PasswordFormManager::ShouldIgnoreResult(const PasswordForm& form) const {
// Don't match an invalid SSL form with one saved under secure circumstances.
if (form.ssl_valid && !observed_form_.ssl_valid)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,8 @@ class PasswordFormManager : public PasswordStoreConsumer {
// delayed until the data arrives.
void ProcessFrame(const base::WeakPtr<PasswordManagerDriver>& driver);

// PasswordStoreConsumer:
void OnGetPasswordStoreResults(
ScopedVector<autofill::PasswordForm> results) override;
void OnGetSiteStatistics(scoped_ptr<InteractionsStats> stats) override;

// A user opted to 'never remember' passwords for this form.
// Blacklist it so that from now on when it is seen we ignore it.
Expand Down Expand Up @@ -203,10 +201,6 @@ class PasswordFormManager : public PasswordStoreConsumer {
// Just need to update the internal states.
state_ = MATCHING_PHASE;
}

const InteractionsStats* interactions_stats() const {
return interactions_stats_.get();
}
#endif

const autofill::PasswordForm& observed_form() const { return observed_form_; }
Expand Down Expand Up @@ -350,9 +344,6 @@ class PasswordFormManager : public PasswordStoreConsumer {
// The PasswordForm from the page or dialog managed by |this|.
const autofill::PasswordForm observed_form_;

// Statistics for the current domain.
scoped_ptr<InteractionsStats> interactions_stats_;

// Stores provisionally saved form until |pending_credentials_| is created.
scoped_ptr<const autofill::PasswordForm> provisionally_saved_form_;
// Stores if for creating |pending_credentials_| other possible usernames
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1520,35 +1520,4 @@ TEST_F(PasswordFormManagerTest, PasswordToSave_NewValue) {
EXPECT_EQ(kNewValue, PasswordFormManager::PasswordToSave(form));
}

TEST_F(PasswordFormManagerTest, FetchStatistics) {
TestPasswordManagerClient client_with_store(mock_store());
TestPasswordManager manager(&client_with_store);
PasswordFormManager form_manager(&manager, &client_with_store,
client_with_store.driver(), *observed_form(),
false);

const PasswordStore::AuthorizationPromptPolicy auth_policy =
PasswordStore::DISALLOW_PROMPT;
InteractionsStats stats;
stats.origin_domain = observed_form()->origin.GetOrigin();
stats.nopes_count = 5;
stats.dismissal_count = 6;
EXPECT_CALL(*mock_store(),
GetLogins(*observed_form(), auth_policy, &form_manager));
EXPECT_CALL(*mock_store(),
GetSiteStats(observed_form()->origin.GetOrigin(), &form_manager));
form_manager.FetchMatchingLoginsFromPasswordStore(auth_policy);

ScopedVector<PasswordForm> simulated_results;
form_manager.OnGetPasswordStoreResults(simulated_results.Pass());
form_manager.OnGetSiteStatistics(
make_scoped_ptr(new InteractionsStats(stats)));
ASSERT_TRUE(form_manager.interactions_stats());
EXPECT_EQ(stats.origin_domain,
form_manager.interactions_stats()->origin_domain);
EXPECT_EQ(stats.nopes_count, form_manager.interactions_stats()->nopes_count);
EXPECT_EQ(stats.dismissal_count,
form_manager.interactions_stats()->dismissal_count);
}

} // namespace password_manager
14 changes: 7 additions & 7 deletions components/password_manager/core/browser/password_store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,6 @@ void PasswordStore::ReportMetrics(const std::string& sync_username,
}
}

void PasswordStore::GetSiteStats(const GURL& origin_domain,
PasswordStoreConsumer* consumer) {
scoped_ptr<GetLoginsRequest> request(new GetLoginsRequest(consumer));
ScheduleTask(base::Bind(&PasswordStore::NotifySiteStats, this, origin_domain,
base::Passed(&request)));
}

void PasswordStore::AddSiteStats(const InteractionsStats& stats) {
ScheduleTask(base::Bind(&PasswordStore::AddSiteStatsImpl, this, stats));
}
Expand All @@ -186,6 +179,13 @@ void PasswordStore::RemoveSiteStats(const GURL& origin_domain) {
base::Bind(&PasswordStore::RemoveSiteStatsImpl, this, origin_domain));
}

void PasswordStore::GetSiteStats(const GURL& origin_domain,
PasswordStoreConsumer* consumer) {
scoped_ptr<GetLoginsRequest> request(new GetLoginsRequest(consumer));
ScheduleTask(base::Bind(&PasswordStore::NotifySiteStats, this, origin_domain,
base::Passed(&request)));
}

void PasswordStore::AddObserver(Observer* observer) {
observers_->AddObserver(observer);
}
Expand Down
10 changes: 5 additions & 5 deletions components/password_manager/core/browser/password_store.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,16 @@ class PasswordStore : protected PasswordStoreSync,
virtual void ReportMetrics(const std::string& sync_username,
bool custom_passphrase_sync_enabled);

// Retrieves the statistics for |origin_domain| and notifies |consumer| on
// completion. The request will be cancelled if the consumer is destroyed.
virtual void GetSiteStats(const GURL& origin_domain,
PasswordStoreConsumer* consumer);

// Adds or replaces the statistics for the domain |stats.origin_domain|.
void AddSiteStats(const InteractionsStats& stats);

// Removes the statistics for |origin_domain|.
void RemoveSiteStats(const GURL& origin_domain);

// Retrieves the statistics for |origin_domain| and notifies |consumer| on
// completion. The request will be cancelled if the consumer is destroyed.
void GetSiteStats(const GURL& origin_domain, PasswordStoreConsumer* consumer);

// Adds an observer to be notified when the password store data changes.
void AddObserver(Observer* observer);

Expand Down Expand Up @@ -252,6 +251,7 @@ class PasswordStore : protected PasswordStoreSync,
// Synchronous implementation for manipulating with statistics.
virtual void AddSiteStatsImpl(const InteractionsStats& stats) = 0;
virtual void RemoveSiteStatsImpl(const GURL& origin_domain) = 0;
// Returns a raw pointer so that InteractionsStats can be forward declared.
virtual scoped_ptr<InteractionsStats> GetSiteStatsImpl(
const GURL& origin_domain) WARN_UNUSED_RESULT = 0;

Expand Down

0 comments on commit 0a8e9de

Please sign in to comment.