Skip to content

Commit

Permalink
Tab DnD: Allow moving tabs between any two visible Chrome visible Win…
Browse files Browse the repository at this point in the history 
…dows instances.

Android users should be able to move the tabs freely between multiple instances of Chrome with more than one tab using drag and drop capability. All actions should end up with a dragged tab moving to the destination Chrome instance from the source Chrome instance. Currently if there is only one tab in the Chrome instance then that tab cannot be selected/picked using drag-n-drop to move to another Chrome instance.

(cherry picked from commit c3e5931)

Bug: 1463783
Bug: b/290321632
Change-Id: Iddc5013e935716efa744b3398071a6fd0c395d87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4671470
Commit-Queue: Gurmeet Kalra <gurmeetk@google.com>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1168284}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4676147
Cr-Commit-Position: refs/branch-heads/5845@{#421}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
@gurmeetk
gurmeetk authored and Chromium LUCI CQ committed Jul 11, 2023
1 parent b982568 commit 5adcd16
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 19 deletions.
8 changes: 5 additions & 3 deletions ...android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/TabDropTarget.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.chromium.chrome.browser.compositor.overlays.strip;

import android.app.Activity;
import android.content.ClipData;
import android.util.Pair;
import android.view.View;
Expand Down Expand Up @@ -49,7 +50,6 @@ public ContentInfoCompat onReceiveContent(View view, ContentInfoCompat payload)
Tab tabBeingDragged = tabDragSource.getTabBeingDragged();
if (tabDragSource.getDragSourceTabsToolbarHashCode() != System.identityHashCode(view)
&& (tabBeingDragged != null) && tabDragSource.getAcceptNextDrop()) {
ClipData.Item tabItem = payload.getClip().getItemAt(0);
Pair<ContentInfoCompat, ContentInfoCompat> split =
payload.partition(item -> item.getText() != null);
ContentInfoCompat uriContent = split.first;
Expand All @@ -66,8 +66,10 @@ public ContentInfoCompat onReceiveContent(View view, ContentInfoCompat payload)
Log.w(TAG, "DnD: Received an invalid tab drop.");
return payload;
}
tabDragSource.getMultiInstanceManager().moveTabToCurrentWindow(
tabBeingDragged);
// TODO(b/290648035): Pass the Activity explicitly in place of casting the
// context handle.
tabDragSource.getMultiInstanceManager().moveTabToWindow(
(Activity) view.getContext(), tabBeingDragged);
tabDragSource.clearTabBeingDragged();
tabDragSource.clearAcceptNextDrop();
}
Expand Down
2 changes: 1 addition & 1 deletion chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiInstanceManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ public void moveTabToNewWindow(Tab tab) {
// Not implemented
}

public void moveTabToCurrentWindow(Tab tab) {
public void moveTabToWindow(Activity activity, Tab tab) {
// Not implemented
}

Expand Down
35 changes: 29 additions & 6 deletions ...e/android/java/src/org/chromium/chrome/browser/multiwindow/MultiInstanceManagerApi31.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.chromium.base.ApplicationStatus.ActivityStateListener;
import org.chromium.base.ContextUtils;
import org.chromium.base.IntentUtils;
import org.chromium.base.Log;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.RecordUserAction;
import org.chromium.base.supplier.ObservableSupplier;
Expand Down Expand Up @@ -57,6 +58,8 @@
import java.util.Set;

class MultiInstanceManagerApi31 extends MultiInstanceManager implements ActivityStateListener {
private static final String TAG = "MIMApi31";

public static final int INVALID_INSTANCE_ID = MultiWindowUtils.INVALID_INSTANCE_ID;
public static final int INVALID_TASK_ID = MultiWindowUtils.INVALID_TASK_ID;

Expand Down Expand Up @@ -727,26 +730,46 @@ public void moveTabToNewWindow(Tab tab) {

/**
* Move the specified tab to the current instance of the ChromeTabbedActivity window.
* @param activity Activity of the Chrome Window in which the tab is to be moved.
* @param tab Tab that is to be moved to the current instance.
*/
@Override
public void moveTabToCurrentWindow(Tab tab) {
public void moveTabToWindow(Activity activity, Tab tab) {
if (!ChromeFeatureList.sTabDragDropAndroid.isEnabled()) return;

// Get the current instance and move tab there.
InstanceInfo info = getCurrentInstanceInfo();
moveTabAction(info, tab);
InstanceInfo info = getInstanceInfoFor(activity);
if (info != null) {
moveTabAction(info, tab);
} else {
Log.w(TAG, "DnD: InstanceInfo of Chrome Window not found.");
}
}

@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
InstanceInfo getCurrentInstanceInfo() {
InstanceInfo getInstanceInfoFor(Activity activity) {
// Loop thru all instances to determine if the destination activity is present.
int destinationWindowTaskId = INVALID_TASK_ID;
for (int i = 0; i < mMaxInstances; ++i) {
if (!instanceEntryExists(i)) continue;
Activity activityById = getActivityById(i);
if (activityById != null) {
// The task for the activity must match the one found in our mapping.
assert getTaskFromMap(i) == activityById.getTaskId();
if (activityById == activity) {
destinationWindowTaskId = activityById.getTaskId();
break;
}
}
}
if (destinationWindowTaskId == INVALID_TASK_ID) return null;

List<InstanceInfo> allInstances = getInstanceInfo();
for (InstanceInfo instanceInfo : allInstances) {
if (instanceInfo.type == InstanceInfo.Type.CURRENT) {
if (instanceInfo.taskId == destinationWindowTaskId) {
return instanceInfo;
}
}
assert false;
return null;
}
}
5 changes: 3 additions & 2 deletions ...id/junit/src/org/chromium/chrome/browser/compositor/overlays/strip/TabDropTargetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;

import android.app.Activity;
Expand Down Expand Up @@ -121,7 +122,7 @@ public void test_TabDropAction_onDifferentTabsLayout_ReturnsSuccess() {
mTabDragSource.setMultiInstanceManager(mMultiInstanceManager);
Mockito.doNothing()
.when(mMultiInstanceManager)
.moveTabToCurrentWindow(eq(mTabBeingDragged));
.moveTabToWindow(any(), eq(mTabBeingDragged));

// Perform action of a simulated drop of ClipData payload on drop target view.
ContentInfoCompat remainingPayload =
Expand Down Expand Up @@ -170,7 +171,7 @@ public void test_TabDropAction_onDifferentTabsLayout_WithBadClipDataDrop_Returns
mTabDragSource.setMultiInstanceManager(mMultiInstanceManager);
Mockito.doNothing()
.when(mMultiInstanceManager)
.moveTabToCurrentWindow(eq(mTabBeingDragged));
.moveTabToWindow(any(), eq(mTabBeingDragged));

// Create ClipData for non-Chrome apps with invalid tab id.
Tab tabForBadClipData = MockTab.createAndInitialize(555, false);
Expand Down
14 changes: 7 additions & 7 deletions .../junit/src/org/chromium/chrome/browser/multiwindow/MultiInstanceManagerApi31UnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ private void createInstance(int instanceId, Activity activity) {
MultiInstanceManagerApi31.writeUrl(instanceId, "https://id-" + instanceId + ".com");
ShadowApplicationStatus.addRunningActivity(instanceId, activity);
updateTasks(instanceId, activity);
addInstanceInfo(instanceId);
addInstanceInfo(instanceId, activity.getTaskId());
}

private void setAdjacentInstance(Activity activity) {
Expand All @@ -239,12 +239,12 @@ private void closeInstanceOnly(int instanceId) {
ShadowApplicationStatus.deleteRunningActivity(instanceId);
}

private void addInstanceInfo(int instanceId) {
private void addInstanceInfo(int instanceId, int taskId) {
if (mTestBuildInstancesList) {
int numberOfInstances = mTestInstanceInfos.size();
int type = (numberOfInstances == 0) ? InstanceInfo.Type.CURRENT
: InstanceInfo.Type.ADJACENT;
mTestInstanceInfos.add(new InstanceInfo(numberOfInstances, instanceId, type,
mTestInstanceInfos.add(new InstanceInfo(numberOfInstances, taskId, type,
MultiInstanceManagerApi31.readUrl(instanceId), "", 0, 0, false));
}
}
Expand Down Expand Up @@ -909,23 +909,23 @@ public void testTabMove_MoveTabToCurrentWindow_calledWithDesiredParameters() {
Mockito.doNothing().when(mMultiInstanceManager).moveTabAction(any(), eq(mTab1));

// Action
mMultiInstanceManager.moveTabToCurrentWindow(mTab1);
mMultiInstanceManager.moveTabToWindow(mTabbedActivityTask63, mTab1);

// Verify moveTabAction and getCurrentInstanceInfo are each called once.
verify(mMultiInstanceManager, times(1)).moveTabAction(any(), eq(mTab1));
verify(mMultiInstanceManager, times(1)).getCurrentInstanceInfo();
verify(mMultiInstanceManager, times(1)).getInstanceInfoFor(any());
}

@Test
@UiThreadTest
@Features.DisableFeatures(ChromeFeatureList.TAB_DRAG_DROP_ANDROID)
public void testTabMove_MoveTabToCurrentWindow_notCalled() {
public void testTabMove_MoveTabToWindow_notCalled() {
MultiInstanceManagerApi31 multiInstanceManager = Mockito.spy(
createChromeInstance(INSTANCE_ID_1, TASK_ID_62, List.of(mTab1, mTab2, mTab3)));
Mockito.doNothing().when(multiInstanceManager).moveTabAction(any(), eq(mTab2));

// Action
multiInstanceManager.moveTabToCurrentWindow(mTab2);
multiInstanceManager.moveTabToWindow(mTabbedActivityTask62, mTab2);

// Verify moveTabAction is not called.
verify(multiInstanceManager, times(0)).moveTabAction(any(), eq(mTab2));
Expand Down

0 comments on commit 5adcd16

Please sign in to comment.