Skip to content

Commit

Permalink
Revert "[Files F2] Support volumeType for FakeEntry"
Browse files Browse the repository at this point in the history
This reverts commit 956720e.

Reason for revert: Reason for revert: https://crrev.com/c/4150614, which this CL was based on, broke several Crostini tast tests according to the bisector.

Bug: b/272557385

Original change's description:
> [Files F2] Support volumeType for FakeEntry
>
> Support volumeType for FakeEntry, because FakeEntry can represents
> a placeholder for a real volume. This can simplify the logic in the all_entries reducer.
>
> Bug: b:271485133
> Test: browser_tests --gtest_filter="FileManagerJsTest*Reducer*"
> Change-Id: I57cba35e66f240dbb04b00937b98cce5f42f07d6
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4323358
> Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
> Commit-Queue: Wenbo Jie <wenbojie@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1115473}

Bug: b:271485133
Change-Id: I3a53486451a17ae838e2eee6e26646e215e1ab53
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4327998
Auto-Submit: Colin Kincaid <ckincaid@chromium.org>
Commit-Queue: Colin Kincaid <ckincaid@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Colin Kincaid <ckincaid@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1115776}
  • Loading branch information
Colin Kincaid authored and Chromium LUCI CQ committed Mar 10, 2023
1 parent 1581878 commit c7d8ba9
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 107 deletions.
27 changes: 1 addition & 26 deletions ui/file_manager/file_manager/common/js/files_app_entry_types.js
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,7 @@ export class VolumeEntry {
}
this.type_name = 'VolumeEntry';

// TODO(b/271485133): consider deriving this from volumeInfo. Setting
// rootType here breaks some integration tests, e.g.
// saveAsDlpRestrictedAndroid.
// TODO(lucmult): consider deriving this from volumeInfo.
this.rootType = null;

this.disabled_ = false;
Expand Down Expand Up @@ -758,21 +756,6 @@ export class FakeEntryImpl {
createReader() {
return new StaticReader([]);
}

/**
* FakeEntry can be a placeholder for the real volume, if so this field will
* be the volume type of the volume it represents.
* @return {VolumeManagerCommon.VolumeType|null}
*/
get volumeType() {
// Recent rootType has no corresponding volume type, and it will throw error
// in the below getVolumeTypeFromRootType() call, we need to return null
// here.
if (this.rootType === VolumeManagerCommon.RootType.RECENT) {
return null;
}
return VolumeManagerCommon.getVolumeTypeFromRootType(this.rootType);
}
}

/**
Expand Down Expand Up @@ -819,12 +802,4 @@ export class GuestOsPlaceholder extends FakeEntryImpl {
toURL() {
return `fake-entry://guest-os/${this.guest_id}`;
}

/** @override */
get volumeType() {
if (this.vm_type === chrome.fileManagerPrivate.VmType.ARCVM) {
return VolumeManagerCommon.VolumeType.ANDROID_FILES;
}
return VolumeManagerCommon.VolumeType.GUEST_OS;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,4 @@ export class FakeEntry extends FilesAppDirEntry {
* @return {string}
*/
get iconName() {}

/**
* FakeEntry can be a placeholder for the real volume, if so this field will
* be the volume type of the volume it represents.
* @return {VolumeManagerCommon.VolumeType|null}
*/
get volumeType() {}
}
34 changes: 23 additions & 11 deletions ui/file_manager/file_manager/state/reducers/all_entries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import {isVolumeEntry, sortEntries} from '../../common/js/entry_utils.js';
import {FileType} from '../../common/js/file_type.js';
import {EntryList, VolumeEntry} from '../../common/js/files_app_entry_types.js';
import {EntryList, FakeEntryImpl, GuestOsPlaceholder, VolumeEntry} from '../../common/js/files_app_entry_types.js';
import {str, util} from '../../common/js/util.js';
import {VolumeManagerCommon} from '../../common/js/volume_manager_types.js';
import {EntryLocation} from '../../externs/entry_location.js';
Expand Down Expand Up @@ -217,22 +217,34 @@ function appendChildIfNotExisted(
*/
export function convertEntryToFileData(entry: Entry|FilesAppEntry): FileData {
// TODO: get VolumeManager/MetadataModel properly.
const {volumeManager, metadataModel} = window.fileManager;
const volumeInfo = volumeManager.getVolumeInfo(entry);
const locationInfo = volumeManager.getLocationInfo(entry);
const volumeManager = window.fileManager?.volumeManager;
const metadataModel = window.fileManager?.metadataModel;
const volumeInfo = volumeManager?.getVolumeInfo(entry);
const locationInfo = volumeManager?.getLocationInfo(entry);
// getEntryLabel() can accept locationInfo=null, but TS doesn't recognize the
// type definition in closure, hence the ! here.
const label = util.getEntryLabel(locationInfo!, entry);
const volumeType = volumeInfo?.volumeType || null;
const icon = getEntryIcon(entry, locationInfo, volumeType);

/**
* Update disabled attribute if entry supports disabled attribute and has a
* non-null volumeType.
*/
if ('disabled' in entry && 'volumeType' in entry && entry.volumeType) {
entry.disabled = volumeManager.isDisabled(
entry.volumeType as VolumeManagerCommon.VolumeType);
// TODO(b/271485133): populate rootType for VolumeEntry in its constructor,
// add volumeType property to FakeEntry so we don't need the following nested
// if logic to check.
if (entry instanceof VolumeEntry) {
entry.disabled = volumeManager?.isDisabled(volumeType!);
entry.rootType = locationInfo?.rootType;
} else if (entry instanceof FakeEntryImpl) {
if (entry.rootType === VolumeManagerCommon.RootType.CROSTINI) {
entry.disabled = volumeManager?.isDisabled(entry.rootType);
} else if (entry instanceof GuestOsPlaceholder) {
if (entry.vm_type == chrome.fileManagerPrivate.VmType.ARCVM) {
entry.disabled = volumeManager.isDisabled(
VolumeManagerCommon.VolumeType.ANDROID_FILES);
} else {
entry.disabled =
volumeManager.isDisabled(VolumeManagerCommon.VolumeType.GUEST_OS);
}
}
}

const metadata = metadataModel ?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export function changeDirectory(
// At the end of the change directory, DirectoryContents will send an Action
// with the Entry to be cached.
if (fileData) {
const {volumeManager} = window.fileManager;
const volumeManager = window.fileManager?.volumeManager;
if (!volumeManager) {
console.debug(`VolumeManager not available yet.`);
currentDirectory = currentState.currentDirectory || currentDirectory;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,11 @@
import {MockFileSystem} from '../../common/js/mock_entry.js';
import {State} from '../../externs/ts/state.js';
import {addFolderShortcut, refreshFolderShortcut, removeFolderShortcut} from '../actions/folder_shortcuts.js';
import {setUpFileManagerOnWindow, setupStore, waitDeepEquals} from '../for_tests.js';
import {setupStore, waitDeepEquals} from '../for_tests.js';
import {getEmptyState} from '../store.js';

import {convertEntryToFileData} from './all_entries.js';

export function setUp() {
setUpFileManagerOnWindow();
}

/** Generate a fake file system with fake file entries. */
function setupFileSystem(): MockFileSystem {
const fileSystem = new MockFileSystem('fake-fs');
Expand Down
25 changes: 0 additions & 25 deletions ui/file_manager/file_manager/state/reducers/ui_entries_unittest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {assertEquals} from 'chrome://webui-test/chai_assert.js';

import {MockVolumeManager} from '../../background/js/mock_volume_manager.js';
import {FakeEntryImpl, GuestOsPlaceholder, VolumeEntry} from '../../common/js/files_app_entry_types.js';
import {waitUntil} from '../../common/js/test_error_reporting.js';
import {VolumeManagerCommon} from '../../common/js/volume_manager_types.js';
import {FileData, State} from '../../externs/ts/state.js';
import {VolumeInfo} from '../../externs/volume_info.js';
Expand Down Expand Up @@ -209,30 +208,6 @@ export async function testAddDuplicateUiEntryForMyFilesWhenVolumeExists(
done();
}

/**
* Tests that UI entry will be disabled if the corresponding volume
* type is disabled in the volume manager.
*/
export async function testAddUiEntryWithDisabledVolumeType(done: () => void) {
const initialState = getEmptyState();
const store = setupStore(initialState);

// Dispatch an action to add UI entry.
const {volumeManager} = window.fileManager;
// Disable Android files volume type.
volumeManager.isDisabled = (volumeType) => {
return volumeType === VolumeManagerCommon.VolumeType.ANDROID_FILES;
};
const uiEntry = new GuestOsPlaceholder(
'Play files', 0, chrome.fileManagerPrivate.VmType.ARCVM);
store.dispatch(addUiEntry({entry: uiEntry}));

// Expect the UI entry is being disabled.
await waitUntil(() => uiEntry.disabled === true);

done();
}

/** Tests that UI entry can be removed from store correctly. */
export async function testRemoveUiEntry(done: () => void) {
const initialState = getEmptyState();
Expand Down
33 changes: 1 addition & 32 deletions ui/file_manager/file_manager/state/reducers/volumes_unittest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@

import {MockVolumeManager} from '../../background/js/mock_volume_manager.js';
import {EntryList, FakeEntryImpl, VolumeEntry} from '../../common/js/files_app_entry_types.js';
import {waitUntil} from '../../common/js/test_error_reporting.js';
import {str} from '../../common/js/util.js';
import {VolumeManagerCommon} from '../../common/js/volume_manager_types.js';
import {FileData, State} from '../../externs/ts/state.js';
import {VolumeInfo} from '../../externs/volume_info.js';
import {constants} from '../../foreground/js/constants.js';
import {addVolume, removeVolume} from '../actions/volumes.js';
import {createFakeVolumeMetadata, setUpFileManagerOnWindow, setupStore, waitDeepEquals} from '../for_tests.js';
import {getEmptyState, getEntry} from '../store.js';
import {getEmptyState} from '../store.js';

import {convertEntryToFileData} from './all_entries.js';
import {convertVolumeInfoAndMetadataToVolume, driveRootEntryListKey} from './volumes.js';
Expand Down Expand Up @@ -328,36 +327,6 @@ export async function testAddVolumeForMultipleUsbPartitionsGrouping(
done();
}

/**
* Tests that volume will be disabled if it is disabled in the volume manager.
*/
export async function testAddDisabledVolume(done: () => void) {
const initialState = getEmptyState();
const store = setupStore(initialState);

// Dispatch an action to add crostini volume.
const {volumeManager} = window.fileManager;
const volumeInfo = MockVolumeManager.createMockVolumeInfo(
VolumeManagerCommon.VolumeType.CROSTINI, 'crostini', 'Linux files');
volumeManager.volumeInfoList.add(volumeInfo);
const volumeMetadata = createFakeVolumeMetadata(volumeInfo);
// Disable crostini volume type.
volumeManager.isDisabled = (volumeType) => {
return volumeType === VolumeManagerCommon.VolumeType.CROSTINI;
};
store.dispatch(addVolume({volumeInfo, volumeMetadata}));

// Expect the volume entry is being disabled.
await waitUntil(() => {
const volumeEntry =
getEntry(store.getState(), volumeInfo.displayRoot.toURL()) as
VolumeEntry;
return volumeEntry && volumeEntry.disabled === true;
});

done();
}

/** Tests that volume can be removed correctly. */
export async function testRemoveVolume(done: () => void) {
const initialState = getEmptyState();
Expand Down

0 comments on commit c7d8ba9

Please sign in to comment.