Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix FXIOS-10996, FXIOS-11158 Fix app crashes in HistoryPanel due to duplicate Site identifiers #24116

Merged
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
7dea3c0
FXIOS-11014 - Remove deprecated Site GUID from SQL Lite Database
ih-codes Jan 9, 2025
775496f
FXIOS-11015 - Bugfix - Sites inserted into the SQLite database with S…
ih-codes Jan 9, 2025
4fadb16
Add unit test for int64 size Site IDs to TestSQLitePinnedSites.swift.…
ih-codes Jan 9, 2025
f239bb5
Cleanup.
ih-codes Jan 9, 2025
125b057
Refactor the Site class to be a BasicSite struct adhering to the Site…
ih-codes Jan 6, 2025
0846222
Update SQLLite factories for new Site type. Remove deprecated GUID ro…
ih-codes Jan 6, 2025
4c2525f
Move all Site related files to Storage > Sites.
ih-codes Jan 7, 2025
e3bbd78
Rough in new Site struct and related types enum and its associatedTyp…
ih-codes Jan 7, 2025
e50363c
Adjust file naming for new model naming.
ih-codes Jan 7, 2025
5cee67e
Remove site info model types from Client membership.
ih-codes Jan 7, 2025
70f26c9
Migrate DefaultSuggestedSites, SQLLite related files, and RustPlaces …
ih-codes Jan 7, 2025
c7fd99c
Migrate MainMenuActionHelper, SearchHighlightItem, SearchViewControll…
ih-codes Jan 7, 2025
ef6ed09
Migrate TopSites related code to use the new Site type.
ih-codes Jan 8, 2025
bee92de
More Site cleanup.
ih-codes Jan 8, 2025
5597bf5
Fix unit tests and other compilation errors with Site updates. Add ex…
ih-codes Jan 9, 2025
c031774
Merge branch 'ih/FXIOS-11015-fix-sqlite-int64-storage' into ih/FXIOS-…
ih-codes Jan 9, 2025
b8883fc
Amend fix merge for new Site with forced unique non-nil IDs.
ih-codes Jan 9, 2025
1bcfbea
Fix widget extension by making encode/decode of IDs optional (for mig…
ih-codes Jan 10, 2025
1370f0e
Rename "suggested" google site to "pinned" site data.
ih-codes Jan 10, 2025
3738596
Unit test fixes for google pinned site rename.
ih-codes Jan 10, 2025
fbf7a76
Refactoring and cleanup. Added documentation. Renaming. Cleaning up F…
ih-codes Jan 10, 2025
8e44370
Swiftlint fixes.
ih-codes Jan 10, 2025
64c46c6
Simplify some logic.
ih-codes Jan 10, 2025
19e1fc6
Merge remote-tracking branch 'mozilla/main' into ih/FXIOS-10996-refac…
ih-codes Jan 10, 2025
d8076b1
Bugfix from refactor.
ih-codes Jan 10, 2025
477fc9f
Move SiteType to its own file.
ih-codes Jan 13, 2025
dafabd9
Remove unused placeholder "google guid."
ih-codes Jan 13, 2025
3d8a49d
Cleanup. Revert a doc change.
ih-codes Jan 13, 2025
06ccf62
Add type helper to TopSiteState.
ih-codes Jan 16, 2025
3159687
Be more explicit about ignoring places deferred closure.
ih-codes Jan 16, 2025
a2db44c
Add documentation to be more explicit about sending telemetry for spo…
ih-codes Jan 16, 2025
7208770
Add some Site helpers for type. Use to simplify some type checks.
ih-codes Jan 16, 2025
48800cc
Make Site default inits private so we consistently use factory method…
ih-codes Jan 16, 2025
04d95da
Simplify countPinnedSites.
ih-codes Jan 17, 2025
8641fed
Fix passing favicon resource to the google pinned tile.
ih-codes Jan 17, 2025
6145d2e
Merge remote-tracking branch 'mozilla/main' into ih/FXIOS-10996-refac…
ih-codes Jan 17, 2025
3f75507
Fix unit tests compiling.
ih-codes Jan 17, 2025
4775310
Merge with main and resolve conflicts from PR 24257 related to sponso…
ih-codes Jan 22, 2025
2a0d10a
Additional merge fixes and unit test fixes.
ih-codes Jan 22, 2025
8bc6779
Merge remote-tracking branch 'mozilla/main' into ih/FXIOS-10996-refac…
ih-codes Jan 23, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions firefox-ios/Client.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,9 @@ class BackForwardListViewController: UIViewController,
let isAboutHomeURL = InternalURL(item.url)?.isAboutHomeURL ?? false
var site: Site
if isAboutHomeURL {
site = Site(url: item.url.absoluteString, title: .FirefoxHomePage)
site = Site.createBasicSite(url: item.url.absoluteString, title: .FirefoxHomePage)
} else {
site = sites[urlString] ?? Site(url: urlString, title: item.title ?? "")
site = sites[urlString] ?? Site.createBasicSite(url: urlString, title: item.title ?? "")
}

let viewModel = BackForwardCellViewModel(site: site,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1703,15 +1703,13 @@ class BrowserViewController: UIViewController,
QuickActionsImplementation().addDynamicApplicationShortcutItemOfType(.openLastBookmark,
withUserData: userData,
toApplication: .shared)
site?.setBookmarked(true)
showBookmarkToast(action: .add)
}

func removeBookmark(url: URL, title: String?, site: Site? = nil) {
profile.places.deleteBookmarksWithURL(url: url.absoluteString).uponQueue(.main) { result in
guard result.isSuccess else { return }
self.showBookmarkToast(bookmarkURL: url, title: title, action: .remove)
site?.setBookmarked(false)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,9 @@ class MainMenuActionHelper: PhotonActionSheetProtocol,
iconString: StandardImageIdentifiers.Large.pin) { _ in
guard let url = self.selectedTab?.url?.displayURL,
let title = self.selectedTab?.displayTitle else { return }
let site = Site(url: url.absoluteString, title: title)

let site = Site.createBasicSite(url: url.absoluteString, title: title)

self.profile.pinnedSites.addPinnedTopSite(site).uponQueue(.main) { result in
guard result.isSuccess else { return }
self.delegate?.showToast(message: .LegacyAppMenu.AddPinToShortcutsConfirmMessage, toastAction: .pinPage)
Expand All @@ -800,7 +802,9 @@ class MainMenuActionHelper: PhotonActionSheetProtocol,
iconString: StandardImageIdentifiers.Large.pinSlash) { _ in
guard let url = self.selectedTab?.url?.displayURL,
let title = self.selectedTab?.displayTitle else { return }
let site = Site(url: url.absoluteString, title: title)

let site = Site.createBasicSite(url: url.absoluteString, title: title)

self.profile.pinnedSites.removeFromPinnedTopSites(site).uponQueue(.main) { result in
if result.isSuccess {
self.delegate?.showToast(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ struct SearchHighlightItem {
return highlightItem.urlString ?? ""
}
var siteURL: String {
return Site(url: urlString, title: highlightItem.displayTitle).url
Cramsden marked this conversation as resolved.
Show resolved Hide resolved
return urlString
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,7 @@ class SearchViewController: SiteTableViewController,
twoLineCell,
site.title,
site.url,
site.bookmarked ?? false
site.isBookmarked ?? false
)
cell = twoLineCell
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ class SearchViewModel: FeatureFlaggable, LoaderListener {

var bookmarkSites: [Site] {
delegate?.searchData.compactMap { $0 }
.filter { $0.bookmarked == true } ?? []
.filter { $0.isBookmarked == true } ?? []
}

var historySites: [Site] {
delegate?.searchData.compactMap { $0 }
.filter { $0.bookmarked == false } ?? []
.filter { $0.isBookmarked == false } ?? []
}

private let maxNumOfFirefoxSuggestions: Int32 = 1
Expand Down
4 changes: 2 additions & 2 deletions firefox-ios/Client/Frontend/Browser/SearchLoader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ final class SearchLoader: Loader<Cursor<Site>, SearchViewModel>, FeatureFlaggabl
return
}

let sites = bookmarkItems.map({ Site(url: $0.url, title: $0.title, bookmarked: true, guid: $0.guid) })
let sites = bookmarkItems.map({ Site.createBasicSite(url: $0.url, title: $0.title, isBookmarked: true) })
completionHandler(sites)
}
}
Expand All @@ -74,7 +74,7 @@ final class SearchLoader: Loader<Cursor<Site>, SearchViewModel>, FeatureFlaggabl
// Sort descending by frecency score
$0.frecency > $1.frecency
}.map({
return Site(url: $0.url, title: $0.title )
return Site.createBasicSite(url: $0.url, title: $0.title )
}).uniqued()
completionHandler(sites)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -956,7 +956,8 @@ class TabManagerMiddleware: BookmarksRefactorFeatureFlagProvider {
let url = tab.url?.displayURL?.absoluteString
else { return }

let site = Site(url: url, title: tab.displayTitle)
let site = Site.createBasicSite(url: url, title: tab.displayTitle)

profile.pinnedSites.addPinnedTopSite(site).uponQueue(.main) { result in
guard result.isSuccess else { return }

Expand All @@ -977,7 +978,8 @@ class TabManagerMiddleware: BookmarksRefactorFeatureFlagProvider {
let url = tab.url?.displayURL?.absoluteString
else { return }

let site = Site(url: url, title: tab.displayTitle)
let site = Site.createBasicSite(url: url, title: tab.displayTitle)

profile.pinnedSites.removeFromPinnedTopSites(site).uponQueue(.main) { result in
guard result.isSuccess else { return }
store.dispatch(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ extension BookmarksViewModel: HomepageSectionHandler {
guard let bookmarksCell = cell as? BookmarksCell else { return UICollectionViewCell() }

if let item = bookmarkItems[safe: indexPath.row] {
let site = Site(url: item.url, title: item.title, bookmarked: true)
let site = Site.createBasicSite(url: item.url, title: item.title, isBookmarked: true)
let viewModel = BookmarksCellViewModel(site: site)
bookmarksCell.configure(viewModel: viewModel, theme: theme)
}
Expand All @@ -156,8 +156,8 @@ extension BookmarksViewModel: HomepageSectionHandler {
func handleLongPress(with collectionView: UICollectionView, indexPath: IndexPath) {
guard let onLongPressTileAction = onLongPressTileAction else { return }

let site = Site(url: bookmarkItems[indexPath.row].url,
title: bookmarkItems[indexPath.row].title)
let site = Site.createBasicSite(url: bookmarkItems[indexPath.row].url,
title: bookmarkItems[indexPath.row].title)
let sourceView = collectionView.cellForItem(at: indexPath)
onLongPressTileAction(site, sourceView)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import Storage

struct ContextMenuConfiguration: Equatable {
var homepageSection: HomepageSection
var sourceView: UIView?
Expand All @@ -13,11 +14,9 @@ struct ContextMenuConfiguration: Equatable {
case .topSite(let state, _):
return state.site
case .pocket(let state):
return Site(url: state.url?.absoluteString ?? "",
title: state.title)
return Site.createBasicSite(url: state.url?.absoluteString ?? "", title: state.title)
case .pocketDiscover(let state):
return Site(url: state.url?.absoluteString ?? "",
title: state.title)
return Site.createBasicSite(url: state.url?.absoluteString ?? "", title: state.title)
default:
return nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,16 @@ struct ContextMenuState {
// MARK: - Top sites item's context menu actions
private func getTopSitesActions(site: Site) -> [PhotonRowActions] {
let topSiteActions: [PhotonRowActions]
if site is PinnedSite {
topSiteActions = getPinnedTileActions(site: site)
} else if site as? SponsoredTile != nil {

switch site.type {
case .sponsoredSite:
topSiteActions = getSponsoredTileActions(site: site)
} else {
case .pinnedSite:
topSiteActions = getPinnedTileActions(site: site)
default:
topSiteActions = getOtherTopSitesActions(site: site)
}

return topSiteActions
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/

import Storage

extension Site {
/// A helper to instantiate a Site from the Storage target using the Client target `Contile` type.
static func createSponsoredSite(fromContile contile: Contile) -> Site {
let siteInfo = SponsoredSiteInfo(
tileId: contile.id,
impressionURL: contile.impressionUrl,
clickURL: contile.clickUrl,
imageURL: contile.imageUrl
)

return Site.createSponsoredSite(url: contile.url, title: contile.name, siteInfo: siteInfo)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,21 @@ class TopSiteCell: UICollectionViewCell, ReusableCell {
let siteURLString = topSite.site.url
var imageResource: SiteResource?

if let site = topSite.site as? SponsoredTile,
let url = URL(string: site.imageURL, invalidCharacters: false) {
imageResource = .remoteURL(url: url)
} else if let site = topSite.site as? PinnedSite {
imageResource = site.faviconResource
} else if let site = topSite.site as? SuggestedSite {
imageResource = site.faviconResource
} else if let siteURL = URL(string: siteURLString),
let domainNoTLD = siteURL.baseDomain?.split(separator: ".").first,
domainNoTLD == "google" {
switch topSite.type {
case .sponsoredSite(let siteInfo):
if let url = URL(string: siteInfo.imageURL, invalidCharacters: false) {
imageResource = .remoteURL(url: url)
}
case .pinnedSite, .suggestedSite:
imageResource = topSite.site.faviconResource
default:
break
}

if imageResource == nil,
let siteURL = URL(string: siteURLString),
let domainNoTLD = siteURL.baseDomain?.split(separator: ".").first,
domainNoTLD == "google" {
// Exception for Google top sites, which all return blurry low quality favicons that on the home screen.
// Return our bundled G icon for all of the Google Suite.
// Parse example: "https://drive.google.com/drive/home" > "drive.google.com" > "google"
Expand Down Expand Up @@ -237,7 +242,7 @@ class TopSiteCell: UICollectionViewCell, ReusableCell {
}

private func configureSponsoredSite(_ topSite: TopSiteState) {
guard topSite.isSponsoredTile else { return }
guard topSite.isSponsored else { return }

sponsoredLabel.text = topSite.sponsoredText
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import Storage
import Shared

/// Top site UI class, used in the homepage top site section
final class TopSiteState: Hashable, Equatable {
struct TopSiteState: Hashable, Equatable {
var site: Site
var title: String

Expand All @@ -16,23 +16,31 @@ final class TopSiteState: Hashable, Equatable {
}

var accessibilityLabel: String? {
return isSponsoredTile ? "\(title), \(sponsoredText)" : title
return isSponsored ? "\(title), \(sponsoredText)" : title
}

var isPinned: Bool {
return (site as? PinnedSite) != nil
return site.isPinnedSite
}

var isSuggested: Bool {
return (site as? SuggestedSite) != nil
return site.isSuggestedSite
}

var isSponsoredTile: Bool {
return (site as? SponsoredTile) != nil
var isSponsored: Bool {
return site.isSponsoredSite
}

var isGoogleGUID: Bool {
return site.guid == GoogleTopSiteManager.Constants.googleGUID
var type: SiteType {
return site.type
}

var isGooglePinnedTile: Bool {
guard case SiteType.pinnedSite(let siteInfo) = site.type else {
return false
}

return siteInfo.isGooglePinnedTile
}

var isGoogleURL: Bool {
Expand All @@ -52,42 +60,22 @@ final class TopSiteState: Hashable, Equatable {

func impressionTracking(position: Int) {
// Only sending sponsored tile impressions for now
guard let tile = site as? SponsoredTile else { return }
guard site.isSponsoredSite else { return }

SponsoredTileTelemetry.sendImpressionTelemetry(tile: tile, position: position)
SponsoredTileTelemetry.sendImpressionTelemetry(tileSite: site, position: position)
}

func getTelemetrySiteType() -> String {
if isPinned && isGoogleGUID {
if isGooglePinnedTile {
return "google"
} else if isPinned {
return "user-added"
} else if isSuggested {
return "suggested"
} else if isSponsoredTile {
} else if isSponsored {
return "sponsored"
}

return "history-based"
}

// MARK: - Equatable
static func == (lhs: TopSiteState, rhs: TopSiteState) -> Bool {
lhs.site == rhs.site &&
lhs.isPinned == rhs.isPinned &&
lhs.isSuggested == rhs.isSuggested &&
lhs.isSponsoredTile == rhs.isSponsoredTile &&
lhs.isGoogleGUID == rhs.isGoogleGUID &&
lhs.isGoogleURL == rhs.isGoogleURL
}

// MARK: - Hashable
func hash(into hasher: inout Hasher) {
hasher.combine(self.site)
hasher.combine(self.isPinned)
hasher.combine(self.isSuggested)
hasher.combine(self.isSponsoredTile)
hasher.combine(self.isGoogleGUID)
hasher.combine(self.isGoogleURL)
}
}
Loading