Closed Bug 1065303 Opened 10 years ago Closed 10 years ago

Prepare autocomplete.xml/UnifiedComplete for adding new special result types and heuristics

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal
Points:
5

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla35
Iteration:
35.2

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch v2
25.55 KB, patch
mak
: review+
Details | Diff | Splinter Review 
This is basically prep work for the different new items to be added for bug 951624. Which is two main areas:
* Re-organising parts of UnifiedComplete/autocomplete.xml so it's better able to handle new result types and heuristics
* Modify how we handle the autofill result types
Flags: qe-verify+
Flags: firefox-backlog+
Marco: Guess what else I split off from bug 951624?!
Flags: needinfo?(mmucci)
Attached patch Patch v1 (obsolete) — — Splinter Review 
Note: This is missing test coverage, but then so is everything else. I plan to add that in bug 1065301.

Also, the documentation added here is intentionally pre-emptive, to avoid having to rewrite it for every new item/heuristic that I already have patches for.
Attachment #8487089 - Flags: review?(paolo.mozmail)
Added to IT 35.1

(In reply to Blair McBride [:Unfocused] from comment #1)
> Marco: Guess what else I split off from bug 951624?!
Iteration: --- → 35.1
Flags: needinfo?(mmucci)
Comment on attachment 8487089 [details] [diff] [review]
Patch v1

Stealing review, since I have some time left.
Attachment #8487089 - Flags: review?(paolo.mozmail) → review?(mak77)
Comment on attachment 8487089 [details] [diff] [review]
Patch v1

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

::: toolkit/components/places/UnifiedComplete.js
@@ +666,5 @@
>      // wait for the initialization of PlacesSearchAutocompleteProvider first.
>      yield PlacesSearchAutocompleteProvider.ensureInitialized();
>  
> +    // For any given search, we run many queries/heuristics:
> +    // 1) search engine by alias

"by alias (as defined in SearchService)"

@@ +667,5 @@
>      yield PlacesSearchAutocompleteProvider.ensureInitialized();
>  
> +    // For any given search, we run many queries/heuristics:
> +    // 1) search engine by alias
> +    // 2) inline completion from search engine domains

s/domains/resultDomains/ (that is how we defined them in SearchService)

@@ +668,5 @@
>  
> +    // For any given search, we run many queries/heuristics:
> +    // 1) search engine by alias
> +    // 2) inline completion from search engine domains
> +    // 3) inline completion from database (this._hostQuery or this._urlQuery)

I'd avoid "from database" since most queries are, and rather
3) inline completion for hosts (this._hostQuery) or urls (this._urlQuery)

@@ +669,5 @@
> +    // For any given search, we run many queries/heuristics:
> +    // 1) search engine by alias
> +    // 2) inline completion from search engine domains
> +    // 3) inline completion from database (this._hostQuery or this._urlQuery)
> +    // 4) plain url (ie, can be navigated to as-is)

"user typed plain url" or "directly typed-in url" maybe?

@@ +670,5 @@
> +    // 1) search engine by alias
> +    // 2) inline completion from search engine domains
> +    // 3) inline completion from database (this._hostQuery or this._urlQuery)
> +    // 4) plain url (ie, can be navigated to as-is)
> +    // 5) current search engine

"submission for the current search engine" maybe?

@@ +679,4 @@
>      //
> +    // (6) only gets ran if we get any filtered tokens, since if there are no
> +    // tokens, there is nothing to match. This is the *first* query we check if
> +    // we want to run, but it gets queued to be be run later.

typo: to be be

@@ +694,5 @@
>                      this._switchToTabQuery,
>                      this._searchQuery ];
>  
> +    // When actions are enabled, we run a series of heuristics to determine what
> +    // the first first result should be - which is always a special result.

first first

@@ +697,5 @@
> +    // When actions are enabled, we run a series of heuristics to determine what
> +    // the first first result should be - which is always a special result.
> +    // |hasFirstResult| is used to keep track of whether we've obtained such a
> +    // result yet, so we can skip further heuristics and not add an additional
> +    // special results.

s/results/result/

@@ +708,5 @@
>      }
>  
> +    let shouldAutofill = this._shouldAutofill;
> +    if (this.pending && !hasFirstResult && shouldAutofill) {
> +      // Or it may look like a URL we know about from search engines.

The "Or" seems to point to a previous comment but can't find it.

@@ +715,4 @@
>  
> +    if (this.pending && !hasFirstResult && shouldAutofill) {
> +      // It may also look like a URL we know from the database.
> +      hasFirstResult = yield this._matchKnownUrl(conn);

I feel like matchKnownUrl is a little bit generic, also the following urls are known and matching...

Btw, there is a worst problem here, the current code is running host query immediately, while url query is unshifted into queries. There is a reason for that, the host query can use an index and it's quite fast, the url query cannot (it must scan table), thus it was delayed by browser.urlbar.delay, like any other queries.

This means this change will cause urlbar slowdowns unfortunately.
You should either restore the code so that it will delay the url query, or rewrite the query to use an index.
Unfortunately the only way I might think to do the latter would be to write a fancy query like:
url BETWEEN "http://" || token AND "http://" || token || X'FFFF' OR
url BETWEEN "http://www." || token AND "http://www." || token || X'FFFF' OR
url BETWEEN "https://" || token AND "https://" || token || || X'FFFF' OR
url BETWEEN "https://www" || token AND "https://www" || token || || X'FFFF' OR
url BETWEEN "ftp://" || token AND "ftp://" || token || || X'FFFF'

This should somehow work, but the fact it won't do a case-insensitive comparison of the path... but maybe we don't care too much...

@@ +749,5 @@
>      }
>    }),
>  
> +  _matchKnownUrl: function* (conn) {
> +    let autofillQuery = null;

fwiw, this should only happen if (this._searchTokens.length == 1)... maybe this check should be put into _shouldAutofill...

@@ +780,5 @@
>      if (!Prefs.autofillSearchEngines)
> +      return false;
> +
> +    if (!this._searchTokens.length == 1)
> +      return false;

...and removed from here

@@ +787,5 @@
>                                                             this._searchString);
> +    if (!match)
> +      return false;
> +
> +    this._result.setDefaultIndex(0);

warning: bug 1059846 will bitrot this

@@ +925,5 @@
>      let match = {};
>      let trimmedHost = row.getResultByIndex(QUERYINDEX_URL);
>      let untrimmedHost = row.getResultByIndex(QUERYINDEX_TITLE);
>      let frecency = row.getResultByIndex(QUERYINDEX_FRECENCY);
> +    let iconurl = row.getResultByIndex(QUERYINDEX_ICONURL) || "";

we don't fetch the icon for hosts, it will always be null... we could try building a fake moz-anno:favicon:host/favicon.ico and hope to have it... but I don't see the point of fetching ICONURL here so far.

::: toolkit/content/widgets/autocomplete.xml
@@ +1469,5 @@
> +        <parameter name="aSourceString"/>
> +        <parameter name="aReplacements"/>
> +        <body>
> +          <![CDATA[
> +            let parts = aSourceString.split(/(#[0-9]+)/);

this method clearly needs documentation and some inline comments.

I have no idea what it's doing, what's the expected input and output... it's like a black box so far.

@@ +1551,5 @@
> +          if (types.has("autofill")) {
> +            emphasiseUrl = false;
> +
> +            let sourceStr = this._stringBundle.GetStringFromName("visitURL");
> +            title = this._generateEmphasisPairs(sourceStr, [

how is this replacing the %S in visitURL with the url?
Attachment #8487089 - Flags: review?(mak77) → feedback+
QA Contact: andrei.vaida
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #5)
> Btw, there is a worst problem here, the current code is running host query
> immediately, while url query is unshifted into queries. There is a reason
> for that, the host query can use an index and it's quite fast, the url query
> cannot (it must scan table), thus it was delayed by browser.urlbar.delay,
> like any other queries.


Hmm, I think we may be able to just fake it instead. I had to convert that to an immediate query because we need to know if we have a result or not. But if the host portion of the input gets a match from a *host* query, then (I think) it's safe to assume that it will eventually match a url query. And if it doesn't, then it's still a valid URL. As I said, the important part is thinking we have a result - not actually *getting* the result. I need to play around with it some more.
Iteration: 35.1 → 35.2
Attached patch Patch v2 — — Splinter Review 
Crazy idea in comment 6 actual works.
Attachment #8487089 - Attachment is obsolete: true
Attachment #8491347 - Flags: review?(mak77)
Comment on attachment 8491347 [details] [diff] [review]
Patch v2

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

::: toolkit/components/places/UnifiedComplete.js
@@ +810,5 @@
> +    //  * If the user typed "www." but the final url doesn't have it, we
> +    //    should not match as well, the two urls may point to different pages.
> +    if (this._strippedPrefix.endsWith("www.") &&
> +        !stripHttpAndTrim(match.url).startsWith("www."))
> +      return;

shouldn't this return false?

@@ +820,5 @@
> +    // are not handling properly yet.
> +    if (!value.startsWith(this._originalSearchString)) {
> +      Components.utils.reportError(`Trying to inline complete in-the-middle
> +                                    ${this._originalSearchString} to ${value}`);
> +      return;

as well as here

@@ +831,5 @@
> +      icon: match.iconUrl,
> +      style: "priority-search",
> +      finalCompleteValue: match.url,
> +      frecency: FRECENCY_SEARCHENGINES_DEFAULT
> +    });

and here should return true

@@ +961,5 @@
>      let match = {};
>      let trimmedHost = row.getResultByIndex(QUERYINDEX_URL);
>      let untrimmedHost = row.getResultByIndex(QUERYINDEX_TITLE);
>      let frecency = row.getResultByIndex(QUERYINDEX_FRECENCY);
> +    let iconurl = row.getResultByIndex(QUERYINDEX_ICONURL) || "";

did you forget to remove this? it looks unused now.

@@ +1283,5 @@
> +   * result. We do by extracting what should be a host out of the input and
> +   * performing a host query based on that.
> +   */
> +  get _urlPredictQuery() {
> +    // We expect thias to be a full URL, not just a host. We want to extract the

typo: thias

@@ +1284,5 @@
> +   * performing a host query based on that.
> +   */
> +  get _urlPredictQuery() {
> +    // We expect thias to be a full URL, not just a host. We want to extract the
> +    // host and use that as a guess for whewther we'll get a result from a URL

typo: whewther

::: toolkit/content/widgets/autocomplete.xml
@@ +1478,5 @@
> +            //   "textA %S"
> +            //     becomes ["textA ", "%S"]
> +            //   "textA %1$S textB textC %2$S"
> +            //     becomes ["textA ", "%1$S", " textB textC ", "%2$S"]
> +            let parts = aSourceString.split(/(%S|%[0-9]+\$S)/);

can avoid the or: /(%(?:[0-9]+\$)?S)/

@@ +1490,5 @@
> +
> +              // Determine if this token is a replacement token or a normal text
> +              // token. If it is a replacement token, we want to extract the
> +              // numerical number. However, we still want to match on "$S".
> +              let match = part.match(/^%(?:([0-9]+)\$S|S)$/);

this should work too /^%(?:([0-9]+)\$)?S$/)

@@ +1516,5 @@
> +      <!--
> +        _setUpEmphasisedSections() has the same use as _setUpDescription,
> +        except instead of taking a string and highlighting given tokens, it takes
> +        an array of pairs generated by _generateEmphasisPairs(). This allows
> +        control over emphasing based on specific blocks of text, rather than

typo: emphasing
Attachment #8491347 - Flags: review?(mak77) → review+
And backed out due to bc1 bustage (that didn't show on the Try run):
https://hg.mozilla.org/integration/fx-team/rev/e55ff6fab899
Relanded with a test fix:
https://hg.mozilla.org/integration/fx-team/rev/86a707d5ffdb

browser_urlbarAutoFillTrimURLs.js was failing because we now only add one autofill URL, which in that case became the search engine domain for amazon.
https://hg.mozilla.org/mozilla-central/rev/86a707d5ffdb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blair, is there something in particular that manual QA should look at here? Or perhaps this can be covered by an overall smoke test? Please let me know.
Flags: needinfo?(bmcbride)
Depends on: 1071481
(In reply to Andrei Vaida, QA [:avaida] from comment #15)
> Blair, is there something in particular that manual QA should look at here?
> Or perhaps this can be covered by an overall smoke test? Please let me know.

Msot of this is backend stuff that shouldn't be noticable when using urlbar autocomplete - so just a general smoketest for that area would be good.

However, there is one visible change: When typing in something that looks like a URL, you should see a new result (the first result), what should be "Visit INPUT", with your input emphasised (using the same emphasis style we use for freeform searching, where we highlight words in match titles and URLs).
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] from comment #16)
> Msot of this is backend stuff that shouldn't be noticable when using urlbar
> autocomplete - so just a general smoketest for that area would be good.
> 
> However, there is one visible change: When typing in something that looks
> like a URL, you should see a new result (the first result), what should be
> "Visit INPUT", with your input emphasised (using the same emphasis style we
> use for freeform searching, where we highlight words in match titles and
> URLs).

Testing performed on Nightly 35.0a1 (2014-09-24), using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Acceptance criteria for this patch:
- Autocomplete passes manual smoketests.
- The suggestions panel now also displays the first entry as "Visit <input>", if a URL match is found for <input>.

Potential issues:
- [Mac, Ubuntu] The suggestions pane displays the 2nd entry partially if only two matches are found.
  * screenshot: http://i.imgur.com/76gZQAI.png
- [Mac, Ubuntu] The suggestions pane displays a scrollbar if most of an URL match is already typed inside the awesomebar.
  * screenshot: http://i.imgur.com/D6uKO4w.png

I suspect these are not related to this specific fix. Blair, what do you think?
(In reply to Andrei Vaida, QA [:avaida] from comment #17)
> I suspect these are not related to this specific fix. Blair, what do you
> think?

Yea, agreed. And I think they're the same bug of the panel not resizing correctly. New bug?
(In reply to Blair McBride [:Unfocused] from comment #18)
> (In reply to Andrei Vaida, QA [:avaida] from comment #17)
> > I suspect these are not related to this specific fix. Blair, what do you
> > think?
> 
> Yea, agreed. And I think they're the same bug of the panel not resizing
> correctly. New bug?

Thank you for confirming, I'll file a separate bug for it. Marking this one as verified.
Status: RESOLVED → VERIFIED
Depends on: 1073243
(In reply to Blair McBride [:Unfocused] from comment #18)
> (In reply to Andrei Vaida, QA [:avaida] from comment #17)
> > I suspect these are not related to this specific fix. Blair, what do you
> > think?
> 
> Yea, agreed. And I think they're the same bug of the panel not resizing
> correctly. New bug?

Nope, those issues are bug 1073243 and are a regression of this bug, likely the margins are breaking rlb vertical rhythm
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: