Query the existence other forms of browsing history.

components/browsing_data_ui/history_notice_utils.cc

Index: components/browsing_data_ui/history_notice_utils.cc
diff --git a/components/browsing_data_ui/history_notice_utils.cc b/components/browsing_data_ui/history_notice_utils.cc
index c1568f2a188375f9629ffe3c3ea73e110fea8a62..f509922db44fd4a91f16c1c83137a798b14ba70d 100644
--- a/components/browsing_data_ui/history_notice_utils.cc
+++ b/components/browsing_data_ui/history_notice_utils.cc
@@ -5,8 +5,37 @@
 #include "components/browsing_data_ui/history_notice_utils.h"
 
 #include "base/callback.h"
+#include "base/message_loop/message_loop.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/history/core/browser/web_history_service.h"
+#include "components/version_info/version_info.h"
+
+namespace {
+
+class MergeTwoBooleanCallbacks {

[msarda, 2016/05/18 08:58:07]

Please comment this class as it is not really to understand what it does.

[sdefresne, 2016/05/18 14:29:40]

+1

[msramek, 2016/05/18 19:38:36]

Done. Sorry about that, I hoped it would be clear from the usage, but that's of
course on the very bottom of this file.

+ public:
+  MergeTwoBooleanCallbacks(
+      const base::Callback<void(bool)>& merged_callback)
+      : merged_callback_(merged_callback),
+        final_response_(true) {
+  }
+
+  void Callback(bool response) {

[msarda, 2016/05/18 08:58:07]

Nit: s/Callback/RunCallback

[msramek, 2016/05/18 19:38:37]

Done.

+    final_response_ &= response;
+
+    if (++callbacks_ == 2) {

[sdefresne, 2016/05/18 14:29:40]

Avoid hard coded magic values.

BTW, I found the logic really hard to understand, can you maybe rework and
rename as:

void OnQueryResult(bool response) {
  DCHECK_LT(call_count_, expected_call_count_);
  final_response_ &= response;
  if (++call_count_ < expected_call_count_)
    return;

  callback_.Run(final_response_);
  base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
}

[msramek, 2016/05/18 19:38:37]

Done.

Also changed 2 to |expected_call_count_|. I just didn't want it to
overgeneralize either, since this is just a helper class in an unnamed
namespace.

+      merged_callback_.Run(final_response_);
+      base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
+    }
+  }
+
+ private:
+  base::Callback<void(bool)> merged_callback_;

[msarda, 2016/05/18 08:58:07]

Optional nit: I would rename this to target_callback_

[sdefresne, 2016/05/18 14:29:40]

I would call it callback_

[msramek, 2016/05/18 19:38:37]

I would prefer keeping some kind of adjective to distinguish the inbound
callbacks and the one outbound callback.

+  bool final_response_;

[sdefresne, 2016/05/18 14:29:40]

bool final_response_ = false;

[msramek, 2016/05/18 19:38:37]

This is currently initialized in the constructor as true. I can move the
initialization here, but in any case, we're doing a boolean product, so we must
start with true.

[sdefresne, 2016/05/18 19:42:39]

Oh, yes I should have said "bool final_response_ = true;".

My real comment was that you should either use default value in definition or
initialisation in the constructor consistently (callbacks_ with default value at
definition but final_response_ initialised in the constructor looks
inconsistent). I'm fine one way or another, just want consistency.

[msramek, 2016/05/19 19:47:39]

Makes sense. Let me move it up to the constructor then; that's usually my
preference, since I need to initialize some member variables from constructor
params anyway.

+  int callbacks_ = 0;
+};
+
+}  // namespace
 
 namespace browsing_data_ui {
 
@@ -14,7 +43,7 @@ namespace testing {
 
 bool g_override_other_forms_of_browsing_history_query = false;
 
-}
+}  // namespace testing
 
 void ShouldShowNoticeAboutOtherFormsOfBrowsingHistory(
     const ProfileSyncService* sync_service,
@@ -34,16 +63,36 @@ void ShouldShowNoticeAboutOtherFormsOfBrowsingHistory(
 void ShouldPopupDialogAboutOtherFormsOfBrowsingHistory(

[msarda, 2016/05/18 08:58:07]

The function is not more complex: it waits for 2 callbacks to be called, merges
the responses and then calls the callback. I think it deserves a unit test.

[sdefresne, 2016/05/18 14:29:40]

+1

[msramek, 2016/05/18 19:38:37]

Done, added a unittest. Two things that I noticed during testing and fixed here:

- It's easier to use a fake SyncService than ProfileSyncService, so I replaced
that in the method signature. This has no effect on callers, since the latter
inherits from the former. It's also cleaner, because we don't really depend on
its 'profile-ness'.

- So far we relied on the fact that WebHistoryService will be null when history
(deletion) is not synced, because WebHistoryServiceFactory will not create it in
that case. But it's easier to test if that condition is explicit. And more
explicit is better anyways.

     const ProfileSyncService* sync_service,
     history::WebHistoryService* history_service,
+    const version_info::Channel channel,
+    const std::string& user_agent,
     base::Callback<void(bool)> callback) {
-  if (!history_service ||
-      (!testing::g_override_other_forms_of_browsing_history_query &&
-       !history_service->HasOtherFormsOfBrowsingHistory())) {
+  // If the query for other forms of browsing history is overriden for testing,
+  // the conditions are identical with
+  // ShouldShowNoticeAboutOtherFormsOfBrowsingHistory.
+  if (testing::g_override_other_forms_of_browsing_history_query) {
+    ShouldShowNoticeAboutOtherFormsOfBrowsingHistory(
+        sync_service, history_service, callback);
+    return;
+  }
+
+  if (!sync_service ||
+      !sync_service->IsSyncActive() ||
+      sync_service->IsUsingSecondaryPassphrase() ||
+      !history_service) {
     callback.Run(false);
     return;
   }
 
-  ShouldShowNoticeAboutOtherFormsOfBrowsingHistory(
-      sync_service, history_service, callback);
+  // Return the boolean product of QueryWebAndAppActivity and
+  // QueryOtherFormsOfBrowsingHistory. MergeTwoBooleanCallbacks deletes itself
+  // after processing the two callbacks.
+  MergeTwoBooleanCallbacks* merger = new MergeTwoBooleanCallbacks(callback);
+  history_service->QueryWebAndAppActivity(base::Bind(
+      &MergeTwoBooleanCallbacks::Callback, base::Unretained(merger)));
+  history_service->QueryOtherFormsOfBrowsingHistory(
+      channel, user_agent,
+      base::Bind(
+          &MergeTwoBooleanCallbacks::Callback, base::Unretained(merger)));
 }
 
 }  // namespace browsing_data_ui