Closed Bug 1370881 Opened 7 years ago Closed 6 years ago

Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

Categories

(Toolkit :: Places, enhancement, P2)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mak, Assigned: hemantsingh1612, Mentored)

References

Details

(Whiteboard: [lang=js][fxsearch])

Attachments

(1 file, 3 obsolete files)

http://searchfox.org/mozilla-central/search?q=.isURIVisited
Everywhere BUT toolkit/components/places/tests/unit/test_isURIVisited.js that is directly testing the API and will continue doing so.

AND

http://searchfox.org/mozilla-central/search?q=promiseIsURIVisited&path=
Hi,
I would like to work on this bug. I am noob here, but seems like its an easy one
You can start by creating a Firefox build following this introduction guide, an artifact build will suffice:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
I'd also suggest to setup your text editor for our coding style, so unix newlines, newline at the end of file, 2 spaces indentation, no trailing spaces.
I have already built firefox, I have changed all the PlacesUtils.asyncHistory.isURIVisited to PlacesUtils.history.hasVisits (and in first result from asyncHistory.isURIVisited to PlacesUtils.history.hasVisit) from these files:
http://searchfox.org/mozilla-central/search?q=.isURIVisited

But Do I need to change gAsyncHistory.isURIVisited(kUniqueURI, errorAsyncListener); also?
It is present at
 http://searchfox.org/mozilla-central/source/toolkit/components/places/tests/browser/browser_bug680727.js

Thanks
(In reply to 3DIndian from comment #3)
> But Do I need to change gAsyncHistory.isURIVisited(kUniqueURI,
> errorAsyncListener); also?
> It is present at
>  http://searchfox.org/mozilla-central/source/toolkit/components/places/tests/
> browser/browser_bug680727.js

yes
Sorry, My Mistake...I submitted a wrong patch. I will correct it and submit it again
Hi @Marco, what's the status? 
Have you reviewed the last patch I sent?
Can you assign to bug to me?
Thanks
Comment on attachment 8875906 [details] [diff] [review]
Replaced calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits except toolkit/components/places/tests/unit/test_isURIVisited.js

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

I worked on implementing PlacesUtils.history.hasVisits and hence thought of reviewing this.

::: browser/base/content/test/general/head.js
@@ +303,4 @@
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(aURI, aExpectedValue) {

This does not seem correct. You have replaced the function name. While actually the whole function is no more needed as we need to replace all instances of calls to promiseIsURIVisited by  PlacesUtils.history.hasVisits.

@@ +351,4 @@
>          resolve();
>      }
>      aURIs.forEach(function(aURI) {
> +      PlacesUtils.history.hasVisits(aURI, function(uri, isVisited) {

hasVisits does not accept a callback.

::: browser/base/content/test/social/head.js
@@ +23,4 @@
>  function promiseSocialUrlNotRemembered(url) {
>    return new Promise(resolve => {
>      let uri = Services.io.newURI(url);
> +    PlacesUtils.history.hasVisits(uri, function(aURI, aIsVisited) {

hasVisits does not accept a callback

::: browser/components/places/tests/browser/head.js
@@ +145,4 @@
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(aURI) {

This does not seem correct. You have replaced the function name. While actually the whole function is no more needed as we need to replace all instances of calls to promiseIsURIVisited by  PlacesUtils.history.hasVisits.

::: dom/tests/browser/browser_prerendering.js
@@ +8,4 @@
>  function prerenderedVisited() {
>    let uri = Services.io.newURI(PRERENDERED_URL);
>    return new Promise(resolve => {
> +    PlacesUtils.history.hasVisits(uri, (aUri, aIsVisited) => {

hasVisits does not accept a callback.

::: services/sync/tests/unit/test_corrupt_keys.js
@@ +199,4 @@
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(url) {

This function is no longer needed.

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +225,4 @@
>   * @resolves Boolean indicating whether the URI has been visited.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(aUrl) {

This does not seem correct. You have replaced the function name. While actually the whole function is no more needed as we need to replace all instances of calls to promiseIsURIVisited by  PlacesUtils.history.hasVisits.

::: toolkit/components/places/History.jsm
@@ +528,4 @@
>      guidOrURI = PlacesUtils.normalizeToURLOrGUID(guidOrURI);
>  
>      return new Promise(resolve => {
> +      PlacesUtils.history.hasVisits(guidOrURI, (aURI, aIsVisited) => {

You do not need to replace it here as it is the actual implementation of PlacesUtils.history.hasVisits where the call to PlacesUtils.asyncHistory.isURIVisited is needed

::: toolkit/components/places/nsLivemarkService.js
@@ +563,4 @@
>  
>      // Update visited status for each entry.
>      for (let child of this._children) {
> +      PlacesUtils.history.hasVisits(child.uri, (aURI, aIsVisited) => {

PlacesUtils.history.hasVisits only takes a uri as a parameter and returns a promise, you need to handle the updateURIVisitedStatus once the promise is resolved and not as a call back. e.g. PlacesUtils.history.hasVisits(child.uri).then((aIsVisited) => {
    //call to updateURIVisitedStatus
})

::: toolkit/components/places/tests/browser/browser_bug680727.js
@@ +52,4 @@
>    }).then(() => {
>      // Global history does not record URI of a failed request.
>      return PlacesTestUtils.promiseAsyncUpdates().then(() => {
> +      PlacesUtils.history.hasVisits(kUniqueURI, errorAsyncListener);

hasVisits does not accept a callback

@@ +91,4 @@
>    }).then(() => {
>      // Check if global history remembers the successfully-requested URI.
>      PlacesTestUtils.promiseAsyncUpdates().then(() => {
> +      PlacesUtils.history.hasVisits(kUniqueURI, reloadAsyncListener);

hasVisits does not accept a callback

::: toolkit/components/places/tests/browser/head.js
@@ +304,4 @@
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(aURI, aExpectedValue) {

This function is no longer needed.

::: toolkit/components/places/tests/head_common.js
@@ +809,4 @@
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(aURI) {

This function is no longer needed.

::: toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js
@@ +60,4 @@
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> +function PlacesUtils.history.hasVisits(aURI) {

This function is no longer needed.
Thank you for cooperating, that's great!
Priority: -- → P2
Hi Marco,
This bug seems unattended for a while. If its still valid, I would like to pick it up.
Looking forward for a good first bug since returning here after a long gap.
feel free to.
Attached patch bug-1370881-fix (obsolete) — Splinter Review
Raising the first patch based on my understanding from the previous comments.
Assignee: nobody → amod.narvekar
Attachment #8875816 - Attachment is obsolete: true
Attachment #8875906 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8891743 - Flags: review?(mak77)
Comment on attachment 8891743 [details] [diff] [review]
bug-1370881-fix

I'll be on PTO in a few days, thus I'm trying to reprioritize things for the remaining time. Moving the request to Mark (sorry!)
Attachment #8891743 - Flags: review?(mak77) → review?(standard8)
Mentor: mak77 → standard8
Comment on attachment 8891743 [details] [diff] [review]
bug-1370881-fix

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

Thank you for taking this on. There's still various calls that need replacing and fixing up. I've tried to describe it all below and give you examples, but please ask if it isn't clear enough.

::: browser/base/content/test/general/head.js
@@ +307,1 @@
>  function promiseIsURIVisited(aURI, aExpectedValue) {

Commenting out the function isn't quite the right thing to do by itself - since all the files currently calling it will fail.

You'll need to go through and replace the current calls to promiseIsURIVisited with hasVisits.

Here's a link to find the files where they are called within:

http://searchfox.org/mozilla-central/search?q=promiseIsURIVisited&case=false&regexp=false&path=

For example, a line like:

    do_check_true(await promiseIsURIVisited(uri1));

needs to be replaced with:

    do_check_true(await PlacesUtils.history.hasVisits(uri1));



Additionally, calls of the style:

    ok((await promiseIsURIVisited(makeURI("http://1hour.com")))

can be replaced by:

    ok((await PlacesUtils.history.hasVisits("http://1hour.com"))


You can run an individual test with:

    ./mach test browser/base/content/test/general/browser_sanitize-timespans.js

(to check the test still passes).

::: browser/base/content/test/social/head.js
@@ +22,5 @@
>  // in history, will not appear in about:newtab or auto-complete, etc.)
>  function promiseSocialUrlNotRemembered(url) {
>    return new Promise(resolve => {
>      let uri = Services.io.newURI(url);
> +    PlacesUtils.history.hasVisits(uri).then( function(aURI, aIsVisited) {

Please rewrite this function in a similar way to how I've described prerenderedVisited() below.

::: dom/tests/browser/browser_prerendering.js
@@ +7,5 @@
>  // Returns a promise which resolves to whether or not PRERENDERED_URL has been visited.
>  function prerenderedVisited() {
>    let uri = Services.io.newURI(PRERENDERED_URL);
>    return new Promise(resolve => {
> +    PlacesUtils.history.hasVisits(uri).then( (aURI, aIsVisited) {

Since `PlacesUtils.history.hasVisits(uri)` returns a promise, we can simplify this function to:

function prerenderedVisited() {
  let uri = Services.io.newURI(PRERENDERED_URL);
  return PlacesUtils.history.hasVisits(uri);
}


xref http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/places/History.jsm#533

::: toolkit/components/places/nsLivemarkService.js
@@ +562,5 @@
>      }
>  
>      // Update visited status for each entry.
>      for (let child of this._children) {
> +      PlacesUtils.history.hasVisits(child.uri).then((aIsVisited) => {

nit: no brackets around the aIsVisited please.

::: toolkit/components/places/tests/browser/browser_bug680727.js
@@ +50,5 @@
>      // But location bar should show the original request.
>      Assert.equal(content.location.href, uri, "Docshell URI is the original URI.");
>    }).then(() => {
>      // Global history does not record URI of a failed request.
> +    return PlacesUtils.history.hasVisits(kUniqueURI).then(errorAsyncListener);

We need to keep the promiseAsyncUpdates to ensure the database writes are complete.

We could really do with rewriting this test file to use async and await, however in the meantime, I think you could do something like:

ContentTask.spawn(..., function(uri) {
  ...
}).then(() => PlacesTestUtils.promiseAsyncUpdates())
  .then(() => PlacesUtils.history.hasVisits(kUniqueURI))
  .then(isVisited => errorAsyncListener(kUniqueURI, isVisited));

You should also be able to apply a similar pattern to the same case below.

::: toolkit/components/places/tests/unit/test_isURIVisited.js
@@ +40,5 @@
>        do_print("With transition " + t);
>        let transition = PlacesUtils.history.TRANSITIONS[t];
>  
>        let uri = NetUtil.newURI(scheme + "mozilla.org/");
> +      PlacesUtils.history.hasVisits(uri).then(function(aURI, aIsVisited) {

Please undo the changes to this file. It exists purely for testing isURIVisited, and hasVisits is already tested (in test_hasVisits.js).

Hence, we can leave this test as it is, and remove it when we do the actual removal of isURIVisited.
Attachment #8891743 - Flags: review?(standard8)
Amoz, did you see my previous comments? Would you be able to continue updating the patch?
Flags: needinfo?(amod.narvekar)
Sure. I would update the patch within a day.
Flags: needinfo?(amod.narvekar)
hi. I wasn't able to find time for this bug. 
So, making it available for other contributors.
Thank you Mark for the help and support.
Status: ASSIGNED → NEW
@Amod, thank you for your work, and for letting us know.
Assignee: amod.narvekar → nobody
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
I would like to work on it.
Hi, you can work on it, just look at the previous comments and patches, and feel free to attach a new version of the patch. We assign the bug once we get the first patch. Thanks.
Assignee: nobody → hemantsingh1612
Status: NEW → ASSIGNED
Comment on attachment 8943914 [details]
Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

https://reviewboard.mozilla.org/r/214262/#review220202

Hi Hemant, thank you for taking this on. It is looking very good. There's a few things to address, but I think we are largely in the right place now.

There's also one more file to update - toolkit/components/places/tests/browser/browser_bug680727.js there's a couple of gAsyncHistory.isURIVisited in there, you should be able to convert those, and remove the gAsyncHistory global.

::: commit-message-d4d53:1
(Diff revision 1)
> +Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

If you add `. r=Standard8` onto the end of the commit message line, it'll automatically flag me for review next to you push an update.

(mozreview lets you do this for all reviewers typically using their short names).

::: browser/base/content/test/general/head.js:285
(Diff revision 1)
>   * @param aExpectedValue The expected value.
>   * @return {Promise}
>   * @resolves When the check has been added successfully.
>   * @rejects JavaScript exception.
>   */
> -function promiseIsURIVisited(aURI, aExpectedValue) {
> + function promiseIsURIVisited(aURI, aExpectedValue) {

Please remove all instances of the function definition for promiseIsURIVisited - now they've been replaced we don't need them anymore.

::: toolkit/components/places/nsLivemarkService.js:605
(Diff revision 1)
>      }
>  
>      // Update visited status for each entry.
>      for (let child of this._children) {
> -      asyncHistory.isURIVisited(child.uri, (aURI, aIsVisited) => {
> +      PlacesUtils.history.hasVisits(child.uri)
> +      .then((aURI, aIsVisited) => {

hasVisits is resolved with a boolean only. So you need to make this something like:

.then(isVisited => {
  this.updateURIVisitedStatus(child.uri, isVisited);

::: toolkit/components/places/tests/history/test_removeVisits.js:47
(Diff revision 1)
>      let visitTime = root.getChild(i).time;
>      Assert.equal(visitTime, DB_NOW - 100000 - (i * 1000));
>    }
>    root.containerOpen = false;
>  
>    info("asyncHistory.isURIVisited should return true.");

Please can you update the `info` statements in this file as well.
Attachment #8891743 - Attachment is obsolete: true
Comment on attachment 8943914 [details]
Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

https://reviewboard.mozilla.org/r/214262/#review220202

> Please remove all instances of the function definition for promiseIsURIVisited - now they've been replaced we don't need them anymore.

Sorry, I wasn't quite clear enough - you need to remove all the definitions of promiseIsURIVisited across the files (there's still a few places in the patch where it is just being modified).
Comment on attachment 8943914 [details]
Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

https://reviewboard.mozilla.org/r/214262/#review220502

Thank you for the update, just a couple more things to tidy up and we should be there.

::: toolkit/components/places/tests/browser/browser_bug680727.js:48
(Diff revision 2)
>  
>      // But location bar should show the original request.
>      Assert.equal(content.location.href, uri, "Docshell URI is the original URI.");
> -  }).then(() => {
>      // Global history does not record URI of a failed request.
> -    return PlacesTestUtils.promiseAsyncUpdates().then(() => {
> +    PlacesTestUtils.promiseAsyncUpdates().then(() => {

You need to keep the `}).then(() => {` and the return statement.

This can't be combined as otherwise the promiseAsyncUpdates and hasVisits would be run inside the content task which is in a different process with different permissions & will fail.
Attachment #8943914 - Flags: review?(standard8)
Comment on attachment 8943914 [details]
Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

https://reviewboard.mozilla.org/r/214262/#review220518

::: toolkit/components/places/nsLivemarkService.js:604
(Diff revision 3)
>        this._nodes.delete(container);
>      }
>  
>      // Update visited status for each entry.
>      for (let child of this._children) {
> -      asyncHistory.isURIVisited(child.uri, (aURI, aIsVisited) => {
> +      PlacesUtils.history.hasVisits(child.uri)

drive-by comment: I think there is a bug here, since the asyncHistory getter at the top of the file is no more used and thus the history observer is no more added (and livemarks don't listen to history changes anymore).
Comment on attachment 8943914 [details]
Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

https://reviewboard.mozilla.org/r/214262/#review220518

> drive-by comment: I think there is a bug here, since the asyncHistory getter at the top of the file is no more used and thus the history observer is no more added (and livemarks don't listen to history changes anymore).

Thanks, I'd missed that. Hemant, lets go with doing this at the top of the file:

```
XPCOMUtils.defineLazyGetter(this, "history", function() {
  // Lazily add an history observer when it's actually needed.
  PlacesUtils.history.addObserver(PlacesUtils.livemarks, true);
  return PlacesUtils.history;
});
```

(basically the same as what was there previously, but now defining `history` and returning `PlacesUtils.history`).

In the for loop we can do:

```
      history.hasVisits(child.uri, isVisited => {
        this.updateURIVisitedStatus(child.uri, isVisited);
      });
```


That seems to keep it all working still :-)
Comment on attachment 8943914 [details]
Bug 1370881 - Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits

https://reviewboard.mozilla.org/r/214262/#review220988

Great, thank you for the update. Try builds look good as well. r=Standard8
Attachment #8943914 - Flags: review?(standard8) → review+
Done.
Flags: needinfo?(standard8)
Thanks, hopefully this should land soon.
Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bfa9cafcc0d
Replace calls to asyncHistory.isURIVisited and promiseIsURIVisited with PlacesUtils.history.hasVisits r=standard8
https://hg.mozilla.org/mozilla-central/rev/8bfa9cafcc0d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [lang=js] → [lang=js][fxsearch]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: