Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass redirect source and target flags to history tracking delegates #4268

Merged
merged 1 commit into from Oct 12, 2019

Conversation

@linacambridge
Copy link
Contributor

@linacambridge linacambridge commented Aug 30, 2019

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.


Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.
@linacambridge linacambridge requested a review from as a code owner Aug 30, 2019
@linacambridge
Copy link
Contributor Author

@linacambridge linacambridge commented Aug 30, 2019

This isn't complete yet—I think we'll also need a way to filter out hidden URLs in views. But we'll need to change a-s to pass the hidden flag in HistoryVisitInfo for that.

@linacambridge
Copy link
Contributor Author

@linacambridge linacambridge commented Sep 3, 2019

I think we'll also need a way to filter out hidden URLs in views.

After thinking more about this, let's do the filtering in a-s for now (mozilla/application-services#1715). We can always change a-c or Fenix to exclude hidden on a per-view basis as we need, and that minimizes the changes that Fenix will need to make (just bump the a-c and a-s versions).

See also mozilla/application-services#1492.

@linacambridge
Copy link
Contributor Author

@linacambridge linacambridge commented Sep 11, 2019

@grigoryk Please review at your convenience!

@pocmo
Copy link
Contributor

@pocmo pocmo commented Sep 16, 2019

Will this fix this? mozilla-mobile/fenix#2542

@linacambridge
Copy link
Contributor Author

@linacambridge linacambridge commented Sep 16, 2019

Will this fix this? mozilla-mobile/fenix#2542

Nope, that's different; we intentionally don't filter out hidden visits from the awesomebar. I'd be curious to hear if they persist once a profile accumulates more history, like @thomcc mentioned in #3027 (comment).

I see the same thing in a fresh Desktop profile: visiting https://www.mozilla.org/firefox/new (which redirects to https://www.mozilla.org/en-US/firefox/new/), then typing "mozilla firefox" into the awesomebar shows this:

Screenshot of redirect source and target in Firefox 71 awesomebar

Doing the same in my regular profile doesn't, because I've visited and bookmarked waaay more Mozilla and Firefox sites.

But Desktop's suggestion algorithm probably wasn't written with mobile in mind. It sounds like it's worth diverging, because suggestions for redirects are much lower value here.

@jdragojevic
Copy link
Contributor

@jdragojevic jdragojevic commented Oct 4, 2019

A little nudge here: @linacambridge does this need more work, or @grigoryk does this need more review?

@@ -14,7 +14,7 @@ interface HistoryStorage : Storage {
* @param uri of the page which was visited.
* @param visitType type of the visit, see [VisitType].
Copy link
Contributor

@rocketsroger rocketsroger Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comments with updated param. But please see comment below first.

@@ -14,6 +14,8 @@ permalink: /changelog/

* **feature-pwa**
* Adds `WebAppUseCases.isInstallable` to check if the current session can be installed as a Progressive Web App.
* **browser-engine-gecko-nightly**, **browser-engine-gecko-beta** and **browser-engine-gecko**
Copy link
Contributor

@rocketsroger rocketsroger Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line needed

Copy link
Contributor

@rocketsroger rocketsroger Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linacambridge Please move this to the latest version.

@linacambridge
Copy link
Contributor Author

@linacambridge linacambridge commented Oct 8, 2019

Thanks, @rocketsroger! 🎊 Would you like me to go ahead and flatten PageVisit into visitType and redirectSource arguments, or keep it as is?

@rocketsroger
Copy link
Contributor

@rocketsroger rocketsroger commented Oct 8, 2019

Thanks, @rocketsroger! 🎊 Would you like me to go ahead and flatten PageVisit into visitType and redirectSource arguments, or keep it as is?

I'm good with keeping it as is. Since there's a good reason for it. Please update the kdoc parameters and also move the change in changlog.md to the latest version. I think this is is good to go except the small changes.

@codecov
Copy link

@codecov codecov bot commented Oct 11, 2019

Codecov Report

Merging #4268 into master will decrease coverage by 0.92%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4268      +/-   ##
============================================
- Coverage     81.95%   81.02%   -0.93%     
- Complexity     4190     4203      +13     
============================================
  Files           538      540       +2     
  Lines         18074    18454     +380     
  Branches       2697     2746      +49     
============================================
+ Hits          14812    14952     +140     
- Misses         2181     2384     +203     
- Partials       1081     1118      +37
Impacted Files Coverage Δ Complexity Δ
...mponents/browser/engine/system/SystemEngineView.kt 78.67% <50%> (+0.06%) 46 <0> (ø) ⬇️
...nents/browser/storage/sync/PlacesHistoryStorage.kt 89.83% <50%> (-6.33%) 20 <0> (ø)
...s/browser/storage/memory/InMemoryHistoryStorage.kt 85.91% <50%> (-2.5%) 28 <0> (ø)
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100% <0%> (ø) 11% <0%> (ø) ⬇️
...lla/components/feature/session/SettingsUseCases.kt
...la/components/feature/session/FullScreenFeature.kt
...ozilla/components/feature/session/WindowFeature.kt
...la/components/feature/session/ThumbnailsFeature.kt
...ature/session/behavior/EngineViewBottomBehavior.kt
...illa/components/feature/session/SessionUseCases.kt
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e30b307...15b873f. Read the comment docs.

@linacambridge
Copy link
Contributor Author

@linacambridge linacambridge commented Oct 11, 2019

docs/changelog.md Show resolved Hide resolved
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.
Copy link
Contributor

@rocketsroger rocketsroger left a comment

Looks good to me. 🚢

@rocketsroger
Copy link
Contributor

@rocketsroger rocketsroger commented Oct 12, 2019

bors r+

bors bot pushed a commit that referenced this issue Oct 12, 2019
4268: Pass redirect source and target flags to history tracking delegates r=rocketsroger a=linacambridge

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.



Co-authored-by: Lina Cambridge <lina@yakshaving.ninja>
@bors
Copy link

@bors bors bot commented Oct 12, 2019

Build succeeded

  • complete-push

@bors bors bot merged commit 15b873f into mozilla-mobile:master Oct 12, 2019
106 of 107 checks passed
@rocketsroger rocketsroger added this to the 🍩 17.0.0 milestone Oct 12, 2019
@linacambridge linacambridge deleted the history-redirects branch Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants