46 bytes, text/x-phabricator-request
|Details | Review|
4.77 KB, text/plain
6.26 KB, text/plain
4.26 KB, text/plain
5.97 KB, patch
|Details | Diff | Splinter Review|
We broke this in bug 1467627. Before that bug, the autofill frecency threshold was the max frecency of all URLs with a given origin -- i.e., it was the frecency of a single URL, albeit the one with the max frecency. So it wasn't invalid to compare moz_places in the URL query to this threshold. But that's not true anymore, and we should have fixed the URL query (somehow) as part of that bug. I think what we should do is just drop the threshold for the URL query. The threshold will still apply as the user is typing the origin. But after that, we should just autofill the matching URL with the highest frecency as the user continues typing. Especially because in that case there's no danger of autofilling something the user doesn't want autofilled. This will be a simple patch we can easily uplift. Marco, do you agree?
Yeah, I had a few doubts about that, but didn't investigate. Your analysis and solution LGTM.
Attaching a couple of files that I'll discuss over in phabricator.
Posting some more data that I'll discuss in phabricator
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/94bc301b5bd4 Ignore the autofill threshold when autofilling URLs r=mak
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
STR: 1. New profile 2. Visit https://www.nytimes.com/ 3. Visit https://www.nytimes.com/section/world 4. Visit http://example.com/ 10 times. All you need to do is copy and paste the URL into the current tab, hit enter, and then 9 more times press Ctrl+L (or Command-L on a Mac) and enter. 5. In the urlbar, type "ny". It should be autofilled to "nytimes.com/" 6. Press the right arrow key so that the caret moves to the end of the text. Continue typing "se". It should be autofilled to "section/", and the entire text in the urlbar should be "nytimes.com/section/" 7. Press the right arrow key so that the caret moves to the end of the text. Continue typing "wo". It should be autofilled to "world/", and the entire text in the urlbar should be "nytimes.com/section/world/"
[Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1239708 User impact if declined: The new autofill algorithm (bug 1239708) landed in 62, and this bug landed in 64, so if declined, users will need to wait another cycle for this fix. We've already fixed and uplifted at least two other autofill bugs recently. 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 10 List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): Small patch that only slightly tweaks a SQL query used in autofill. Has test, and I've manually verified String changes made/needed: None
Attachment #9016779 - Flags: approval-mozilla-beta?
status-firefox62: --- → unaffected
status-firefox63: --- → affected
status-firefox-esr60: --- → unaffected
I have reproduced this issue using Firefox 64.0a1 (2018.09.26) on Windows 10 x64. I can confirm this issue is fixed, I verified using Firefox 64.0a1 (latest nightly) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.13.6.
status-firefox64: fixed → verified
Comment on attachment 9016779 [details] [diff] [review] Beta/63 patch P1 regression, mlinimal patch that seemss afe, I am approving for 63RC1 as we are already past our beta cycle.
Attachment #9016779 - Flags: approval-mozilla-beta? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-release/rev/0a9b0f637ecb https://hg.mozilla.org/releases/mozilla-beta/rev/33d3fba56989f5782602063b40c35dd3742c0e59 (FIREFOX_63b_RELBRANCH)
status-firefox63: affected → fixed
I verified this issue using build 63.0 (build ID = 20181015152800; https://archive.mozilla.org/pub/firefox/candidates/63.0-candidates/build1/win64/en-US/) on Windows 10 x64 and Windows 7 x64, the issue is still reproducible, at step 5 from Comment 10, typing "ny" in URL bar not working the autofill to "nytimes.com/".
Sorry, Nightly and beta/release have different initial bookmarks and start pages, so the frecency calculations are slightly different. In the STR, in step 4, please visit example.com only 3 times total. The other steps are the same. Also, step 7 has a typo: "world" shouldn't actually have a "/" at the end.
I can confirm this issue is fixed, I verified using 63.0RC build on Windows 10 x64, Ubuntu 16.04 x64 and on Mac OS X 10.13.6, based on Comment 16 I mark this bug verified fixed on Fx63.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
You need to log in before you can comment on or make changes to this bug.