1521702 - Hook up autofill with URL canonization

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P2)

Description

[Dão Gottwald [:dao]]

See https://searchfox.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1556-1558

Comment 1

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #0)

See https://searchfox.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1556-1558

permalink: https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/browser/base/content/urlbarBindings.xml#1556-1558

Comment 2

[Drew Willcoxon :adw]

Attachment: Bug 1521702 - Hook up autofill with URL canonization.

• Try to canonize all result types that aren't otherwise handled in the case statement. If canonization fails, then just fall back to the URL previously returned by UrlbarUtils.getUrlFromResult(). (Maybe we should only be doing this for URL results instead of all other results?)

• When the first result is picked and the input autofilled a value, use the last search string as the value to canonize.

• For search results, use the autofill value to canonize if there is one.

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1521702 - Hook up autofill with URL canonization.

This is based on https://phabricator.services.mozilla.com/D18618

I'll request review after that's reviewed.

Comment 4

[Marco Bonardo [:mak]]

I just hit a problem with canonization due to the behavior in handleCommand, in particular:

// Use the selected result if we have one; this should always be the case
// when the view is open.
let result = this.view.selectedResult;
if (result) {
  this.pickResult(event, result);
  return;
}

Well, this is not the case, canonization can transform "example/foo" in "www.example.com/foo", it doesn't only affect autofill.
Either we expand the reach of this added code to also handle other results, or we invert the logic and check canonization first, before pickResult.

Comment 5

[Marco Bonardo [:mak]]

note browser_canonizeUrl doesn't work correctly ATM, because we don't have the EventBufferer... I found that problem after implementing it (I have it running locally, but failing a couple tests due to actual bugs/misses)

Comment 6

[Marco Bonardo [:mak]]

we could make pickResult a pass-through... Something like:

let where = openWhere || this._whereToOpen(event);
...
let value = this.pickResult(event, result, where, openParams);
if (!value) {
    // pickResult has handled the load.
    return;
}

let url = this._maybeCanonizeURL ...

I think this would look and work more similarly to the old urlbar.

Comment 7

[Drew Willcoxon :adw]

You're right about the problem. We only check canonization for search results right now, and then the patch adds autofill results. But it could apply to any type of result.

One thing that I don't like about the legacy implementation is that it checks event.ctrlKey in order to determine whether canonization should maybe happen, and then canonization checks event.ctrlKey again. It would be nice to have the canonization logic in one place and not rely on a "pre-check" like that.

So it seems like your solution of checking canonization first is a good idea. If pickResult is a pass through, then it would need to determine whether canonization should happen, only to call _maybeCanonizeURL later after pickResult returns, which would be splitting the logic in two places again.

Comment 8

[Marco Bonardo [:mak]]

Sure, it's just matter of deciding which architecture to follow. pickResult is also used by the mouse picks, so if it'd become pass-through we'd need a third method to actually handle the load (so handleCommand for keys, pickResult for mouse, prepareLoad for both?). If we move canonization first we have to pay some attention in pickResult to not override values set by canonization.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec61d092ed67
Hook up autofill with URL canonization. r=dao

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ec61d092ed67