Closed
Bug 1493636
Opened 6 years ago
Closed 6 years ago
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold
Categories
(Firefox :: Address Bar, enhancement, P1)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox62 | --- | unaffected |
firefox63 | --- | verified |
firefox64 | --- | verified |
People
(Reporter: danibodea, Assigned: adw)
References
Details
(Whiteboard: [fxsearch])
Attachments
(2 files)
46 bytes,
text/x-phabricator-request
|
mak
:
feedback+
|
Details | Review |
16.66 KB,
patch
|
pascalc
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[Note]:
- This is a followup on bug 1488879, based on comment https://bugzilla.mozilla.org/show_bug.cgi?id=1488879#c36, using the STR from comment https://bugzilla.mozilla.org/show_bug.cgi?id=1488879#c49 and considering the user's dissatisfaction from bug's comments, the process of the auto-filling of the address bar with previously saved bookmarks is not working as expected. It appears that a sub-groups of users would like bookmarks to get auto-filled by default, indifferent to the frequency of the bookmark's visitation (as explained in comment https://bugzilla.mozilla.org/show_bug.cgi?id=1488879#c35).
[Affected versions]:
- 64.0a1, 63.0b9, 62.0.2
[Affected platforms]:
-All
[Steps to reproduce]:
1. Clear browser history and uncheck "remember my history"
2. Save two bookmark of different domains, https://www.mozilla.org/ and https://www.youtube.com/ (autofill should work properly for now)
3. Save another bookmark to https://www.youtube.com/user/Mozilla
4. Type "moz" in the address bar. The URL should be auto-filled as mozilla.org, but it does not.
5. Type "you" in the address bar.
[Expected result]:
- The URL should is auto-filled as youtube.com; any of the bookmarks should get auto-filled.
[Actual result]:
- It turns out If I have two or more bookmarks of the same domain, other bookmarks of unique domains will not be auto-filled.
Reporter | ||
Updated•6 years ago
|
Severity: normal → enhancement
status-firefox62:
--- → unaffected
status-firefox63:
--- → affected
See Also: → 1488879
Comment 2•6 years ago
|
||
Unfortunately if history is disabled we don't have the last time a bookmark was accessed, the only data we could rely on is the decayed frecency, though it may not be that good (because maybe I created a bookmark 10 years ago, but I use it everyday). It was working before because we were not clearing the typed information with history, that is also wrong.
We could distinguish 2 cases:
1. history is enabed: use the normal threshold
2. history is disabled, or clearonshutdown is enabled: autofill bookmarks
The problem is in the middle, what if history is enabled but the user clears history completely? From that point on, nothing gets autofilled anymore.
Maybe in the end the simplest path is what you suggested, just always autofill bookmarks. Though, that requires a performant way to figure out if an origin is bookmarked.
Flags: needinfo?(adw)
Priority: -- → P1
Whiteboard: [fxsearch]
I don't get it: When the "browsing and download history" checkbox is unchecked, the browsing and download history is NOT supposed to get deleted. That's the reason for this checkbox, as far as I apprehend.
Comment 4•6 years ago
|
||
Axel, I'm sorry, reopening your report and keeping it separate, it looks like I misread your log.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Assignee | ||
Comment 5•6 years ago
|
||
I still think exempting bookmarks from the autofill frecency threshold is probably the right thing to do, but I'd also like to test/investigate why exactly they aren't getting autofilled in these cases -- and there are slightly different cases being reported. As long as your history is 100% cleared, then the threshold should be zero, so bookmarks *should* be autofilled since we fixed bug 1488879. Once you start visiting URLs after that, if you are not in permanent private browsing mode, then the threshold will start to go up, and in that case it's clear why unvisited bookmarks would not autofill. Exempting bookmarks is the fix for that. But, if you are in permanent private browsing mode, theoretically autofilling bookmarks should Just Work, because presumably frecencies never change, so the threshold remains at zero. I need to look into how that works.
Comment 6•6 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #5)
> I still think exempting bookmarks from the autofill frecency threshold is
> probably the right thing to do, but I'd also like to test/investigate why
> exactly they aren't getting autofilled in these cases -- and there are
> slightly different cases being reported. As long as your history is 100%
> cleared, then the threshold should be zero
I'm not sure about this.
https://searchfox.org/mozilla-central/rev/881a3c5664ede5e08ee986d76433bc5c4b5680e6/toolkit/components/places/SQLFunctions.cpp#832
a bookmark, even if unvisited, won't have a zero frecency, and as such I'd expect the threshold to not be zero.
Assignee | ||
Comment 7•6 years ago
|
||
Ah yes, you're right
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 9014235 [details]
Bug 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold.
Marco, this just unconditionally includes the bookmarked subquery in the main query, and then does `HAVING host_frecency >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD} OR bookmarked`. This seems like a simple fix that we can uplift to address the problem immediately, as opposed to a more comprehensive fix that we've briefly discussed that possibly involves a schema change. IMO we could take this now but still continue to evaluate a more performant fix involving a schema change. However, we do have an index on moz_places.origin_id, so maybe there's not much of a perf win to be had here? And obviously we already use this exact query when sugest.history is disabled, but we'd be extending that perf to all users with this, not only those with suggest.history=false.
What do you think? I know you already had some comments on this in a similar patch in bug 1489060 comment 7, but here I'm not doing the extra sorting.
Attachment #9014235 -
Flags: feedback?(mak77)
Comment 10•6 years ago
|
||
Comment on attachment 9014235 [details]
Bug 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold.
off-hand it sounds ok to me, there is a small perf hit but it looks acceptable
Attachment #9014235 -
Flags: feedback?(mak77) → feedback+
Assignee | ||
Comment 11•6 years ago
|
||
Thanks. This bitrots bug 1494471, but I'll handle that one after this one.
Assignee | ||
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9c7d14fc4a9
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak
Comment 14•6 years ago
|
||
Backed out 1 changesets (bug 1493636) for high frequency test_escape_autocomplete.py failures
push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=203766661&revision=e9c7d14fc4a9360b2a4ae94636aa8fe65b16ed72
failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&searchStr=en-us
backout: https://hg.mozilla.org/integration/autoland/rev/9c9ef9cced433d48a75465f4a9245206d073b8c2
Flags: needinfo?(adw)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(adw)
Comment 15•6 years ago
|
||
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/237c55ddb112
Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak
Comment 16•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Assignee | ||
Comment 17•6 years ago
|
||
Please see comment 0 for one set of STR.
Additional STR, similar to comment 0 but not the same:
1. Save two bookmarks of different domains, https://www.mozilla.org/ and https://www.youtube.com/
2. Save another bookmark, https://www.youtube.com/user/Mozilla
3. Clear history (using "last hour" or "everything", shouldn't matter)
4. Type "moz" in the address bar. The URL should be auto-filled as mozilla.org.
5. Type "you" in the address bar. The URL should be auto-filled as youtube.com.
Flags: qe-verify+
Assignee | ||
Comment 18•6 years ago
|
||
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: Bug 1239708
User impact if declined: Bug 1239708 (new autofill algorithm) landed in 62, and this bug landed in 64. So if it's not uplifted to 63, users will have to wait another cycle before they see this bug fixed.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: No
Needs manual test from QE?: Yes
If yes, steps to reproduce: Please see comment 17
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): The patch makes slight tweaks to two SQL queries used in autofill. This code has automated tests, and I've verified manually. Most of this patch is test additions/changes.
String changes made/needed: None
Attachment #9015100 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•6 years ago
|
Summary: Improve the auto-fill process of the address bar, considering bookmarked pages → Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold
Comment 19•6 years ago
|
||
I've just installed Nightly to test it out and I can confirm that autofill works once again, great! Looking forward to the backport to 63 beta.
Comment 20•6 years ago
|
||
I've successfully reproduced this bug on Firefox 63.0a9 and I can Confirm Autofill is working back again on Firefox 64.0a1.
Comment 21•6 years ago
|
||
I have tested as well, it is working back again in Firefox 64.0a1. Thank you!
Comment 22•6 years ago
|
||
Comment on attachment 9015100 [details] [diff] [review]
Beta/63 patch
Follow up fix to bug 1488879 which was a tracked regression for 63 that already got uplifted, fix verified on Nightly, uplift approved for 63 beta 13, thanks.
Attachment #9015100 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 23•6 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 24•6 years ago
|
||
The STR from comment 0 and comment 17 were attempted and the results are promising: This issue is now fixed in Nightly v64.0a1 from 2018-10-08.
Awaiting Beta build for uplift verification.
Thank you!
Status: RESOLVED → VERIFIED
Comment 26•6 years ago
|
||
I've been testing it out properly with beta 13 and I'm sad to report that it's not completely fixed yet.
The autofill works the first time when you haven't visited any pages on the bookmarked page yet.
Once you visit some pages on the bookmarked page (tabs or no tabs doesn't seem to matter) and you then type in the name of the bookmarked page again it's no longer autofilling anything.
Reporter | ||
Comment 27•6 years ago
|
||
The test cases in comment 0 and comment 17 (with both history enabled and not) are passed (as expected) in Firefox Beta v63.0b13, on my test machine with Windows 10 x64.
@maarnuwels: I could not reproduce your case and I've tried this STR:
1. Clear browser history and set browser not to "remember my history";
2. Save two bookmarks of different domains, https://www.mozilla.org/ and https://www.youtube.com/;
3. Save another bookmark, https://www.youtube.com/user/Mozillam;
4. Navigate to several different child pages of the https://www.mozilla.org/ and https://www.youtube.com/ domains;
5. Type "moz" in the address bar. -> The URL is auto-filled as mozilla.org as expected.
6. Type "you" in the address bar. -> The URL is auto-filled as youtube.com as expected.
Are my steps different from yours? If yes, please write detailed steps to reproduce with actual and expected results that will show the issue you are experiencing.
Thank you for your contribution!
Flags: needinfo?(maarnuwels)
Comment 28•6 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1493636#c26
I have many bookmarks set.
As an example, I'm using http://tweakers.net (which I also have as a homepage).
After manually clearing history, if I enter tweak, it autofills OK.
I then visit the homepage, i.e., just open the bookmark in same tab.
Then I try to autofill tweakers again (with 'tweak' again), and tweakers.net isn't autofilled anymore.
Actually, while I'm writing this, I think I found the problem.
Most of my bookmarks are still http:// (been a Firefox user since 1.0!), but most sites nowadays use https:// of course and redirect to it.
It seems that all bookmarks set to http:// will break autofill after the page has been loaded once. If the bookmark is https://, it seems to work OK.
I think this issue needs to be addressed. Not on my end as it used to work fine in previous Firefox version (+ migrating all bookmarks to https:// is no option for me, I have about 2500).
Flags: needinfo?(maarnuwels)
Comment 29•6 years ago
|
||
*visit the homepage -> visit the filled bookmark. I can't find out how to edit my previous comment.
Assignee | ||
Comment 30•6 years ago
|
||
Sorry, I can't reproduce that. Once I bookmark http://tweakers.net, tweakers.net always autofills.
We do handle both http and https.
Reporter | ||
Comment 31•6 years ago
|
||
I have tested your presumption that your issue could be related to having bookmarks saved with http instead of https but I still can't reproduce it. Considering that you are performing the tests in your old profile, it may be caused by add-ons or extensions.
Please test if the issue is reproducible in safe mode, here is a link that can help you:
https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
If you still have the issue please create a new profile, you have the steps here: https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles?redirectlocale=en-US&redirectslug=Managing-profiles#w_starting-the-profile-manager
and retest the issue. These tests are necessary to know whether the issue is linked to extensions or profile related settings.
I will have to mark this issue as verified, but let's continue talking on this thread until we will hopefully find the cause.
Thank you!
Flags: qe-verify+ → needinfo?(maarnuwels)
Comment 32•6 years ago
|
||
If it would work in safe mode; how would I then determine what is causing the issue?
Flags: needinfo?(maarnuwels)
Comment 33•6 years ago
|
||
I just disabled places.sqlite and so far so good. Shouldn't that file be reset if you clear history on close?
Comment 34•6 years ago
|
||
It's no longer working again, sadly. I'll investigate this in safe mode too.
Comment 35•6 years ago
|
||
Safe mode isn't working either for me, I'll now see if a new profile is working.
Updated•6 years ago
|
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•