[feature_notification_guide] Implemented notification scheduling logic

This CL adds notification scheduling logic to feature guide. - Moved the bridge and factory to live in chrome target. - The bridge will implement a Delegate interface for the service. - We will skip notifications if they are already in the scheduled queue, or have been already shown which will be handled by the Tracker. - Notifications will have a fixed hardcoded ID so that they can be easily deleted if user starts using the feature. Bug: 1277400 Change-Id: Ic8b26cd0b93e226202497646658deb338760ae76 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3320077 Reviewed-by: Min Qin <qinmin@chromium.org> Reviewed-by: Xing Liu <xingliu@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Commit-Queue: Shakti Sahu <shaktisahu@chromium.org> Cr-Commit-Position: refs/heads/main@{#950183}
chromium · Dec 9, 2021 · bfa6585 · bfa6585
1 parent 885604e
commit bfa6585
Show file tree

Hide file tree

Showing 26 changed files with 553 additions and 218 deletions.
diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
@@ -356,7 +356,6 @@ android_library("chrome_java") {
     "//chrome/browser/download/android:java_resources",
     "//chrome/browser/enterprise/util:java",
     "//chrome/browser/feature_engagement:java",
-    "//chrome/browser/feature_guide/notifications:factory_java",
     "//chrome/browser/feature_guide/notifications:java",
     "//chrome/browser/feed/android:java",
     "//chrome/browser/feedback/android:java",
@@ -752,7 +751,6 @@ java_group("chrome_all_java") {
     "//chrome/browser/content_creation/notes/internal/android:java",
     "//chrome/browser/content_creation/reactions/internal/android:java",
     "//chrome/browser/download/internal/android:java",
-    "//chrome/browser/feature_guide/notifications/internal:internal_java",
     "//chrome/browser/page_annotations/android:java",
     "//chrome/browser/password_check:internal_java",
     "//chrome/browser/password_edit_dialog/android:java",
@@ -3884,6 +3882,8 @@ generate_jni("chrome_jni_headers") {
     "java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesCategory.java",
     "java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesCategoryTile.java",
     "java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesSite.java",
+    "java/src/org/chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideBridge.java",
+    "java/src/org/chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideServiceFactory.java",
     "java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java",
     "java/src/org/chromium/chrome/browser/feedback/ScreenshotTask.java",
     "java/src/org/chromium/chrome/browser/firstrun/FirstRunUtils.java",

diff --git a/chrome/android/chrome_java_sources.gni b/chrome/android/chrome_java_sources.gni
@@ -591,6 +591,8 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/explore_sites/StableScrollLayoutManager.java",
   "java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java",
   "java/src/org/chromium/chrome/browser/externalnav/IntentWithRequestMetadataHandler.java",
+  "java/src/org/chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideBridge.java",
+  "java/src/org/chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideServiceFactory.java",
   "java/src/org/chromium/chrome/browser/feedback/ChromeFeedbackCollector.java",
   "java/src/org/chromium/chrome/browser/feedback/ConnectivityChecker.java",
   "java/src/org/chromium/chrome/browser/feedback/ConnectivityFeedbackSource.java",

diff --git a/chrome/android/java/src/org/chromium/chrome/browser/feature_guide/notifications/DIR_METADATA b/chrome/android/java/src/org/chromium/chrome/browser/feature_guide/notifications/DIR_METADATA
@@ -0,0 +1,4 @@
+monorail {
+  component: "Upboarding"
+}
+team_email: "chrome-upboarding-eng@google.com"
diff --git a/...tions/FeatureNotificationGuideBridge.java → ...tions/FeatureNotificationGuideBridge.java b/...tions/FeatureNotificationGuideBridge.java → ...tions/FeatureNotificationGuideBridge.java
@@ -13,7 +13,6 @@
 @JNINamespace("feature_guide")
 public final class FeatureNotificationGuideBridge implements FeatureNotificationGuideService {
     private long mNativeFeatureNotificationGuideBridge;
-    private Delegate mDelegate;
 
     @CalledByNative
     private static FeatureNotificationGuideBridge create(long nativePtr) {
@@ -29,24 +28,16 @@ private FeatureNotificationGuideBridge(long nativePtr) {
         mNativeFeatureNotificationGuideBridge = nativePtr;
     }
 
-    @Override
-    public void setDelegate(Delegate delegate) {
-        assert mDelegate == null;
-        mDelegate = delegate;
-    }
-
     @CalledByNative
-    private String getNotificationTitle(int featureType) {
-        return mDelegate.getNotificationTitle(featureType);
+    private String getNotificationTitle(@FeatureType int featureType) {
+        return null;
     }
 
     @CalledByNative
-    private String getNotificationMessage(int featureType) {
-        return mDelegate.getNotificationMessage(featureType);
+    private String getNotificationMessage(@FeatureType int featureType) {
+        return null;
     }
 
     @CalledByNative
-    private void onNotificationClick(int featureType) {
-        mDelegate.onNotificationClick(featureType);
-    }
+    private void onNotificationClick(@FeatureType int featureType) {}
 }
diff --git a/...atureNotificationGuideServiceFactory.java → ...atureNotificationGuideServiceFactory.java b/...atureNotificationGuideServiceFactory.java → ...atureNotificationGuideServiceFactory.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/feature_guide/notifications/OWNERS b/chrome/android/java/src/org/chromium/chrome/browser/feature_guide/notifications/OWNERS
@@ -0,0 +1 @@
+shaktisahu@chromium.org
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -3116,7 +3116,9 @@ static_library("browser") {
       "enterprise/reporting/reporting_delegate_factory_android.h",
       "enterprise/util/android_enterprise_info.cc",
       "enterprise/util/android_enterprise_info.h",
-      "feature_guide/notifications/internal/android/feature_notification_guide_service_factory_android.cc",
+      "feature_guide/notifications/android/feature_notification_guide_bridge.cc",
+      "feature_guide/notifications/android/feature_notification_guide_bridge.h",
+      "feature_guide/notifications/android/feature_notification_guide_service_factory_android.cc",
       "feed/android/background_refresh_task.cc",
       "feed/android/background_refresh_task.h",
       "feed/android/feed_image_fetch_client.cc",
@@ -3383,7 +3385,6 @@ static_library("browser") {
       "//chrome/browser/device_reauth/android:jni_headers",
       "//chrome/browser/download/internal/android",
       "//chrome/browser/endpoint_fetcher:jni_headers",
-      "//chrome/browser/feature_guide/notifications/internal:jni_headers",
       "//chrome/browser/feed/android:jni_headers",
       "//chrome/browser/feedback/android",
       "//chrome/browser/flags:flags_android",

diff --git a/chrome/browser/feature_guide/notifications/BUILD.gn b/chrome/browser/feature_guide/notifications/BUILD.gn
@@ -10,6 +10,8 @@ if (is_android) {
 
 source_set("public") {
   sources = [
+    "config.cc",
+    "config.h",
     "feature_notification_guide_service.cc",
     "feature_notification_guide_service.h",
     "feature_type.h",
@@ -40,19 +42,6 @@ if (is_android) {
     ]
     srcjar_deps = [ ":jni_enums" ]
   }
-
-  android_library_factory("factory_java") {
-    # These deps will be inherited by the resulting android_library target.
-    deps = [
-      ":java",
-      "//base:base_java",
-      "//chrome/browser/profiles/android:java",
-    ]
-
-    # This internal file will be replaced by a generated file so the resulting
-    # android_library target does not actually depend on this internal file.
-    sources = [ "internal/android/java/src/org/chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideServiceFactory.java" ]
-  }
 }
 
 group("unit_tests") {

diff --git a/...roid/feature_notification_guide_bridge.cc → ...roid/feature_notification_guide_bridge.cc b/...roid/feature_notification_guide_bridge.cc → ...roid/feature_notification_guide_bridge.cc
@@ -2,47 +2,23 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/feature_guide/notifications/internal/android/feature_notification_guide_bridge.h"
+#include "chrome/browser/feature_guide/notifications/android/feature_notification_guide_bridge.h"
 
 #include <string>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
+#include "chrome/android/chrome_jni_headers/FeatureNotificationGuideBridge_jni.h"
 #include "chrome/browser/feature_guide/notifications/feature_notification_guide_service.h"
 #include "chrome/browser/feature_guide/notifications/feature_type.h"
-#include "chrome/browser/feature_guide/notifications/internal/jni_headers/FeatureNotificationGuideBridge_jni.h"
 
 namespace feature_guide {
-namespace {
-const char kFeatureNotificationGuideBridgeKey[] =
-    "feature_notification_guide_bridge";
-}  // namespace
-
-// static
-FeatureNotificationGuideBridge*
-FeatureNotificationGuideBridge::GetFeatureNotificationGuideBridge(
-    FeatureNotificationGuideService* feature_notification_guide_service) {
-  if (!feature_notification_guide_service->GetUserData(
-          kFeatureNotificationGuideBridgeKey)) {
-    feature_notification_guide_service->SetUserData(
-        kFeatureNotificationGuideBridgeKey,
-        std::make_unique<FeatureNotificationGuideBridge>(
-            feature_notification_guide_service));
-  }
-
-  return static_cast<FeatureNotificationGuideBridge*>(
-      feature_notification_guide_service->GetUserData(
-          kFeatureNotificationGuideBridgeKey));
-}
 
 ScopedJavaLocalRef<jobject> FeatureNotificationGuideBridge::GetJavaObj() {
   return ScopedJavaLocalRef<jobject>(java_obj_);
 }
 
-FeatureNotificationGuideBridge::FeatureNotificationGuideBridge(
-    FeatureNotificationGuideService* feature_notification_guide_service)
-    : feature_notification_guide_service_(feature_notification_guide_service) {
-  DCHECK(feature_notification_guide_service);
+FeatureNotificationGuideBridge::FeatureNotificationGuideBridge() {
   JNIEnv* env = base::android::AttachCurrentThread();
   java_obj_.Reset(env, Java_FeatureNotificationGuideBridge_create(
                            env, reinterpret_cast<int64_t>(this))

diff --git a/chrome/browser/feature_guide/notifications/android/feature_notification_guide_bridge.h b/chrome/browser/feature_guide/notifications/android/feature_notification_guide_bridge.h
@@ -0,0 +1,42 @@
+// 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 CHROME_BROWSER_FEATURE_GUIDE_NOTIFICATIONS_ANDROID_FEATURE_NOTIFICATION_GUIDE_BRIDGE_H_
+#define CHROME_BROWSER_FEATURE_GUIDE_NOTIFICATIONS_ANDROID_FEATURE_NOTIFICATION_GUIDE_BRIDGE_H_
+
+#include <string>
+
+#include "base/android/jni_android.h"
+#include "base/supports_user_data.h"
+#include "chrome/browser/feature_guide/notifications/feature_notification_guide_service.h"
+#include "chrome/browser/feature_guide/notifications/feature_type.h"
+
+using base::android::ScopedJavaGlobalRef;
+using base::android::ScopedJavaLocalRef;
+
+namespace feature_guide {
+class FeatureNotificationGuideService;
+
+// Contains JNI methods needed by the feature notification guide.
+class FeatureNotificationGuideBridge
+    : public base::SupportsUserData::Data,
+      public FeatureNotificationGuideService::Delegate {
+ public:
+  FeatureNotificationGuideBridge();
+  ~FeatureNotificationGuideBridge() override;
+
+  ScopedJavaLocalRef<jobject> GetJavaObj();
+  std::u16string GetNotificationTitle(FeatureType feature) override;
+  std::u16string GetNotificationMessage(FeatureType feature) override;
+  void OnNotificationClick(FeatureType feature) override;
+
+ private:
+  // A reference to the Java counterpart of this class.  See
+  // FeatureNotificationGuideBridge.java.
+  ScopedJavaGlobalRef<jobject> java_obj_;
+};
+
+}  // namespace feature_guide
+
+#endif  // CHROME_BROWSER_FEATURE_GUIDE_NOTIFICATIONS_ANDROID_FEATURE_NOTIFICATION_GUIDE_BRIDGE_H_
diff --git a/...feature_guide/notifications/android/feature_notification_guide_service_factory_android.cc b/...feature_guide/notifications/android/feature_notification_guide_service_factory_android.cc
@@ -0,0 +1,26 @@
+// 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.
+
+#include "base/android/scoped_java_ref.h"
+#include "chrome/android/chrome_jni_headers/FeatureNotificationGuideServiceFactory_jni.h"
+#include "chrome/browser/feature_guide/notifications/android/feature_notification_guide_bridge.h"
+#include "chrome/browser/feature_guide/notifications/feature_notification_guide_service_factory.h"
+#include "chrome/browser/feature_guide/notifications/internal/feature_notification_guide_service_impl.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_android.h"
+
+static base::android::ScopedJavaLocalRef<jobject>
+JNI_FeatureNotificationGuideServiceFactory_GetForProfile(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& jprofile) {
+  Profile* profile = ProfileAndroid::FromProfileAndroid(jprofile);
+  DCHECK(profile);
+  feature_guide::FeatureNotificationGuideServiceImpl* service =
+      static_cast<feature_guide::FeatureNotificationGuideServiceImpl*>(
+          feature_guide::FeatureNotificationGuideServiceFactory::GetForProfile(
+              profile->GetOriginalProfile()));
+  return static_cast<feature_guide::FeatureNotificationGuideBridge*>(
+             service->GetDelegate())
+      ->GetJavaObj();
+}
diff --git a/.../chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideService.java b/.../chromium/chrome/browser/feature_guide/notifications/FeatureNotificationGuideService.java
@@ -7,34 +7,4 @@
 /**
  * Central class representing feature notification guide.
  */
-public interface FeatureNotificationGuideService {
-    /**
-     * A delegate containing helper methods needed by the service, such as providing notification
-     * texts, and handling notification interactions.
-     */
-    public interface Delegate {
-        /**
-         * Called when the notification associated with {@code featureType} is clicked.
-         * @param featureType The {@link FeatureType} for the notification.
-         */
-        void onNotificationClick(@FeatureType int featureType);
-
-        /**
-         * Called to get the notification title text associated with {@code featureType}.
-         * @param featureType The {@link FeatureType} for the notification.
-         */
-        String getNotificationTitle(@FeatureType int featureType);
-
-        /**
-         * Called to get the notification body text associated with {@code featureType}.
-         * @param featureType The {@link FeatureType} for the notification.
-         */
-        String getNotificationMessage(@FeatureType int featureType);
-    }
-
-    /**
-     * Called by the embedder to set the delegate.
-     * @param delegate The {@link Delegate} to handle chrome layer logic.
-     */
-    void setDelegate(Delegate delegate);
-}
+public interface FeatureNotificationGuideService {}
diff --git a/chrome/browser/feature_guide/notifications/config.cc b/chrome/browser/feature_guide/notifications/config.cc
@@ -0,0 +1,17 @@
+// 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.
+
+#include "chrome/browser/feature_guide/notifications/config.h"
+
+namespace feature_guide {
+
+Config::Config() = default;
+
+Config::~Config() = default;
+
+Config::Config(const Config& other) = default;
+
+Config& Config::operator=(const Config& other) = default;
+
+}  // namespace feature_guide
diff --git a/chrome/browser/feature_guide/notifications/config.h b/chrome/browser/feature_guide/notifications/config.h
@@ -0,0 +1,33 @@
+// 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 CHROME_BROWSER_FEATURE_GUIDE_NOTIFICATIONS_CONFIG_H_
+#define CHROME_BROWSER_FEATURE_GUIDE_NOTIFICATIONS_CONFIG_H_
+
+#include <vector>
+
+#include "base/time/time.h"
+#include "chrome/browser/feature_guide/notifications/feature_type.h"
+
+namespace feature_guide {
+
+// Contains various finch configuration params used by the feature notification
+// guide.
+struct Config {
+  Config();
+  ~Config();
+
+  Config(const Config& other);
+  Config& operator=(const Config& other);
+
+  // The list of features enabled via finch for showing feature notifications.
+  std::vector<FeatureType> enabled_features;
+
+  // Relative start time for launching the notification.
+  base::TimeDelta notification_deliver_time_delta;
+};
+
+}  // namespace feature_guide
+
+#endif  // CHROME_BROWSER_FEATURE_GUIDE_NOTIFICATIONS_CONFIG_H_
diff --git a/chrome/browser/feature_guide/notifications/feature_notification_guide_service.cc b/chrome/browser/feature_guide/notifications/feature_notification_guide_service.cc
@@ -17,4 +17,16 @@ FeatureNotificationGuideService::FeatureNotificationGuideService() = default;
 
 FeatureNotificationGuideService::~FeatureNotificationGuideService() = default;
 
+FeatureNotificationGuideService::Delegate::~Delegate() = default;
+
+void FeatureNotificationGuideService::Delegate::SetService(
+    FeatureNotificationGuideService* service) {
+  service_ = service;
+}
+
+FeatureNotificationGuideService*
+FeatureNotificationGuideService::Delegate::GetService() {
+  return service_;
+}
+
 }  // namespace feature_guide