Reland "[data sharing] Refactor libs to not depend on factory"
This reverts commit 8ceca719d39f7acd2eb128d7ff802d5ffc831f81.
Reason for revert: rename target to refresh build cache. Build
failures are because the cached tab_group target depends back on
factory causing circular dep.
failed build:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8730632363171594961/+/u/Incremental_build_with_1-day_of_changes/Build_chrome_public_apk_with_remote_cache/stdout
it says tab_group_ui -> factory_java, but not the case in reality.
Original change's description:
> Revert "[data sharing] Refactor libs to not depend on factory"
>
> This reverts commit 1ed6d5393b19c4d98845399c32453e5cdc303959.
>
> Reason for revert: It seems that this CL introduced a dependency cycle, and Android builds are failing.
>
> Original change's description:
> > [data sharing] Refactor libs to not depend on factory
> >
> > Factory will construct delegates, root UI will get the services and pass
> > to the delegates. The tab manager will not depend on the factory, so
> > delegates can use this class. It gets initialized when profile is ready.
> > Rename messages_java to delegates_java.
> > Move message factory out of delegates target.
> >
> > Change-Id: Ia51afeb895c5e6e4575bde0645106d1fabfdea35
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6035940
> > Commit-Queue: Siddhartha S <[email protected]>
> > Reviewed-by: Calder Kitagawa <[email protected]>
> > Reviewed-by: Sky Malice <[email protected]>
> > Reviewed-by: Hailey Wang <[email protected]>
> > Code-Coverage: [email protected] <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1386028}
>
> Change-Id: I2b78702d7849d5efa41082268d97aa9c898cb27e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6042068
> Bot-Commit: Rubber Stamper <[email protected]>
> Reviewed-by: Calder Kitagawa <[email protected]>
> Commit-Queue: Scott Lee <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1386526}
Change-Id: Idff93b2b23e09b467330cba106b9868a1d97bf60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6041451
Reviewed-by: Wenyu Fu <[email protected]>
Commit-Queue: Siddhartha S <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1386951}
diff --git a/chrome/browser/data_sharing/BUILD.gn b/chrome/browser/data_sharing/BUILD.gn
index e436fb6..e41adca 100644
--- a/chrome/browser/data_sharing/BUILD.gn
+++ b/chrome/browser/data_sharing/BUILD.gn
@@ -17,6 +17,7 @@
]
deps = [
+ ":internal_library_java",
":java",
"//base:base_java",
"//base:service_loader_java",
@@ -29,6 +30,20 @@
]
}
+ # TODO(crbug.com/380154734): Move this to collaboration/ directory near messages factory.
+ android_library("instant_message_delegate_factory_java") {
+ sources = [ "android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateFactory.java" ]
+
+ deps = [
+ ":factory_java",
+ ":internal_library_java",
+ "//base:base_java",
+ "//chrome/browser/profiles/android:java",
+ "//components/data_sharing/public:public_java",
+ "//third_party/androidx:androidx_annotation_annotation_java",
+ ]
+ }
+
android_resources("java_resources") {
sources = [
"android/java/res/drawable/round_image_filled.xml",
@@ -42,7 +57,7 @@
deps = [ "//components/browser_ui/styles/android:java_resources" ]
}
- android_library("tab_group_ui_java") {
+ android_library("data_sharing_public_java") {
resources_package = "org.chromium.chrome.browser.data_sharing"
sources = [
"android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingMetrics.java",
@@ -53,7 +68,6 @@
]
deps = [
- ":factory_java",
":java_resources",
"//base:base_java",
"//chrome/browser/android/intents:java",
@@ -129,18 +143,18 @@
]
}
- android_library("messages_java") {
+ android_library("internal_library_java") {
resources_package = "org.chromium.chrome.browser.data_sharing"
sources = [
"android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingNotificationManager.java",
"android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingUiDelegateAndroid.java",
- "android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateFactory.java",
"android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImpl.java",
]
deps = [
+ ":data_sharing_public_java",
+ ":java",
":java_resources",
- ":tab_group_ui_java",
"//base:base_java",
"//base:service_loader_java",
"//base:supplier_java",
@@ -191,10 +205,10 @@
"android/java/src/org/chromium/chrome/browser/data_sharing/ui/shared_image_tiles/SharedImageTilesCoordinatorUnitTest.java",
]
deps = [
+ ":data_sharing_public_java",
+ ":internal_library_java",
":java",
- ":messages_java",
":shared_image_tiles_java",
- ":tab_group_ui_java",
"//base:base_java",
"//base:base_junit_test_support",
"//chrome/browser/collaboration:messaging_factory_java",
diff --git a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManager.java b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManager.java
index 06cc900..adebbaf 100644
--- a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManager.java
+++ b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManager.java
@@ -11,6 +11,7 @@
import android.view.LayoutInflater;
import android.view.ViewGroup;
+import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import org.chromium.base.Callback;
@@ -52,6 +53,7 @@
import org.chromium.url.GURL;
import java.util.HashMap;
+import java.util.LinkedList;
import java.util.Map;
/**
@@ -61,15 +63,18 @@
public class DataSharingTabManager {
private static final String TAG = "DataSharing";
- private final ObservableSupplier<Profile> mProfileSupplier;
private final DataSharingTabSwitcherDelegate mDataSharingTabSwitcherDelegate;
private final Supplier<BottomSheetController> mBottomSheetControllerSupplier;
private final ObservableSupplier<ShareDelegate> mShareDelegateSupplier;
private final WindowAndroid mWindowAndroid;
private final Resources mResources;
private final OneshotSupplier<TabGroupUiActionHandler> mTabGroupUiActionHandlerSupplier;
- private Callback<Profile> mProfileObserver;
- private Map</*collaborationId*/ String, SyncObserver> mSyncObserversList;
+ private final Map</*collaborationId*/ String, SyncObserver> mSyncObserversList =
+ new HashMap<>();
+ private final LinkedList<Runnable> mTasksToRunOnProfileAvailable = new LinkedList<>();
+
+ private @Nullable Profile mProfile;
+ private @Nullable DataSharingService mDataSharingService;
/** This class is responsible for observing sync tab activities. */
private static class SyncObserver implements TabGroupSyncService.Observer {
@@ -107,7 +112,6 @@
* Constructor for a new {@link DataSharingTabManager} object.
*
* @param tabSwitcherDelegate The delegate used to communicate with the tab switcher.
- * @param profileSupplier The supplier of the currently applicable profile.
* @param bottomSheetControllerSupplier The supplier of bottom sheet state controller.
* @param shareDelegateSupplier The supplier of share delegate.
* @param windowAndroid The window base class that has the minimum functionality.
@@ -117,25 +121,36 @@
*/
public DataSharingTabManager(
DataSharingTabSwitcherDelegate tabSwitcherDelegate,
- ObservableSupplier<Profile> profileSupplier,
Supplier<BottomSheetController> bottomSheetControllerSupplier,
ObservableSupplier<ShareDelegate> shareDelegateSupplier,
WindowAndroid windowAndroid,
Resources resources,
OneshotSupplier<TabGroupUiActionHandler> tabGroupUiActionHandlerSupplier) {
mDataSharingTabSwitcherDelegate = tabSwitcherDelegate;
- mProfileSupplier = profileSupplier;
mBottomSheetControllerSupplier = bottomSheetControllerSupplier;
mShareDelegateSupplier = shareDelegateSupplier;
mWindowAndroid = windowAndroid;
mResources = resources;
mTabGroupUiActionHandlerSupplier = tabGroupUiActionHandlerSupplier;
- mSyncObserversList = new HashMap<>();
- assert mProfileSupplier != null;
assert mBottomSheetControllerSupplier != null;
assert mShareDelegateSupplier != null;
}
+ /**
+ * Initializes when profile is available.
+ *
+ * @param profile The loaded profile.
+ * @param dataSharingService Data sharing service associated with the profile.
+ */
+ public void initWithProfile(@NonNull Profile profile, DataSharingService dataSharingService) {
+ mProfile = profile;
+ mDataSharingService = dataSharingService;
+ while (!mTasksToRunOnProfileAvailable.isEmpty()) {
+ Runnable task = mTasksToRunOnProfileAvailable.removeFirst();
+ task.run();
+ }
+ }
+
/** Cleans up any outstanding resources. */
public void destroy() {
for (Map.Entry<String, SyncObserver> entry : mSyncObserversList.entrySet()) {
@@ -152,19 +167,15 @@
public void initiateJoinFlow(Activity activity, GURL dataSharingUrl) {
DataSharingMetrics.recordJoinActionFlowState(
DataSharingMetrics.JoinActionStateAndroid.JOIN_TRIGGERED);
- if (mProfileSupplier.hasValue() && mProfileSupplier.get().getOriginalProfile() != null) {
+ if (mProfile != null) {
initiateJoinFlowWithProfile(activity, dataSharingUrl);
return;
}
- assert mProfileObserver == null;
- mProfileObserver =
- profile -> {
- mProfileSupplier.removeObserver(mProfileObserver);
+ mTasksToRunOnProfileAvailable.addLast(
+ () -> {
initiateJoinFlowWithProfile(activity, dataSharingUrl);
- };
-
- mProfileSupplier.addObserver(mProfileObserver);
+ });
}
/**
@@ -226,16 +237,13 @@
private void initiateJoinFlowWithProfile(Activity activity, GURL dataSharingUrl) {
DataSharingMetrics.recordJoinActionFlowState(
DataSharingMetrics.JoinActionStateAndroid.PROFILE_AVAILABLE);
- Profile originalProfile = mProfileSupplier.get().getOriginalProfile();
TabGroupSyncService tabGroupSyncService =
- TabGroupSyncServiceFactory.getForProfile(originalProfile);
- DataSharingService dataSharingService =
- DataSharingServiceFactory.getForProfile(originalProfile);
+ TabGroupSyncServiceFactory.getForProfile(mProfile);
assert tabGroupSyncService != null;
- assert dataSharingService != null;
+ assert mDataSharingService != null;
DataSharingService.ParseUrlResult parseResult =
- dataSharingService.parseDataSharingUrl(dataSharingUrl);
+ mDataSharingService.parseDataSharingUrl(dataSharingUrl);
if (parseResult.status != ParseUrlStatus.SUCCESS) {
showInvitationFailureDialog();
DataSharingMetrics.recordJoinActionFlowState(
@@ -245,7 +253,7 @@
GroupToken groupToken = parseResult.groupToken;
String collaborationId = groupToken.collaborationId;
- DataSharingUIDelegate uiDelegate = dataSharingService.getUiDelegate();
+ DataSharingUIDelegate uiDelegate = mDataSharingService.getUiDelegate();
assert uiDelegate != null;
JoinFlowTracker joinFlowTracker = new JoinFlowTracker(uiDelegate);
@@ -326,7 +334,7 @@
return;
}
- dataSharingService.addMember(
+ mDataSharingService.addMember(
groupToken.collaborationId,
groupToken.accessToken,
result -> {
@@ -400,8 +408,7 @@
void openLocalTabGroup(SavedTabGroup group) {
TabGroupSyncService tabGroupSyncService =
- TabGroupSyncServiceFactory.getForProfile(
- mProfileSupplier.get().getOriginalProfile());
+ TabGroupSyncServiceFactory.getForProfile(mProfile);
mTabGroupUiActionHandlerSupplier.runSyncOrOnAvailable(
(tabGroupUiActionHandler) -> {
@@ -423,8 +430,7 @@
*/
protected void deleteSyncObserver(SyncObserver observer) {
TabGroupSyncService tabGroupSyncService =
- TabGroupSyncServiceFactory.getForProfile(
- mProfileSupplier.get().getOriginalProfile());
+ TabGroupSyncServiceFactory.getForProfile(mProfile);
if (tabGroupSyncService != null) {
tabGroupSyncService.removeObserver(observer);
@@ -446,20 +452,18 @@
Callback<Boolean> createGroupFinishedCallback) {
DataSharingMetrics.recordShareActionFlowState(
DataSharingMetrics.ShareActionStateAndroid.SHARE_TRIGGERED);
- Profile profile = mProfileSupplier.get().getOriginalProfile();
- assert profile != null;
- TabGroupSyncService tabGroupService = TabGroupSyncServiceFactory.getForProfile(profile);
- DataSharingService dataSharingService = DataSharingServiceFactory.getForProfile(profile);
+ assert mProfile != null;
+ TabGroupSyncService tabGroupService = TabGroupSyncServiceFactory.getForProfile(mProfile);
SavedTabGroup existingGroup = tabGroupService.getGroup(localTabGroupId);
assert existingGroup != null : "Group not found in TabGroupSyncService.";
if (existingGroup.collaborationId != null) {
- onShareClickExistingGroup(activity, dataSharingService, existingGroup);
+ onShareClickExistingGroup(activity, mDataSharingService, existingGroup);
return;
}
if (ChromeFeatureList.isEnabled(ChromeFeatureList.DATA_SHARING_ANDROID_V2)) {
- DataSharingUIDelegate uiDelegate = dataSharingService.getUiDelegate();
+ DataSharingUIDelegate uiDelegate = mDataSharingService.getUiDelegate();
DataSharingStringConfig stringConfig =
new DataSharingStringConfig.Builder()
.setResourceId(
@@ -536,17 +540,19 @@
}
};
- dataSharingService.createGroup(tabGroupDisplayName, createGroupCallback);
+ mDataSharingService.createGroup(tabGroupDisplayName, createGroupCallback);
}
private void onShareClickExistingGroup(
- Activity activity, DataSharingService dataSharingService, SavedTabGroup existingGroup) {
+ Activity activity,
+ DataSharingService mDataSharingService,
+ SavedTabGroup existingGroup) {
if (ChromeFeatureList.isEnabled(ChromeFeatureList.DATA_SHARING_ANDROID_V2)) {
assert existingGroup.collaborationId != null;
showManageSharing(activity, existingGroup.collaborationId);
return;
}
- dataSharingService.ensureGroupVisibility(
+ mDataSharingService.ensureGroupVisibility(
existingGroup.collaborationId,
(result) -> {
if (result.actionFailure != PeopleGroupActionFailure.UNKNOWN
@@ -563,10 +569,7 @@
}
private void showShareSheet(GroupData groupData) {
- DataSharingService dataSharingService =
- DataSharingServiceFactory.getForProfile(
- mProfileSupplier.get().getOriginalProfile());
- GURL url = dataSharingService.getDataSharingUrl(groupData);
+ GURL url = mDataSharingService.getDataSharingUrl(groupData);
if (url == null) {
// TODO(ritikagup) : Show error dialog showing fetching URL failed. Contact owner for
// new link.
@@ -598,15 +601,10 @@
* @param collaborationId The collaboration ID to show the UI for.
*/
public void showManageSharing(Activity activity, String collaborationId) {
- Profile profile = mProfileSupplier.get().getOriginalProfile();
if (!ChromeFeatureList.isEnabled(ChromeFeatureList.DATA_SHARING_ANDROID_V2)) {
return;
}
- assert profile != null;
- DataSharingService dataSharingService = DataSharingServiceFactory.getForProfile(profile);
-
- DataSharingUIDelegate uiDelegate = dataSharingService.getUiDelegate();
- assert uiDelegate != null;
+ DataSharingUIDelegate uiDelegate = mDataSharingService.getUiDelegate();
DataSharingStringConfig stringConfig =
new DataSharingStringConfig.Builder()
diff --git a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManagerUnitTest.java b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManagerUnitTest.java
index ba47a4ad..c13ca82 100644
--- a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManagerUnitTest.java
+++ b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/DataSharingTabManagerUnitTest.java
@@ -144,7 +144,6 @@
private DataSharingTabManager mDataSharingTabManager;
private SavedTabGroup mSavedTabGroup;
private Activity mActivity;
- private ObservableSupplier<Profile> mProfileSupplier;
private Supplier<BottomSheetController> mBottomSheetControllerSupplier;
private ObservableSupplier<ShareDelegate> mShareDelegateSupplier;
private SyncedGroupTestHelper mSyncedGroupTestHelper;
@@ -155,7 +154,6 @@
DataSharingServiceFactory.setForTesting(mDataSharingService);
TabGroupSyncServiceFactory.setForTesting(mTabGroupSyncService);
- mProfileSupplier = new ObservableSupplierImpl<>(mProfile);
mBottomSheetControllerSupplier = new ObservableSupplierImpl<>(mBottomSheetController);
mShareDelegateSupplier = new ObservableSupplierImpl<>(mShareDelegate);
mTabGroupUiActionHandlerSupplier.set(mTabGroupUiActionHandler);
@@ -163,13 +161,14 @@
mDataSharingTabManager =
new DataSharingTabManager(
mDataSharingTabSwitcherDelegate,
- mProfileSupplier,
mBottomSheetControllerSupplier,
mShareDelegateSupplier,
mWindowAndroid,
ApplicationProvider.getApplicationContext().getResources(),
mTabGroupUiActionHandlerSupplier);
+ mDataSharingTabManager.initWithProfile(mProfile, mDataSharingService);
+
mSyncedGroupTestHelper = new SyncedGroupTestHelper(mTabGroupSyncService);
mSavedTabGroup = mSyncedGroupTestHelper.newTabGroup(SYNC_GROUP_ID1);
mSavedTabGroup.collaborationId = COLLABORATION_ID1;
@@ -210,11 +209,9 @@
@Test
public void testNoProfile() {
- mProfileSupplier = new ObservableSupplierImpl<>();
mDataSharingTabManager =
new DataSharingTabManager(
mDataSharingTabSwitcherDelegate,
- mProfileSupplier,
mBottomSheetControllerSupplier,
mShareDelegateSupplier,
mWindowAndroid,
diff --git a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateFactory.java b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateFactory.java
index af4c7e90..dd826f4 100644
--- a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateFactory.java
+++ b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateFactory.java
@@ -35,7 +35,12 @@
sProfileMap = new ProfileKeyedMap<>(ProfileKeyedMap.NO_REQUIRED_CLEANUP_ACTION);
}
- return sProfileMap.getForProfile(profile, InstantMessageDelegateImpl::new);
+ return sProfileMap.getForProfile(profile, InstantMessageDelegateFactory::buildForProfile);
+ }
+
+ private static InstantMessageDelegateImpl buildForProfile(Profile profile) {
+ return new InstantMessageDelegateImpl(
+ profile, DataSharingServiceFactory.getForProfile(profile));
}
/**
diff --git a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImpl.java b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImpl.java
index bd4dda6..28660527 100644
--- a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImpl.java
+++ b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImpl.java
@@ -32,6 +32,7 @@
import org.chromium.components.collaboration.messaging.MessageUtils;
import org.chromium.components.collaboration.messaging.MessagingBackendService;
import org.chromium.components.collaboration.messaging.MessagingBackendService.InstantMessageDelegate;
+import org.chromium.components.data_sharing.DataSharingService;
import org.chromium.components.data_sharing.DataSharingUIDelegate;
import org.chromium.components.data_sharing.GroupMember;
import org.chromium.components.data_sharing.configs.DataSharingAvatarBitmapConfig;
@@ -77,14 +78,19 @@
private final DataSharingUIDelegate mDataSharingUiDelegate;
/**
+ * TODO(ssid): Pass in the necessary dependencies rather than profile and fetching the services
+ * to avoid depending on the factory.
+ *
* @param profile The current profile to get dependencies with.
+ * @param dataSharingService Data sharing service for the profile.
*/
- /* package */ InstantMessageDelegateImpl(Profile profile) {
+ /* package */ InstantMessageDelegateImpl(
+ Profile profile, DataSharingService dataSharingService) {
profile = profile.getOriginalProfile();
MessagingBackendService messagingBackendService =
MessagingBackendServiceFactory.getForProfile(profile);
messagingBackendService.setInstantMessageDelegate(this);
- mDataSharingUiDelegate = DataSharingServiceFactory.getForProfile(profile).getUiDelegate();
+ mDataSharingUiDelegate = dataSharingService.getUiDelegate();
}
/**
diff --git a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImplUnitTest.java b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImplUnitTest.java
index 199b4c68..752ab58 100644
--- a/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImplUnitTest.java
+++ b/chrome/browser/data_sharing/android/java/src/org/chromium/chrome/browser/data_sharing/InstantMessageDelegateImplUnitTest.java
@@ -140,7 +140,7 @@
when(mTabGroupModelFilter.getTabModel()).thenReturn(mTabModel);
when(mTabModel.getTabCreator()).thenReturn(mTabCreator);
- mDelegate = new InstantMessageDelegateImpl(mProfile);
+ mDelegate = new InstantMessageDelegateImpl(mProfile, mDataSharingService);
mDelegate.attachWindow(
mWindowAndroid,
mTabGroupModelFilter,
