2
0
mirror of https://github.com/fork-maintainers/iceraven-browser synced 2024-11-03 23:15:31 +00:00

Use UpdateUrlAction to record viewTime observations

We discovered that in a tab restore scenario we were recording view time
observations that were wrong - we'd record time deltas as-if user was
looking at the page while the browser wasn't running.

This happens because when we record a viewTime observation, we compare
current time with lastAccess time of the tab. In a restore scenario,
that lastAccess time happens to be from when the browser was last
running - which could be days ago.

The simplest solution was to not record a viewTime observation if the
url for a tab didn't change during a load event. To achieve this, we
needed to change which action we were using as a proxy for "navigation
events" - UpdateUrlAction contains the new url, allowing us to compare
against the current tab url.

Alternative solutions would be to keep using loading actions, but
dispatch a lastAccess event before performing a metadata update. This
would have worked, but would result in two lastAccess events being
dispatched for each navigation event instead of just one.
This commit is contained in:
Grisha Kruglov 2021-09-17 17:33:51 -07:00 committed by mergify[bot]
parent b56d8ff545
commit b7b8de1c2f
2 changed files with 25 additions and 7 deletions

View File

@ -67,13 +67,19 @@ class HistoryMetadataMiddleware(
}
}
}
is ContentAction.UpdateLoadingStateAction -> {
is ContentAction.UpdateUrlAction -> {
context.state.findNormalTab(action.sessionId)?.let { tab ->
val selectedTab = tab.id == context.state.selectedTabId
if (!tab.content.loading && action.loading && selectedTab) {
// When a page starts loading (e.g. user navigated away by
// clicking on a link) we update metadata for the selected
// (i.e. previous) url of this tab.
// When page url changes (e.g. user navigated away by clicking on a link)
// we update metadata for the selected (i.e. previous) url of this tab.
// We don't update metadata for cases or reload or restore.
// In case of a reload it's not necessary - metadata will be updated when
// user moves away from the page or tab.
// In case of restore, it's both unnecessary (like for a reload) and
// problematic, since our lastAccess time will be from before the tab was
// restored, resulting in an incorrect (too long) viewTime observation, as if
// the user was looking at the page while the browser wasn't even running.
if (selectedTab && action.url != tab.content.url) {
updateHistoryMetadata(tab)
}
}

View File

@ -174,20 +174,32 @@ class HistoryMetadataMiddlewareTest {
}
@Test
fun `GIVEN normal tab WHEN user navigates and new page starts loading THEN meta data is updated`() {
fun `GIVEN normal tab WHEN update url action event with a different url is received THEN meta data is updated`() {
val existingKey = HistoryMetadataKey(url = "https://mozilla.org")
val tab = createTab(url = existingKey.url, historyMetadata = existingKey)
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify { service wasNot Called }
store.dispatch(ContentAction.UpdateLoadingStateAction(tab.id, true)).joinBlocking()
store.dispatch(ContentAction.UpdateUrlAction(tab.id, "https://www.someother.url")).joinBlocking()
val capturedTab = slot<TabSessionState>()
verify { service.updateMetadata(existingKey, capture(capturedTab)) }
assertEquals(tab.id, capturedTab.captured.id)
}
@Test
fun `GIVEN normal tab WHEN update url action event with the same url is received THEN meta data is not updated`() {
val existingKey = HistoryMetadataKey(url = "https://mozilla.org")
val tab = createTab(url = existingKey.url, historyMetadata = existingKey)
store.dispatch(TabListAction.AddTabAction(tab)).joinBlocking()
verify { service wasNot Called }
store.dispatch(ContentAction.UpdateUrlAction(tab.id, existingKey.url)).joinBlocking()
verify { service wasNot Called }
}
@Test
fun `GIVEN tab without meta data WHEN user navigates and new page starts loading THEN nothing happens`() {
val tab = createTab(url = "https://mozilla.org")