[navigation #3+4] canGoBack/GoForward; SERP headers #885
Conversation
mallexxx
force-pushed
the
alex/gobackforward-republished
branch
from
1eba6ec
to
2d9c1b9
Compare
mallexxx
changed the title
mallexxx
changed the title
mallexxx
changed the base branch from
alex/dist-nav-delegate
to
alex/navigation/adclick
mallexxx
changed the title
|
@tomasstrba this one not related, it's been fixed in the #884, not merged to this one yet |
|
@tomasstrba I updated the branch to match #884, the assertion shouldn‘t happen anymore |
|
|
||
| /// `CurrentValueSubject` wrapper used to receive and store a published property and re-publish it when used as a $projectedValue | ||
| @propertyWrapper | ||
| struct RePublished<Owner: AnyObject, Value, Pub: Publisher<Value, Never>> { |
There was a problem hiding this comment.
In general, it's always a good idea to consider using existing components from the framework before introducing custom implementations, as this reduces the amount of code you need to maintain, ensures compatibility with other apps and makes it easier for new team members to start contributing to codebase.
In order to descope your project where possible, do you think we could focus on this property wrapper after you release the refactor? It is already too complicated and introducing custom property wrapper like this doesn't make it easier.
There was a problem hiding this comment.
LGTM! 👍
One more issue I found that is probably not related to this PR but related to the refactoring is how quickly is the tab released from memory. If you play any video and close the tab, it continues playing for few seconds. If possible, please make sure this issue is resolved before merging this to develop
|
|
||
| // MARK: - Back/Forward navigation | ||
|
|
||
| func testCanGoBack() throws { |
There was a problem hiding this comment.
Does it make sense to break down the test into multiple tests or do you think it would lead to unnecessary code duplication?
There was a problem hiding this comment.
yes, splitting the test into multiple will lead to repeating the whole test but with different assertion points, so let‘s keep it as it is
mallexxx
changed the title
* SERP headers handling in SerpHeadersNavigationResponder
mallexxx
changed the title
|
the issue with the delayed Tab release you mentioned was caused by the long time decision making checker in BSK PR (DEBUG only) which is fixed already. |
* 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/72649045549333/1202061524436301/f
Tech Design URL: (RePublished) https://app.asana.com/0/481882893211075/1203312264447430/f
CC: @tomasstrba
Description:
Steps to test this PR:
Testing checklist:
Internal references:
Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM