Closed Bug 1366231 Opened 7 years ago Closed 7 years ago

implement History.hasVisits as a wrapper of asyncHistory.isURIVisited

Categories

(Toolkit :: Places, enhancement, P3)

Product:
Toolkit
Component:
Places
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: meet1995, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [good first bug][lang=js])

Attachments

(1 file)

Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;
59 bytes, text/x-review-board-request
mak
: review+
Details 
The current API signature in History.jsm has an onResult callback that is not needed, this should just return a promise that resolves to a bool.
it should have a test_hasVisits.js test in toolkit/components/places/tests/history.

all direct use of isURIVisited should be converted to the new API, apart from test_isURIVisited.js
promiseIsURIVisited should as well be removed and all of its usage replaced with the new API.
Hi Marco,
I'd like to take this up as my first bug.. It would be great if you could mentor to solve this bug.

Thank you.
(In reply to meet1995 from comment #1)
> Hi Marco,
> I'd like to take this up as my first bug.. It would be great if you could
> mentor to solve this bug.

Sure, have you gone through https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction already? Do you have a local working build?
Thanks. 
I have installed VS 2015 , I should have a local working build by tomorrow.
(In reply to Marco Bonardo [::mak] from comment #2)
> (In reply to meet1995 from comment #1)
> > Hi Marco,
> > I'd like to take this up as my first bug.. It would be great if you could
> > mentor to solve this bug.
> 
> Sure, have you gone through
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction already? Do you have a local working build?

I was able to build mozilla-central in artifact mode, do I need to build it in full mode ?
artifact mode should be fine since all of the involved code is javascript.
(In reply to Marco Bonardo [::mak] from comment #5)
> artifact mode should be fine since all of the involved code is javascript.

Thanks, I'll in to the code and try to understand the bug and get back to you in case of queries.
(In reply to meet1995 from comment #6)
> (In reply to Marco Bonardo [::mak] from comment #5)
> > artifact mode should be fine since all of the involved code is javascript.
> 
> Thanks, I'll in to the code and try to understand the bug and get back to
> you in case of queries.

*Thanks, I'll look in to the code and try to understand the bug and get back to you in case of queries.
Hi Marco, I tried understanding the code and was able come up with this https://gist.github.com/xtroncode/78de9a9cf5f6d23cbf9542e16a424874 . I am still struggling to understand how to use this function, I have used `PlacesUtils.history.hasVisits()` by looking at other tests but the function is defined in History.jsm . It would be great if you could point me to some resources where I can understand the pattern ( like what is .jsm etc). Besides this there are around 132 references of promiseIsURIVisited (including definitions) which need to be replaced with `PlacesUtils.history.hasVisits` also any direct reference to isURIVisited needs to be replaced by the new api, right?
Hi Marco, I have been reading JavaScript code modules documentation on MDN and have a bit more clarity around how things integrate. Did you get a chance to look at the gist ?

Thanks.
Please set the needinfo flag (bottom of the bug report) when you have questions or need help, we get hundreds of bugmails every day and simple comments may end up being unnoticed.

Your function should be implemented inside the History.jsm file that is in the toolkit/components/places/ folder of the src dir.
There is already a placeholder there and you can look at how are implemented. You can likely use an async function.
The old isURIVisited API is documented here: http://searchfox.org/mozilla-central/source/toolkit/components/places/mozIAsyncHistory.idl#182 it just directly takes a function callback, you don't need an object.

The test should go to toolkit/components/places/tests/history/

(In reply to meet1995 from comment #8)

> Besides this there are around 132 references of promiseIsURIVisited
> (including definitions) which need to be replaced with
> `PlacesUtils.history.hasVisits`

yes, this could happen in a separate changeset from the implementation.
Ideally I'd say one changeset to implement the API and one to fix old consumers.
In most cases hasVisits will be a drop-in replacement to promiseIsURIVisited, so it will end up being mostly a search&replace task.

> also any direct reference to isURIVisited
> needs to be replaced by the new api, right?

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.
Thanks. Can you assign this bug to me ?
Flags: needinfo?(mak77)
we usually assign on first patch attachment, but if it helps you tracking your work this won't hurt.
Assignee: nobody → meet1995
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Comment on attachment 8872421 [details]
Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;

https://reviewboard.mozilla.org/r/143926/#review148692

::: toolkit/components/places/History.jsm:531
(Diff revision 2)
>     * @throws (Error)
>     *      If `pages` has an unexpected type or if a string provided
>     *      is neither not a valid GUID nor a valid URI.
>     */
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(page) {
> +    let uri = PlacesUtils.normalizeToURLOrGUID(page);

This will also allow a guid, that's a good idea. let's change the argument to be named guidOrURI and also update the javadoc accordingly. see fetch()

::: toolkit/components/places/History.jsm:533
(Diff revision 2)
>     *      is neither not a valid GUID nor a valid URI.
>     */
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(page) {
> +    let uri = PlacesUtils.normalizeToURLOrGUID(page);
> +    return PlacesUtils.withConnectionWrapper("History.jsm: hasVisits",
> +     db => hasVisits(db,uri)

This is not needed, since the implementation here is very simple you can just inline it here, the need for "inner" implementations is so that the API part is shorter, but it doesn't matter for very small implementations.

You also don't need withConnectionWrapper because isURIVisited uses its own connection handler.
here you just need to return new Promise...

::: toolkit/components/places/History.jsm:1265
(Diff revision 2)
> +// Inner implementation of History.hasVisits.
> +var hasVisits = async function(db, uri) {
> +  return new Promise((resolve, reject) => {
> +     PlacesUtils.asyncHistory.isURIVisited(uri,{
> +          isVisited:(aURI,aIsVisited)=>{
> +            resolve(aIsVisited);

please, always indent by 2 spaces, and add whitespaces before braces, after : and around =>
Attachment #8872421 - Flags: review?(mak77)
Comment on attachment 8872421 [details]
Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;

https://reviewboard.mozilla.org/r/143926/#review149694

Almost there, still some minor details to fix.

::: toolkit/components/places/History.jsm:530
(Diff revision 3)
>     *      is neither not a valid GUID nor a valid URI.
>     */
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(guidOrURI) {
> +    guidOrURI = PlacesUtils.normalizeToURLOrGUID(guidOrURI);
> +
> +    return new Promise((resolve, reject) => {

don't pass reject, since we don't use it. just resolve => {

::: toolkit/components/places/History.jsm:532
(Diff revision 3)
> -  hasVisits(page, onResult) {
> -    throw new Error("Method not implemented");
> +  hasVisits(guidOrURI) {
> +    guidOrURI = PlacesUtils.normalizeToURLOrGUID(guidOrURI);
> +
> +    return new Promise((resolve, reject) => {
> +      PlacesUtils.asyncHistory.isURIVisited(guidOrURI, {
> +          isVisited: (aURI, aIsVisited) => {

the indentation here is still wrong (4 spaces instead of 2), and you still don't need to pass an object, just pass an arrow function callback.

::: toolkit/components/places/tests/history/test_hasVisits.js:27
(Diff revision 3)
> +    "passing an invalid (not of type URI or nsIURI) object to History.hasVisits should throw a TypeError"
> +  );
> +});
> +
> +add_task(async function test_history_has_visits(){
> +    const TEST_URL = "http://mozilla.com/";

wrong indentation in the whole function, should be 2 spaces

::: toolkit/components/places/tests/history/test_hasVisits.js:29
(Diff revision 3)
> +});
> +
> +add_task(async function test_history_has_visits(){
> +    const TEST_URL = "http://mozilla.com/";
> +    await PlacesTestUtils.clearHistory();
> +    Assert.equal(await PlacesUtils.history.hasVisits(TEST_URL),false,"Test Url should not be in history.")

please always put a space after commas
And try to wrap lines at about 80 chars, for example in this case:
Assert.equal(await PlacesUtils.history.hasVisits(TEST_URL), false,
             "Test Url should not be in history.");
             
(this line is also missing a semicolon)
Attachment #8872421 - Flags: review?(mak77)
Blocks: 1370881
Comment on attachment 8872421 [details]
Bug 1366231 - Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited;

https://reviewboard.mozilla.org/r/143926/#review150746

LGTM, thank you.
I'm starting a Try run to quickly check tests, then will land.

Note I filed bug 1370881 to replace consumers, since this is ready there's no reason to delay its landing.
Attachment #8872421 - Flags: review?(mak77) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/aa3d11c78ac9
Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited; r=mak
Sorry, this had to be backed out for linting failures, e.g. in History.jsm:

https://hg.mozilla.org/integration/autoland/rev/525fb3e1314b9da3d86c0f68c7bb76301eaae1bb

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=aa3d11c78ac9a7ee493b595e91bf49f4c77e50bc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=105219545&repo=autoland

>[task 2017-06-07T16:07:31.460230Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/places/History.jsm:1258:3 | Newline required at end of file but not found. (eol-last)
Just add a linebreak at the end of the file.

>[task 2017-06-07T16:07:31.460362Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/places/tests/history/test_hasVisits.js:1:52 | Expected linebreaks to be 'LF' but found 'CRLF'. (linebreak-style)
Please convert these Windows linebreaks to Unix ones.

See http://gecko.readthedocs.io/en/latest/tools/lint/usage.html?highlight=eslint for how to run the linting locally.

Please fix the issues and submit an updated patch. Thank you.
Flags: needinfo?(meet1995)
I have submitted an updated patch for review.
Flags: needinfo?(meet1995) → needinfo?(mak77)
I've applied the patch locally and ran mach eslint, it seems ok now. It's unfortunate mozreview doesn't run it, and the Try run requesting xpcshell tests should also have run it...

Btw, once you setup your text editor to satisfy newline at end of file and unix newlines you should be fine for a while :)
Flags: needinfo?(mak77)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/90d3eb2f3d00
Implement History.hasVisits as a wrapper of asyncHistory.isURIVisited; r=mak
https://hg.mozilla.org/mozilla-central/rev/90d3eb2f3d00
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Keywords: checkin-needed
this has already landed, it doesn't need further checkin-needed :)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: