[Nearby] Suppress 'discoverable' Bluetooth notification during sharing.

If Nearby Share is actively broadcasting itself as able to receive
content (by making the device discoverable over Bluetooth), do not
display the "Your device is discoverable..." notification.

This change requires the addition of a new method,
NearbyShareDelegateImpl::IsEnableHighVisibilityRequestActive().
Using the existing method
NearbyShareDelegateImpl::IsHighVisibilityOn() is not sufficient
because BluetoothNotificationController::AdapterDiscoverableChanged()
is triggered before NearbyShareDelegateImpl::IsHighVisibilityOn()
becomes true.

Making generic Bluetooth UI logic (BluetoothNotificationController)
aware of the inner workings of Nearby is not ideal, but is the
quickest solution we have available before launch. We will clean
up this behavior leak in crbug.com/1155669.

Bug: 1155668
Change-Id: I3174d9e1af777b97ef6e5daed9a80f75831ca41f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2566407
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#833897}

ash/public/cpp/nearby_share_delegate.h

@@ -27,6 +27,11 @@ class ASH_PUBLIC_EXPORT NearbyShareDelegate {
   // Gets the current high visibility state from the NearbySharingService.
   virtual bool IsHighVisibilityOn() = 0;
 
+  // Returns true if EnableHighVisibility() has been called but
+  // NearbyShareDelegate has not yet been informed that the request has
+  // concluded.
+  virtual bool IsEnableHighVisibilityRequestActive() const = 0;
+
   // If high visibility is on, returns the time when the delegate
   // will turn it off. May return any value if high visibility is off.
   virtual base::TimeTicks HighVisibilityShutoffTime() const = 0;

ash/public/cpp/test/test_nearby_share_delegate.cc

@@ -20,6 +20,10 @@ bool TestNearbyShareDelegate::IsHighVisibilityOn() {
   return is_high_visibility_on_;
 }
 
+bool TestNearbyShareDelegate::IsEnableHighVisibilityRequestActive() const {
+  return is_enable_high_visibility_request_active_;
+}
+
 base::TimeTicks TestNearbyShareDelegate::HighVisibilityShutoffTime() const {
   return high_visibility_shutoff_time_;
 }

ash/public/cpp/test/test_nearby_share_delegate.h

@@ -28,6 +28,7 @@ class ASH_PUBLIC_EXPORT TestNearbyShareDelegate : public NearbyShareDelegate {
   // NearbyShareDelegate
   bool IsPodButtonVisible() override;
   bool IsHighVisibilityOn() override;
+  bool IsEnableHighVisibilityRequestActive() const override;
   base::TimeTicks HighVisibilityShutoffTime() const override;
   void EnableHighVisibility() override;
   void DisableHighVisibility() override;
@@ -37,6 +38,12 @@ class ASH_PUBLIC_EXPORT TestNearbyShareDelegate : public NearbyShareDelegate {
     is_pod_button_visible_ = visible;
   }
 
+  void set_is_enable_high_visibility_request_active(
+      bool is_enable_high_visibility_request_active) {
+    is_enable_high_visibility_request_active_ =
+        is_enable_high_visibility_request_active;
+  }
+
   void set_is_high_visibility_on(bool on) { is_high_visibility_on_ = on; }
 
   void set_high_visibility_shutoff_time(base::TimeTicks time) {
@@ -47,6 +54,7 @@ class ASH_PUBLIC_EXPORT TestNearbyShareDelegate : public NearbyShareDelegate {
 
  private:
   bool is_pod_button_visible_ = false;
+  bool is_enable_high_visibility_request_active_ = false;
   bool is_high_visibility_on_ = false;
   base::TimeTicks high_visibility_shutoff_time_;
   std::vector<Method> method_calls_;

ash/system/bluetooth/bluetooth_notification_controller.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "ash/public/cpp/nearby_share_delegate.h"
 #include "ash/public/cpp/notification_utils.h"
 #include "ash/public/cpp/system_tray_client.h"
 #include "ash/resources/vector_icons/vector_icons.h"
@@ -296,8 +297,18 @@ void BluetoothNotificationController::OnGetAdapter(
 }
 
 void BluetoothNotificationController::NotifyAdapterDiscoverable() {
-  message_center::RichNotificationData optional;
+  // If Nearby Share has made the local device discoverable, do not
+  // unnecessarily display this notification.
+  // TODO(crbug.com/1155669): Generalize this logic to prevent leaking Nearby
+  // implementation details.
+  auto* nearby_share_delegate = Shell::Get()->nearby_share_delegate();
+  if (nearby_share_delegate &&
+      (nearby_share_delegate->IsEnableHighVisibilityRequestActive() ||
+       nearby_share_delegate->IsHighVisibilityOn())) {
+    return;
+  }
 
+  message_center::RichNotificationData optional;
   std::unique_ptr<Notification> notification = CreateSystemNotification(
       message_center::NOTIFICATION_TYPE_SIMPLE,
       kBluetoothDeviceDiscoverableNotificationId, base::string16() /* title */,

ash/system/bluetooth/bluetooth_notification_controller_unittest.cc

@@ -7,8 +7,10 @@
 #include <memory>
 #include <utility>
 
+#include "ash/public/cpp/test/test_nearby_share_delegate.h"
 #include "ash/public/cpp/test/test_system_tray_client.h"
 #include "ash/session/test_session_controller_client.h"
+#include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/test/ash_test_base.h"
@@ -16,17 +18,23 @@
 #include "base/containers/flat_map.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
 #include "device/bluetooth/test/mock_bluetooth_device.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/message_center/fake_message_center.h"
 
+using testing::NiceMock;
 using testing::Return;
 
 namespace ash {
 namespace {
 
+const char kTestAdapterName[] = "Chromebook";
+const char kTestAdapterAddress[] = "01:23:45:67:89:AB";
+
 class TestMessageCenter : public message_center::FakeMessageCenter {
  public:
   TestMessageCenter() = default;
@@ -51,19 +59,29 @@ class BluetoothNotificationControllerTest : public AshTestBase {
 
   void SetUp() override {
     AshTestBase::SetUp();
+
+    mock_adapter_ =
+        base::MakeRefCounted<NiceMock<device::MockBluetoothAdapter>>();
+    ON_CALL(*mock_adapter_, IsPresent()).WillByDefault(Return(true));
+    ON_CALL(*mock_adapter_, IsPowered()).WillByDefault(Return(true));
+    ON_CALL(*mock_adapter_, GetName()).WillByDefault(Return(kTestAdapterName));
+    ON_CALL(*mock_adapter_, GetAddress())
+        .WillByDefault(Return(kTestAdapterAddress));
+    device::BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter_);
+
     notification_controller_ =
         std::make_unique<BluetoothNotificationController>(
             &test_message_center_);
     system_tray_client_ = GetSystemTrayClient();
 
     bluetooth_device_1_ =
-        std::make_unique<testing::NiceMock<device::MockBluetoothDevice>>(
-            nullptr /* adapter */, 0 /* bluetooth_class */, "name_1",
-            "address_1", false /* paired */, false /* connected */);
+        std::make_unique<NiceMock<device::MockBluetoothDevice>>(
+            mock_adapter_.get(), 0 /* bluetooth_class */, "name_1", "address_1",
+            false /* paired */, false /* connected */);
     bluetooth_device_2_ =
-        std::make_unique<testing::NiceMock<device::MockBluetoothDevice>>(
-            nullptr /* adapter */, 0 /* bluetooth_class */, "name_2",
-            "address_2", false /* paired */, false /* connected */);
+        std::make_unique<NiceMock<device::MockBluetoothDevice>>(
+            mock_adapter_.get(), 0 /* bluetooth_class */, "name_2", "address_2",
+            false /* paired */, false /* connected */);
   }
 
   void ClickPairedNotification(const device::BluetoothDevice* device) {
@@ -78,6 +96,26 @@ class BluetoothNotificationControllerTest : public AshTestBase {
         by_user);
   }
 
+  void VerifyDiscoverableNotificationIsNotVisible() {
+    EXPECT_FALSE(test_message_center_.FindVisibleNotificationById(
+        BluetoothNotificationController::
+            kBluetoothDeviceDiscoverableNotificationId));
+  }
+
+  void VerifyDiscoverableNotificationIsVisible() {
+    message_center::Notification* visible_notification =
+        test_message_center_.FindVisibleNotificationById(
+            BluetoothNotificationController::
+                kBluetoothDeviceDiscoverableNotificationId);
+    EXPECT_TRUE(visible_notification);
+    EXPECT_EQ(base::string16(), visible_notification->title());
+    EXPECT_EQ(
+        l10n_util::GetStringFUTF16(IDS_ASH_STATUS_TRAY_BLUETOOTH_DISCOVERABLE,
+                                   base::UTF8ToUTF16(kTestAdapterName),
+                                   base::UTF8ToUTF16(kTestAdapterAddress)),
+        visible_notification->message());
+  }
+
   void VerifyPairedNotificationIsNotVisible(
       const device::BluetoothDevice* device) {
     EXPECT_FALSE(test_message_center_.FindVisibleNotificationById(
@@ -98,6 +136,11 @@ class BluetoothNotificationControllerTest : public AshTestBase {
 
   // Run the notification controller to simulate showing a notification by
   // adding it to the TestMessageCenter.
+  void ShowDiscoverableNotification(
+      BluetoothNotificationController* notification_controller) {
+    notification_controller->NotifyAdapterDiscoverable();
+  }
+
   void ShowPairedNotification(
       BluetoothNotificationController* notification_controller,
       device::MockBluetoothDevice* bluetooth_device) {
@@ -105,6 +148,7 @@ class BluetoothNotificationControllerTest : public AshTestBase {
   }
 
   TestMessageCenter test_message_center_;
+  scoped_refptr<device::MockBluetoothAdapter> mock_adapter_;
   std::unique_ptr<BluetoothNotificationController> notification_controller_;
   TestSystemTrayClient* system_tray_client_;
   std::unique_ptr<device::MockBluetoothDevice> bluetooth_device_1_;
@@ -113,6 +157,40 @@ class BluetoothNotificationControllerTest : public AshTestBase {
   DISALLOW_COPY_AND_ASSIGN(BluetoothNotificationControllerTest);
 };
 
+TEST_F(BluetoothNotificationControllerTest, DiscoverableNotification) {
+  VerifyDiscoverableNotificationIsNotVisible();
+
+  ShowDiscoverableNotification(notification_controller_.get());
+
+  VerifyDiscoverableNotificationIsVisible();
+}
+
+TEST_F(BluetoothNotificationControllerTest,
+       DiscoverableNotification_NearbyShareEnableHighVisibilityRequestActive) {
+  VerifyDiscoverableNotificationIsNotVisible();
+
+  auto* nearby_share_delegate_ = static_cast<TestNearbyShareDelegate*>(
+      Shell::Get()->nearby_share_delegate());
+  nearby_share_delegate_->set_is_enable_high_visibility_request_active(true);
+
+  ShowDiscoverableNotification(notification_controller_.get());
+
+  VerifyDiscoverableNotificationIsNotVisible();
+}
+
+TEST_F(BluetoothNotificationControllerTest,
+       DiscoverableNotification_NearbyShareHighVisibilityOn) {
+  VerifyDiscoverableNotificationIsNotVisible();
+
+  auto* nearby_share_delegate_ = static_cast<TestNearbyShareDelegate*>(
+      Shell::Get()->nearby_share_delegate());
+  nearby_share_delegate_->set_is_high_visibility_on(true);
+
+  ShowDiscoverableNotification(notification_controller_.get());
+
+  VerifyDiscoverableNotificationIsNotVisible();
+}
+
 TEST_F(BluetoothNotificationControllerTest,
        PairedDeviceNotification_TapNotification) {
   // Show the notification to the user.

chrome/browser/nearby_sharing/nearby_share_delegate_impl.cc

@@ -59,6 +59,10 @@ bool NearbyShareDelegateImpl::IsHighVisibilityOn() {
   return nearby_share_service_ && nearby_share_service_->IsInHighVisibility();
 }
 
+bool NearbyShareDelegateImpl::IsEnableHighVisibilityRequestActive() const {
+  return is_enable_high_visibility_request_active_;
+}
+
 base::TimeTicks NearbyShareDelegateImpl::HighVisibilityShutoffTime() const {
   return shutoff_time_;
 }
@@ -72,6 +76,8 @@ void NearbyShareDelegateImpl::EnableHighVisibility() {
   if (!nearby_share_service_->GetSettings()->GetEnabled()) {
     onboarding_wait_timer_.Reset();
   }
+
+  is_enable_high_visibility_request_active_ = true;
 }
 
 void NearbyShareDelegateImpl::DisableHighVisibility() {
@@ -127,6 +133,8 @@ void NearbyShareDelegateImpl::OnEnabledChanged(bool enabled) {
 }
 
 void NearbyShareDelegateImpl::OnHighVisibilityChanged(bool high_visibility_on) {
+  is_enable_high_visibility_request_active_ = false;
+
   if (high_visibility_on) {
     shutoff_time_ = base::TimeTicks::Now() + kShutoffTimeout;
     shutoff_timer_.Reset();

chrome/browser/nearby_sharing/nearby_share_delegate_impl.h

@@ -56,6 +56,7 @@ class NearbyShareDelegateImpl
   // ash::NearbyShareDelegate
   bool IsPodButtonVisible() override;
   bool IsHighVisibilityOn() override;
+  bool IsEnableHighVisibilityRequestActive() const override;
   base::TimeTicks HighVisibilityShutoffTime() const override;
   void EnableHighVisibility() override;
   void DisableHighVisibility() override;
@@ -92,6 +93,10 @@ class NearbyShareDelegateImpl
   NearbySharingService* nearby_share_service_ = nullptr;
   std::unique_ptr<SettingsOpener> settings_opener_;
 
+  // Track if there is an outstanding request to enable high visibility. Reset
+  // to false once the request finishes (via OnHighVisibilityChanged());
+  bool is_enable_high_visibility_request_active_ = false;
+
   // This timer is used to automatically turn off high visibility after a
   // timeout.
   base::RetainingOneShotTimer shutoff_timer_;

chrome/browser/nearby_sharing/nearby_share_delegate_impl_unittest.cc

@@ -139,6 +139,18 @@ TEST_F(NearbyShareDelegateImplTest, StartStopHighVisibility) {
   SetHighVisibilityOn(false);
 }
 
+TEST_F(NearbyShareDelegateImplTest, TestIsEnableHighVisibilityRequestActive) {
+  settings()->SetEnabled(true);
+
+  EXPECT_CALL(*settings_opener_, ShowSettingsPage(_));
+  EXPECT_CALL(controller_, HighVisibilityEnabledChanged(true));
+
+  delegate_.EnableHighVisibility();
+  EXPECT_TRUE(delegate_.IsEnableHighVisibilityRequestActive());
+  SetHighVisibilityOn(true);
+  EXPECT_FALSE(delegate_.IsEnableHighVisibilityRequestActive());
+}
+
 TEST_F(NearbyShareDelegateImplTest, ShowOnboardingAndTurnOnHighVisibility) {
   settings()->SetEnabled(false);