Commit 5b2b9711 authored by Kyle Horimoto's avatar Kyle Horimoto Committed by Commit Bot

Revert "[CrOS PhoneHub] Attempt fetching browser tabs on new devices synced."

This reverts commit bad1090b.

Reason for revert: Did not fix the underlying issue; another fix will be required.

Original change's description:
> [CrOS PhoneHub] Attempt fetching browser tabs on new devices synced.
>
> Sometimes, the browser tabs do not appear in PhoneHub. This may be
> because the decrypted metadata (which includes the pii free name) may
> not have been ready during initialization. This CL adds a refetch of
> browser tab metadata when the decrypted metadata is ready in those
> instances.
>
> Fixed: 1143045
> Bug: 1106937
> Change-Id: I7ccba192336b77fe860be9accde007dd9be70a7a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505585
> Commit-Queue: Regan Hsu <hsuregan@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#822891}

TBR=khorimoto@chromium.org,hsuregan@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1106937
Change-Id: If22e0449074ce91434e65df55fa693fbc704e9a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533690Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826644}
parent d5bf4401
......@@ -14,15 +14,12 @@ namespace chromeos {
namespace phonehub {
BrowserTabsModelProviderImpl::BrowserTabsModelProviderImpl(
device_sync::DeviceSyncClient* device_sync_client,
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
sync_sessions::SessionSyncService* session_sync_service,
std::unique_ptr<BrowserTabsMetadataFetcher> browser_tabs_metadata_fetcher)
: device_sync_client_(device_sync_client),
multidevice_setup_client_(multidevice_setup_client),
: multidevice_setup_client_(multidevice_setup_client),
session_sync_service_(session_sync_service),
browser_tabs_metadata_fetcher_(std::move(browser_tabs_metadata_fetcher)) {
device_sync_client_->AddObserver(this);
multidevice_setup_client_->AddObserver(this);
session_updated_subscription_ =
session_sync_service->SubscribeToForeignSessionsChanged(
......@@ -33,7 +30,6 @@ BrowserTabsModelProviderImpl::BrowserTabsModelProviderImpl(
}
BrowserTabsModelProviderImpl::~BrowserTabsModelProviderImpl() {
device_sync_client_->RemoveObserver(this);
multidevice_setup_client_->RemoveObserver(this);
}
......@@ -48,12 +44,6 @@ base::Optional<std::string> BrowserTabsModelProviderImpl::GetSessionName()
return host_device_with_status.second->pii_free_name();
}
void BrowserTabsModelProviderImpl::OnNewDevicesSynced() {
// The decrypted metadata (which includes the pii free name) may not have been
// ready during initialization. Refetch when the decrypted metadata is ready.
AttemptBrowserTabsModelUpdate();
}
void BrowserTabsModelProviderImpl::OnHostStatusChanged(
const multidevice_setup::MultiDeviceSetupClient::HostStatusWithDevice&
host_device_with_status) {
......@@ -64,7 +54,7 @@ void BrowserTabsModelProviderImpl::AttemptBrowserTabsModelUpdate() {
base::Optional<std::string> session_name = GetSessionName();
sync_sessions::OpenTabsUIDelegate* open_tabs =
session_sync_service_->GetOpenTabsUIDelegate();
// Tab sync is disabled or no valid |pii_free_name|.
// Tab sync is disabled or no valid |pii_free_name_|.
if (!open_tabs || !session_name) {
InvalidateWeakPtrsAndClearTabMetadata(/*is_tab_sync_enabled=*/false);
return;
......@@ -110,7 +100,8 @@ void BrowserTabsModelProviderImpl::AttemptBrowserTabsModelUpdate() {
void BrowserTabsModelProviderImpl::InvalidateWeakPtrsAndClearTabMetadata(
bool is_tab_sync_enabled) {
weak_ptr_factory_.InvalidateWeakPtrs();
NotifyBrowserTabsUpdated(/*is_tab_sync_enabled=*/is_tab_sync_enabled, {});
BrowserTabsModelProvider::NotifyBrowserTabsUpdated(
/*is_tab_sync_enabled=*/is_tab_sync_enabled, {});
}
void BrowserTabsModelProviderImpl::OnMetadataFetched(
......@@ -119,7 +110,8 @@ void BrowserTabsModelProviderImpl::OnMetadataFetched(
// The operation to fetch metadata was cancelled.
if (!metadata)
return;
NotifyBrowserTabsUpdated(/*is_tab_sync_enabled=*/true, *metadata);
BrowserTabsModelProvider::NotifyBrowserTabsUpdated(
/*is_tab_sync_enabled=*/true, *metadata);
}
} // namespace phonehub
......
......@@ -11,7 +11,6 @@
#include "base/memory/weak_ptr.h"
#include "chromeos/components/phonehub/browser_tabs_metadata_fetcher.h"
#include "chromeos/components/phonehub/browser_tabs_model_provider.h"
#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
#include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
namespace sync_sessions {
......@@ -24,11 +23,9 @@ namespace phonehub {
// Responsible for providing the BrowserTabsModel to observers.
class BrowserTabsModelProviderImpl
: public BrowserTabsModelProvider,
public device_sync::DeviceSyncClient::Observer,
public multidevice_setup::MultiDeviceSetupClient::Observer {
public:
BrowserTabsModelProviderImpl(
device_sync::DeviceSyncClient* device_sync_client,
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
sync_sessions::SessionSyncService* session_sync_service,
std::unique_ptr<BrowserTabsMetadataFetcher>
......@@ -38,9 +35,6 @@ class BrowserTabsModelProviderImpl
private:
friend class BrowserTabsModelProviderImplTest;
// device_sync::DeviceSyncClient::Observer:
void OnNewDevicesSynced() override;
// multidevice_setup::MultiDeviceSetupClient::Observer:
void OnHostStatusChanged(
const multidevice_setup::MultiDeviceSetupClient::HostStatusWithDevice&
......@@ -53,7 +47,6 @@ class BrowserTabsModelProviderImpl
metadata);
base::Optional<std::string> GetSessionName() const;
device_sync::DeviceSyncClient* device_sync_client_;
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
sync_sessions::SessionSyncService* session_sync_service_;
std::unique_ptr<BrowserTabsMetadataFetcher> browser_tabs_metadata_fetcher_;
......
......@@ -8,7 +8,6 @@
#include "chromeos/components/phonehub/fake_browser_tabs_metadata_fetcher.h"
#include "chromeos/components/phonehub/mutable_phone_model.h"
#include "chromeos/components/phonehub/phone_model_test_util.h"
#include "chromeos/services/device_sync/public/cpp/fake_device_sync_client.h"
#include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
#include "components/sync_sessions/open_tabs_ui_delegate.h"
#include "components/sync_sessions/session_sync_service.h"
......@@ -96,13 +95,11 @@ class BrowserTabsModelProviderImplTest
const BrowserTabsModelProviderImplTest&) = delete;
~BrowserTabsModelProviderImplTest() override = default;
// BrowserTabsModelProvider::Observer:
void OnBrowserTabsUpdated(
bool is_sync_enabled,
const std::vector<BrowserTabsModel::BrowserTabMetadata>&
browser_tabs_metadata) override {
num_observer_calls_++;
phone_model()->SetBrowserTabsModel(BrowserTabsModel(
phone_model_.SetBrowserTabsModel(BrowserTabsModel(
/*is_tab_sync_enabled=*/is_sync_enabled, browser_tabs_metadata));
}
......@@ -121,16 +118,13 @@ class BrowserTabsModelProviderImplTest
.WillByDefault(Invoke(this, &BrowserTabsModelProviderImplTest::
MockSubscribeToForeignSessionsChanged));
fake_device_sync_client_.NotifyReady();
provider_ = std::make_unique<BrowserTabsModelProviderImpl>(
&fake_device_sync_client_, &fake_multidevice_setup_client_,
&mock_session_sync_service_,
&fake_multidevice_setup_client_, &mock_session_sync_service_,
std::make_unique<FakeBrowserTabsMetadataFetcher>());
provider_->AddObserver(this);
}
void SetNewDeviceWithPiiFreeName(const std::string& pii_free_name) {
void SetPiiFreeName(const std::string& pii_free_name) {
fake_multidevice_setup_client_.SetHostStatusWithDevice(std::make_pair(
multidevice_setup::mojom::HostStatus::kEligibleHostExistsButNoHostSet,
CreatePhoneDevice(/*pii_name=*/pii_free_name)));
......@@ -169,26 +163,16 @@ class BrowserTabsModelProviderImplTest
provider_->browser_tabs_metadata_fetcher_.get());
}
MutablePhoneModel* phone_model() { return &phone_model_; }
device_sync::FakeDeviceSyncClient* fake_device_sync_client() {
return &fake_device_sync_client_;
}
size_t GetNumObserverCalls() { return num_observer_calls_; }
private:
MutablePhoneModel phone_model_;
device_sync::FakeDeviceSyncClient fake_device_sync_client_;
multidevice_setup::FakeMultiDeviceSetupClient fake_multidevice_setup_client_;
testing::NiceMock<SessionSyncServiceMock> mock_session_sync_service_;
std::unique_ptr<BrowserTabsModelProviderImpl> provider_;
testing::NiceMock<OpenTabsUIDelegateMock> open_tabs_ui_delegate_;
bool enable_tab_sync_ = true;
std::vector<const sync_sessions::SyncedSession*>* sessions_ = nullptr;
base::RepeatingClosure foreign_sessions_changed_callback_;
size_t num_observer_calls_ = 0;
};
TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
......@@ -196,42 +180,31 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
set_enable_tab_sync(true);
set_synced_sessions(nullptr);
NotifySubscription();
EXPECT_FALSE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(
fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
EXPECT_EQ(GetNumObserverCalls(), 1);
fake_device_sync_client()->NotifyNewDevicesSynced();
EXPECT_EQ(GetNumObserverCalls(), 2);
// Set new host device. Tests that OnHostStatusChanged causes a fetch attempt.
SetNewDeviceWithPiiFreeName(kPhoneNameOne);
EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(
fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
EXPECT_EQ(GetNumObserverCalls(), 3);
// Set name of phone. Tests that OnHostStatusChanged causes name change.
SetPiiFreeName(kPhoneNameOne);
// Test disabling tab sync with no browser tab metadata.
set_enable_tab_sync(false);
set_synced_sessions(nullptr);
NotifySubscription();
EXPECT_FALSE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(
fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
EXPECT_EQ(GetNumObserverCalls(), 4);
// Test enabling tab sync with no browser tab metadata.
set_enable_tab_sync(true);
set_synced_sessions(nullptr);
NotifySubscription();
EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_TRUE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(
fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
EXPECT_EQ(GetNumObserverCalls(), 5);
// Test enabling tab sync with no matching pii name with session_name.
std::vector<const sync_sessions::SyncedSession*> sessions;
......@@ -240,11 +213,10 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
set_enable_tab_sync(true);
set_synced_sessions(&sessions);
NotifySubscription();
EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_TRUE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
EXPECT_FALSE(
fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
EXPECT_EQ(GetNumObserverCalls(), 6);
// Test enabling tab sync with matching pii name with session_name, which
// will cause the |fake_browser_tabs_metadata_fetcher()| to have a pending
......@@ -254,8 +226,8 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
set_enable_tab_sync(true);
set_synced_sessions(&sessions);
NotifySubscription();
EXPECT_TRUE(phone_model()->browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_TRUE(phone_model_.browser_tabs_model()->is_tab_sync_enabled());
EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
EXPECT_TRUE(fake_browser_tabs_metadata_fetcher()->DoesPendingCallbackExist());
// Test that once |fake_browser_tabs_metadata_fetcher()| responds, the phone
......@@ -264,12 +236,11 @@ TEST_F(BrowserTabsModelProviderImplTest, AttemptBrowserTabsModelUpdate) {
metadata.push_back(CreateFakeBrowserTabMetadata());
fake_browser_tabs_metadata_fetcher()->RespondToCurrentFetchAttempt(
std::move(metadata));
EXPECT_EQ(phone_model()->browser_tabs_model()->most_recent_tabs().size(), 1U);
EXPECT_EQ(GetNumObserverCalls(), 7);
EXPECT_EQ(phone_model_.browser_tabs_model()->most_recent_tabs().size(), 1U);
}
TEST_F(BrowserTabsModelProviderImplTest, ClearTabMetadataDuringMetadataFetch) {
SetNewDeviceWithPiiFreeName(kPhoneNameOne);
SetPiiFreeName(kPhoneNameOne);
sync_sessions::SyncedSession* new_session = CreateNewSession(kPhoneNameOne);
std::vector<const sync_sessions::SyncedSession*> sessions({new_session});
......@@ -288,11 +259,11 @@ TEST_F(BrowserTabsModelProviderImplTest, ClearTabMetadataDuringMetadataFetch) {
metadata.push_back(CreateFakeBrowserTabMetadata());
fake_browser_tabs_metadata_fetcher()->RespondToCurrentFetchAttempt(
std::move(metadata));
EXPECT_TRUE(phone_model()->browser_tabs_model()->most_recent_tabs().empty());
EXPECT_TRUE(phone_model_.browser_tabs_model()->most_recent_tabs().empty());
}
TEST_F(BrowserTabsModelProviderImplTest, SessionCorrectlySelected) {
SetNewDeviceWithPiiFreeName(kPhoneNameOne);
SetPiiFreeName(kPhoneNameOne);
sync_sessions::SyncedSession* session_a =
CreateNewSession(kPhoneNameOne, base::Time::FromDoubleT(1));
sync_sessions::SyncedSession* session_b =
......
......@@ -93,7 +93,6 @@ KeyedService* PhoneHubManagerFactory::BuildServiceInstanceFor(
multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(profile),
secure_channel::SecureChannelClientProvider::GetInstance()->GetClient(),
std::make_unique<BrowserTabsModelProviderImpl>(
device_sync::DeviceSyncClientFactory::GetForProfile(profile),
multidevice_setup::MultiDeviceSetupClientFactory::GetForProfile(
profile),
SessionSyncServiceFactory::GetInstance()->GetForProfile(profile),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment