Add autocomplete result type for arbitrary URLs

VERIFIED FIXED in Firefox 35

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

unspecified
mozilla35
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox35 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Part of bug 951624. Typing in an arbitrary (but valid) URL that isn't an exact match of any other autocomplete match is one of the possible actions of typing into the urlbar. Currently that's not exposed anywhere in the UI, so is very opaque as to what's going to actually happen. So we want to add a heuristic and a new result type for when this will happen.
Flags: qe-verify+
Flags: firefox-backlog+
Iteration: --- → 35.2

Updated

5 years ago
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida

Comment 2

5 years ago
I don't think the dummy favicon with dotted borders is the most indicative iconography here to show that the browser will be redirected to a website, but I haven't found any mockups with a better favicon.
(In reply to Guillaume C. [:ge3k0s] from comment #2)
> I don't think the dummy favicon with dotted borders is the most indicative
> iconography here to show that the browser will be redirected to a website,
> but I haven't found any mockups with a better favicon.

We do try to show a favicon with this patch. But we're limited by bug 1045283.

Comment 4

5 years ago
Maybe we could have a better fallback icon : i.e. a sheet of paper (à la Chrome)
(In reply to Guillaume C. [:ge3k0s] from comment #4)
> Maybe we could have a better fallback icon : i.e. a sheet of paper (à la
> Chrome)

it's worth to evaluate that, in a separate bug though. I suspect this won't be the only favicon issue we'll have here...
Comment on attachment 8491416 [details] [diff] [review]
Patch v1

Review of attachment 8491416 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/PlacesUtils.jsm
@@ +1532,5 @@
> +      aPageUrl = NetUtil.newURI(aPageUrl);
> +
> +    PlacesUtils.favicons.getFaviconURLForPage(aPageUrl, uri => {
> +      if (uri) {
> +        uri = PlacesUtils.favicons.getFaviconLinkForIcon(uri);

this is the usual case where the moz-anno:favicon protocol should be able to:
1. fetch the favicon for a page url, not only for a favicon url
2. if the favicon doesn't exist forward the request to the web

unfortunately it doen't do any of these nowadays :/

::: toolkit/components/places/UnifiedComplete.js
@@ +898,5 @@
> +  // TODO (bug 1054814): Use visited URLs to inform which scheme to use, if the
> +  // scheme isn't specificed.
> +  _matchUnknownUrl: function* () {
> +    // It's possible to get a moz-action URI thanks to a race-condition,
> +    // filter that out here.

I'd be happy if this would explain which is the race condition or point to a bug that explains and tries to solve that.

@@ +943,5 @@
> +      // It's possible we don't have a favicon for this - and that's ok.
> +    };
> +
> +    this._addMatch(match);
> +    this.notifyResults(true);

same question as the other bug, this notifyResults should not be needed... if it is it should be centralized to addMatch

::: toolkit/components/places/tests/unifiedcomplete/test_actions.js
@@ +34,5 @@
> +  yield check_autocomplete({
> +    search: "about:config",
> +    searchParam: "enable-actions",
> +    matches: [ { uri: makeActionURI("visiturl", {url: "about:config", input: "about:config"}), title: "about:config" }, ]
> +  });

I'd vote to split these to a separate test file
Attachment #8491416 - Flags: review?(mak77) → feedback+
Realised I now need to fixup the case where we do URL autofill prediction, but the real URL autofill query doesn't return any results.



(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> this is the usual case where the moz-anno:favicon protocol should be able to:
> 1. fetch the favicon for a page url, not only for a favicon url
> 2. if the favicon doesn't exist forward the request to the web
> 
> unfortunately it doen't do any of these nowadays :/

I know :\ Better than nothing though, right?

> > +    // It's possible to get a moz-action URI thanks to a race-condition,
> > +    // filter that out here.
> 
> I'd be happy if this would explain which is the race condition or point to a
> bug that explains and tries to solve that.

Turns out this is no longer the case! Removed.
Posted patch Patch v2 (obsolete) — Splinter Review
Attachment #8491416 - Attachment is obsolete: true
Attachment #8495209 - Flags: review?(mak77)
Comment on attachment 8495209 [details] [diff] [review]
Patch v2

Review of attachment 8495209 [details] [diff] [review]:
-----------------------------------------------------------------

not yet, sorry.

::: toolkit/components/places/UnifiedComplete.js
@@ +736,5 @@
>      if (!this.pending)
>        return;
>  
>      for (let [query, params] of queries) {
> +      let result = yield conn.executeCached(query, params, this._onResultRow.bind(this));

I'm not sure how this is supposed to work, if a row handler is passed to executeCached the promise is always resolved to null...
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm#632

so basically here result is always null and we always call this._recoverFromUrlPrediction.

@@ +894,5 @@
> +  _matchUnknownUrl: function* () {
> +    // It's possible to get a moz-action URI thanks to a race-condition,
> +    // filter that out here.
> +    /*if (this._originalSearchString.startsWith("moz-action:"))
> +      return false;*/

if it can't happen anymore, why is both the code and the comment here yet?

@@ +939,5 @@
> +
> +    this._addMatch(match);
> +    return true;
> +  },
> +  

trailing space

@@ +943,5 @@
> +  
> +  _recoverFromURLPrediction: function* () {
> +    // We try to predict whether the URL autofill query is likely to return
> +    // a result. If it doesn't, then we at least know that:
> +    // (1) We dop't have any other special first result

typo: dop't

@@ +946,5 @@
> +    // a result. If it doesn't, then we at least know that:
> +    // (1) We dop't have any other special first result
> +    // (2) The input looks like a URL
> +    // Therefore, we make up for the lack of a result here.
> +    let gotResult = yield this._matchUnknownUrl();

I think this comment should be moved to where we invoke _matchKnownUrl, explaining the whole process.

And it could be worth to refactor all of the code after matchKnownUrl into a generator method and call it in both places.
It's indeed currently very unclear that we are repeating the above code here, and that if someone adds more heuristics they should also be added here.

the alternative would be, if matchKnownUrl matches a url (not a host) to always run the heuristics after it and push the first good result to a property (like _recoverHeuristicMatch) and then here just addMatch that.

::: toolkit/components/places/tests/unifiedcomplete/test_visiturl.js
@@ +22,5 @@
> +    search: "about:config",
> +    searchParam: "enable-actions",
> +    matches: [ { uri: makeActionURI("visiturl", {url: "about:config", input: "about:config"}), title: "about:config" }, ]
> +  });
> +  

trailing space
Attachment #8495209 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #10)
> >      for (let [query, params] of queries) {
> > +      let result = yield conn.executeCached(query, params, this._onResultRow.bind(this));
> 
> I'm not sure how this is supposed to work, if a row handler is passed to
> executeCached the promise is always resolved to null...
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm#632
> 
> so basically here result is always null and we always call
> this._recoverFromUrlPrediction.

Well, crap. I miss-read that code. Which means somewhere a test is failing, but isn't.

Er, hm. Only obvious way I can see around that is modifying Sqlite.jsm...

> 
> @@ +894,5 @@
> > +  _matchUnknownUrl: function* () {
> > +    // It's possible to get a moz-action URI thanks to a race-condition,
> > +    // filter that out here.
> > +    /*if (this._originalSearchString.startsWith("moz-action:"))
> > +      return false;*/
> 
> if it can't happen anymore, why is both the code and the comment here yet?

Because I forgot to qrefresh the patch? :)



> 
> @@ +946,5 @@
> > +    // a result. If it doesn't, then we at least know that:
> > +    // (1) We dop't have any other special first result
> > +    // (2) The input looks like a URL
> > +    // Therefore, we make up for the lack of a result here.
> > +    let gotResult = yield this._matchUnknownUrl();
> 
> I think this comment should be moved to where we invoke _matchKnownUrl,
> explaining the whole process.
> 
> And it could be worth to refactor all of the code after matchKnownUrl into a
> generator method and call it in both places.
> It's indeed currently very unclear that we are repeating the above code
> here, and that if someone adds more heuristics they should also be added
> here.

Yes, good point.
(In reply to Blair McBride [:Unfocused] from comment #11)
> Er, hm. Only obvious way I can see around that is modifying Sqlite.jsm...

Split this off into bug 1073358, so it's more isolated - since it has potential to affect areas of code completely unrelated to this bug.
Depends on: 1073358
Posted patch Patch v2.1Splinter Review
This relies on the little patch in bug 1073358.
Attachment #8495209 - Attachment is obsolete: true
Attachment #8495774 - Flags: review?(mak77)
Comment on attachment 8495774 [details] [diff] [review]
Patch v2.1

Review of attachment 8495774 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/urlbarBindings.xml
@@ +1025,5 @@
>                // TODO (bug 1054816): Centralise the implementation of actions
>                // into a JS module.
>                switch (action.type) {
>                  case "switchtab": //Fall through.
> +                case "keyword": // Fall through.

please while here fix the missing whitespace in the comment on the previous line

::: toolkit/components/places/UnifiedComplete.js
@@ +721,5 @@
>        // It may also look like a URL we know from the database.
> +      // We try to predict whether the URL autofill query is likely to return
> +      // a result. If it doesn't, then we at least know that:
> +      // (1) We don't have any other special first result
> +      // (2) The input looks like a URL

I'm not sure about the latter, remember we only try to match TYPED urls, so it might not look like a url. we might just not have any result

@@ +722,5 @@
> +      // We try to predict whether the URL autofill query is likely to return
> +      // a result. If it doesn't, then we at least know that:
> +      // (1) We don't have any other special first result
> +      // (2) The input looks like a URL
> +      // Therefore, we may need to make up for the lack of a result later on.

Globally this comment is very unclear. I'd probably replace it with:

// It may also look like a URL we know from the database.
// Here we can only try to predict whether the URL autofill query is likely to
// return a result.  If the prediction ends up being wrong, later we will need
// to make up for the lack of a special first result.

@@ +729,5 @@
>  
>      if (this.pending && this._enableActions && !hasFirstResult) {
> +      // If we don't have a result that matches what we know about, then
> +      // we use a fallback for things we don't know about.
> +      yield this._matchHeristicFallback();

typo: Heristic (copy/pasted all around).

@@ +734,3 @@
>      }
>  
>      yield this._sleep(Prefs.delay);

please add a comment above the _sleep point:

// No other first result heuristics should run after _matchHeuristicFallback().

@@ +737,5 @@
>      if (!this.pending)
>        return;
>  
>      for (let [query, params] of queries) {
> +      let result = yield conn.executeCached(query, params, this._onResultRow.bind(this));

based on how the Sqlite patch will change, I'd call this hasResult

@@ +739,5 @@
>  
>      for (let [query, params] of queries) {
> +      let result = yield conn.executeCached(query, params, this._onResultRow.bind(this));
> +
> +      if (params.query_type == QUERYTYPE_AUTOFILL_URL && !result) {

check this.pending? since we yielded it might make sense

@@ +894,5 @@
>    },
>  
> +  // These are separated out so we can run them in two distinct cases:
> +  // (1) We didn't match on anything that we know about
> +  // (2) Our predictive query for URL autofill thought we may get a result, 

trailing space
Attachment #8495774 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/8e4b92b32b15
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Testing done on Nightly 35.0a1 (2014-09-28) using Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 LTS 32-bit.

Acceptance criteria for this patch:
- A 'Visit <url>' entry is displayed in the suggestions pane for arbitrary URLs that don't have any other matches.
- The 'Visit <url>' entry includes the display of the standard (dotted) favicon.

Potential issues:
1. [All platforms] The 'Visit <url>' entry is not displayed for an arbitrary URL typed in a private window.

I'm not sure if Issue#1 is an actual bug or intended behavior, Blair - what do you think?
Flags: needinfo?(bmcbride)
(In reply to Andrei Vaida, QA [:avaida] from comment #17)
> 1. [All platforms] The 'Visit <url>' entry is not displayed for an arbitrary
> URL typed in a private window.
> 
> I'm not sure if Issue#1 is an actual bug or intended behavior, Blair - what
> do you think?

Yea, that's a bug... that's private windows abusing an API. Filed bug 1075450.
Flags: needinfo?(bmcbride)
Thank you for confirming, Blair. Since this issue will be treated in a separate bug, I think it's safe to mark this one as verified. Testing details for this patch are available in Comment 17.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.