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

Tracking pixels for content blocking fetch and lookup #1084

Merged
merged 10 commits into from
Nov 27, 2024

Conversation

studiosutara
Copy link
Contributor

@studiosutara studiosutara commented Nov 20, 2024

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1208613456171888/1208801514911204/f
iOS PR: duckduckgo/iOS#3597
macOS PR:
What kind of version bump will this require?: Major/Minor/Patch

Optional:

Tech Design URL:
CC:

Description:

Steps to test this PR:
1.
1.

OS Testing:

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

Internal references:

Software Engineering Expectations
Technical Design Template

@studiosutara studiosutara marked this pull request as draft November 20, 2024 06:22
@studiosutara studiosutara force-pushed the smodi/content_blocking_lookup_time_tracking branch 2 times, most recently from 258f999 to 0961198 Compare November 22, 2024 17:43
@studiosutara studiosutara marked this pull request as ready for review November 22, 2024 18:04
@studiosutara studiosutara changed the title DRAFT - Tracking pixels for content blocking fetch and lookup Tracking pixels for content blocking fetch and lookup Nov 25, 2024
Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

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

Great job, I've added one comment around a comment that probably was not intended there. :)

I think we also should be looking at how the event you've added are fired for ContentBlockerRulesManagerInitialCompilationTests can you add that please?

@@ -377,6 +388,9 @@ public class ContentBlockerRulesManager: CompiledRuleListsSource {
unprotectedSitesHash: nil))
}

// if task is main tds task, extract time and iteration count from the result and fire the pixel
// can result be a struct? can we add fields to compliatiotask instead?
// can we sen dpixels from compialtion task?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not an intended change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

XCTAssertEqual(mockLastCompiledRulesStore.rules.first?.name, mockRulesSource.ruleListName)
XCTAssertEqual(mockLastCompiledRulesStore.rules.first?.trackerData, mockRulesSource.trackerData?.tds)
XCTAssertEqual(mockLastCompiledRulesStore.rules.first?.identifier, newIdentifier)
if expRecompiled.currentFulfillmentCount == 1 { // finished compilation after cold start (using last compiled rules)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not needed for my work, but I noticed that code wasn’t getting called. I moved it up into the block. Please check that this is correct.

Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

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

Great job!

@studiosutara studiosutara force-pushed the smodi/content_blocking_lookup_time_tracking branch from be2cbbb to 0f82487 Compare November 26, 2024 22:15
@studiosutara studiosutara merged commit 5a24a88 into main Nov 27, 2024
7 checks passed
@studiosutara studiosutara deleted the smodi/content_blocking_lookup_time_tracking branch November 27, 2024 19:39
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