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

Exclude download, redirects, reload, embed and framed link visit types from the get_top_frecent_site_infos query #3505

merged 1 commit into from Aug 19, 2020


Copy link

@gabrielluong gabrielluong commented Aug 18, 2020

Fixes #3503

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • automation/ runs to completion and produces no failures
    • Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Copy link
Member Author

gabrielluong commented Aug 18, 2020

This follows the allowed types (complement of the excluded types) used in the "History" screen of Fenix

@gabrielluong gabrielluong requested a review from linabutler Aug 18, 2020
@gabrielluong gabrielluong changed the title Add allowed types in get_top_frecent_site_infos query Exclude download, redirects, reload, embed and framed link visit types from the get_top_frecent_site_infos query Aug 18, 2020
Copy link

codecov-commenter commented Aug 18, 2020

Codecov Report

Merging #3505 into main will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3505      +/-   ##
- Coverage   76.53%   76.52%   -0.01%     
  Files         230      230              
  Lines       30753    30754       +1     
  Hits        23536    23536              
- Misses       7217     7218       +1     
Impacted Files Coverage Δ
components/places/src/storage/ 95.49% <0.00%> (-0.07%) ⬇️

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 b45bb56...b6376ef. Read the comment docs.

Copy link

@linabutler linabutler left a comment

Great, thank you so much @gabrielluong! I left a comment about performance concerns (one table scan to two), but I think we should get this landed now to unblock you, and file a follow-up to figure out an overarching plan for top sites storage in Places.

LIMIT :limit",
"SELECT h.frecency, h.title, h.url
FROM moz_places h
Copy link

@linabutler linabutler Aug 18, 2020

Choose a reason for hiding this comment

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

So, I'm wondering about the performance implications for large history sets. We're already doing a full table scan on moz_places, since we don't index on anything except frecency; now we'll need another table scan on moz_historyvisits.

I don't want to block your PR on that, but it's something to think about if we want to push top site storage down into Places. Having a top_site tri-state column in moz_places (always show, never show, or show based on frecency) would let us do that without the second scan—we'd pick the "always show" sites first, and then fill the rest based on frecency up to the limit. "Never show" would be for sites that the user explicitly removes from top sites, as well as ones that have forbidden types—and we can determine those cheaply at insert time! 🎉

@gabrielluong gabrielluong merged commit d0c67c9 into mozilla:main Aug 19, 2020
14 checks passed
@gabrielluong gabrielluong deleted the 3503 branch Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
3 participants