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)
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
Reporter | ||
Comment 2•7 years ago
|
||
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
Reporter | ||
Comment 4•7 years ago
|
||
(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.
Reporter | ||
Comment 10•7 years ago
|
||
Thank you for cooperating, that's great!
Reporter | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 11•7 years ago
|
||
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.
Reporter | ||
Comment 12•7 years ago
|
||
feel free to.
Comment 13•7 years ago
|
||
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)
Reporter | ||
Comment 14•7 years ago
|
||
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)
Reporter | ||
Updated•7 years ago
|
Mentor: mak77 → standard8
Comment 15•7 years ago
|
||
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®exp=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)
Comment 16•7 years ago
|
||
Amoz, did you see my previous comments? Would you be able to continue updating the patch?
Flags: needinfo?(amod.narvekar)
Comment 17•7 years ago
|
||
Sure. I would update the patch within a day.
Flags: needinfo?(amod.narvekar)
Comment 18•7 years ago
|
||
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.
Updated•7 years ago
|
Status: ASSIGNED → NEW
Comment 19•7 years ago
|
||
@Amod, thank you for your work, and for letting us know.
Assignee: amod.narvekar → nobody
Updated•7 years ago
|
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
Assignee | ||
Comment 20•6 years ago
|
||
I would like to work on it.
Reporter | ||
Comment 21•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Assignee: nobody → hemantsingh1612
Status: NEW → ASSIGNED
Comment 23•6 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Attachment #8891743 -
Attachment is obsolete: true
Comment 25•6 years ago
|
||
mozreview-review-reply |
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 26•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Reporter | ||
Comment 28•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 30•6 years ago
|
||
mozreview-review-reply |
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 :-)
Updated•6 years ago
|
Attachment #8943914 -
Flags: review?(standard8)
Comment hidden (mozreview-request) |
Comment 32•6 years ago
|
||
mozreview-review |
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+
Comment 35•6 years ago
|
||
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
Comment 36•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8bfa9cafcc0d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Reporter | ||
Updated•6 years ago
|
Whiteboard: [lang=js] → [lang=js][fxsearch]
You need to log in
before you can comment on or make changes to this bug.
Description
•