Issue 501916 : Add data type counts to profile deletion flow

chrome/browser/profiles/profile_statistics.cc

+// 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 "base/time/time.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 {
+
+struct ProfileStatValue {
+  int count;

[achuithb, 2015/10/02 23:48:36]

Add a ctor like
ProfileStatValue(int count, bool success) : count(count), success(success) {}

[lwchkg, 2015/10/09 19:36:25]

Mike and I agreed to use ProfileStatValue in the following way before. 

    ProfileStatValue result;
    result.count = 0;
    result.success = false;

See
https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile...

Adding the ctor means we need to change this into
    ProfileStatValue(0, false)
We prefer to use the first one, because the code easier to read (but at the
expense of possibility of forgetting to assign values to members).

+  bool success; // false means the statistics failed to load
+};
+
+class ProfileStatisticsAggregator
+    : public base::RefCountedThreadSafe<ProfileStatisticsAggregator> {
+  // This class collects 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.

[Roger Tawa OOO till Jul 10th, 2015/10/16 12:22:35]

Put this comment above class declaration at line 30.

[lwchkg, 2015/10/17 14:06:45]

Acknowledged.

+
+ public:
+  // Constructor

[achuithb, 2015/10/02 23:48:36]

drop this comment

[lwchkg, 2015/10/09 19:36:25]

Acknowledged.

+  explicit ProfileStatisticsAggregator(Profile* profile,
+      const profiles::ProfileStatisticsCallback& callback,
+      base::CancelableTaskTracker* tracker);

[Roger Tawa OOO till Jul 10th, 2015/10/16 12:22:35]

Don't need explicit for ctor wirh more than one arg.

[lwchkg, 2015/10/17 14:06:45]

Acknowledged.

+
+ private:
+  // Destructor
+  friend class base::RefCountedThreadSafe<ProfileStatisticsAggregator>;
+  ~ProfileStatisticsAggregator() {}
+
+  Profile* profile_;

[achuithb, 2015/10/02 23:48:36]

I think it's more common to have data members follow methods.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_...

[lwchkg, 2015/10/09 19:36:25]

Acknowledged.

+  profiles::ProfileCategoryStats profile_category_stats_;
+
+  // Callback function to be called when results arrive. Will be called
+  // multiple times (once for each statistics).
+  const profiles::ProfileStatisticsCallback callback_;

[achuithb, 2015/10/02 23:48:36]

It's a bit strange to have a const data member. We usually don't do this.

[lwchkg, 2015/10/09 19:36:25]

Agree. mlerman@: how do you think about this? ("const" was added as a response
to a comment in patch #1). See
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...

+
+  base::CancelableTaskTracker* tracker_;
+
+  // Initialization. Called by constructors.

[achuithb, 2015/10/02 23:48:36]

Drop this comment

[lwchkg, 2015/10/09 19:36:25]

Acknowledged.

+  void Init();

[achuithb, 2015/10/02 23:48:36]

What's the point of this method?

[lwchkg, 2015/10/09 19:36:25]

So far we have only one constructor, so this method looks redundant. But I don't
know whether we will have another constructor (e.g. a specially crafted
constructor for unit tests).

With Init() method these kind of changes should be easy.

+
+  // Callback functions
+  // Normal callback. Appends result to |profile_category_stats_|, and then call
+  // the external callback. All other callbacks call this function.
+  void StatisticsCallback(const char* category, ProfileStatValue result);
+  // Callback for reporting success.
+  void StatisticsCallbackSuccess(const char* category, int count);
+  // Callback for reporting failure.
+  void StatisticsCallbackFailure(const char* category);
+  // Callback for history.
+  void StatisticsCallbackHistory(history::HistoryCountResult result);
+
+  // Bookmark counting.
+  static int CountBookmarksFromNode(const bookmarks::BookmarkNode* node);

[achuithb, 2015/10/02 23:48:36]

Could you move this out of this class into the top level anonymous namespace?

[lwchkg, 2015/10/09 19:36:25]

Sounds good.

+  ProfileStatValue CountBookmarks() const;
+
+  // Preference counting.
+  ProfileStatValue CountPrefs() const;
+
+  // Password counting.
+  class PasswordStoreConsumerHelper

[achuithb, 2015/10/02 23:48:36]

What's the point of this class? Why not have ProfileStatisticsAggregator inherit
from password_manager::PasswordStoreConsumer

[lwchkg, 2015/10/09 19:36:25]

I don't want PasswordStoreConsumer.* to pollute ProfileStatisticsAggregator.
Inheriting is convenient, but it looks like "using namespace std" to me, so I
avoid inheriting another class unless strictly necessary. This may also may
cause problems if PasswordStoreConsumer is modified to use a method/data member
of the same names..

+      : public password_manager::PasswordStoreConsumer {
+   public:
+    explicit PasswordStoreConsumerHelper(ProfileStatisticsAggregator* parent)
+        : parent_(parent) {}
+
+    void OnGetPasswordStoreResults(
+        ScopedVector<autofill::PasswordForm> results) override {
+      parent_->StatisticsCallbackSuccess(profiles::kProfileStatisticsPasswords,
+                                         results.size());
+    }
+
+   private:
+    ProfileStatisticsAggregator* parent_;

[achuithb, 2015/10/02 23:48:36]

parent_ = nullptr;

[lwchkg, 2015/10/09 19:36:25]

Acknowledged.

+
+    DISALLOW_COPY_AND_ASSIGN(PasswordStoreConsumerHelper);
+  };
+  PasswordStoreConsumerHelper password_store_consumer_helper_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileStatisticsAggregator);
+};
+
+ProfileStatisticsAggregator::ProfileStatisticsAggregator(
+    Profile* profile,
+    const profiles::ProfileStatisticsCallback& callback,
+    base::CancelableTaskTracker* tracker)
+    : profile_(profile),
+      callback_(callback),
+      tracker_(tracker),
+      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::CountBookmarks, this),
+          base::Bind(&ProfileStatisticsAggregator::StatisticsCallback,
+                     this, profiles::kProfileStatisticsBookmarks));
+
+  // Initiate history counting (async).
+  history::HistoryService* history_service =
+      HistoryServiceFactory::GetForProfileWithoutCreating(profile_);
+
+  if (history_service) {

[achuithb, 2015/10/02 23:48:36]

When can this be null? 

[lwchkg, 2015/10/09 19:36:25]

If GetForProfileWithoutCreating fails this is null. I'm not sure when this
happens, but for safety I don't assume that any resource allocation is an
automatic success.

+    history_service->GetHistoryCount(
+        base::Time(),
+        base::Time::Max(),
+        base::Bind(&ProfileStatisticsAggregator::StatisticsCallbackHistory,
+                   this),
+        tracker_);
+  } else {
+    StatisticsCallbackFailure(profiles::kProfileStatisticsBrowsingHistory);
+  }
+
+  // Initiate stored password counting (async).
+  // TODO(anthonyvd): make password task cancellable.
+  scoped_refptr<password_manager::PasswordStore> password_store =
+      PasswordStoreFactory::GetForProfile(
+          profile_, ServiceAccessType::EXPLICIT_ACCESS);
+  if (password_store) {
+    password_store->GetAutofillableLogins(&password_store_consumer_helper_);
+  } else {
+    StatisticsCallbackFailure(profiles::kProfileStatisticsPasswords);
+  }
+
+  // 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, profiles::kProfileStatisticsSettings));
+}
+
+void ProfileStatisticsAggregator::StatisticsCallback(
+    const char* category, ProfileStatValue result) {
+  profiles::ProfileCategoryStat datum;
+  datum.category = category;
+  datum.count = result.count;
+  datum.success = result.success;
+  profile_category_stats_.push_back(datum);
+  callback_.Run(profile_category_stats_);
+}
+
+void ProfileStatisticsAggregator::StatisticsCallbackSuccess(
+    const char* category, int count) {
+  ProfileStatValue result;
+  result.count = count;
+  result.success = true;
+  StatisticsCallback(category, result);
+}
+
+void ProfileStatisticsAggregator::StatisticsCallbackFailure(
+    const char* category) {
+  ProfileStatValue result;
+  result.count = 0;
+  result.success = false;
+  StatisticsCallback(category, result);
+}
+
+void ProfileStatisticsAggregator::StatisticsCallbackHistory(
+    history::HistoryCountResult result) {
+  ProfileStatValue result_converted;
+  result_converted.count = result.count;
+  result_converted.success = result.success;
+  StatisticsCallback(profiles::kProfileStatisticsBrowsingHistory,
+                     result_converted);
+}
+
+int ProfileStatisticsAggregator::CountBookmarksFromNode(
+    const bookmarks::BookmarkNode* node) {
+  int count = 0;
+  if (node->is_url()) {
+    ++count;
+  } else {
+    for (int i = 0; i < node->child_count(); ++i)
+      count += CountBookmarksFromNode(node->GetChild(i));
+  }
+  return count;
+}
+
+ProfileStatValue ProfileStatisticsAggregator::CountBookmarks() const {
+  bookmarks::BookmarkModel* bookmark_model =
+      BookmarkModelFactory::GetForProfileIfExists(profile_);
+
+  ProfileStatValue result;
+  if (bookmark_model) {
+    result.count = CountBookmarksFromNode(bookmark_model->bookmark_bar_node()) +
+                   CountBookmarksFromNode(bookmark_model->other_node()) +
+                   CountBookmarksFromNode(bookmark_model->mobile_node());
+    result.success = true;
+  } else {
+    result.count = 0;
+    result.success = false;
+  }
+  return result;
+}
+
+ProfileStatValue ProfileStatisticsAggregator::CountPrefs() const {
+  const PrefService* pref_service = profile_->GetPrefs();
+
+  ProfileStatValue result;
+  if (pref_service) {
+    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;
+      }
+    }
+
+    result.count = count;
+    result.success = true;
+  } else {
+    result.count = 0;
+    result.success = false;
+  }
+  return result;
+}
+
+}  // namespace
+
+namespace profiles {
+
+// Constants for the categories in ProfileCategoryStats
+const char kProfileStatisticsBrowsingHistory[] = "BrowsingHistory";
+const char kProfileStatisticsPasswords[] = "Passwords";
+const char kProfileStatisticsBookmarks[] = "Bookmarks";
+const char kProfileStatisticsSettings[] = "Settings";
+
+void GetProfileStatistics(Profile* profile,
+    const ProfileStatisticsCallback& callback,
+    base::CancelableTaskTracker* tracker) {
+  scoped_refptr<ProfileStatisticsAggregator> aggregator =
+      new ProfileStatisticsAggregator(profile, callback, tracker);
+}
+
+}  // namespace profiles