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

Don't report CancellationError from BookmarksFaviconsFetcher #634

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

ayoy
Copy link
Contributor

@ayoy ayoy commented Jan 25, 2024

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1201493110486074/1206407973669779/f
iOS PR: duckduckgo/iOS#2385
macOS PR: duckduckgo/macos-browser#2120
What kind of version bump will this require?: Patch

Optional:

CC: @bwaresiak

Description:
Keep cancelling operations when requested, but without reporting the cancellation
to errorEvents, which could have caused them being reported as Pixels.

Steps to test this PR:

  1. The easiest is to edit code in BSK and add throw CancellationError() e.g. in line 122 of FaviconsFetchOperation.swift
  2. Then run the client app, enable Sync and Pixel logging (on macOS), go to Sync, enable sync (1 device is enough) and enable "Automatically Download Icons"
  3. Verify that Favicons fetch operation has been cancelled but no Pixel has been reported.

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

Internal references:

Software Engineering Expectations
Technical Design Template

@ayoy ayoy self-assigned this Jan 25, 2024
@ayoy ayoy requested a review from SabrinaTardio January 25, 2024 12:21
@ayoy ayoy assigned SabrinaTardio and unassigned ayoy Jan 25, 2024
@ayoy ayoy marked this pull request as ready for review January 25, 2024 12:21
Copy link
Contributor

@SabrinaTardio SabrinaTardio left a comment

Choose a reason for hiding this comment

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

Works as expected!

@ayoy ayoy merged commit 072df96 into main Jan 26, 2024
5 checks passed
@ayoy ayoy deleted the dominik/favicons-fetcher-cancellation-error branch January 26, 2024 13:43
samsymons added a commit that referenced this pull request Feb 8, 2024
* main:
  add alternateHtmlLoad navigation type (HTML Error Page) (#644)
  Check subfeature state before reading rollout data (#649)
  Add NavigationPreferences CustomHeaderFields (#651)
  Update autofill to 10.1.0 (#643)
  Add Autoconsent onByDefault subfeature (#647)
  Add error codes to site breakage reports (#642)
  Run unit tests also on iOS Simulator (#641)
  Improve VPN auth token storage (#639)
  Bump content-scope-scripts to 4.59.2 (#638)
  Fix `site:` queries escaping with iOS 17 SDK (#640)
  Breakage improvement (#621)
  Don't report CancellationError from BookmarksFaviconsFetcher (#634)
  Add explicit mapping of SyncError to error code (#637)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants