Closed Bug 752218 Opened 11 years ago Closed 10 years ago

Replace usage of AddURI with updatePlaces

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: mak, Assigned: raymondlee)

References

Details

Attachments

(1 file, 2 obsolete files)

ehr, the first one (in docshell) should not be touched.
Blocks: 700250
Summary: Replace usge of AddURI with updatePlaces → Replace usage of AddURI with updatePlaces
Attached patch v1 (obsolete) — Splinter Review
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)
(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 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)
Depends on: 831407
you can skip converting test_399606.js here, and we can handle it in bug 831407
Attached patch v2 (obsolete) — Splinter Review
> @@ +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)
Attachment #702703 - Attachment is obsolete: true
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+
Attached patch Patch to checkinSplinter Review
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93794651309a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.