1341081 - Pre-seeded autofill entries are not de-duped properly

Status: RESOLVED WORKSFORME

(Firefox :: Address Bar, defect, P3)

Description

[Marco Bonardo [:mak]]

After bug 1322747 autofill matches are de-duped though the comment field, prefilled pages are setting a title there rather than the final url.

in  _matchPrefillSiteForAutofill() {

comment: site.title, => should be comment: stripHttpAndTrim(site.uri.spec)

Comment 1

[Svetlana Orlik]

Attachment: Bug 1341081 - put final URL to comment field of autofill entry,

Review commit: https://reviewboard.mozilla.org/r/113914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113914/

Comment 2

[Svetlana Orlik]

Comment on attachment 8839216 [details]
Bug 1341081 - put final URL to comment field of autofill entry,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113914/diff/1-2/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8839216 [details]
Bug 1341081 - put final URL to comment field of autofill entry,

https://reviewboard.mozilla.org/r/113914/#review115440

LGTM!

Comment 4

[:Gijs (he/him)]

Can we write a test that this works correctly?

Comment 5

[Marco Bonardo [:mak]]

note: in bug 1339497 I'm removing the need to supply a comment for autofill entries, _addMatch will make one.
You can still proceed with this, just that, if this ends up landing after bug 1339497, you should only push the test. If it lands before, no worries, I'll just unbitrot my patch.

Comment 6

[Svetlana Orlik]

What's to be tested here, de-duping?

Comment 7

[Svetlana Orlik]

In _matchPrefillSiteForAutofill() I return as soon as first match is found.
Seems like I made a mistake by this, or rather misunderstood the conception.

I see the behaviour and look through _matchKnownUrl() code again now.
Do I get it right now, that the whole bunch of matches should be added here?
For example if user typed "y", we got to _addMatch Yahoo and Youtube?

Comment 8

[Marco Bonardo [:mak]]

autofill only supports one entry. so what you are doing is correct.
Basically we run _matchFirstHeuristicResult() first, this calls various handlers like _matchPrefillSiteForAutofill and returns a single entry that is used to populate the "action row" (the first row of the awesomebar).
Then the other handlers are invoked and they populate the rest of the awesomebar, so they can return multiple entries too.

The de-duping here is between the heuristic result returned by _matchPrefillSiteForAutofill and a following result returned by these following handlers.
In your case for example, you could add a "youtube.com" result in _matchPrefillSiteForAutofill and another "Youtube.com" result in _matchPrefillSites, the latter should not be added since there's already the first one added, but as the code is this deduping is not happening because autofill entries generate a dedupe key out of .comment rather than .value.

Comment 9

[Svetlana Orlik]

Then please help me understand the example:

I've visited "gijsk.com" and "www.gijsk.com".
In location bar I type "g" and have an autofill "gijsk.com/".

Below I have list:
gijsk.com/ - Visit
Gijs>Home - https://www.gijsk.com
www.gijsk.com - www.gijsk.com
Gijs>Home - https://gijsk.com
other unrelated entries

What is the list entry "www.gijsk.com - www.gijsk.com"?

Comment 10

[Svetlana Orlik]

I mean I thought it's the second autofill entry after the first "gijsk.com".

Comment 11

[Marco Bonardo [:mak]]

I'm not sure I understand your question, there can only be ONE autofill entry, since there is only one input field to autofill.

(In reply to Svetlana Orlik from comment #9)
> gijsk.com/ - Visit

this is the autofill entry, any other entry below it is a normal history entry

> Gijs>Home - https://www.gijsk.com

this is not deduped because it's https://www

> www.gijsk.com - www.gijsk.com

This looks like a normal history entry without a title. It's not deduped because it has www. I don't know what is generating this, but it's not autofill.

> Gijs>Home - https://gijsk.com

another normal entry, not deduped because it has https:// but not www

Comment 12

[Svetlana Orlik]

Sorry, I'm not so good in English.
And thanks for clarifying about autofill.

My question was exactly about entries like "www.gijsk.com - www.gijsk.com".
They appear after such url, in this case "www.gijsk.com", was _typed_ and visited. And they are obviously de-duped with autofill entry (that's why I firs assumed they were generated by autofill).

So it's probably not the topic of this bug, but are there any suggestions who they are and where they're from?

Comment 13

[Marco Bonardo [:mak]]

(In reply to Svetlana Orlik from comment #12)
> So it's probably not the topic of this bug, but are there any suggestions
> who they are and where they're from?

Unless they have some special icon before the favicon, they are just history entries.
Fwiw I tried to visit the pages in comment 9 and I don't see that entry without a title... Maybe it was a temporary redirect, no idea.

Also, in case you are in lack of time and didn't begin working on a test, at this point I'd also be fine marking this as a WFM. Since bug 1339497 is landing and is moving the code to a common location, the existing duping tests are covering that code. There won't be a test specific for preseeded pages, but should matter less if all the code paths are unified.

Comment 14

[Svetlana Orlik]

(In reply to Marco Bonardo [::mak] from comment #13)
> Fwiw I tried to visit the pages in comment 9 and I don't see that entry
> without a title... Maybe it was a temporary redirect, no idea.
Did you type it into location bar, or just clicked the link?
It needs to be typed to make that entry happen.

> Also, in case you are in lack of time and didn't begin working on a test, at
> this point I'd also be fine marking this as a WFM. Since bug 1339497 is
> landing and is moving the code to a common location, the existing duping
> tests are covering that code. There won't be a test specific for preseeded
> pages, but should matter less if all the code paths are unified.
I didn't begin working on test yet. Is WFM - "works for me", no test needed?

Comment 15

[Marco Bonardo [:mak]]

yeah, WORKSFORME and no test strictly needed, if you didn't spend time on it yet. Not worth it.
It will be fixed by bug 1339497 and tested by existing duping bugs.

(In reply to Svetlana Orlik from comment #14)
> (In reply to Marco Bonardo [::mak] from comment #13)
> > Fwiw I tried to visit the pages in comment 9 and I don't see that entry
> > without a title... Maybe it was a temporary redirect, no idea.
> Did you type it into location bar, or just clicked the link?
> It needs to be typed to make that entry happen.

I typed it but looks like I had some more frecent result. In a new profile I see it, but it's not autofill, indeed I get it even setting browser.urlbar.autoFill to false.
I think it's a redirect (non-secure http is redirecting to https). Indeed in the moz_places table I see an entry for http://www.gijsk.com/ with a NULL title.

It looks odd in the UI though, I will file a bug to not provide both a url and a title if the page has no title. Thank you for noticing that.

Comment 16

[Marco Bonardo [:mak]]

Filed Bug 1342132 for the UI ugliness.