feed: Fork NoticeCardTracker for iOS

Creates a fork of this class for iOS, so that we can later
change the API for other platforms(android).

Now that this fork is in place, i was able to clean up
a few things:

* removed locking from the non-public notice card
tracker, as it was only necessary on iOS.

* re-arrange build targets, fewer sources are needed
  in ios now.

* add 'public' headers for feed_core_v2 to restrict
  direct access to internal classes.

Bug: 1213474
Change-Id: Idbfdf278cfd46d8fca8518b0e9812e6dca6af6ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2956401
Reviewed-by: Ian Wells <iwells@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892285}

chrome/test/BUILD.gn

@@ -763,7 +763,8 @@ if (is_android) {
           [ "../browser/android/feed/v2/rss_links_fetcher_browsertest.cc" ]
       deps += [
         "//components/feed:feature_list",
-        "//components/feed/core/v2:feed_core_base",
+        "//components/feed/core/v2:feed_core_v2",
+        "//components/feed/core/v2:ios_shared",
         "//components/feed/core/v2:test_helpers",
         "//components/feed/mojom:mojo_bindings",
       ]

components/BUILD.gn

@@ -227,7 +227,7 @@ test("components_unittests") {
       "//components/autofill/ios/browser:unit_tests",
       "//components/autofill/ios/form_util:unit_tests",
       "//components/breadcrumbs/ios:unit_tests",
-      "//components/feed/core/v2:feed_core_base_unit_tests",
+      "//components/feed/core/v2/public/ios:feed_ios_unit_tests",
       "//components/image_fetcher/ios:unit_tests",
       "//components/language/ios/browser:unit_tests",
       "//components/password_manager/core/common:unit_tests",

components/feed/BUILD.gn

@@ -27,5 +27,8 @@ static_library("feature_list") {
 
 source_set("unit_tests") {
   testonly = true
-  deps = [ "core/v2:core_unit_tests" ]
+  deps = [
+    "core/v2:core_unit_tests",
+    "core/v2/public/ios:feed_ios_unit_tests",
+  ]
 }

components/feed/core/v2/BUILD.gn

@@ -9,8 +9,30 @@ if (is_android) {
 }
 
 source_set("feed_core_v2") {
+  public = [
+    "public/feed_api.h",
+    "public/feed_service.h",
+    "public/feed_stream_surface.h",
+    "public/persistent_key_value_store.h",
+    "public/persistent_key_value_store.h",
+    "public/refresh_task_scheduler.h",
+    "public/refresh_task_scheduler.h",
+    "public/reliability_logger.h",
+    "public/stream_type.h",
+    "public/types.h",
+    "public/unread_content_observer.h",
+    "public/web_feed_subscriptions.h",
+
+    # Includes that should be made private:
+    "config.h",
+  ]
+
   sources = [
     "algorithm.h",
+    "config.cc",
+    "config.h",
+    "enums.cc",
+    "enums.h",
     "feed_network.cc",
     "feed_network.h",
     "feed_network_impl.cc",
@@ -25,6 +47,8 @@ source_set("feed_core_v2") {
     "image_fetcher.h",
     "metrics_reporter.cc",
     "metrics_reporter.h",
+    "notice_card_tracker.cc",
+    "notice_card_tracker.h",
     "persistent_key_value_store_impl.cc",
     "persistent_key_value_store_impl.h",
     "prefs.cc",
@@ -34,19 +58,12 @@ source_set("feed_core_v2") {
     "protocol_translator.cc",
     "protocol_translator.h",
     "public/feed_api.cc",
-    "public/feed_api.h",
     "public/feed_service.cc",
-    "public/feed_service.h",
     "public/feed_stream_surface.cc",
-    "public/feed_stream_surface.h",
     "public/persistent_key_value_store.cc",
-    "public/persistent_key_value_store.h",
-    "public/refresh_task_scheduler.h",
-    "public/reliability_logger.h",
-    "public/types.cc",
+    "public/public_types.cc",
+    "public/stream_type.cc",
     "public/unread_content_observer.cc",
-    "public/unread_content_observer.h",
-    "public/web_feed_subscriptions.h",
     "request_throttler.cc",
     "request_throttler.h",
     "scheduling.cc",
@@ -77,6 +94,8 @@ source_set("feed_core_v2") {
     "tasks/upload_actions_task.h",
     "tasks/wait_for_store_initialize_task.cc",
     "tasks/wait_for_store_initialize_task.h",
+    "types.cc",
+    "types.h",
     "web_feed_subscription_coordinator.cc",
     "web_feed_subscription_coordinator.h",
     "web_feed_subscriptions/fetch_recommended_web_feeds_task.cc",
@@ -94,6 +113,11 @@ source_set("feed_core_v2") {
     "wire_response_translator.cc",
     "wire_response_translator.h",
   ]
+  friend = [
+    ":core_unit_tests",
+    "//components/feed/core/v2/ios/public:feed_ios_public",
+    "//components/feed/core/v2/ios/public:feed_ios_unit_tests",
+  ]
   deps = [
     "public:common",
     "//components/feed:feature_list",
@@ -115,7 +139,7 @@ source_set("feed_core_v2") {
   ]
 
   public_deps = [
-    ":feed_core_base",
+    ":ios_shared",
     "//base",
     "//base/util/values:values_util",
     "//components/feed/core/common:feed_core_common",
@@ -124,30 +148,16 @@ source_set("feed_core_v2") {
 }
 
 # This smaller source set is used by Chrome for iOS.
-source_set("feed_core_base") {
+source_set("ios_shared") {
   sources = [
-    "config.cc",
-    "config.h",
-    "enums.cc",
-    "enums.h",
-    "notice_card_tracker.cc",
-    "notice_card_tracker.h",
-    "public/stream_type.cc",
-    "public/stream_type.h",
-    "public/types.h",
-    "types.cc",
-    "types.h",
+    "ios_shared_prefs.cc",
+    "ios_shared_prefs.h",
   ]
   deps = [
     "//base",
-    "//base/util/type_safety",
     "//base/util/values:values_util",
-    "//components/feed:feature_list",
     "//components/feed/core/common:feed_core_common",
-    "//components/feed/core/proto:proto_v2",
     "//components/prefs",
-    "//components/version_info:channel",
-    "//url",
   ]
 }
 
@@ -177,11 +187,12 @@ source_set("core_unit_tests") {
     "feedstore_util_unittest.cc",
     "image_fetcher_unittest.cc",
     "metrics_reporter_unittest.cc",
+    "notice_card_tracker_unittest.cc",
     "persistent_key_value_store_impl_unittest.cc",
     "proto_util_unittest.cc",
     "protocol_translator_unittest.cc",
     "public/feed_service_unittest.cc",
-    "public/types_unittest.cc",
+    "public/public_types_unittest.cc",
     "request_throttler_unittest.cc",
     "scheduling_unittest.cc",
     "stream_model_unittest.cc",
@@ -192,12 +203,12 @@ source_set("core_unit_tests") {
     "test/stream_builder.h",
     "test/test_util.cc",
     "test/test_util.h",
+    "types_unittest.cc",
     "web_feed_subscriptions/web_feed_index_unittest.cc",
   ]
 
   public_deps = [ ":test_helpers" ]
   deps = [
-    ":feed_core_base_unit_tests",
     ":feed_core_v2",
     ":unit_tests_bundle_data",
     "public:common",
@@ -235,27 +246,6 @@ source_set("feed_core_stubs") {
   deps = [ ":feed_core_v2" ]
 }
 
-# Tests for :feed_core_base that can be compiled in iOS.
-# This source set is added as a dep of :core_unit_tests.
-source_set("feed_core_base_unit_tests") {
-  testonly = true
-  sources = [
-    "notice_card_tracker_unittest.cc",
-    "types_unittest.cc",
-  ]
-  deps = [
-    ":feed_core_base",
-    "//base",
-    "//base/test:test_support",
-    "//components/feed:feature_list",
-    "//components/feed/core/common:feed_core_common",
-    "//components/prefs",
-    "//components/prefs:test_support",
-    "//testing/gmock",
-    "//testing/gtest",
-  ]
-}
-
 bundle_data("unit_tests_bundle_data") {
   visibility = [ ":core_unit_tests" ]
   testonly = true

components/feed/core/v2/feed_stream.cc

@@ -27,6 +27,7 @@
 #include "components/feed/core/v2/feed_store.h"
 #include "components/feed/core/v2/feedstore_util.h"
 #include "components/feed/core/v2/image_fetcher.h"
+#include "components/feed/core/v2/ios_shared_prefs.h"
 #include "components/feed/core/v2/metrics_reporter.h"
 #include "components/feed/core/v2/prefs.h"
 #include "components/feed/core/v2/protocol_translator.h"

components/feed/core/v2/public/ios/prefs.cc → components/feed/core/v2/ios_shared_prefs.cc

@@ -2,12 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/feed/core/v2/public/ios/prefs.h"
+#include "components/feed/core/v2/ios_shared_prefs.h"
 
-#include "components/feed/core/v2/public/ios/pref_names.h"
+#include "components/feed/core/common/pref_names.h"
 #include "components/prefs/pref_service.h"
 
-namespace ios_feed {
+namespace feed {
 namespace prefs {
 
 void SetLastFetchHadNoticeCard(PrefService& pref_service, bool value) {
@@ -30,5 +30,23 @@ bool GetHasReachedClickAndViewActionsUploadConditions(
       feed::prefs::kHasReachedClickAndViewActionsUploadConditions);
 }
 
+void IncrementNoticeCardViewsCount(PrefService& pref_service) {
+  int count = pref_service.GetInteger(feed::prefs::kNoticeCardViewsCount);
+  pref_service.SetInteger(feed::prefs::kNoticeCardViewsCount, count + 1);
+}
+
+int GetNoticeCardViewsCount(const PrefService& pref_service) {
+  return pref_service.GetInteger(feed::prefs::kNoticeCardViewsCount);
+}
+
+void IncrementNoticeCardClicksCount(PrefService& pref_service) {
+  int count = pref_service.GetInteger(feed::prefs::kNoticeCardClicksCount);
+  pref_service.SetInteger(feed::prefs::kNoticeCardClicksCount, count + 1);
+}
+
+int GetNoticeCardClicksCount(const PrefService& pref_service) {
+  return pref_service.GetInteger(feed::prefs::kNoticeCardClicksCount);
+}
+
 }  // namespace prefs
-}  // namespace ios_feed
+}  // namespace feed

components/feed/core/v2/ios_shared_prefs.h

@@ -0,0 +1,28 @@
+// Copyright 2021 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.
+
+#ifndef COMPONENTS_FEED_CORE_V2_IOS_SHARED_PREFS_H_
+#define COMPONENTS_FEED_CORE_V2_IOS_SHARED_PREFS_H_
+
+class PrefService;
+
+namespace feed {
+namespace prefs {
+void SetLastFetchHadNoticeCard(PrefService& pref_service, bool value);
+bool GetLastFetchHadNoticeCard(const PrefService& pref_service);
+void SetHasReachedClickAndViewActionsUploadConditions(PrefService& pref_service,
+                                                      bool value);
+bool GetHasReachedClickAndViewActionsUploadConditions(
+    const PrefService& pref_service);
+
+// Increment the stored notice card views count by 1.
+void IncrementNoticeCardViewsCount(PrefService& pref_service);
+// Increment the stored notice card clicks count by 1.
+void IncrementNoticeCardClicksCount(PrefService& pref_service);
+int GetNoticeCardClicksCount(const PrefService& pref_service);
+int GetNoticeCardViewsCount(const PrefService& pref_service);
+}  // namespace prefs
+}  // namespace feed
+
+#endif  // COMPONENTS_FEED_CORE_V2_IOS_SHARED_PREFS_H_

components/feed/core/v2/notice_card_tracker.cc

@@ -5,13 +5,13 @@
 #include "components/feed/core/v2/notice_card_tracker.h"
 
 #include "components/feed/core/common/pref_names.h"
+#include "components/feed/core/v2/ios_shared_prefs.h"
 #include "components/feed/feed_feature_list.h"
 #include "components/prefs/pref_service.h"
 
 namespace feed {
-namespace {
 
-int GetNoticeCardIndex() {
+int GetNoticeCardExpectedIndex() {
   // Infer that the notice card is at the 2nd position when the feature related
   // to putting the notice card at the second position is enabled.
   if (base::FeatureList::IsEnabled(
@@ -21,50 +21,6 @@ int GetNoticeCardIndex() {
   return 0;
 }
 
-}  // namespace
-
-namespace prefs {
-
-void IncrementNoticeCardViewsCount(PrefService& pref_service) {
-  int count = pref_service.GetInteger(feed::prefs::kNoticeCardViewsCount);
-  pref_service.SetInteger(feed::prefs::kNoticeCardViewsCount, count + 1);
-}
-
-int GetNoticeCardViewsCount(const PrefService& pref_service) {
-  return pref_service.GetInteger(feed::prefs::kNoticeCardViewsCount);
-}
-
-void IncrementNoticeCardClicksCount(PrefService& pref_service) {
-  int count = pref_service.GetInteger(feed::prefs::kNoticeCardClicksCount);
-  pref_service.SetInteger(feed::prefs::kNoticeCardClicksCount, count + 1);
-}
-
-int GetNoticeCardClicksCount(const PrefService& pref_service) {
-  return pref_service.GetInteger(feed::prefs::kNoticeCardClicksCount);
-}
-
-void SetLastFetchHadNoticeCard(PrefService& pref_service, bool value) {
-  pref_service.SetBoolean(feed::prefs::kLastFetchHadNoticeCard, value);
-}
-
-bool GetLastFetchHadNoticeCard(const PrefService& pref_service) {
-  return pref_service.GetBoolean(feed::prefs::kLastFetchHadNoticeCard);
-}
-
-void SetHasReachedClickAndViewActionsUploadConditions(PrefService& pref_service,
-                                                      bool value) {
-  pref_service.SetBoolean(
-      feed::prefs::kHasReachedClickAndViewActionsUploadConditions, value);
-}
-
-bool GetHasReachedClickAndViewActionsUploadConditions(
-    const PrefService& pref_service) {
-  return pref_service.GetBoolean(
-      feed::prefs::kHasReachedClickAndViewActionsUploadConditions);
-}
-
-}  // namespace prefs
-
 NoticeCardTracker::NoticeCardTracker(PrefService* profile_prefs)
     : profile_prefs_(profile_prefs) {
   DCHECK(profile_prefs_);
@@ -98,12 +54,10 @@ bool NoticeCardTracker::HasAcknowledgedNoticeCard() const {
   if (!base::FeatureList::IsEnabled(feed::kInterestFeedNoticeCardAutoDismiss))
     return false;
 
-  base::AutoLock auto_lock_views(views_count_lock_);
   if (views_count_threshold_ > 0 && views_count_ >= views_count_threshold_) {
     return true;
   }
 
-  base::AutoLock auto_lock_clicks(clicks_count_lock_);
   if (clicks_count_threshold_ > 0 && clicks_count_ >= clicks_count_threshold_) {
     return true;
   }
@@ -120,7 +74,7 @@ bool NoticeCardTracker::HasNoticeCardActionsCountPrerequisites(int index) {
     return false;
   }
 
-  if (index != GetNoticeCardIndex()) {
+  if (index != GetNoticeCardExpectedIndex()) {
     return false;
   }
   return true;
@@ -131,7 +85,6 @@ void NoticeCardTracker::MaybeUpdateNoticeCardViewsCount(int index) {
     return;
 
   prefs::IncrementNoticeCardViewsCount(*profile_prefs_);
-  base::AutoLock auto_lock(views_count_lock_);
   views_count_++;
 }
 
@@ -140,7 +93,6 @@ void NoticeCardTracker::MaybeUpdateNoticeCardClicksCount(int index) {
     return;
 
   prefs::IncrementNoticeCardClicksCount(*profile_prefs_);
-  base::AutoLock auto_lock(clicks_count_lock_);
   clicks_count_++;
 }