Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
Pass redirect source and target flags to history tracking delegates.
Browse files Browse the repository at this point in the history 
There's some confusion in `GeckoEngineSession` about redirect flags.
The `VISIT_REDIRECT_SOURCE` and `VISIT_REDIRECT_SOURCE_PERMANENT` flags
that we get from GeckoView's history delegate are for the redirect
_source_, not the visit type. They indicate if the URL passed to
`onVisited` is redirecting _to_ another URL, most likely because the
server returned an HTTP 3xy status code with a `Location` header.
Rust Places decides whether to mark the URL as hidden based on
these flags.

`VISIT_REDIRECT_{PERMANENT, TEMPORARY}`, however, indicate if the
URL passed to `onVisited` is the _target_ of a redirect (in other
words, the page that's _in_ the `Location` header). These get
translated into `VisitType` flags, which Rust Places stores as the
visit transition type. These two flags don't affect whether a URL
is hidden.

Note that, in a redirect chain, the middle links are both sources and
targets. For example, in "mozilla.org" -> "www.mozilla.org" ->
"www.mozilla.org/en-US", "www.mozilla.org" is both a redirect target
(since "mozilla.org" redirected to it), and a source (it redirected
to "www.mozilla.org/en-US").

See mozilla-mobile/fenix#3526.
  • Loading branch information
@linabutler
linabutler committed Sep 3, 2019
1 parent ee70ba7 commit ed61507
Show file tree
Hide file tree
Showing 10 changed files with 181 additions and 82 deletions.
27 changes: 24 additions & 3 deletions ...ne-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import mozilla.components.concept.engine.history.HistoryTrackingDelegate
import mozilla.components.concept.engine.manifest.WebAppManifestParser
import mozilla.components.concept.engine.request.RequestInterceptor
import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.ktx.android.util.Base64
import mozilla.components.support.ktx.kotlin.isEmail
Expand Down Expand Up @@ -437,14 +439,33 @@ class GeckoEngineSession(
val visitType = if (isReload) {
VisitType.RELOAD
} else {
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE_PERMANENT != 0) {
// Note the difference between `VISIT_REDIRECT_PERMANENT`,
// `VISIT_REDIRECT_TEMPORARY`, `VISIT_REDIRECT_SOURCE`, and
// `VISIT_REDIRECT_SOURCE_PERMANENT`.
//
// The former two indicate if the visited page is the *target*
// of a redirect; that is, another page redirected to it.
//
// The latter two indicate if the visited page is the *source*
// of a redirect: it's redirecting to another page, because the
// server returned an HTTP 3xy status code.
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_PERMANENT != 0) {
VisitType.REDIRECT_PERMANENT
} else if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE != 0) {
} else if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_TEMPORARY != 0) {
VisitType.REDIRECT_TEMPORARY
} else {
VisitType.LINK
}
}
val redirectSource = if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE != 0) {
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE_PERMANENT != 0) {
RedirectSource.PERMANENT
} else {
RedirectSource.TEMPORARY
}
} else {
RedirectSource.NOT_A_REDIRECT
}

val delegate = settings.historyTrackingDelegate ?: return GeckoResult.fromValue(false)

Expand All @@ -454,7 +475,7 @@ class GeckoEngineSession(
}

return launchGeckoResult {
delegate.onVisited(url, visitType)
delegate.onVisited(url, PageVisit(visitType, redirectSource))
true
}
}
Expand Down
27 changes: 24 additions & 3 deletions ...gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import mozilla.components.concept.engine.history.HistoryTrackingDelegate
import mozilla.components.concept.engine.manifest.WebAppManifestParser
import mozilla.components.concept.engine.request.RequestInterceptor
import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.ktx.android.util.Base64
import mozilla.components.support.ktx.kotlin.isEmail
Expand Down Expand Up @@ -445,14 +447,33 @@ class GeckoEngineSession(
val visitType = if (isReload) {
VisitType.RELOAD
} else {
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE_PERMANENT != 0) {
// Note the difference between `VISIT_REDIRECT_PERMANENT`,
// `VISIT_REDIRECT_TEMPORARY`, `VISIT_REDIRECT_SOURCE`, and
// `VISIT_REDIRECT_SOURCE_PERMANENT`.
//
// The former two indicate if the visited page is the *target*
// of a redirect; that is, another page redirected to it.
//
// The latter two indicate if the visited page is the *source*
// of a redirect: it's redirecting to another page, because the
// server returned an HTTP 3xy status code.
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_PERMANENT != 0) {
VisitType.REDIRECT_PERMANENT
} else if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE != 0) {
} else if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_TEMPORARY != 0) {
VisitType.REDIRECT_TEMPORARY
} else {
VisitType.LINK
}
}
val redirectSource = if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE != 0) {
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE_PERMANENT != 0) {
RedirectSource.PERMANENT
} else {
RedirectSource.TEMPORARY
}
} else {
RedirectSource.NOT_A_REDIRECT
}

val delegate = settings.historyTrackingDelegate ?: return GeckoResult.fromValue(false)

Expand All @@ -462,7 +483,7 @@ class GeckoEngineSession(
}

return launchGeckoResult {
delegate.onVisited(url, visitType)
delegate.onVisited(url, PageVisit(visitType, redirectSource))
true
}
}
Expand Down
27 changes: 24 additions & 3 deletions .../engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import mozilla.components.concept.engine.history.HistoryTrackingDelegate
import mozilla.components.concept.engine.request.RequestInterceptor
import mozilla.components.concept.engine.content.blocking.Tracker
import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.ktx.android.util.Base64
import mozilla.components.support.ktx.kotlin.isEmail
Expand Down Expand Up @@ -434,14 +436,33 @@ class GeckoEngineSession(
val visitType = if (isReload) {
VisitType.RELOAD
} else {
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE_PERMANENT != 0) {
// Note the difference between `VISIT_REDIRECT_PERMANENT`,
// `VISIT_REDIRECT_TEMPORARY`, `VISIT_REDIRECT_SOURCE`, and
// `VISIT_REDIRECT_SOURCE_PERMANENT`.
//
// The former two indicate if the visited page is the *target*
// of a redirect; that is, another page redirected to it.
//
// The latter two indicate if the visited page is the *source*
// of a redirect: it's redirecting to another page, because the
// server returned an HTTP 3xy status code.
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_PERMANENT != 0) {
VisitType.REDIRECT_PERMANENT
} else if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE != 0) {
} else if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_TEMPORARY != 0) {
VisitType.REDIRECT_TEMPORARY
} else {
VisitType.LINK
}
}
val redirectSource = if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE != 0) {
if (flags and GeckoSession.HistoryDelegate.VISIT_REDIRECT_SOURCE_PERMANENT != 0) {
RedirectSource.PERMANENT
} else {
RedirectSource.TEMPORARY
}
} else {
RedirectSource.NOT_A_REDIRECT
}

val delegate = settings.historyTrackingDelegate ?: return GeckoResult.fromValue(false)

Expand All @@ -452,7 +473,7 @@ class GeckoEngineSession(

val result = GeckoResult<Boolean>()
launch {
delegate.onVisited(url, visitType)
delegate.onVisited(url, PageVisit(visitType, redirectSource))
result.complete(true)
}
return result
Expand Down
5 changes: 4 additions & 1 deletion .../engine-system/src/main/java/mozilla/components/browser/engine/system/SystemEngineView.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ import mozilla.components.concept.engine.HitResult
import mozilla.components.concept.engine.content.blocking.Tracker
import mozilla.components.concept.engine.prompt.PromptRequest
import mozilla.components.concept.engine.request.RequestInterceptor.InterceptionResponse
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.ktx.android.view.getRectWithViewLocation
import mozilla.components.support.utils.DownloadUtils
Expand Down Expand Up @@ -152,7 +154,8 @@ class SystemEngineView @JvmOverloads constructor(
}

runBlocking {
session?.settings?.historyTrackingDelegate?.onVisited(url, visitType)
session?.settings?.historyTrackingDelegate?.onVisited(url,
PageVisit(visitType, RedirectSource.NOT_A_REDIRECT))
}
}

Expand Down
11 changes: 8 additions & 3 deletions ...-memory/src/main/java/mozilla/components/browser/storage/memory/InMemoryHistoryStorage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import mozilla.components.concept.storage.HistoryAutocompleteResult
import mozilla.components.concept.storage.HistoryStorage
import mozilla.components.concept.storage.PageObservation
import mozilla.components.concept.storage.SearchResult
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitInfo
import mozilla.components.concept.storage.VisitType
import mozilla.components.support.utils.StorageUtils.levenshteinDistance
Expand All @@ -28,14 +30,17 @@ class InMemoryHistoryStorage : HistoryStorage {
@VisibleForTesting
internal val pageMeta: HashMap<String, PageObservation> = hashMapOf()

override suspend fun recordVisit(uri: String, visitType: VisitType) {
override suspend fun recordVisit(uri: String, visit: PageVisit) {
val now = System.currentTimeMillis()
if (visit.redirectSource != RedirectSource.NOT_A_REDIRECT) {
return
}

synchronized(pages) {
if (!pages.containsKey(uri)) {
pages[uri] = mutableListOf(Visit(now, visitType))
pages[uri] = mutableListOf(Visit(now, visit.visitType))
} else {
pages[uri]!!.add(Visit(now, visitType))
pages[uri]!!.add(Visit(now, visit.visitType))
}
}
}
Expand Down
16 changes: 14 additions & 2 deletions ...torage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import mozilla.appservices.places.VisitObservation
import mozilla.components.concept.storage.HistoryAutocompleteResult
import mozilla.components.concept.storage.HistoryStorage
import mozilla.components.concept.storage.PageObservation
import mozilla.components.concept.storage.PageVisit
import mozilla.components.concept.storage.SearchResult
import mozilla.components.concept.storage.RedirectSource
import mozilla.components.concept.storage.VisitInfo
import mozilla.components.concept.storage.VisitType
import mozilla.components.concept.sync.SyncAuthInfo
Expand All @@ -29,12 +31,22 @@ open class PlacesHistoryStorage(context: Context) : PlacesStorage(context), Hist

override val logger = Logger("PlacesHistoryStorage")

override suspend fun recordVisit(uri: String, visitType: VisitType) {
override suspend fun recordVisit(uri: String, visit: PageVisit) {
withContext(scope.coroutineContext) {
// Ignore exceptions related to uris. This means we may drop some of the data on the floor
// if the underlying storage layer refuses it.
ignoreUrlExceptions("recordVisit") {
places.writer().noteObservation(VisitObservation(uri, visitType = visitType.into()))
places.writer().noteObservation(VisitObservation(uri,
visitType = visit.visitType.into(),
isRedirectSource = when (visit.redirectSource) {
RedirectSource.PERMANENT, RedirectSource.TEMPORARY -> true
RedirectSource.NOT_A_REDIRECT -> false
},
isPermanentRedirectSource = when (visit.redirectSource) {
RedirectSource.PERMANENT -> true
RedirectSource.TEMPORARY, RedirectSource.NOT_A_REDIRECT -> false
}
))
}
}
}
Expand Down
Loading

0 comments on commit ed61507

Please sign in to comment.