Announcement notification: Fix navigation to URL.

Currently when clicking on the "Review" button, on Mac, it will always
not navigate to the expected URL. On Debian, sometimes nothing happens.

On Mac, if the notification has no origin, then click event will not be
forwarded to NotificationHandler. This CL removes the origin check.

Also changed the URL navigation API used to open the URL.

TBR=dtrainor@chromium.org, peter@chromium.org

Bug: 1042124
Change-Id: Ife2c1a5e5949b1d933e4048dfa11a62144ba6bc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023747Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735736}

chrome/android/java/src/org/chromium/chrome/browser/announcement/AnnouncementNotificationManager.java

@@ -9,7 +9,6 @@ import android.content.BroadcastReceiver;
 import android.content.Context;
 import android.content.Intent;
 import android.support.annotation.IntDef;
-import android.text.TextUtils;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.annotations.CalledByNative;
@@ -30,7 +29,6 @@ import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
 import org.chromium.chrome.browser.notifications.PendingIntentProvider;
 import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
 import org.chromium.chrome.browser.util.IntentUtils;
-import org.chromium.ui.base.LocalizationUtils;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
@@ -44,8 +42,7 @@ public class AnnouncementNotificationManager {
     private static final int ANNOUNCEMENT_NOTIFICATION_ID = 100;
     private static final String EXTRA_INTENT_TYPE =
             "org.chromium.chrome.browser.announcement.EXTRA_INTENT_TYPE";
-    private static final String EXTRA_REMOTE_URL =
-            "org.chromium.chrome.browser.announcement.EXTRA_REMOTE_URL";
+    private static final String EXTRA_URL = "org.chromium.chrome.browser.announcement.EXTRA_URL";
 
     @IntDef({IntentType.UNKNOWN, IntentType.CLICK, IntentType.CLOSE, IntentType.ACK,
             IntentType.OPEN})
@@ -70,13 +67,13 @@ public class AnnouncementNotificationManager {
                     @IntentType
                     int intentType = IntentUtils.safeGetIntExtra(
                             intent, EXTRA_INTENT_TYPE, IntentType.UNKNOWN);
-                    String remoteUrl = IntentUtils.safeGetStringExtra(intent, EXTRA_REMOTE_URL);
+                    String url = IntentUtils.safeGetStringExtra(intent, EXTRA_URL);
                     switch (intentType) {
                         case IntentType.UNKNOWN:
                             break;
                         case IntentType.CLICK:
                             recordHistogram(AnnouncementNotificationEvent.CLICK);
-                            openUrl(context, remoteUrl);
+                            openUrl(context, url);
                             break;
                         case IntentType.CLOSE:
                             recordHistogram(AnnouncementNotificationEvent.CLOSE);
@@ -87,7 +84,7 @@ public class AnnouncementNotificationManager {
                             break;
                         case IntentType.OPEN:
                             recordHistogram(AnnouncementNotificationEvent.OPEN);
-                            openUrl(context, remoteUrl);
+                            openUrl(context, url);
                             close();
                             break;
                     }
@@ -101,21 +98,17 @@ public class AnnouncementNotificationManager {
     }
 
     private static PendingIntentProvider createIntent(
-            Context context, @IntentType int intentType, String remoteUrl) {
+            Context context, @IntentType int intentType, String url) {
         Intent intent = new Intent(context, AnnouncementNotificationManager.Receiver.class);
         intent.putExtra(EXTRA_INTENT_TYPE, intentType);
-        intent.putExtra(EXTRA_REMOTE_URL, remoteUrl);
+        intent.putExtra(EXTRA_URL, url);
         return PendingIntentProvider.getBroadcast(
                 context, intentType /*requestCode*/, intent, PendingIntent.FLAG_UPDATE_CURRENT);
     }
 
-    private static void openUrl(Context context, String remoteUrl) {
+    private static void openUrl(Context context, String url) {
         // Open the announcement URL, fallback to default URL if remote URL is empty.
-        String url = TextUtils.isEmpty(remoteUrl)
-                ? context.getResources().getString(R.string.tos_notification_link)
-                : remoteUrl;
-        CustomTabActivity.showInfoPage(
-                context, LocalizationUtils.substituteLocalePlaceholder((url)));
+        CustomTabActivity.showInfoPage(context, url);
     }
 
     private static void close() {
@@ -126,11 +119,11 @@ public class AnnouncementNotificationManager {
 
     private static void recordHistogram(@AnnouncementNotificationEvent int event) {
         RecordHistogram.recordEnumeratedHistogram("Notifications.Announcement.Events", event,
-                org.chromium.chrome.browser.announcement.AnnouncementNotificationEvent.MAX);
+                org.chromium.chrome.browser.announcement.AnnouncementNotificationEvent.MAX_VALUE);
     }
 
     @CalledByNative
-    private static void showNotification(String remoteUrl) {
+    private static void showNotification(String url) {
         Context context = ContextUtils.getApplicationContext();
         ChromeNotificationBuilder builder =
                 NotificationBuilderFactory
@@ -142,18 +135,18 @@ public class AnnouncementNotificationManager {
                                         ANNOUNCEMENT_NOTIFICATION_TAG,
                                         ANNOUNCEMENT_NOTIFICATION_ID))
                         .setContentTitle(context.getString(R.string.tos_notification_title))
-                        .setContentIntent(createIntent(context, IntentType.CLICK, remoteUrl))
-                        .setDeleteIntent(createIntent(context, IntentType.CLOSE, remoteUrl))
+                        .setContentIntent(createIntent(context, IntentType.CLICK, url))
+                        .setDeleteIntent(createIntent(context, IntentType.CLOSE, url))
                         .setContentText(context.getString(R.string.tos_notification_body_text))
                         .setSmallIcon(R.drawable.ic_chrome)
                         .setShowWhen(false)
                         .setAutoCancel(true)
                         .setLocalOnly(true);
         builder.addAction(0, context.getString(R.string.tos_notification_ack_button_text),
-                createIntent(context, IntentType.ACK, remoteUrl),
+                createIntent(context, IntentType.ACK, url),
                 NotificationUmaTracker.ActionType.ANNOUNCEMENT_ACK);
         builder.addAction(0, context.getString(R.string.tos_notification_review_button_text),
-                createIntent(context, IntentType.OPEN, remoteUrl),
+                createIntent(context, IntentType.OPEN, url),
                 NotificationUmaTracker.ActionType.ANNOUNCEMENT_OPEN);
 
         NotificationManagerProxy nm = new NotificationManagerProxyImpl(context);

chrome/android/java/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitions.java

@@ -76,7 +76,7 @@ public class ChannelDefinitions {
         String COMPLETED_DOWNLOADS = "completed_downloads";
         String PERMISSION_REQUESTS = "permission_requests";
         String PERMISSION_REQUESTS_HIGH = "permission_requests_high";
-        String ANNOUNCEMENT = "Announcement";
+        String ANNOUNCEMENT = "announcement";
     }
 
     @StringDef({

chrome/app/generated_resources.grd

@@ -1233,7 +1233,7 @@ are declared in tools/grit/grit_rule.gni.
       <message name="IDS_TOS_NOTIFICATION_REVIEW_BUTTON_TEXT" desc="The button text on the TOS notification for the user to review the TOS change." formatter_data="android_java">
         Review
       </message>
-      <message name="IDS_TOS_NOTIFICATION_LINK" desc="The URL for the new TOS change link." translateable="false" formatter_data="android_java">
+      <message name="IDS_TOS_NOTIFICATION_LINK" desc="The URL for the new TOS change link." translateable="false">
         https://www.google.com/chrome/privacy/eula_text.html
       </message>
 

chrome/browser/notifications/notification_platform_bridge_mac.mm

@@ -460,16 +460,6 @@ bool NotificationPlatformBridgeMac::VerifyNotificationData(
     return false;
   }
 
-  // Origin is not actually required but if it's there it should be a valid one.
-  NSString* origin =
-      [response objectForKey:notification_constants::kNotificationOrigin];
-  if (origin) {
-    std::string notificationOrigin = base::SysNSStringToUTF8(origin);
-    GURL url(notificationOrigin);
-    if (!url.is_valid())
-      return false;
-  }
-
   return true;
 }
 

chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm

@@ -206,17 +206,6 @@ TEST_F(NotificationPlatformBridgeMacTest,
   EXPECT_FALSE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
 }
 
-TEST_F(NotificationPlatformBridgeMacTest, TestNotificationVerifyOrigin) {
-  NSMutableDictionary* response = BuildDefaultNotificationResponse();
-  [response setValue:@"invalidorigin"
-              forKey:notification_constants::kNotificationOrigin];
-  EXPECT_FALSE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
-
-  // If however the origin is not present the response should be fine.
-  [response removeObjectForKey:notification_constants::kNotificationOrigin];
-  EXPECT_TRUE(NotificationPlatformBridgeMac::VerifyNotificationData(response));
-}
-
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayNoButtons) {
   std::unique_ptr<Notification> notification =
       CreateBanner("Title", "Context", "https://gmail.com", nullptr, nullptr);

chrome/browser/updates/announcement_notification/announcement_notification_delegate.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/updates/announcement_notification/announcement_notification_delegate.h"
 
+#include <string>
+
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/first_run/first_run.h"
 #include "chrome/browser/notifications/notification_display_service.h"
@@ -19,15 +21,14 @@ AnnouncementNotificationDelegate::AnnouncementNotificationDelegate(
 
 AnnouncementNotificationDelegate::~AnnouncementNotificationDelegate() = default;
 
-void AnnouncementNotificationDelegate::ShowNotification(
-    const std::string& remote_url) {
+void AnnouncementNotificationDelegate::ShowNotification() {
   auto rich_notification_data = message_center::RichNotificationData();
   message_center::ButtonInfo button1(
       l10n_util::GetStringUTF16(IDS_TOS_NOTIFICATION_ACK_BUTTON_TEXT));
   message_center::ButtonInfo button2(
       l10n_util::GetStringUTF16(IDS_TOS_NOTIFICATION_REVIEW_BUTTON_TEXT));
-  rich_notification_data.buttons.emplace_back(button1);
-  rich_notification_data.buttons.emplace_back(button2);
+  rich_notification_data.buttons.push_back(button1);
+  rich_notification_data.buttons.push_back(button2);
 
   message_center::Notification notification(
       message_center::NOTIFICATION_TYPE_SIMPLE, kAnnouncementNotificationId,

chrome/browser/updates/announcement_notification/announcement_notification_delegate.h

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_UPDATES_ANNOUNCEMENT_NOTIFICATION_ANNOUNCEMENT_NOTIFICATION_DELEGATE_H_
 #define CHROME_BROWSER_UPDATES_ANNOUNCEMENT_NOTIFICATION_ANNOUNCEMENT_NOTIFICATION_DELEGATE_H_
 
-#include <string>
-
 #include "base/macros.h"
 #include "chrome/browser/updates/announcement_notification/announcement_notification_service.h"
 
@@ -26,7 +24,7 @@ class AnnouncementNotificationDelegate
 
  private:
   // AnnouncementNotificationService::Delegate implementation.
-  void ShowNotification(const std::string& remote_url) override;
+  void ShowNotification() override;
   bool IsFirstRun() override;
 
   // Used to show the notification.

chrome/browser/updates/announcement_notification/announcement_notification_delegate_android.cc

@@ -17,11 +17,11 @@ AnnouncementNotificationDelegateAndroid::
 AnnouncementNotificationDelegateAndroid::
     ~AnnouncementNotificationDelegateAndroid() = default;
 
-void AnnouncementNotificationDelegateAndroid::ShowNotification(
-    const std::string& remote_url) {
+void AnnouncementNotificationDelegateAndroid::ShowNotification() {
   auto* env = base::android::AttachCurrentThread();
+  GURL url = AnnouncementNotificationService::GetAnnouncementURL();
   Java_AnnouncementNotificationManager_showNotification(
-      env, base::android::ConvertUTF8ToJavaString(env, remote_url));
+      env, base::android::ConvertUTF8ToJavaString(env, url.spec()));
 }
 
 bool AnnouncementNotificationDelegateAndroid::IsFirstRun() {

chrome/browser/updates/announcement_notification/announcement_notification_delegate_android.h

@@ -16,7 +16,7 @@ class AnnouncementNotificationDelegateAndroid
 
  private:
   // AnnouncementNotificationService::Delegate implementation.
-  void ShowNotification(const std::string& remote_url) override;
+  void ShowNotification() override;
   bool IsFirstRun() override;
 
   DISALLOW_COPY_AND_ASSIGN(AnnouncementNotificationDelegateAndroid);

chrome/browser/updates/announcement_notification/announcement_notification_handler.cc

@@ -8,11 +8,10 @@
 #include "chrome/browser/notifications/notification_display_service.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
+#include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/browser/updates/announcement_notification/announcement_notification_delegate.h"
 #include "chrome/browser/updates/announcement_notification/announcement_notification_metrics.h"
-#include "chrome/grit/generated_resources.h"
-#include "ui/base/l10n/l10n_util.h"
 
 namespace {
 
@@ -71,17 +70,9 @@ void AnnouncementNotificationHandler::OnClick(
 
 void AnnouncementNotificationHandler::OpenAnnouncement(Profile* profile) {
   // Open the announcement URL in a new tab.
-  // Fallback to default URL if |remote_url| is empty.
-  // TODO(xingliu): Pass |remote_url| from AnnouncementNotificationService.
-  std::string remote_url = base::GetFieldTrialParamValueByFeature(
-      kAnnouncementNotification, kAnnouncementUrl);
-  std::string url = remote_url.empty()
-                        ? l10n_util::GetStringUTF8(IDS_TOS_NOTIFICATION_LINK)
-                        : remote_url;
-  chrome::ScopedTabbedBrowserDisplayer browser_displayer(profile);
-  content::OpenURLParams params(GURL(url), content::Referrer(),
-                                WindowOpenDisposition::NEW_FOREGROUND_TAB,
-                                ui::PageTransition::PAGE_TRANSITION_FIRST,
-                                false /*is_renderer_initiated*/);
-  browser_displayer.browser()->OpenURL(params);
+  GURL url = AnnouncementNotificationService::GetAnnouncementURL();
+  NavigateParams params(profile, url, ui::PAGE_TRANSITION_LINK);
+  params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+  params.window_action = NavigateParams::SHOW_WINDOW;
+  Navigate(&params);
 }

chrome/browser/updates/announcement_notification/announcement_notification_metrics.cc

@@ -7,6 +7,5 @@
 #include "base/metrics/histogram_macros.h"
 
 void RecordAnnouncementHistogram(AnnouncementNotificationEvent event) {
-  UMA_HISTOGRAM_ENUMERATION("Notifications.Announcement.Events", event,
-                            AnnouncementNotificationEvent::kMax);
+  UMA_HISTOGRAM_ENUMERATION("Notifications.Announcement.Events", event);
 }

chrome/browser/updates/announcement_notification/announcement_notification_metrics.h

@@ -23,7 +23,7 @@ enum class AnnouncementNotificationEvent {
   kAck = 4,
   // The open button is clicked.
   kOpen = 5,
-  kMax = 6,
+  kMaxValue = kOpen,
 };
 
 // Records announcement notification event.

chrome/browser/updates/announcement_notification/announcement_notification_service.cc

@@ -14,8 +14,10 @@
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/updates/announcement_notification/announcement_notification_metrics.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace {
 
@@ -121,7 +123,7 @@ class AnnouncementNotificationServiceImpl
       return;
 
     notification_shown = true;
-    delegate_->ShowNotification(remote_url_);
+    delegate_->ShowNotification();
   }
 
   bool IsUserSignIn() {
@@ -195,6 +197,17 @@ AnnouncementNotificationService* AnnouncementNotificationService::Create(
       profile_path, new_profile, pref_service, std::move(delegate));
 }
 
+// static
+GURL AnnouncementNotificationService::GetAnnouncementURL() {
+  std::string remote_url = base::GetFieldTrialParamValueByFeature(
+      kAnnouncementNotification, kAnnouncementUrl);
+  // Fallback to default URL if |remote_url| is empty.
+  std::string url = remote_url.empty()
+                        ? l10n_util::GetStringUTF8(IDS_TOS_NOTIFICATION_LINK)
+                        : remote_url;
+  return GURL(url);
+}
+
 AnnouncementNotificationService::AnnouncementNotificationService() = default;
 
 AnnouncementNotificationService::~AnnouncementNotificationService() = default;

chrome/browser/updates/announcement_notification/announcement_notification_service.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "url/gurl.h"
 
 namespace {
 class FilePath;
@@ -62,8 +63,8 @@ class AnnouncementNotificationService : public KeyedService {
     Delegate() = default;
     virtual ~Delegate() = default;
 
-    // Show notification. Fallback to default URL when |remote_url| is empty.
-    virtual void ShowNotification(const std::string& remote_url) = 0;
+    // Show notification.
+    virtual void ShowNotification() = 0;
 
     // Is Chrome first time to run.
     virtual bool IsFirstRun() = 0;
@@ -78,6 +79,7 @@ class AnnouncementNotificationService : public KeyedService {
       bool new_profile,
       PrefService* pref_service,
       std::unique_ptr<Delegate> delegate);
+  static GURL GetAnnouncementURL();
 
   AnnouncementNotificationService();
   ~AnnouncementNotificationService() override;

chrome/browser/updates/announcement_notification/announcement_notification_service_unittest.cc

@@ -34,7 +34,7 @@ class MockDelegate : public AnnouncementNotificationService::Delegate {
  public:
   MockDelegate() = default;
   ~MockDelegate() override = default;
-  MOCK_METHOD1(ShowNotification, void(const std::string&));
+  MOCK_METHOD0(ShowNotification, void());
   MOCK_METHOD0(IsFirstRun, bool());
 
  private:
@@ -129,7 +129,7 @@ TEST_F(AnnouncementNotificationServiceTest, RequireSignOut) {
   Init(parameters, true, 1, false);
 
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
-  EXPECT_CALL(*delegate(), ShowNotification(_)).Times(0);
+  EXPECT_CALL(*delegate(), ShowNotification()).Times(0);
   service()->MaybeShowNotification();
   EXPECT_EQ(CurrentVersionPref(), 2);
 }
@@ -142,7 +142,7 @@ TEST_F(AnnouncementNotificationServiceTest, SkipNewProfile) {
   Init(parameters, true, 1, true /*new_profile*/);
 
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
-  EXPECT_CALL(*delegate(), ShowNotification(_)).Times(0);
+  EXPECT_CALL(*delegate(), ShowNotification()).Times(0);
   service()->MaybeShowNotification();
   EXPECT_EQ(CurrentVersionPref(), 2);
 }
@@ -157,7 +157,7 @@ TEST_F(AnnouncementNotificationServiceTest, RemoteUrl) {
   InitProfile(false);
   Init(parameters, true, 1, false);
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(false));
-  EXPECT_CALL(*delegate(), ShowNotification(kRemoteUrl));
+  EXPECT_CALL(*delegate(), ShowNotification());
   service()->MaybeShowNotification();
   EXPECT_EQ(CurrentVersionPref(), 4);
 }
@@ -208,7 +208,7 @@ TEST_P(AnnouncementNotificationServiceVersionTest, VersionTest) {
   Init(param.enable_feature, param.skip_first_run, param.version,
        param.current_version);
   ON_CALL(*delegate(), IsFirstRun()).WillByDefault(Return(param.is_first_run));
-  EXPECT_CALL(*delegate(), ShowNotification(std::string()))
+  EXPECT_CALL(*delegate(), ShowNotification())
       .Times(param.show_notification_called ? 1 : 0);
   service()->MaybeShowNotification();
   EXPECT_EQ(CurrentVersionPref(), param.expected_version_pref);