1040340 - Use URI specs instead of nsIURIs in unifiedcomplete tests

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P5)

Description

[Marco Bonardo [:mak]]

Basically the tests like test_word_boundary_search.js are doing

  let uri1 = NetUtil.newURI("http://matchme/");
  let uri2 = NetUtil.newURI("http://dontmatchme/");
  let uri3 = NetUtil.newURI("http://title/1");
  ...

  yield promiseAddVisits([ { uri: uri1, title: "title1" },
                           { uri: uri2, title: "title1" },
                           { uri: uri3, title: "matchme2" },
  ...
  
  addBookmark( { uri: uri5, title: "title1", tags: [ "matchme2" ] } );
  addBookmark( { uri: uri6, title: "title1", tags: [ "dontmatchme3" ] } );
  ...

  matches: [ { uri: uri1, title: "title1" },
             { uri: uri3, title: "matchme2" },

Everything would be easier to follow if these would be plaintext url spec.

To make this possible we need to:
1. make promiseAddVisits accept either nsIURI or url
2. update all promiseAddVisits instances in the tree with that new code
3. make addBookmark accept either nsIURI or url
4. make matches contain spec in the uri property and update check_autocomplete to not use uri.spec but directly uri
5. update all of the tests in places/tests/unifiedcomplete to use plain specs.

It should be easy, but takes a little bit of search and replace.

Comment 1

[Michael Pruett]

Attachment: Patch

I've updated the UnifiedComplete tests to use URI strings rather than nsIURIs where possible.

Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=e3ea1a4cb0e9

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8489146 [details] [diff] [review]
Patch

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

Thank you, this is petty good! I just have  some notes:

::: toolkit/components/places/tests/head_common.js
@@ +909,5 @@
>    let now = Date.now();
>    for (let i = 0; i < places.length; i++) {
> +    if (typeof places[i].uri === 'string') {
> +      places[i].uri = NetUtil.newURI(places[i].uri);
> +    }

could you please file a follow-up bug to update or replace the other promiseAddVisits/addVisits functions in the tree with this one from head_common.js? ( mxr.mozilla.org/mozilla-central/search?string=function+.*addVisits&regexp=on&find=head )

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ +155,5 @@
>            continue;
>  
>          let { uri, title, tags, searchEngine } = matches[j];
> +        if (typeof uri === 'string')
> +          uri = NetUtil.newURI(uri);

Since here we never need the nsIURI, I'd probably do the opposite, so:

let { url, title, tags, searchEngine } = matches[j];
if (url instanceof Ci.nsIURI)
  url = url.spec

and later use url wherever we use uri.spec.

::: toolkit/components/places/tests/unifiedcomplete/test_416211.js
@@ +13,2 @@
>    yield promiseAddVisits({ uri: uri, title: "Page title" });
>    addBookmark({ uri: uri,

could you please remove the temp var and just hardcode the string instead of uri in the test?

::: toolkit/components/places/tests/unifiedcomplete/test_416214.js
@@ +16,5 @@
>  
>  add_task(function* test_tag_match_url() {
>    do_log_info("Make sure tag matches return the right url as well as '+' remain escaped");
> +  let uri1 = "http://escaped/ユニコード";
> +  let uri2 = "http://asciiescaped/blocking-firefox3%2B";

could you please remove the temp vars and just hardcode the strings in the test?

@@ +21,2 @@
>    yield promiseAddVisits([ { uri: uri1, title: "title" },
>    						   { uri: uri2, title: "title" } ]);

should fix indentation

::: toolkit/components/places/tests/unifiedcomplete/test_417798.js
@@ +10,5 @@
>  add_task(function* test_javascript_match() {
>    Services.prefs.setBoolPref("browser.urlbar.autoFill.searchEngines", false);
>  
> +  let uri1 = "http://abc/def";
> +  let uri2 = "javascript:5";

could you please remove the temp vars and just hardcode the strings in the test?

::: toolkit/components/places/tests/unifiedcomplete/test_422277.js
@@ +9,4 @@
>  
>  add_task(function* test_javascript_match() {
>    do_log_info("Bad escaped uri stays escaped");
> +  let uri1 = "http://site/%EAid";

could you please remove the temp vars and just hardcode the strings in the test?

::: toolkit/components/places/tests/unifiedcomplete/test_enabled.js
@@ +8,4 @@
>   */
>  
>  add_task(function* test_enabled() {
> +  let uri = "http://url/0";

could you please remove the temp vars and just hardcode the strings in the test?

::: toolkit/components/places/tests/unifiedcomplete/test_escape_self.js
@@ +8,5 @@
>   */
>  
>  add_task(function* test_escape() {
> +  let uri1 = "http://unescapeduri/";
> +  let uri2 = "http://escapeduri/%40/";

could you please remove the temp vars and just hardcode the strings in the test?

::: toolkit/components/places/tests/unifiedcomplete/test_ignore_protocol.js
@@ +7,5 @@
>   */
>  
>  add_task(function* test_escape() {
> +  let uri1 = "http://site/";
> +  let uri2 = "http://happytimes/";

could you please remove the temp vars and just hardcode the strings in the test?

::: toolkit/components/places/tests/unifiedcomplete/test_keyword_search.js
@@ +13,5 @@
>   */
>  
>  add_task(function* test_keyword_searc() {
> +  let uri1 = "http://abc/?search=%s";
> +  let uri2 = "http://abc/?search=ThisPageIsInHistory";

could you please remove the temp vars and just hardcode the strings in the test?

@@ +18,2 @@
>    yield promiseAddVisits([ { uri: uri1, title: "Generic page title" },
>                             { uri: uri2, title: "Generic page title" } ]);

hardcoding might require to improve indentation here like

{ uri: "",
  title: "" },
{ uri: ...

::: toolkit/components/places/tests/unifiedcomplete/test_match_beginning.js
@@ +10,5 @@
>  add_task(function* test_match_beginning() {
>    Services.prefs.setBoolPref("browser.urlbar.autoFill.searchEngines", false);
>  
> +  let uri1 = "http://x.com/y";
> +  let uri2 = "https://y.com/x";

could you please remove the temp vars and just hardcode the strings in the test?

Comment 3

[Michael Pruett]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> ::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
> @@ +155,5 @@
> >            continue;
> >  
> >          let { uri, title, tags, searchEngine } = matches[j];
> > +        if (typeof uri === 'string')
> > +          uri = NetUtil.newURI(uri);
> 
> Since here we never need the nsIURI, I'd probably do the opposite, so:
> 
> let { url, title, tags, searchEngine } = matches[j];
> if (url instanceof Ci.nsIURI)
>   url = url.spec
> 
> and later use url wherever we use uri.spec.

While we don't need the URI specifically, calling NetUtil.newURI() provides the encoded URI for the URI string. Since nsIAutoCompleteController.getValueAt() returns encoded URIs, it seems necessary either to encode the URI strings for comparison with the autocomplete controller results or to decode the autocomplete controller results for comparison with unencoded URI strings. Unless I am misunderstanding, the approach you suggest does not seem to address this problem.

> ::: toolkit/components/places/tests/unifiedcomplete/test_416211.js
> @@ +13,2 @@
> >    yield promiseAddVisits({ uri: uri, title: "Page title" });
> >    addBookmark({ uri: uri,
> 
> could you please remove the temp var and just hardcode the string instead of
> uri in the test?

I can make these changes, but are you sure they are desirable? It seems unnecessarily cumbersome not to use a variable to refer to the same string repeated numerous times throughout a source file.

Comment 4

[Marco Bonardo [:mak]]

(In reply to Michael Pruett from comment #3)
> While we don't need the URI specifically, calling NetUtil.newURI() provides
> the encoded URI for the URI string. Since
> nsIAutoCompleteController.getValueAt() returns encoded URIs, it seems
> necessary either to encode the URI strings for comparison with the
> autocomplete controller results or to decode the autocomplete controller
> results for comparison with unencoded URI strings. Unless I am
> misunderstanding, the approach you suggest does not seem to address this
> problem.

I see, nevermind then, thanks for investigating that.

> > ::: toolkit/components/places/tests/unifiedcomplete/test_416211.js
> > @@ +13,2 @@
> > >    yield promiseAddVisits({ uri: uri, title: "Page title" });
> > >    addBookmark({ uri: uri,
> > 
> > could you please remove the temp var and just hardcode the string instead of
> > uri in the test?
> 
> I can make these changes, but are you sure they are desirable? It seems
> unnecessarily cumbersome not to use a variable to refer to the same string
> repeated numerous times throughout a source file.

The problem is that it makes harder to read the test, cause you must go up and check the url to verify what it's comparing the results to.
Efficiency is not a big deal here, being a test.

Comment 5

[Marco Bonardo [:mak]]

the benefit here is reducing considered these tests are moving targets... let's avoid this for now.