Issue 501916 : Add data type counts to profile deletion flow

chrome/browser/profiles/profile_statistics.cc

Index: chrome/browser/profiles/profile_statistics.cc
diff --git a/chrome/browser/profiles/profile_statistics.cc b/chrome/browser/profiles/profile_statistics.cc
new file mode 100644
index 0000000000000000000000000000000000000000..857138b3d511cb06fb0d884a8a77410d746abbec
--- /dev/null
+++ b/chrome/browser/profiles/profile_statistics.cc
@@ -0,0 +1,195 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/profiles/profile_statistics.h"
+
+#include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/prefs/pref_service.h"
+#include "base/task_runner.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/password_manager/password_store_factory.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/password_manager/core/browser/password_store.h"
+#include "components/password_manager/core/browser/password_store_consumer.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+namespace profiles {
+
+class ProfileStatisticsAggregator
+    : public base::RefCountedThreadSafe<ProfileStatisticsAggregator> {
+  // This class collect statistical information about the profile and returns
+  // the information via a callback function. Currently bookmarks, history,
+  // logins and preferences are counted.
+  //
+  // The class is RefCounted because this is needed for CancelableTaskTracker
+  // to function properly. Once all tasks are run (or cancelled) the instance is
+  // automatically destructed.
+  //
+  // The class is used internally by GetProfileStatistics function.
+
+ public:
+  // Constructor
+  explicit ProfileStatisticsAggregator(Profile* profile,
+      const ProfileStatisticsCallback& callback,
+      base::CancelableTaskTracker* tracker);
+
+ private:
+  // Destructor
+  friend class base::RefCountedThreadSafe<ProfileStatisticsAggregator>;
+  ~ProfileStatisticsAggregator() {}
+
+  // Private variables
+  Profile* profile_;
+  ProfileStatisticsValues profile_stats_values_;
+
+  // Callback function to be called when results arrive. Will be called
+  // multiple times (once for each statistics).
+  const ProfileStatisticsCallback callback_;
+
+  base::CancelableTaskTracker* tracker_;
+
+  // Initialization. Called by constructors.
+  void Init();
+
+  // Internal callback. Appends result to profile_stats_values_, and then
+  // call the external callback.
+  void StatisticsCallback(std::string category, int count);
+
+  // Bookmark counting.
+  static int CountURLsFromNode(const bookmarks::BookmarkNode* node);
+  int CountURLs() const;
+
+  // Preference counting.
+  int CountPrefs() const;
+
+  // Password counting
+  class PasswordStoreConsumerHelper
+      : public password_manager::PasswordStoreConsumer {
+   public:
+    explicit PasswordStoreConsumerHelper(ProfileStatisticsAggregator* parent)
+        : parent_(parent) {}

[Mike Lerman, 2015/08/05 19:26:09]

nit: add a newline below

[lwchkg, 2015/08/06 04:32:58]

Acknowledged.

+    void OnGetPasswordStoreResults(
+        ScopedVector<autofill::PasswordForm> results) override {
+      parent_->StatisticsCallback("Passwords", results.size());
+    }
+   private:

[Mike Lerman, 2015/08/05 19:26:09]

newline above private. Also, DISALLOW_COPY_AND_ASSIGN.

[lwchkg, 2015/08/06 04:32:58]

Acknowledged.

+    ProfileStatisticsAggregator* parent_;
+  };
+  PasswordStoreConsumerHelper password_store_consumer_helper_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileStatisticsAggregator);
+};
+
+ProfileStatisticsAggregator::ProfileStatisticsAggregator(Profile* profile,
+    const ProfileStatisticsCallback& callback,
+    base::CancelableTaskTracker* tracker)
+    : profile_(profile), callback_(callback), tracker_(tracker),

[Mike Lerman, 2015/08/05 19:26:09]

nit: 1 initialization per line

[lwchkg, 2015/08/06 04:32:58]

Acknowledged.

+      password_store_consumer_helper_(this) {
+  Init();
+}
+
+void ProfileStatisticsAggregator::Init() {
+  // Initiate bookmark counting (async). Post to UI thread.
+  tracker_->PostTaskAndReplyWithResult(
+    BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI).get(),
+        FROM_HERE,
+        base::Bind(&ProfileStatisticsAggregator::CountURLs, this),
+        base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
+                   this, "Bookmarks"));

[Mike Lerman, 2015/08/05 19:26:09]

all these string parameters to StatisticsCallback should be using constants that
are defined... perhaps in this class's header file.

[lwchkg, 2015/08/06 04:32:58]

Acknowledged.

+
+  // Initiate history counting (async).
+  history::HistoryService* history_service =
+      HistoryServiceFactory::GetForProfileWithoutCreating(profile_);
+
+  if (history_service) {
+    history_service->CountURLs(
+        base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
+                   this, "BrowsingHistory"),
+        tracker_);
+  } else {
+    StatisticsCallback("BrowsingHistory", 0);

[Mike Lerman, 2015/08/05 19:26:09]

If there's no history service, does that necessarily mean there's 0 browsing
history, or perhaps just that the service object hasn't been created yet? 
Perhaps just omit the entry in this case? (e.g. don't have an else condition)

[lwchkg, 2015/08/06 04:32:58]

For chrome://user-manager the absence of history is an exception. Maybe a return
value of "-1" is preferred.
(Note: the same applies to the bookmarks and preferences.)

However, I wonder if there is/will be a case where history is not present.

[Mike Lerman, 2015/08/07 19:37:12]

There are a variety of strange profiles - while you can't currently logically
apply the ProfileStatisticsAggregator to the Guest Profile (since you can't
explicitly delete it), it likely has no HistoryService. Also ephemeral profiles
may not have something like a HistoryService, and could be explicitly
deleteable. So yes, let's consider the case when the HistoryService isn't
present.

I would lean away from having to handle a special value ("-1" or otherwise). I'm
okay with either leaving this else condition in if you'd like to keep it, or
removing it.

[lwchkg, 2015/08/10 12:03:37]

I see. Then I'd like to keep it here (and everywhere else in other statistics.)
There is a consideration: the callback is called async. If a return of some
irrelevant statistics is made, the caller is safe to ignore that value. But if
the caller is expecting certain statistics which is silently dropped, then the
caller can only wait indefinitely or resort to timeout.

I also want to consider future cases that ProfileStatisticsAggregator is not
used for deletion, e.g. having the values passed to profileInfoCache. In this
case there is an advantage using special values (e.g. -1 for error): nonexistent
statistics values can be dropped if return < 0. If "0" is used there is no way
for the caller to tell the statistics exists or not.

[Mike Lerman, 2015/08/13 14:56:33]

My preference, rather than using special values, would be to add a parameter to
StatisticsCallback that conveys the intended meaning, or to change the parameter
type to a struct (containing an int and a bool). In this case, a parameter "bool
success" would likely accomplish our needs, but if we need an enum or other
construct/parameters that's fine too.

I acknowledge this is more verbose and cumbersome, but special values require
constant handling of special cases and attention, and it's easy for future
programmers to make mistakes.

[lwchkg, 2015/08/13 16:20:17]

Sounds good to me. I'll submit a revision tomorrow.

+  }
+
+  // Initiate stored password counting (async).
+  // TODO(lwchkg): make password task cancellable.

[Mike Lerman, 2015/08/05 19:26:09]

I think all TODOs should be assigned to @chromium addresses. Please put
anthonyvd@.

[lwchkg, 2015/08/06 04:32:58]

Acknowledged. Anyway, by design password tasks are cancellable. It is not here
because the internal CancelableTaskTracker cannot be overridden.

+  scoped_refptr<password_manager::PasswordStore> password_store =
+    PasswordStoreFactory::GetForProfile(
+        profile_, ServiceAccessType::EXPLICIT_ACCESS);
+  password_store->GetAutofillableLogins(&password_store_consumer_helper_);
+
+  // Initiate preference counting (async). Post to UI thread.
+  tracker_->PostTaskAndReplyWithResult(
+    BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI).get(),
+        FROM_HERE,
+        base::Bind(&ProfileStatisticsAggregator::CountPrefs, this),
+        base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
+                   this, "Settings"));
+}
+
+void ProfileStatisticsAggregator::StatisticsCallback(std::string category,
+                                                  int count) {
+  profile_stats_values_.push_back(std::make_pair(category, count));
+  callback_.Run(profile_stats_values_);
+}
+
+int ProfileStatisticsAggregator::CountURLsFromNode(
+    const bookmarks::BookmarkNode* node) {
+  int count = 0;
+  if (node->is_url()) {
+    ++count;
+  } else {
+    for (int i = 0; i < node->child_count(); ++i)
+      count += CountURLsFromNode(node->GetChild(i));
+  }
+  return count;
+}
+
+int ProfileStatisticsAggregator::CountURLs() const {

[Mike Lerman, 2015/08/05 19:26:09]

since this method counts bookmarks, could we call it CountBookmarks?

[lwchkg, 2015/08/06 04:32:58]

Yes. Let me change the name.

+  bookmarks::BookmarkModel* bookmark_model =
+    BookmarkModelFactory::GetForProfileIfExists(profile_);
+
+  if (bookmark_model) {
+    return CountURLsFromNode(bookmark_model->bookmark_bar_node()) +
+           CountURLsFromNode(bookmark_model->other_node()) +
+           CountURLsFromNode(bookmark_model->mobile_node());
+  } else {
+    return 0;
+  }
+}
+
+int ProfileStatisticsAggregator::CountPrefs() const {
+  const PrefService* pref_service = profile_->GetPrefs();
+
+  if (!pref_service)
+    return 0;
+
+  scoped_ptr<base::DictionaryValue> prefs =
+      pref_service->GetPreferenceValuesWithoutPathExpansion();
+
+  int count = 0;
+  for (base::DictionaryValue::Iterator it(*(prefs.get()));
+       !it.IsAtEnd(); it.Advance()) {
+    const PrefService::Preference* pref = pref_service->
+                                              FindPreference(it.key());
+    // Skip all dictionaries (which must be empty by the function call above).
+    if (it.value().GetType() != base::Value::TYPE_DICTIONARY &&
+      pref && pref->IsUserControlled() && !pref->IsDefaultValue()) {
+      ++count;
+    }
+  }
+  return count;
+}
+
+void GetProfileStatistics(Profile* profile,
+    const ProfileStatisticsCallback& callback,
+    base::CancelableTaskTracker* tracker) {
+  new ProfileStatisticsAggregator(profile, callback, tracker);
+}
+
+}  // namespace profiles