Skip to content

Commit

Permalink
Bugfix FXIOS-7874 [v120] homepage crash (#17577) (#17591)
Browse files Browse the repository at this point in the history 
* Fix top sites crash

* Fix tests

(cherry picked from commit 69e65b8)

Co-authored-by: OrlaM <[email protected]>
  • Loading branch information
@mergify @OrlaM
mergify[bot] and OrlaM authored Dec 4, 2023
1 parent d962940 commit 5af44ea
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 69 deletions.
28 changes: 13 additions & 15 deletions Client/Frontend/Home/TopSites/DataManagement/TopSitesDataAdaptor.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,8 @@ protocol TopSitesDataAdaptor {
/// In other words, the number of rows shown depends on the actual data and the user preference.
var numberOfRows: Int { get }

/// Get top sites data, already calculated and ready to be shown to the user
/// Get top sites data
func getTopSitesData() -> [TopSite]

/// Calculate top site data
/// This calculation is dependent on the number of tiles per row that is shown in the user interface.
/// Top sites are composed of pinned sites, history, Contiles and Google top site.
/// Google top site is always first, then comes the contiles, pinned sites and history top sites.
/// We only add Google top site or Contiles if number of pins doesn't exceeds the available number shown of tiles.
/// - Parameter numberOfTilesPerRow: The number of tiles per row shown to the user
func recalculateTopSiteData(for numberOfTilesPerRow: Int)
}

class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable {
Expand All @@ -40,6 +32,7 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable {
private var historySites: [Site] = []
private var contiles: [Contile] = []

private let maxTopSites = 4 * 14 // Max rows * max tiles on the largest screen plus some padding
var notificationCenter: NotificationProtocol
weak var delegate: TopSitesManagerDelegate?
private let topSiteHistoryManager: TopSiteHistoryManager
Expand Down Expand Up @@ -82,12 +75,18 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable {
}

func getTopSitesData() -> [TopSite] {
recalculateTopSiteData()
return topSites
}

func recalculateTopSiteData(for numberOfTilesPerRow: Int) {
/// Calculate top site data
/// This calculation is dependent on the number of tiles per row that is shown in the user interface.
/// Top sites are composed of pinned sites, history, Contiles and Google top site.
/// Google top site is always first, then comes the contiles, pinned sites and history top sites.
/// We only add Google top site or Contiles if number of pins doesn't exceeds the available number shown of tiles.
private func recalculateTopSiteData() {
var sites = historySites
let availableSpaceCount = getAvailableSpaceCount(numberOfTilesPerRow: numberOfTilesPerRow)
let availableSpaceCount = getAvailableSpaceCount(maxTopSites: maxTopSites)
let shouldAddGoogle = shouldAddGoogle(availableSpaceCount: availableSpaceCount)

// Add Sponsored tile
Expand Down Expand Up @@ -115,7 +114,7 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable {
loadTopSites()

dispatchGroup.notify(queue: dataQueue) { [weak self] in
self?.recalculateTopSiteData(for: TopSitesDataAdaptorImplementation.defaultTopSitesRowCount)
self?.recalculateTopSiteData()
self?.delegate?.didLoadNewData()
dataLoadingCompletion?()
}
Expand Down Expand Up @@ -151,10 +150,9 @@ class TopSitesDataAdaptorImplementation: TopSitesDataAdaptor, FeatureFlaggable {
/// Get available space count for the sponsored tiles and Google tiles
/// - Parameter numberOfTilesPerRow: Comes from top sites view model and accounts for different layout (landscape, portrait, iPhone, iPad, etc).
/// - Returns: The available space count for the rest of the calculation
private func getAvailableSpaceCount(numberOfTilesPerRow: Int) -> Int {
private func getAvailableSpaceCount(maxTopSites: Int) -> Int {
let pinnedSiteCount = countPinnedSites(sites: historySites)
let totalNumberOfShownTiles = numberOfTilesPerRow * numberOfRows
return totalNumberOfShownTiles - pinnedSiteCount
return maxTopSites - pinnedSiteCount
}

// The number of rows the user wants.
Expand Down
17 changes: 12 additions & 5 deletions Client/Frontend/Home/TopSites/TopSitesViewModel.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ class TopSitesViewModel {

private let profile: Profile
private var sentImpressionTelemetry = [String: Bool]()
private var unfilteredTopSites: [TopSite] = []
private var topSites: [TopSite] = []
private let dimensionManager: TopSitesDimension
private var numberOfItems: Int = 0
private var numberOfRows: Int = 0

private let topSitesDataAdaptor: TopSitesDataAdaptor
private let topSiteHistoryManager: TopSiteHistoryManager
Expand Down Expand Up @@ -165,7 +167,7 @@ extension TopSitesViewModel: HomepageViewModelProtocol, FeatureFlaggable {

let interface = TopSitesUIInterface(trait: traitCollection, availableWidth: size.width)
let sectionDimension = dimensionManager.getSectionDimension(for: topSites,
numberOfRows: topSitesDataAdaptor.numberOfRows,
numberOfRows: numberOfRows,
interface: interface)
let group = NSCollectionLayoutGroup.horizontal(layoutSize: groupSize,
subitem: item,
Expand Down Expand Up @@ -194,11 +196,14 @@ extension TopSitesViewModel: HomepageViewModelProtocol, FeatureFlaggable {
let interface = TopSitesUIInterface(trait: traitCollection,
availableWidth: size.width)
let sectionDimension = dimensionManager.getSectionDimension(for: topSites,
numberOfRows: topSitesDataAdaptor.numberOfRows,
numberOfRows: numberOfRows,
interface: interface)
topSitesDataAdaptor.recalculateTopSiteData(for: sectionDimension.numberOfTilesPerRow)
topSites = topSitesDataAdaptor.getTopSitesData()
numberOfItems = sectionDimension.numberOfRows * sectionDimension.numberOfTilesPerRow
topSites = unfilteredTopSites
if numberOfItems < unfilteredTopSites.count {
let range = numberOfItems..<unfilteredTopSites.count
topSites.removeSubrange(range)
}
}

func screenWasShown() {
Expand All @@ -214,7 +219,9 @@ extension TopSitesViewModel: HomepageViewModelProtocol, FeatureFlaggable {
extension TopSitesViewModel: TopSitesManagerDelegate {
func didLoadNewData() {
ensureMainThread {
self.topSites = self.topSitesDataAdaptor.getTopSitesData()
self.unfilteredTopSites = self.topSitesDataAdaptor.getTopSitesData()
self.topSites = self.unfilteredTopSites
self.numberOfRows = self.topSitesDataAdaptor.numberOfRows
guard self.isEnabled else { return }
self.delegate?.reloadView()
}
Expand Down
69 changes: 20 additions & 49 deletions Tests/ClientTests/Frontend/Home/TopSites/TopSitesDataAdaptorTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,21 +57,19 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
func testCalculateTopSitesData_hasGoogleTopSite_googlePrefsNil() {
let subject = createSubject()

subject.recalculateTopSiteData(for: 6)
let data = subject.getTopSitesData()

// We test that without a pref, google is added
let data = subject.getTopSitesData()
XCTAssertTrue(data[0].isGoogleURL)
XCTAssertTrue(data[0].isGoogleGUID)
}

func testCalculateTopSitesData_hasGoogleTopSiteWithPinnedCount_googlePrefsNil() {
let subject = createSubject(addPinnedSiteCount: 3)

subject.recalculateTopSiteData(for: 1)
let data = subject.getTopSitesData()

// We test that without a pref, google is added even with pinned tiles
let data = subject.getTopSitesData()
XCTAssertTrue(data[0].isGoogleURL)
XCTAssertTrue(data[0].isGoogleGUID)
}
Expand All @@ -81,10 +79,9 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
profile.prefs.setBool(true, forKey: PrefsKeys.GoogleTopSiteAddedKey)
profile.prefs.setBool(true, forKey: PrefsKeys.GoogleTopSiteHideKey)

subject.recalculateTopSiteData(for: 1)
let data = subject.getTopSitesData()

// We test that having more pinned than available tiles, google tile isn't put in
let data = subject.getTopSitesData()
XCTAssertFalse(data[0].isGoogleURL)
XCTAssertFalse(data[0].isGoogleGUID)
}
Expand All @@ -94,9 +91,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
func testCalculateTopSitesData_pinnedSites() {
let subject = createSubject(addPinnedSiteCount: 3)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertEqual(data.count, 14)
XCTAssertTrue(data[0].isPinned)
}
Expand All @@ -118,19 +114,17 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
let expectedContileResult = ContileProviderMock.getContiles(contilesCount: 1)
let subject = createSubject(expectedContileResult: ContileResult.success(expectedContileResult))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertEqual(data.count, 12, "Expects 1 google site, 1 contile, 10 history sites")
}

func testCalculateTopSitesData_addSponsoredTileAfterGoogle() {
let expectedContileResult = ContileProviderMock.getContiles(contilesCount: 1)
let subject = createSubject(expectedContileResult: ContileResult.success(expectedContileResult))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertTrue(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand All @@ -140,9 +134,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
let expectedContileResult = ContileResult.failure(ContileProvider.Error.noDataAvailable)
let subject = createSubject(expectedContileResult: expectedContileResult)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand All @@ -152,9 +145,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
let expectedContileResult = ContileResult.success([])
let subject = createSubject(expectedContileResult: expectedContileResult)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand All @@ -165,8 +157,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
let expectedContileResult = ContileProviderMock.getContiles(contilesCount: 3)
let subject = createSubject(expectedContileResult: ContileResult.success(expectedContileResult))

subject.recalculateTopSiteData(for: 6)
let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertTrue(data[1].isSponsoredTile)
XCTAssertTrue(data[2].isSponsoredTile)
Expand All @@ -178,9 +170,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
duplicateFirstTile: true,
pinnedDuplicateTile: true)
let subject = createSubject(addPinnedSiteCount: 1, expectedContileResult: ContileResult.success(expectedContileResult))
subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand All @@ -190,10 +181,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
let expectedContileResult = ContileProviderMock.getContiles(contilesCount: 1,
duplicateFirstTile: true)
let subject = createSubject(addPinnedSiteCount: 1, expectedContileResult: ContileResult.success(expectedContileResult))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertTrue(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand All @@ -204,10 +193,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
duplicateFirstTile: true,
pinnedDuplicateTile: true)
let subject = createSubject(addPinnedSiteCount: 1, expectedContileResult: ContileResult.success(expectedContileResult))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertTrue(data[1].isSponsoredTile)
XCTAssertEqual(data[1].title, ContileProviderMock.defaultSuccessData[0].name)
Expand All @@ -221,9 +208,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
profile.prefs.setBool(true, forKey: PrefsKeys.GoogleTopSiteAddedKey)
profile.prefs.setBool(true, forKey: PrefsKeys.GoogleTopSiteHideKey)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertFalse(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand All @@ -232,10 +218,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
func testCalculateTopSitesData_doesNotAddTileIfAllSpacesArePinnedAndGoogleIsThere() {
let expectedContileResult = ContileResult.success([])
let subject = createSubject(addPinnedSiteCount: 11, expectedContileResult: expectedContileResult)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
XCTAssertFalse(data[2].isSponsoredTile)
Expand Down Expand Up @@ -305,10 +289,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
// Sponsored > Frequency
func testDuplicates_SponsoredTileHasPrecedenceOnFrequencyTiles() {
let subject = createSubject(expectedContileResult: ContileResult.success([ContileProviderMock.duplicateTile]))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertEqual(data[1].title, ContileProviderMock.duplicateTile.name)
XCTAssertTrue(data[1].isSponsoredTile)
Expand All @@ -320,9 +302,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
let subject = createSubject(addPinnedSiteCount: 1,
expectedContileResult: ContileResult.success([ContileProviderMock.pinnedDuplicateTile]))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
XCTAssertTrue(data[1].isPinned)
Expand All @@ -334,10 +315,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
func testDuplicates_PinnedTilesHasPrecedenceOnFrequencyTiles() {
let expectedPinnedURL = String(format: ContileProviderMock.url, "0")
let subject = createSubject(addPinnedSiteCount: 1, siteCount: 3, duplicatePinnedSiteURL: true)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertEqual(data.count, 4, "Should have 3 sites and 1 pinned")
XCTAssertTrue(data[0].isGoogleURL)

Expand All @@ -361,9 +340,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
func testDuplicates_PinnedTilesOfSameDomainIsntDeduped() {
let subject = createSubject(addPinnedSiteCount: 2, siteCount: 0)

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertEqual(data.count, 3, "Should have google site and 2 pinned sites")
XCTAssertTrue(data[0].isGoogleURL)

Expand All @@ -388,8 +366,6 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
isCustomEngine: false)
add(searchEngine: googleSearchEngine)
let subject = createSubject()
subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
Expand All @@ -405,9 +381,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
isCustomEngine: false)
add(searchEngine: pinnedTileSearchEngine)
let subject = createSubject(addPinnedSiteCount: 3)
subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertEqual(data.count, 14)
XCTAssertTrue(data[0].isPinned)
}
Expand All @@ -421,8 +396,6 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
isCustomEngine: false)
add(searchEngine: historyTileSearchEngine)
let subject = createSubject()
subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertEqual(data[1].title, "A title 0")
Expand All @@ -438,10 +411,8 @@ class TopSitesDataAdaptorTests: XCTestCase, FeatureFlaggable {
add(searchEngine: sponsoredTileSearchEngine)
let expectedContileResult = ContileProviderMock.getContiles(contilesCount: 1)
let subject = createSubject(expectedContileResult: ContileResult.success(expectedContileResult))

subject.recalculateTopSiteData(for: 6)

let data = subject.getTopSitesData()

XCTAssertTrue(data[0].isGoogleURL)
XCTAssertFalse(data[1].isSponsoredTile)
}
Expand Down

1 comment on commit 5af44ea

@firefoxci-taskcluster
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh! Looks like an error! Details

InterpreterError at template.tasks[0].extra[0].treeherder[1].symbol: unknown context value cron

Please sign in to comment.