-
Notifications
You must be signed in to change notification settings - Fork 17
Support domain wildcards/regex via if-frame-url triggers #115
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
base: master
Are you sure you want to change the base?
Support domain wildcards/regex via if-frame-url triggers #115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other comments (2)
- Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift (516-516) The function name `splitDomainsForSafari18` doesn't match the actual Safari version being checked (`isSafari26orGreater()`). Consider renaming it to something more descriptive like `splitDomainsForFrameUrlSupport` to avoid confusion.
- Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift (105-105) There's a duplicate comment here. The new comment at line 105 is redundant with the existing JSDoc comment block above it.
💡 To request another review, post a new comment with "/windsurf-review".
ameshkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging the previous pull request there are some conflicts to resolve.
In addition, there are some changes that needs to be done for any PR like that. For guidance please see the merge commit with changes that was made to your previous pull request: def0b10
Here's what needs to be done:
- README should be updated with the new information on compatibility
- Please run
make lintto make sure that the code is formatted correctly. - Please update the relevant bench tests with your baseline before and after the change.
FilterEngineandFilterRuleat this point don't need to support TLD/regexes, but we'll need a separate issue for this.
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
0e350f9 to
53a66ff
Compare
|
Resolved merge conflicts by rebasing onto Addressed review threads (1–11) and the review summary:
Lint/test notes:
Commit list (top): |
ameshkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Sorry that the review takes so long, this is due to New Year vacation.
Overall, looks good to me, there are just a couple of things to address.
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Sources/ContentBlockerConverter/Compiler/BlockerEntryFactory.swift
Outdated
Show resolved
Hide resolved
Tests/ContentBlockerConverterTests/Rules/NetworkRuleTests.swift
Outdated
Show resolved
Hide resolved
| /// - Swift: 6.2 | ||
| /// - Average execution time: ~0.787 sec | ||
| /// | ||
| /// Baseline results (Dec 25, 2025): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please check performance before the change and tell me what's the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was ~0.892s before, so it’s about a +3.5% increase.
Tests/ContentBlockerConverterTests/ContentBlockerConverterTests.swift
Outdated
Show resolved
Hide resolved
Move $domain=/regexp/ unescaping into SimpleRegex.unescapeDomainRegex and add unit tests.
Split frame-url domain pattern building into helpers, remove continue-based control flow, and document legacy Safari behavior.
Thanks for the review, and no worries about the timing. |
|
Forgot to run the lint 😅 |
Safari 26 adds
if-frame-url/unless-frame-urltriggers. This PR uses a hybrid approach to supportdomain.*and domain-regex values while keeping the fast path (if-domain/unless-domain) for plain domains to avoid unnecessary compilation overhead (addresses #100).Changes:
if-domain/unless-domainfor plain domains.if-frame-url/unless-frame-urlonly for:example.*/test\\.(com|net)/|inside/.../domain regex doesn’t split the domain list.