[navigation #4] SERP headers navigation responder #887
Conversation
mallexxx
changed the title
mallexxx
changed the title
mallexxx
changed the title
mallexxx
changed the title
mallexxx
changed the title
mallexxx
changed the base branch from
alex/navigation/adclick
to
alex/gobackforward-republished
There was a problem hiding this comment.
@mallexxx, I am getting an assertion right after I run the app. Let me know if I am doing something wrong or you can't reproduce it.
The code looks good, I like the code coverage 🥇 . I just added few notes below.
|
|
||
| func decidePolicy(for navigationAction: NavigationAction, preferences: inout NavigationPreferences) async -> NavigationActionPolicy? { | ||
| guard let mainFrame = navigationAction.mainFrameTarget, | ||
| navigationAction.url.isDuckDuckGo, |
There was a problem hiding this comment.
Just to speed things up, do you think it makes sense to have navigationAction.url.isDuckDuckGo as first condition?
There was a problem hiding this comment.
I don‘t think it makes sense since the main frame check is actually faster than comparing strings, but it‘s microoptimization anyways
There was a problem hiding this comment.
The main frame check is definitely faster, but useless for 99 percent of request since the URL isn't duckduckgo.com. Anyway, I don't mind leaving it as it is.
|
|
||
| print(url) | ||
| tab.setContent(.url(url)) | ||
| waitForExpectations(timeout: 50) |
There was a problem hiding this comment.
Is it possible to use a lower value than 50?
There was a problem hiding this comment.
forgotten test value 🙄
|
|
||
| override func setUp() { | ||
| // disable waiting for CBR compilation on navigation | ||
| MockPrivacyConfiguration.isFeatureKeyEnabled = { _, _ in |
There was a problem hiding this comment.
I know it isn't the scope of this PR, but why is MockPrivacyConfiguration global? I am afraid it breaks the encapsulation of unit tests
| @MainActor | ||
| func testOnDDGRequest_headersAdded() { | ||
| var onNavAction: (@MainActor (NavigationAction) -> NavigationActionPolicy?)! | ||
| TestTabExtensionsBuilder.shared = TestTabExtensionsBuilder(load: []) { builder in { _, _ in |
There was a problem hiding this comment.
Again, the test uses TestTabExtensionsBuilder.shared, which is a global state. Can it be refactored to use an instance of TestTabExtensionsBuilder instead of static reference to TestTabExtensionsBuilder? It doesn't need to be refactored in this PR.
|
|
||
| let tab = Tab(content: .none, shouldLoadInBackground: true) | ||
|
|
||
| for child in Mirror(reflecting: urls).children.filter({ $0.label!.hasPrefix("ddg") }) { |
There was a problem hiding this comment.
Does it make sense to refactor this to more readable variant by declaring two structs:
- DDGUrls
- NotDDGUrls
?
|
updated the PR cleaning up things |
* canGoBack, canGoForward RePublished, Tab.publishers refactored * redo redirect(_:NavigationAction, invalidatingBackItemIfNeeded) * tests * don‘t use shared state in tests * fix RELEASE * rollback renaming * drop RePublished * fix header name * [navigation #4] SERP headers navigation responder (#887) * SERP headers handling in SerpHeadersNavigationResponder
* AdClickAttributionTabExtension+NavigationResponder * fix TabExtension test overrides, adClick extension tests * fix AdClickAttributionTabExtensionTests teardown * cleanup * don‘t use shared state in tests * fix adClick inherited attribution initialization * cleanup * Adjust adClick tests to better match UserScripts/UserContentController behaviour; cleanup * fix RELEASE * remove Swifter dependency * fix file header * [navigation #3+4] canGoBack/GoForward; SERP headers (#885) * canGoBack, canGoForward RePublished, Tab.publishers refactored * redo redirect(_:NavigationAction, invalidatingBackItemIfNeeded) * tests * don‘t use shared state in tests * fix RELEASE * rollback renaming * drop RePublished * fix header name * [navigation #4] SERP headers navigation responder (#887) * SERP headers handling in SerpHeadersNavigationResponder * convert TabTests to custom SchemeHandler
Task/Issue URL: https://app.asana.com/0/0/1203487090719126/f
Tech Design URL: https://app.asana.com/0/481882893211075/1203268245242140
CC: @tomasstrba @bwaresiak
Description:
Moves adding headers to Search Queries to a separate Navigation Responder
Steps to test this PR: