Skip to content

Commit

Permalink
[M115] personalization: correct loading error
Browse files Browse the repository at this point in the history 
On smaller window sizes, navigating back and forth from
wallpaper subpage would result in an indexing error. This
was due to assumptions about the timing of loading the
wallpaper collection list and each individual collection.
Delay assigning `this.splitCollections_` so that the
observer functions run in the expected order.

(cherry picked from commit c7a10c1)

Bug: b:282840764 b:280467959
Test: browser_tests --gtest_filter=*WallpaperCollections*
Change-Id: I7c04a3a0af4b52407d25de09a4ce5bdba0ba7ab5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4547912
Reviewed-by: Jason Thai <jasontt@chromium.org>
Commit-Queue: Jeffrey Young <cowmoo@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1148193}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4594781
Cr-Commit-Position: refs/branch-heads/5790@{#470}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
@cwmoo740
cwmoo740 authored and Chromium LUCI CQ committed Jun 7, 2023
1 parent f9a6270 commit cddf033
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 17 deletions.
10 changes: 8 additions & 2 deletions ash/webui/personalization_app/resources/js/wallpaper/wallpaper_collections_element.ts
View file
Expand Up @@ -426,7 +426,9 @@ export class WallpaperCollections extends WithPersonalizationStore {
this.tiles_ = this.tiles_.filter(tile => !isTimeOfDay(tile));
}

this.splitCollections_ = {
// Delay assigning `this.splitCollections_` until the correct number of
// tiles are assigned.
const splitCollections = {
regular: collections.filter(collection => !isTimeOfDay(collection)),
timeOfDay,
};
Expand All @@ -435,7 +437,7 @@ export class WallpaperCollections extends WithPersonalizationStore {
// of day, local images, and google photos.
const firstBackdropIndex = this.getFirstRegularBackdropTileIndex();
const desiredNumTiles =
this.splitCollections_.regular.length + firstBackdropIndex;
splitCollections.regular.length + firstBackdropIndex;

// Adjust the number of loading tiles to match the collections that just
// came in. There may be more (or fewer) loading tiles than necessary to
Expand All @@ -456,6 +458,10 @@ export class WallpaperCollections extends WithPersonalizationStore {
if (this.tiles_.length > desiredNumTiles) {
this.splice('tiles_', desiredNumTiles);
}

// Assign `this.splitCollections_` now that
// `tiles_.length === desiredNumTiles`.
this.splitCollections_ = splitCollections;
}

/**
Expand Down
115 changes: 100 additions & 15 deletions chrome/test/data/webui/chromeos/personalization_app/wallpaper_collections_element_test.ts
View file
Expand Up @@ -5,7 +5,7 @@
import 'chrome://personalization/strings.m.js';
import 'chrome://webui-test/mojo_webui_test_support.js';

import {emptyState, GooglePhotosEnablementState, kDefaultImageSymbol, PersonalizationRouter, WallpaperActionName, WallpaperCollections, WallpaperGridItem, WallpaperImage} from 'chrome://personalization/js/personalization_app.js';
import {emptyState, GooglePhotosEnablementState, kDefaultImageSymbol, PersonalizationRouter, WallpaperActionName, WallpaperCollection, WallpaperCollections, WallpaperGridItem, WallpaperImage} from 'chrome://personalization/js/personalization_app.js';
import {loadTimeData} from 'chrome://resources/js/load_time_data.js';
import {assertDeepEquals, assertEquals, assertGE, assertTrue} from 'chrome://webui-test/chai_assert.js';
import {waitAfterNextRender} from 'chrome://webui-test/polymer_test_util.js';
Expand All @@ -22,6 +22,20 @@ suite('WallpaperCollectionsTest', function() {
const routerOriginal = PersonalizationRouter.instance;
const routerMock = TestMock.fromClass(PersonalizationRouter);

// A simplified representation of WallpaperCollectionElement tile for
// testing.
interface Tile {
id: string;
type: string;
}
function getTiles(): Tile[] {
// Access a private field for testing because iron-list hides elements
// that are out of the viewport. Pick just id and type fields for
// simpler testing.
return (wallpaperCollectionsElement as any)
.tiles_.map(({id, type}: Tile) => ({id, type}));
}

setup(function() {
const mocks = baseSetup();
wallpaperProvider = mocks.wallpaperProvider;
Expand Down Expand Up @@ -268,20 +282,6 @@ suite('WallpaperCollectionsTest', function() {
const timeOfDayCollectionId =
loadTimeData.getString('timeOfDayWallpaperCollectionId');

// A simplified representation of WallpaperCollectionElement tile for
// testing.
interface Tile {
id: string;
type: string;
}
function getTiles(): Tile[] {
// Access a private field for testing because iron-list hides elements
// that are out of the viewport. Pick just id and type fields for
// simpler testing.
return (wallpaperCollectionsElement as any)
.tiles_.map(({id, type}: Tile) => ({id, type}));
}

personalizationStore.data = emptyState();
// Local images are still loading.
personalizationStore.data.wallpaper.loading.local.images = true;
Expand Down Expand Up @@ -380,4 +380,89 @@ suite('WallpaperCollectionsTest', function() {
assertDeepEquals(expectedTiles, tiles, 'tiles expected to match');
});
}

test('no error reopening wallpaper subpage', async () => {
// Wallpaper collections are loaded when first navigating to the
// wallpaper subpage. First the list of collections, then each
// collection, is requested from server - the component somewhat relies
// on this order to render correctly. Test what happens when user
// navigates to, then away from, and back to the wallpaper collections
// subpage. This begins reloading wallpaper while existing wallpaper
// data is already populated.

// Needs a lot of collections to reproduce the error - there must be more
// wallpaper collections than tiles that fit on the screen.
const generatedCollections: WallpaperCollection[] =
Array.from({length: 20}, (i: number) => ({
id: `generated_collection_${i}`,
name: `Generated Collection ${i}`,
descriptionContent: '',
previews: [{url: createSvgDataUrl(`${i}`)}],
}));
wallpaperProvider.setCollections([
...wallpaperProvider.collections!,
...generatedCollections,
]);

loadTimeData.overrideValues({isTimeOfDayWallpaperEnabled: true});

personalizationStore.setReducersEnabled(true);
personalizationStore.expectAction(
WallpaperActionName.SET_IMAGES_FOR_COLLECTION);

wallpaperCollectionsElement = initElement(WallpaperCollections);

await personalizationStore.waitForAction(
WallpaperActionName.SET_IMAGES_FOR_COLLECTION);

const expectedTilesAfterLoading = [
{
id: '_time_of_day_chromebook_collection',
type: 'image_online',
},
{id: 'local_', type: 'image_local'},
{id: 'google_photos_', 'type': 'loading'},
{id: 'id_0', type: 'image_online'},
{id: 'id_1', type: 'image_online'},
{id: 'id_2', type: 'image_online'},
...generatedCollections.map(({id}) => ({id, type: 'image_online'})),
];

assertDeepEquals(
expectedTilesAfterLoading, getTiles(), 'expected tiles should match');

await teardownElement(wallpaperCollectionsElement);

// Do not use initElement because it flushes startup tasks, but test
// needs to verify an initial state.
wallpaperCollectionsElement =
document.createElement(WallpaperCollections.is) as
WallpaperCollections &
HTMLElement;

// Sets up loading tiles again.
assertDeepEquals(
[
{id: '_time_of_day_chromebook_collection', type: 'loading'},
{id: 'local_', type: 'loading'},
{id: 'google_photos_', type: 'loading'},
],
getTiles().slice(0, 3), 'first special tiles should match');
assertGE(getTiles().length, 6, 'at least 6 tiles at first');
getTiles().slice(3).forEach((tile, i) => {
assertDeepEquals({id: `backdrop_collection_${i}`, type: 'loading'}, tile);
});

// Put the element on the page and wait for network requests to re-fetch
// wallpaper collections.
personalizationStore.expectAction(
WallpaperActionName.SET_IMAGES_FOR_COLLECTION);
document.body.appendChild(wallpaperCollectionsElement);
await personalizationStore.waitForAction(
WallpaperActionName.SET_IMAGES_FOR_COLLECTION);

assertDeepEquals(
expectedTilesAfterLoading, getTiles(),
'expected tiles match the second time');
});
});

0 comments on commit cddf033

Please sign in to comment.