Skip to content

Commit

Permalink
Use UpdateUrlAction to record viewTime observations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Grisha Kruglov authored and mergify[bot] committed Sep 20, 2021
1 parent b56d8ff commit b7b8de1
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down

0 comments on commit b7b8de1

Please sign in to comment.