Retrieve the web&app activity enabled state from WebHistoryService.

components/browsing_data_ui/history_notice_utils.cc

 // Copyright 2016 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 "components/browsing_data_ui/history_notice_utils.h"
 
 #include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/history/core/browser/web_history_service.h"
 
 namespace browsing_data_ui {
 
-bool ShouldShowNoticeAboutOtherFormsOfBrowsingHistory(
+void ShouldShowNoticeAboutOtherFormsOfBrowsingHistory(
     const ProfileSyncService* sync_service,
-    const history::WebHistoryService* history_service) {
-  return sync_service &&
-         sync_service->IsSyncActive() &&
-         !sync_service->IsUsingSecondaryPassphrase() &&
-         history_service &&
-         history_service->HasOtherFormsOfBrowsingHistory();
+    history::WebHistoryService* history_service,
+    base::Callback<void(bool)> callback) {
+  // TODO(msramek): The API to query other forms of browsing history is not

[sdefresne, 2016/03/24 09:50:07]

I really dislike this. The function prototype (taking a ProfileSyncService and
WebHistoryService) is making our iOS tests more complex (as we have to
instantiate those services and their dependencies as they are not mockable).
This is already painful, but if the function is not even using them, this is
doubly so.

Can you instead work on a proper design instead? Then we wouldn't have to patch
things up every time and include such hacks.

[msramek, 2016/03/24 12:53:22]

ProfileSyncService* and WebHistoryService* are in the signature precisely
because of iOS. I would normally just use Profile*, but we use different
BrowserContextKeyedService factories on iOS. I cleared this signature with
Mihai, and I have to rely on his judgement here, because I don't know what are
the requirements for iOS UI. We know that the response has to be one
asynchronous callback, and how exactly this method internally assembles the
value is not important. I already have a Desktop UI CL ready, so I'm sure that
this design works for me, and if Mihai is happy, I do not foresee any future
change of the interface.

Naturally, we could replace ProfileSyncService* and WebHistoryService* in this
method with something else that is mockable, but then this component has no
point at all - it would literally just do a boolean operation on provided
values. That's not what Mihai and I intended - we have this component to assure
that we are consistent in how we check the status of sync and history, and it
already proved valuable, since we recently realized that it might not be as
straightforward to determine what does it mean that "sync is running".

Regarding the early return - the problem is that the API I need to call does not
exist yet, and folks working on the serverside part were not able to tell me how
it's going to look like yet. It might be bundled to client=web_app, or it might
be something completely different. I admit the early return here was a bad call,
since it makes the method untestable. What we can do is keep the dummy "return
false;" method in WebHistoryService to represent the not-yet implemented API,
which will make it testable by overriding through e.g. FakeWebHistoryService.
Regardless of how that part might change when I learn how exactly the API is
going to look like, the interface between this layer and Desktop/Android/iOS UI
will not need to change. The tests involving WebHistoryService might still
change, but that's unfortunately out of my control.

Does that work for you (both)?

+  // fully available yet, so for now, assume they are not present.
+  callback.Run(false);
+  return;
+
+  if (!sync_service ||
+      !sync_service->IsSyncActive() ||
+      sync_service->IsUsingSecondaryPassphrase() ||
+      !history_service) {
+    callback.Run(false);
+    return;
+  }
+
+  history_service->QueryWebAndAppActivity(callback);
 }
 
 }  // namespace browsing_data_ui