Ensure we can enable Unified Autocomplete in Nightly

RESOLVED FIXED in mozilla33

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

Trunk
mozilla33
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
After converting tests and getting a review pass, we should be able to enable the component in Nightly (by flipping a pref)
(Assignee)

Updated

3 years ago
Depends on: 995094
Flags: firefox-backlog+
(Assignee)

Comment 1

3 years ago
Here we should also ensure that once enabled it won't break mochitest-browser tests, especially switch-to-tab ones may depend on results sorting.

Updated

3 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 33.2
Points: --- → 5
QA Whiteboard: [qa+]
(Assignee)

Comment 2

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=048cf2c45fbf
(Assignee)

Comment 3

3 years ago
Created attachment 8447150 [details] [diff] [review]
patch v1

Tests are passing, this also fixes a minor glitch in the component with switch to tab styling and adds a small improvement to avoid duplicate uris in results. I added tests for both since we were missing them.

It's easy to disable the component through the pref, so I think the sooner we can enable it, the better. (Clearly we still need the dependencies to land first).
Attachment #8447150 - Flags: review?(ttaubert)
(Assignee)

Comment 4

3 years ago
Created attachment 8447151 [details] [diff] [review]
patch v2

The right one :/
Attachment #8447150 - Attachment is obsolete: true
Attachment #8447150 - Flags: review?(ttaubert)
Attachment #8447151 - Flags: review?(ttaubert)
Comment on attachment 8447151 [details] [diff] [review]
patch v2

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

::: toolkit/components/places/UnifiedComplete.js
@@ +441,5 @@
> + *        The text to modify.
> + * @return the modified spec.
> + */
> +function stripHttpAndTrim(spec)
> +{

Nit: opening brace should be on the same line as "function"

@@ +737,5 @@
>      }
>  
>      // Must check both id and url, cause keywords dinamically modify the url.
>      if ((!match.placeId || !this._usedPlaceIds.has(match.placeId)) &&
> +        !this._usedURLs.has(stripHttpAndTrim(match.value))) {

So you remove stripPrefix() in the bug that fixes the tests and re-add a slightly different version here? Wouldn't it make sense to put that fix there?

@@ +746,5 @@
>        // include the search string, and would be returned multiple times.  Ids
>        // are faster too.
>        if (match.placeId)
>          this._usedPlaceIds.add(match.placeId);
> +      this._usedURLs.add(stripHttpAndTrim(match.value));

You could call stripHttpAndTrim() only once before the if-branch and save that to a variable.

::: toolkit/components/places/tests/unifiedcomplete/test_dupe_urls.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: should be the public domain header.

@@ +12,5 @@
> +  yield check_autocomplete({
> +    search: "moz",
> +    autofilled: "mozilla.org/",
> +    completed:  "mozilla.org/",
> +    matches: [ { uri: NetUtil.newURI("http://mozilla.org/"), title: "mozilla.org/" } ] 

Nit: trailing white space
Attachment #8447151 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 6

3 years ago
(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #5)
> @@ +737,5 @@
> >      }
> >  
> >      // Must check both id and url, cause keywords dinamically modify the url.
> >      if ((!match.placeId || !this._usedPlaceIds.has(match.placeId)) &&
> > +        !this._usedURLs.has(stripHttpAndTrim(match.value))) {
> 
> So you remove stripPrefix() in the bug that fixes the tests and re-add a
> slightly different version here? Wouldn't it make sense to put that fix
> there?

the final result is the same, since the test is here I left it here, this version is better than the original one.

Updated

3 years ago
Iteration: 33.2 → 33.3

Updated

3 years ago
Blocks: 1034381
(Assignee)

Comment 7

3 years ago
I'm going to push everything here but the pref flip. I don't think it's sane to enable this just a few days before the merge, I'll push the pref flip just after we merge. So, I'm renaming this and filing a new bug just for the pref flip.
Summary: Enable Unified Autocomplete in Nightly → Ensure we can enable Unified Autocomplete in Nightly
(Assignee)

Updated

3 years ago
Blocks: 1040335
(Assignee)

Updated

3 years ago
No longer depends on: 995094
(Assignee)

Comment 8

3 years ago
this one becomes QA-, I'm setting QA+ in bug 1040335
QA Whiteboard: [qa+] → [qa-]
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/52769d6fc58a
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/52769d6fc58a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.