Reland "Delay SharedClipboardShareActivity init if DeviceInfoProvider not loaded"

This reverts commit 74ae0216.

Reason for revert: Looks like not a root cause

Original change's description:
> Revert "Delay SharedClipboardShareActivity init if DeviceInfoProvider not loaded"
> 
> This reverts commit 0b7170d7.
> 
> Reason for revert: Speculative: suspected of causing failures of
> InfoCollection_basic
> InfoCollection_direct_composition
> https://ci.chromium.org/p/chromium/builders/ci/Win10%20x64%20Debug%20%28NVIDIA%29/829?blamelist=1#blamelist-tab
> 
> Original change's description:
> > Delay SharedClipboardShareActivity init if DeviceInfoProvider not loaded
> > 
> > - Add a JNI method to callback when both DeviceInfoTracker and
> >   LocalDeviceInfoProvider are ready
> > - Also removed redundant setVibration in SharingNotificationUtil
> > 
> > Bug: 996318
> > Change-Id: Id4df6b407ac2fb1c1622071ffb7ee8bb3a747b41
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1787379
> > Commit-Queue: Alex Chau <alexchau@chromium.org>
> > Reviewed-by: Marc Treib <treib@chromium.org>
> > Reviewed-by: Richard Knoll <knollr@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#694208}
> 
> TBR=treib@chromium.org,alexchau@chromium.org,knollr@chromium.org
> 
> Change-Id: I3837352ddf7345751b679f4065b0c00de28d5d43
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 996318
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789234
> Reviewed-by: Olga Sharonova <olka@chromium.org>
> Commit-Queue: Olga Sharonova <olka@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#694223}

TBR=treib@chromium.org,olka@chromium.org,alexchau@chromium.org,knollr@chromium.org

Change-Id: I377486659e744c6d9f69659e845d6b5d88253fd7
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 996318
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789533Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694247}

chrome/android/java/src/org/chromium/chrome/browser/sharing/SharingNotificationUtil.java

@@ -94,7 +94,6 @@ public final class SharingNotificationUtil {
                         .setColor(ApiCompatibilityUtils.getColor(context.getResources(), color))
                         .setGroup(group)
                         .setPriorityBeforeO(NotificationCompat.PRIORITY_HIGH)
-                        .setVibrate(new long[0])
                         .setSmallIcon(smallIconId)
                         .setAutoCancel(true)
                         .setDefaults(Notification.DEFAULT_ALL);
@@ -151,7 +150,6 @@ public final class SharingNotificationUtil {
                         .setColor(ApiCompatibilityUtils.getColor(
                                 context.getResources(), R.color.default_icon_color_blue))
                         .setPriorityBeforeO(NotificationCompat.PRIORITY_HIGH)
-                        .setVibrate(new long[0])
                         .setSmallIcon(R.drawable.ic_devices_16dp)
                         .setProgress(/*max=*/0, /*percentage=*/0, true)
                         .setOngoing(true)
@@ -197,7 +195,6 @@ public final class SharingNotificationUtil {
                         .setGroup(group)
                         .setColor(ApiCompatibilityUtils.getColor(resources, R.color.google_red_600))
                         .setPriorityBeforeO(NotificationCompat.PRIORITY_HIGH)
-                        .setVibrate(new long[0])
                         .setSmallIcon(R.drawable.ic_error_outline_red_24dp)
                         .setContentText(contentText)
                         .setDefaults(Notification.DEFAULT_ALL)

chrome/android/java/src/org/chromium/chrome/browser/sharing/SharingServiceProxy.java

@@ -95,7 +95,7 @@ public class SharingServiceProxy {
 
     /**
      * Returns a list of devices for with given capabilities.
-     * @param capability A bitmask of capabilities created from.
+     * @param capabilities A bitmask of capabilities created from.
      *         org.chromium.chrome.browser.sharing.SharingDeviceCapability enum values.
      */
     public ArrayList<DeviceInfo> getDeviceCandidates(int capabilities) {
@@ -109,6 +109,16 @@ public class SharingServiceProxy {
         return deviceInfo;
     }
 
+    public void addDeviceCandidatesInitializedObserver(Runnable callback) {
+        if (sNativeSharingServiceProxyAndroid == 0) {
+            callback.run();
+            return;
+        }
+
+        Natives jni = SharingServiceProxyJni.get();
+        jni.addDeviceCandidatesInitializedObserver(sNativeSharingServiceProxyAndroid, callback);
+    }
+
     @NativeMethods
     interface Natives {
         void initSharingService(Profile profile);
@@ -116,5 +126,7 @@ public class SharingServiceProxy {
                 String text, Callback<Integer> callback);
         void getDeviceCandidates(long nativeSharingServiceProxyAndroid,
                 ArrayList<DeviceInfo> deviceInfo, int capabilities);
+        void addDeviceCandidatesInitializedObserver(
+                long nativeSharingServiceProxyAndroid, Runnable runnable);
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/sharing/shared_clipboard/SharedClipboardShareActivity.java

@@ -23,6 +23,7 @@ import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.init.AsyncInitializationActivity;
 import org.chromium.chrome.browser.sharing.SharingAdapter;
 import org.chromium.chrome.browser.sharing.SharingDeviceCapability;
+import org.chromium.chrome.browser.sharing.SharingServiceProxy;
 import org.chromium.chrome.browser.sharing.SharingServiceProxy.DeviceInfo;
 
 /**
@@ -82,6 +83,12 @@ public class SharedClipboardShareActivity
         onInitialLayoutInflationComplete();
     }
 
+    @Override
+    public void startNativeInitialization() {
+        SharingServiceProxy.getInstance().addDeviceCandidatesInitializedObserver(
+                this::finishNativeInitialization);
+    }
+
     @Override
     public void finishNativeInitialization() {
         super.finishNativeInitialization();

chrome/browser/sharing/sharing_service.cc

@@ -31,7 +31,6 @@
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/sync_device_info/device_info.h"
-#include "components/sync_device_info/device_info_tracker.h"
 #include "components/sync_device_info/local_device_info_provider.h"
 #include "content/public/browser/browser_task_traits.h"
 
@@ -55,7 +54,8 @@ SharingService::SharingService(
       local_device_info_provider_(local_device_info_provider),
       sync_service_(sync_service),
       backoff_entry_(&kRetryBackoffPolicy),
-      state_(State::DISABLED) {
+      state_(State::DISABLED),
+      is_observing_device_info_tracker_(false) {
   // Remove old encryption info with empty authrozed_entity to avoid DCHECK.
   // See http://crbug/987591
   if (gcm_driver) {
@@ -131,11 +131,14 @@ std::unique_ptr<syncer::DeviceInfo> SharingService::GetDeviceByGuid(
 std::vector<std::unique_ptr<syncer::DeviceInfo>>
 SharingService::GetDeviceCandidates(int required_capabilities) const {
   std::vector<std::unique_ptr<syncer::DeviceInfo>> device_candidates;
-  if (IsSyncDisabled())
-    return device_candidates;
-
   std::vector<std::unique_ptr<syncer::DeviceInfo>> all_devices =
       device_info_tracker_->GetAllDeviceInfo();
+  const syncer::DeviceInfo* local_device_info =
+      local_device_info_provider_->GetLocalDeviceInfo();
+
+  if (IsSyncDisabled() || all_devices.empty() || !local_device_info)
+    return device_candidates;
+
   std::map<std::string, SharingSyncPreference::Device> synced_devices =
       sync_prefs_->GetSyncedDevices();
 
@@ -149,8 +152,6 @@ SharingService::GetDeviceCandidates(int required_capabilities) const {
             });
 
   std::unordered_set<std::string> device_names;
-  const syncer::DeviceInfo* local_device_info =
-      local_device_info_provider_->GetLocalDeviceInfo();
   for (auto& device : all_devices) {
     // If the current device is considered expired for our purposes, stop here
     // since the next devices in the vector are at least as expired than this
@@ -158,10 +159,8 @@ SharingService::GetDeviceCandidates(int required_capabilities) const {
     if (device->last_updated_timestamp() < min_updated_time)
       break;
 
-    if (local_device_info &&
-        (local_device_info->client_name() == device->client_name())) {
+    if (local_device_info->client_name() == device->client_name())
       continue;
-    }
 
     auto synced_device = synced_devices.find(device->guid());
     if (synced_device == synced_devices.end())
@@ -183,6 +182,36 @@ SharingService::GetDeviceCandidates(int required_capabilities) const {
   return device_candidates;
 }
 
+void SharingService::AddDeviceCandidatesInitializedObserver(
+    base::OnceClosure callback) {
+  if (IsSyncDisabled()) {
+    std::move(callback).Run();
+    return;
+  }
+
+  bool is_device_info_tracker_ready = device_info_tracker_->IsSyncing();
+  bool is_local_device_info_ready =
+      local_device_info_provider_->GetLocalDeviceInfo();
+  if (is_device_info_tracker_ready && is_local_device_info_ready) {
+    std::move(callback).Run();
+    return;
+  }
+
+  device_candidates_initialized_callbacks_.emplace_back(std::move(callback));
+
+  if (!is_device_info_tracker_ready && !is_observing_device_info_tracker_) {
+    device_info_tracker_->AddObserver(this);
+    is_observing_device_info_tracker_ = true;
+  }
+
+  if (!is_local_device_info_ready && !local_device_info_ready_subscription_) {
+    local_device_info_ready_subscription_ =
+        local_device_info_provider_->RegisterOnInitializedCallback(
+            base::BindRepeating(&SharingService::OnDeviceInfoChange,
+                                weak_ptr_factory_.GetWeakPtr()));
+  }
+}
+
 void SharingService::SendMessageToDevice(
     const std::string& device_guid,
     base::TimeDelta time_to_live,
@@ -261,6 +290,22 @@ void SharingService::InvokeSendMessageCallback(
   LogSendSharingMessageResult(result);
 }
 
+void SharingService::OnDeviceInfoChange() {
+  if (!device_info_tracker_->IsSyncing() ||
+      !local_device_info_provider_->GetLocalDeviceInfo()) {
+    return;
+  }
+
+  device_info_tracker_->RemoveObserver(this);
+  is_observing_device_info_tracker_ = false;
+  local_device_info_ready_subscription_.reset();
+
+  for (base::OnceClosure& callback : device_candidates_initialized_callbacks_) {
+    std::move(callback).Run();
+  }
+  device_candidates_initialized_callbacks_.clear();
+}
+
 void SharingService::RegisterHandler(
     chrome_browser_sharing::SharingMessage::PayloadCase payload_type,
     SharingMessageHandler* handler) {}

chrome/browser/sharing/sharing_service.h

@@ -22,6 +22,7 @@
 #include "components/gcm_driver/web_push_common.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/sync/driver/sync_service_observer.h"
+#include "components/sync_device_info/device_info_tracker.h"
 #include "net/base/backoff_entry.h"
 
 #if defined(OS_ANDROID)
@@ -37,7 +38,6 @@ class GCMDriver;
 
 namespace syncer {
 class DeviceInfo;
-class DeviceInfoTracker;
 class LocalDeviceInfoProvider;
 class SyncService;
 }  // namespace syncer
@@ -53,7 +53,8 @@ enum class SharingDeviceRegistrationResult;
 // sharing messages to other devices.
 class SharingService : public KeyedService,
                        syncer::SyncServiceObserver,
-                       AckMessageHandler::AckMessageObserver {
+                       AckMessageHandler::AckMessageObserver,
+                       syncer::DeviceInfoTracker::Observer {
  public:
   using SendMessageCallback =
       base::OnceCallback<void(SharingSendMessageResult)>;
@@ -92,6 +93,10 @@ class SharingService : public KeyedService,
   virtual std::vector<std::unique_ptr<syncer::DeviceInfo>> GetDeviceCandidates(
       int required_capabilities) const;
 
+  // Register |callback| so it will be invoked after all dependencies of
+  // GetDeviceCandidates are ready.
+  void AddDeviceCandidatesInitializedObserver(base::OnceClosure callback);
+
   // Sends a message to the device specified by GUID.
   // |callback| will be invoked with message_id if synchronous operation
   // succeeded, or base::nullopt if operation failed.
@@ -127,6 +132,9 @@ class SharingService : public KeyedService,
   // AckMessageHandler::AckMessageObserver override.
   void OnAckReceived(const std::string& message_id) override;
 
+  // syncer::DeviceInfoTracker::Observer.
+  void OnDeviceInfoChange() override;
+
   void RegisterDevice();
 
   void UnregisterDevice();
@@ -158,6 +166,10 @@ class SharingService : public KeyedService,
   PingMessageHandler ping_message_handler_;
   net::BackoffEntry backoff_entry_;
   State state_;
+  std::vector<base::OnceClosure> device_candidates_initialized_callbacks_;
+  bool is_observing_device_info_tracker_;
+  std::unique_ptr<syncer::LocalDeviceInfoProvider::Subscription>
+      local_device_info_ready_subscription_;
 
   // Map of random GUID to SendMessageCallback.
   std::map<std::string, SendMessageCallback> send_message_callbacks_;

chrome/browser/sharing/sharing_service_proxy_android.cc

@@ -77,3 +77,11 @@ void SharingServiceProxyAndroid::GetDeviceCandidates(
         device_info->last_updated_timestamp().ToJavaTime());
   }
 }
+
+void SharingServiceProxyAndroid::AddDeviceCandidatesInitializedObserver(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& j_runnable) {
+  sharing_service_->AddDeviceCandidatesInitializedObserver(
+      base::BindOnce(base::android::RunRunnableAndroid,
+                     base::android::ScopedJavaGlobalRef<jobject>(j_runnable)));
+}

chrome/browser/sharing/sharing_service_proxy_android.h

@@ -33,6 +33,10 @@ class SharingServiceProxyAndroid {
       const base::android::JavaParamRef<jobject>& j_device_info,
       jint j_capabilities);
 
+  void AddDeviceCandidatesInitializedObserver(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& j_runnable);
+
  private:
   SharingService* sharing_service_ = nullptr;
   ClickToCallMessageHandler click_to_call_message_handler_;

chrome/browser/sharing/sharing_service_unittest.cc

@@ -12,7 +12,6 @@
 #include "base/optional.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
-#include "chrome/browser/sharing/fake_local_device_info_provider.h"
 #include "chrome/browser/sharing/features.h"
 #include "chrome/browser/sharing/proto/sharing_message.pb.h"
 #include "chrome/browser/sharing/sharing_constants.h"
@@ -29,6 +28,7 @@
 #include "components/sync/driver/test_sync_service.h"
 #include "components/sync_device_info/device_info.h"
 #include "components/sync_device_info/fake_device_info_tracker.h"
+#include "components/sync_device_info/local_device_info_provider.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -49,6 +49,7 @@ const char kDeviceName[] = "other_name";
 const char kMessageId[] = "message_id";
 const char kAuthorizedEntity[] = "authorized_entity";
 constexpr base::TimeDelta kTtl = base::TimeDelta::FromSeconds(10);
+const char kSenderGuid[] = "test_sender_guid";
 
 class FakeGCMDriver : public gcm::FakeGCMDriver {
  public:
@@ -164,6 +165,46 @@ class FakeSharingDeviceRegistration : public SharingDeviceRegistration {
   int unregistration_attempts_ = 0;
 };
 
+class FakeLocalDeviceInfoProvider : public syncer::LocalDeviceInfoProvider {
+ public:
+  FakeLocalDeviceInfoProvider()
+      : local_device_info_(kSenderGuid,
+                           "name",
+                           "chrome_version",
+                           "user_agent",
+                           sync_pb::SyncEnums_DeviceType_TYPE_LINUX,
+                           "device_id",
+                           base::Time::Now(),
+                           false) {}
+  ~FakeLocalDeviceInfoProvider() override {}
+
+  version_info::Channel GetChannel() const override {
+    return version_info::Channel::UNKNOWN;
+  }
+
+  const syncer::DeviceInfo* GetLocalDeviceInfo() const override {
+    return ready_ ? &local_device_info_ : nullptr;
+  }
+
+  std::unique_ptr<syncer::LocalDeviceInfoProvider::Subscription>
+  RegisterOnInitializedCallback(
+      const base::RepeatingClosure& callback) override {
+    return callback_list_.Add(callback);
+  }
+
+  void SetReady(bool ready) {
+    bool got_ready = !ready_ && ready;
+    ready_ = ready;
+    if (got_ready)
+      callback_list_.Notify();
+  }
+
+ private:
+  syncer::DeviceInfo local_device_info_;
+  bool ready_ = true;
+  base::CallbackList<void(void)> callback_list_;
+};
+
 class SharingServiceTest : public testing::Test {
  public:
   SharingServiceTest() {
@@ -187,6 +228,10 @@ class SharingServiceTest : public testing::Test {
     return send_message_result_;
   }
 
+  void OnDeviceCandidatesInitialized() {
+    device_candidates_initialized_ = true;
+  }
+
  protected:
   static std::unique_ptr<syncer::DeviceInfo> CreateFakeDeviceInfo(
       const std::string& id,
@@ -234,6 +279,7 @@ class SharingServiceTest : public testing::Test {
   VapidKeyManager* vapid_key_manager_;
   FakeSharingDeviceRegistration* sharing_device_registration_;
   SharingFCMSender* fcm_sender_;
+  bool device_candidates_initialized_ = false;
 
  private:
   std::unique_ptr<SharingService> sharing_service_ = nullptr;
@@ -637,3 +683,35 @@ TEST_F(SharingServiceTest, NoDevicesWhenSyncDisabled) {
 
   ASSERT_EQ(0u, candidates.size());
 }
+
+TEST_F(SharingServiceTest, DeviceCandidatesAlreadyReady) {
+  std::string id = base::GenerateGUID();
+  std::unique_ptr<syncer::DeviceInfo> device_info =
+      CreateFakeDeviceInfo(id, kDeviceName);
+  device_info_tracker_.Add(device_info.get());
+  fake_local_device_info_provider_.SetReady(true);
+
+  GetSharingService()->AddDeviceCandidatesInitializedObserver(
+      base::BindOnce(&SharingServiceTest::OnDeviceCandidatesInitialized,
+                     base::Unretained(this)));
+
+  ASSERT_TRUE(device_candidates_initialized_);
+}
+
+TEST_F(SharingServiceTest, DeviceCandidatesReadyAfterAddObserver) {
+  fake_local_device_info_provider_.SetReady(false);
+
+  GetSharingService()->AddDeviceCandidatesInitializedObserver(
+      base::BindOnce(&SharingServiceTest::OnDeviceCandidatesInitialized,
+                     base::Unretained(this)));
+
+  ASSERT_FALSE(device_candidates_initialized_);
+
+  std::string id = base::GenerateGUID();
+  std::unique_ptr<syncer::DeviceInfo> device_info =
+      CreateFakeDeviceInfo(id, kDeviceName);
+  device_info_tracker_.Add(device_info.get());
+  fake_local_device_info_provider_.SetReady(true);
+
+  ASSERT_TRUE(device_candidates_initialized_);
+}

components/sync_device_info/fake_device_info_tracker.cc

@@ -49,11 +49,11 @@ FakeDeviceInfoTracker::GetAllDeviceInfo() const {
 }
 
 void FakeDeviceInfoTracker::AddObserver(Observer* observer) {
-  NOTREACHED();
+  observers_.AddObserver(observer);
 }
 
 void FakeDeviceInfoTracker::RemoveObserver(Observer* observer) {
-  NOTREACHED();
+  observers_.RemoveObserver(observer);
 }
 
 int FakeDeviceInfoTracker::CountActiveDevices() const {
@@ -76,10 +76,14 @@ bool FakeDeviceInfoTracker::IsRecentLocalCacheGuid(
 
 void FakeDeviceInfoTracker::Add(const DeviceInfo* device) {
   devices_.push_back(device);
+  for (auto& observer : observers_)
+    observer.OnDeviceInfoChange();
 }
 
 void FakeDeviceInfoTracker::OverrideActiveDeviceCount(int count) {
   active_device_count_ = count;
+  for (auto& observer : observers_)
+    observer.OnDeviceInfoChange();
 }
 
 }  // namespace syncer

components/sync_device_info/fake_device_info_tracker.h

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "base/optional.h"
 #include "components/sync_device_info/device_info_tracker.h"
 
@@ -45,6 +46,8 @@ class FakeDeviceInfoTracker : public DeviceInfoTracker {
   // DeviceInfo stored here are not owned.
   std::vector<const DeviceInfo*> devices_;
   base::Optional<int> active_device_count_;
+  // Registered observers, not owned.
+  base::ObserverList<Observer, true>::Unchecked observers_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeDeviceInfoTracker);
 };