[Update Notification Service]: Modularization effort.

- Move update notification service into one module.
- Wrap up bridge as a class.
- Inject bridge and config as components into serviceImpl.
- TODO: Add test targets.
- TODO: Fix some broken due to ownership change.

Bug: 1013685
Change-Id: I1371176eb5249a048c9cc564a9056bfa747e64fb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1994177Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Hesen Zhang <hesen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734725}

chrome/browser/BUILD.gn

@@ -2939,17 +2939,8 @@ jumbo_static_library("browser") {
       "translate/android/translate_bridge.h",
       "updates/update_notification_client.cc",
       "updates/update_notification_client.h",
-      "updates/update_notification_config.cc",
-      "updates/update_notification_config.h",
-      "updates/update_notification_info.cc",
-      "updates/update_notification_info.h",
-      "updates/update_notification_service.h",
-      "updates/update_notification_service_bridge.cc",
-      "updates/update_notification_service_bridge.h",
-      "updates/update_notification_service_factory.cc",
-      "updates/update_notification_service_factory.h",
-      "updates/update_notification_service_impl.cc",
-      "updates/update_notification_service_impl.h",
+      "updates/update_notification_service_bridge_android.cc",
+      "updates/update_notification_service_bridge_android.h",
     ]
     public_deps += [
       "//chrome/android/features/dev_ui:buildflags",
@@ -2968,6 +2959,7 @@ jumbo_static_library("browser") {
       "//chrome/browser/notifications/chime/android",
       "//chrome/browser/notifications/scheduler/public",
       "//chrome/browser/share",
+      "//chrome/browser/updates",
       "//chrome/services/media_gallery_util/public/cpp",
       "//components/app_modal/android:jni_headers",
       "//components/autofill_assistant/browser",

chrome/browser/updates/BUILD.gn

+# Copyright 2020 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.
+
+source_set("updates") {
+  sources = [
+    "update_notification_config.cc",
+    "update_notification_config.h",
+    "update_notification_info.cc",
+    "update_notification_info.h",
+    "update_notification_service.h",
+    "update_notification_service_bridge.h",
+  ]
+
+  deps = [ ":factory" ]
+}
+
+source_set("factory") {
+  sources = [
+    "update_notification_service_factory.cc",
+    "update_notification_service_factory.h",
+  ]
+
+  deps = [ "//chrome/browser/updates/internal:lib" ]
+}
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [ "update_notification_config_unittest.cc" ]
+  deps = [ "//testing/gtest" ]
+}

chrome/browser/updates/DEPS

+include_rules = [
+  "+chrome/android/chrome_jni_headers",
+]

chrome/browser/updates/internal/BUILD.gn

+# Copyright 2020 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.
+
+source_set("lib") {
+  visibility = [ "//chrome/browser/updates:factory" ]
+
+  sources = [
+    "update_notification_service_impl.cc",
+    "update_notification_service_impl.h",
+  ]
+
+  deps = [
+    "//base",
+    "//components/keyed_service/core",
+    "//skia",
+  ]
+}

chrome/browser/updates/update_notification_service_impl.cc → chrome/browser/updates/internal/update_notification_service_impl.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/updates/update_notification_service_impl.h"
+#include "chrome/browser/updates/internal/update_notification_service_impl.h"
 
 #include <memory>
 #include <utility>
@@ -38,9 +38,12 @@ constexpr int kNumMaxNotificationsLimit = 1;
 constexpr int kNumConsecutiveDismissCountCap = 2;
 
 UpdateNotificationServiceImpl::UpdateNotificationServiceImpl(
-    notifications::NotificationScheduleService* schedule_service)
+    notifications::NotificationScheduleService* schedule_service,
+    std::unique_ptr<UpdateNotificationConfig> config,
+    std::unique_ptr<UpdateNotificationServiceBridge> bridge)
     : schedule_service_(schedule_service),
-      config_(UpdateNotificationConfig::Create()) {}
+      config_(std::move(config)),
+      bridge_(std::move(bridge)) {}
 
 UpdateNotificationServiceImpl::~UpdateNotificationServiceImpl() = default;
 
@@ -54,8 +57,8 @@ void UpdateNotificationServiceImpl::Schedule(UpdateNotificationInfo data) {
 bool UpdateNotificationServiceImpl::IsReadyToDisplay() const {
   if (!config_->is_enabled)
     return false;
-
-  auto last_shown_timestamp = updates::GetLastShownTimeStamp();
+  // TODO(hesen): Get last shown timestamp through bridge.(issue:1043237)
+  base::Optional<base::Time> last_shown_timestamp;
   if (last_shown_timestamp.has_value()) {
     return (GetThrottleInterval() <
             base::Time::Now() - last_shown_timestamp.value());
@@ -64,7 +67,7 @@ bool UpdateNotificationServiceImpl::IsReadyToDisplay() const {
 }
 
 base::TimeDelta UpdateNotificationServiceImpl::GetThrottleInterval() const {
-  auto throttle_interval = updates::GetThrottleInterval();
+  auto throttle_interval = bridge_->GetThrottleInterval();
   return throttle_interval.has_value() ? throttle_interval.value()
                                        : config_->default_interval;
 }
@@ -108,12 +111,12 @@ UpdateNotificationServiceImpl::BuildScheduleParams() {
 }
 
 void UpdateNotificationServiceImpl::OnUserDismiss() {
-  int count = updates::GetUserDismissCount() + 1;
+  int count = bridge_->GetUserDismissCount() + 1;
   if (count >= kNumConsecutiveDismissCountCap) {
     ApplyLinearThrottle();
     count = 0;
   }
-  updates::UpdateUserDismissCount(count);
+  bridge_->UpdateUserDismissCount(count);
 }
 
 void UpdateNotificationServiceImpl::ApplyLinearThrottle() {
@@ -121,11 +124,11 @@ void UpdateNotificationServiceImpl::ApplyLinearThrottle() {
   auto offset =
       base::TimeDelta::FromDays(config_->throttle_interval_linear_co_offset);
   auto interval = GetThrottleInterval();
-  updates::UpdateThrottleInterval(scale * interval + offset);
+  bridge_->UpdateThrottleInterval(scale * interval + offset);
 }
 
 void UpdateNotificationServiceImpl::OnUserClick() {
-  updates::LaunchChromeActivity();
+  bridge_->LaunchChromeActivity();
 }
 
 }  // namespace updates

chrome/browser/updates/update_notification_service_impl.h → chrome/browser/updates/internal/update_notification_service_impl.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_IMPL_H_
-#define CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_IMPL_H_
+#ifndef CHROME_BROWSER_UPDATES_INTERNAL_UPDATE_NOTIFICATION_SERVICE_IMPL_H_
+#define CHROME_BROWSER_UPDATES_INTERNAL_UPDATE_NOTIFICATION_SERVICE_IMPL_H_
 
 #include "chrome/browser/updates/update_notification_service.h"
 
@@ -21,11 +21,14 @@ namespace updates {
 
 struct UpdateNotificationConfig;
 struct UpdateNotificationInfo;
+class UpdateNotificationServiceBridge;
 
 class UpdateNotificationServiceImpl : public UpdateNotificationService {
  public:
   UpdateNotificationServiceImpl(
-      notifications::NotificationScheduleService* schedule_service);
+      notifications::NotificationScheduleService* schedule_service,
+      std::unique_ptr<UpdateNotificationConfig> config,
+      std::unique_ptr<UpdateNotificationServiceBridge> bridge);
   ~UpdateNotificationServiceImpl() override;
 
  private:
@@ -56,6 +59,8 @@ class UpdateNotificationServiceImpl : public UpdateNotificationService {
 
   std::unique_ptr<UpdateNotificationConfig> config_;
 
+  std::unique_ptr<UpdateNotificationServiceBridge> bridge_;
+
   base::WeakPtrFactory<UpdateNotificationServiceImpl> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(UpdateNotificationServiceImpl);
@@ -63,4 +68,4 @@ class UpdateNotificationServiceImpl : public UpdateNotificationService {
 
 }  // namespace updates
 
-#endif  // CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_IMPL_H_
+#endif  // CHROME_BROWSER_UPDATES_INTERNAL_UPDATE_NOTIFICATION_SERVICE_IMPL_H_

chrome/browser/updates/update_notification_client.cc

@@ -7,7 +7,6 @@
 #include <utility>
 
 #include "chrome/browser/updates/update_notification_service.h"
-#include "chrome/browser/updates/update_notification_service_bridge.h"
 
 namespace updates {
 
@@ -25,7 +24,8 @@ void UpdateNotificationClient::BeforeShowNotification(
     std::move(callback).Run(nullptr);
     return;
   }
-  updates::UpdateLastShownTimeStamp(base::Time::Now());
+  // TODO(hesen): Client code doesn't own the bridge, update last shown
+  // timestamp in service layer instead.(issue:1043237)
   // TODO(hesen): Record metrics, and add iHNR buttons.
   std::move(callback).Run(std::move(notification_data));
 }

chrome/browser/updates/update_notification_config.cc

@@ -9,7 +9,6 @@
 
 namespace updates {
 namespace {
-
 // Helper routines to get Finch experiment parameter. If no Finch seed was
 // found, use the |default_value|. The |name| should match an experiment
 // parameter in Finch server configuration.
@@ -33,9 +32,6 @@ bool GetFinchConfigBool(const std::string& name, bool default_value) {
 // Default update notification schedule interval in days.
 constexpr int kDefaultUpdateNotificationInterval = 21;
 
-// Default update notification state.
-constexpr bool kDefaultUpdateNotificationState = false;
-
 // Default start clock of deliver window in the morning.
 constexpr int kDefaultDeliverWindowMorningStart = 5;
 
@@ -54,6 +50,10 @@ constexpr double kDefaultThrottleIntervalScale = 1.0;
 // Default offset coefficient of custom throttle linear function.
 constexpr double kDefaultThrottleIntervalOffset = 0.0;
 
+// Default update notification state.
+constexpr bool kDefaultUpdateNotificationState = false;
+
+// TODO(hesen): Create a CreateFromFinch method (issue 1043237).
 std::unique_ptr<UpdateNotificationConfig> UpdateNotificationConfig::Create() {
   std::unique_ptr<UpdateNotificationConfig> config =
       std::make_unique<UpdateNotificationConfig>();

chrome/browser/updates/update_notification_service_bridge.h

-// Copyright 2019 The Chromium Authors. All rights reserved.
+// Copyright 2020 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 CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_BRIDGE_H_
 #define CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_BRIDGE_H_
 
+#include <memory>
+
+#include "base/macros.h"
 #include "base/optional.h"
 #include "base/time/time.h"
 
 namespace updates {
 
-// Functions for calling into UpdateNotifiactionServiceBridge.java.
+class UpdateNotificationServiceBridge {
+ public:
+  // Create the default UpdateNotificationServiceBridge.
+  static std::unique_ptr<UpdateNotificationServiceBridge> Create();
+
+  // Updates and persists |timestamp| in Android SharedPreferences.
+  virtual void UpdateLastShownTimeStamp(base::Time timestamp);
+
+  // Returns persisted timestamp of last shown notification from Android
+  // SharedPreferences. Return nullopt if there is no data.
+  virtual base::Optional<base::Time> GetLastShownTimeStamp();
+
+  // Updates and persists |interval| in Android SharedPreferences.
+  virtual void UpdateThrottleInterval(base::TimeDelta interval);
 
-// Updates and persists |timestamp| in Android SharedPreferences.
-void UpdateLastShownTimeStamp(base::Time timestamp);
+  // Returns persisted interval that might be throttled from Android
+  // SharedPreferences. Return nullopt if there is no data.
+  virtual base::Optional<base::TimeDelta> GetThrottleInterval();
 
-// Returns persisted timestamp of last shown notification from Android
-// SharedPreferences. Return nullopt if there is no data.
-base::Optional<base::Time> GetLastShownTimeStamp();
+  // Updates and persists |count| in Android SharedPreferences.
+  virtual void UpdateUserDismissCount(int count);
 
-// Updates and persists |interval| in Android SharedPreferences.
-void UpdateThrottleInterval(base::TimeDelta interval);
+  // Returns persisted count from Android SharedPreferences.
+  virtual int GetUserDismissCount();
 
-// Returns persisted interval that might be throttled from Android
-// SharedPreferences. Return nullopt if there is no data.
-base::Optional<base::TimeDelta> GetThrottleInterval();
+  // Launches Chrome activity after user clicked the notification.
+  virtual void LaunchChromeActivity();
 
-// Updates and persists |count| in Android SharedPreferences.
-void UpdateUserDismissCount(int count);
+  virtual ~UpdateNotificationServiceBridge() = default;
 
-// Returns persisted count from Android SharedPreferences.
-int GetUserDismissCount();
+ protected:
+  UpdateNotificationServiceBridge() = default;
 
-// Launches Chrome activity after user clicked the notification.
-void LaunchChromeActivity();
+ private:
+  DISALLOW_COPY_AND_ASSIGN(UpdateNotificationServiceBridge);
+};
 
 }  // namespace updates
 

chrome/browser/updates/update_notification_service_bridge.cc → chrome/browser/updates/update_notification_service_bridge_android.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/updates/update_notification_service_bridge.h"
+#include "chrome/browser/updates/update_notification_service_bridge_android.h"
 
 #include <utility>
 
@@ -41,13 +41,15 @@ void JNI_UpdateNotificationServiceBridge_Schedule(
 //
 // C++ -> Java
 //
-void UpdateLastShownTimeStamp(base::Time timestamp) {
+void UpdateNotificationServiceBridgeAndroid::UpdateLastShownTimeStamp(
+    base::Time timestamp) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_UpdateNotificationServiceBridge_updateLastShownTimeStamp(
       env, timestamp.ToJavaTime());
 }
 
-base::Optional<base::Time> GetLastShownTimeStamp() {
+base::Optional<base::Time>
+UpdateNotificationServiceBridgeAndroid::GetLastShownTimeStamp() {
   JNIEnv* env = base::android::AttachCurrentThread();
   auto timestamp =
       Java_UpdateNotificationServiceBridge_getLastShownTimeStamp(env);
@@ -56,13 +58,15 @@ base::Optional<base::Time> GetLastShownTimeStamp() {
              : (base::make_optional(base::Time::FromJavaTime(timestamp)));
 }
 
-void UpdateThrottleInterval(base::TimeDelta interval) {
+void UpdateNotificationServiceBridgeAndroid::UpdateThrottleInterval(
+    base::TimeDelta interval) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_UpdateNotificationServiceBridge_updateThrottleInterval(
       env, interval.InMilliseconds());
 }
 
-base::Optional<base::TimeDelta> GetThrottleInterval() {
+base::Optional<base::TimeDelta>
+UpdateNotificationServiceBridgeAndroid::GetThrottleInterval() {
   JNIEnv* env = base::android::AttachCurrentThread();
   auto interval = Java_UpdateNotificationServiceBridge_getThrottleInterval(env);
   return interval == 0 ? base::nullopt
@@ -70,17 +74,17 @@ base::Optional<base::TimeDelta> GetThrottleInterval() {
                              base::TimeDelta::FromMilliseconds(interval)));
 }
 
-void UpdateUserDismissCount(int count) {
+void UpdateNotificationServiceBridgeAndroid::UpdateUserDismissCount(int count) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_UpdateNotificationServiceBridge_updateUserDismissCount(env, count);
 }
 
-int GetUserDismissCount() {
+int UpdateNotificationServiceBridgeAndroid::GetUserDismissCount() {
   JNIEnv* env = base::android::AttachCurrentThread();
   return Java_UpdateNotificationServiceBridge_getUserDismissCount(env);
 }
 
-void LaunchChromeActivity() {
+void UpdateNotificationServiceBridgeAndroid::LaunchChromeActivity() {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_UpdateNotificationServiceBridge_launchChromeActivity(env);
 }

chrome/browser/updates/update_notification_service_bridge_android.h

+// Copyright 2020 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 CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_BRIDGE_ANDROID_H_
+#define CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_BRIDGE_ANDROID_H_
+
+#include "chrome/browser/updates/update_notification_service_bridge.h"
+
+namespace updates {
+
+class UpdateNotificationServiceBridgeAndroid
+    : public UpdateNotificationServiceBridge {
+ public:
+  UpdateNotificationServiceBridgeAndroid() = default;
+  ~UpdateNotificationServiceBridgeAndroid() override = default;
+
+ private:
+  // UpdateNotificationServiceBridge implementation.
+  void UpdateLastShownTimeStamp(base::Time timestamp) override;
+  base::Optional<base::Time> GetLastShownTimeStamp() override;
+  void UpdateThrottleInterval(base::TimeDelta interval) override;
+  base::Optional<base::TimeDelta> GetThrottleInterval() override;
+  void UpdateUserDismissCount(int count) override;
+  int GetUserDismissCount() override;
+  void LaunchChromeActivity() override;
+
+  DISALLOW_COPY_AND_ASSIGN(UpdateNotificationServiceBridgeAndroid);
+};
+
+}  // namespace updates
+
+#endif  // CHROME_BROWSER_UPDATES_UPDATE_NOTIFICATION_SERVICE_BRIDGE_ANDROID_H_

chrome/browser/updates/update_notification_service_factory.cc

@@ -4,9 +4,15 @@
 
 #include "chrome/browser/updates/update_notification_service_factory.h"
 
+#include <memory>
+#include <utility>
+
 #include "chrome/browser/notifications/scheduler/notification_schedule_service_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
-#include "chrome/browser/updates/update_notification_service_impl.h"
+#include "chrome/browser/updates/internal/update_notification_service_impl.h"
+#include "chrome/browser/updates/update_notification_config.h"
+#include "chrome/browser/updates/update_notification_service_bridge.h"
+#include "chrome/browser/updates/update_notification_service_bridge_android.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 
 // static
@@ -35,8 +41,11 @@ KeyedService* UpdateNotificationServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   auto* schedule_service =
       NotificationScheduleServiceFactory::GetForBrowserContext(context);
-  return static_cast<KeyedService*>(
-      new updates::UpdateNotificationServiceImpl(schedule_service));
+  auto config = updates::UpdateNotificationConfig::Create();
+  auto bridge =
+      std::make_unique<updates::UpdateNotificationServiceBridgeAndroid>();
+  return static_cast<KeyedService*>(new updates::UpdateNotificationServiceImpl(
+      schedule_service, std::move(config), std::move(bridge)));
 }
 
 content::BrowserContext*

chrome/test/BUILD.gn

@@ -3788,7 +3788,6 @@ test("unit_tests") {
       "../browser/toolbar/toolbar_security_icon_unittest.cc",
       "../browser/touch_to_fill/touch_to_fill_controller_unittest.cc",
       "../browser/translate/translate_manager_render_view_host_android_unittest.cc",
-      "../browser/updates/update_notification_config_unittest.cc",
       "../browser/util/url_utilities_unittest.cc",
     ]
     deps += [
@@ -3799,6 +3798,7 @@ test("unit_tests") {
       "//chrome/android:native_j_unittests_jni_headers",
       "//chrome/android:native_java_unittests_java",
       "//chrome/android/features/media_router:java",
+      "//chrome/browser/updates:unit_tests",
       "//chrome/services/media_gallery_util:unit_tests",
       "//components/download/internal/common:internal_java",
       "//components/favicon/core/test:test_support",