752218 - Replace usage of AddURI with updatePlaces

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

we want to stop implementing nsIGlobalHistory2, to do that we should stop using AddURI.

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDownloadHistory.cpp#75
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_420331_wyciwyg.js#55
and
http://mxr.mozilla.org/mozilla-central/search?string=gh.AddURI

Comment 1

[Marco Bonardo [:mak]]

ehr, the first one (in docshell) should not be touched.

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

test_399606.js: need to change the below because addURI() doesn't add duplicate visits but updatePlaces() does.

-  do_check_eq(cc, 1);
+  do_check_eq(cc, 2);

test_420331_wyciwyg.js: need to remove the below because updatePlaces doesn't accept wyciwyg:// and we have test in the file to check that already.

-
-  // test codepath of docshell caller
-  histsvc.QueryInterface(Ci.nsIGlobalHistory2);
-  placeID = histsvc.addURI(testURI, false, false, null);
-  do_check_false(placeID > 0);

Comment 3

[Marco Bonardo [:mak]]

(In reply to Raymond Lee [:raymondlee] from comment #2)
> Created attachment 702703 [details] [diff] [review]
> v1
> 
> test_399606.js: need to change the below because addURI() doesn't add
> duplicate visits but updatePlaces() does.
> 
> -  do_check_eq(cc, 1);
> +  do_check_eq(cc, 2);

So, the scope of this test is to verify bug 399606, what it does it do add a visit to testURI, then add another visit to testuri that uses testuri itself as referrer.
cc is 1 not because addURI doesn't add multiple visits, but because we should not add a second visit if its referrer is the same site (we should consider this a refresh and don't store history for it).
So, you should try to specify referrer: testURI in the second visit and if cc is still 2 should file a bug to fix that in updatePlaces as it was fixed in addURI.

> test_420331_wyciwyg.js: need to remove the below because updatePlaces
> doesn't accept wyciwyg:// and we have test in the file to check that already.

yes, this is fine.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 702703 [details] [diff] [review]
v1

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

::: toolkit/components/places/tests/unit/test_399606.js
@@ +48,3 @@
>    var now = Date.now();
>    var testURI = uri("http://fez.com");
> +  yield promiseAddVisits([{uri: testURI}, {uri: testURI}]);

should pass referrer: testURI in the second visit

@@ +60,5 @@
>    var result = histsvc.executeQuery(query, options);
>    var root = result.root;
>    root.containerOpen = true;
>    var cc = root.childCount;
> +  do_check_eq(cc, 2);

as said, this should stay 1 or a bug should be filed

::: toolkit/components/places/tests/unit/test_isvisited.js
@@ +3,5 @@
>  /* 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/. */
>  
> +// main

please remove "// main" comments when you see them

@@ +57,5 @@
>        // Note this in the log but ignore as it's not the subject of this test.
>        do_log_info("Could not construct URI for '" + currentURL + "'; ignoring");
>      }
>      if (cantAddUri) {
> +      yield promiseAddVisits(uri3);

should be cantAddUri, not uri3

@@ +58,5 @@
>        do_log_info("Could not construct URI for '" + currentURL + "'; ignoring");
>      }
>      if (cantAddUri) {
> +      yield promiseAddVisits(uri3);
> +      do_check_guid_for_uri(cantAddUri);

I think you misread the guid check, it was done only if the second argument was not provided, so in all cases but this one. That's cause this uri can't have a guid since it can't be added.
Indeed I'm surprised that this passes, we don't add cantAddUri anywhere, so this should fail... may you please check?

::: toolkit/components/places/tests/unit/test_markpageas.js
@@ +18,5 @@
>                 {url: "http://www.espn.com/",
>                  transition: histsvc.TRANSITION_LINK}];
>  
>  // main
> +// main

please remove both "comments"

Comment 5

[Marco Bonardo [:mak]]

you can skip converting test_399606.js here, and we can handle it in bug 831407

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2

> @@ +60,5 @@
> >    var result = histsvc.executeQuery(query, options);
> >    var root = result.root;
> >    root.containerOpen = true;
> >    var cc = root.childCount;
> > +  do_check_eq(cc, 2);
> 
> as said, this should stay 1 or a bug should be filed

filed a bug.

Also, addressed other issues in comment 4

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 702930 [details] [diff] [review]
v2

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

Thanks!

I think you forgot to remove test_399606.js changes from the patch, as it is it would fail when pushed.
As I said we'll fix this test in bug 831407, please leave it unchanged for now and just comment in bug 831407 that this test should be converted when the bug is fixed.
thus, r=me provided that test changes are reverted.

::: toolkit/components/places/tests/unit/test_isvisited.js
@@ +51,5 @@
>      "wyciwyg:/0/http://mozilla.org",
>      "javascript:alert('hello wolrd!');",
>    ];
> +  for (let i = 0; i < URLS.length; i++) {
> +    var currentURL = URLS[i];

for (let currentURL of URLS) {

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch to checkin

(In reply to Marco Bonardo [:mak] from comment #7)
> Comment on attachment 702930 [details] [diff] [review]
> v2
> 
> Review of attachment 702930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> I think you forgot to remove test_399606.js changes from the patch, as it is
> it would fail when pushed.
> As I said we'll fix this test in bug 831407, please leave it unchanged for
> now and just comment in bug 831407 that this test should be converted when
> the bug is fixed.
> thus, r=me provided that test changes are reverted.
> 

I didn't see your last comment before uploading the patch.  Reverted the test changes. 

> ::: toolkit/components/places/tests/unit/test_isvisited.js
> @@ +51,5 @@
> >      "wyciwyg:/0/http://mozilla.org",
> >      "javascript:alert('hello wolrd!');",
> >    ];
> > +  for (let i = 0; i < URLS.length; i++) {
> > +    var currentURL = URLS[i];
> 
> for (let currentURL of URLS) {

Updated

Comment 9

[Raymond Lee [:raymondlee]]

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=04a4fba73d5a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93794651309a

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/93794651309a