If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Replace usage of AddURI with updatePlaces

RESOLVED FIXED in mozilla21

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Assigned: raymondlee)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
ehr, the first one (in docshell) should not be touched.
(Reporter)

Updated

5 years ago
Blocks: 700250
Summary: Replace usge of AddURI with updatePlaces → Replace usage of AddURI with updatePlaces
(Assignee)

Comment 2

5 years ago
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);

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);
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #702703 - Flags: review?(mak77)
(Reporter)

Comment 3

5 years ago
(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.
(Reporter)

Comment 4

5 years ago
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"
Attachment #702703 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Depends on: 831407
(Reporter)

Comment 5

5 years ago
you can skip converting test_399606.js here, and we can handle it in bug 831407
(Assignee)

Comment 6

5 years ago
Created attachment 702930 [details] [diff] [review]
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
Attachment #702930 - Flags: review?(mak77)
(Assignee)

Updated

5 years ago
Attachment #702703 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
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) {
Attachment #702930 - Flags: review?(mak77) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 703143 [details] [diff] [review]
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
Attachment #702930 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Passed try
https://tbpl.mozilla.org/?tree=Try&rev=04a4fba73d5a
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/93794651309a
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93794651309a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.