There isn't much left to kill removeVisitsByTimeframe, this is the mxr search: http://mxr.mozilla.org/mozilla-central/search?string=removeVisitsByTimeframe Sync should spin the events loop for now (see Async.makeSpinningCallback) test_removeVisitsByTimeframe.js should be moved to ../history/test_removeVisits.js the cpp API should call PLACES_WARN_DEPRECATE() the idl should have a @deprecated comment added

(In reply to Marco Bonardo [::mak] from comment #0) > There isn't much left to kill removeVisitsByTimeframe, this is the mxr > search: > > http://mxr.mozilla.org/mozilla-central/search?string=removeVisitsByTimeframe > > Sync should spin the events loop for now (see Async.makeSpinningCallback) > test_removeVisitsByTimeframe.js should be moved to > ../history/test_removeVisits.js > the cpp API should call PLACES_WARN_DEPRECATE() > the idl should have a @deprecated comment added Happy to work on this, is there something this should be replaced with? What should the @deprecated comment message be?

Thank you! History.removeVisitsByFilter is the replacement, the deprecation message should point to that method.

(In reply to Marco Bonardo [::mak] from comment #2) > Thank you! > History.removeVisitsByFilter is the replacement, the deprecation message > should point to that method. Ah right in the title, I totally missed that @_@

Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r=mak

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak https://reviewboard.mozilla.org/r/24615/#review22543 ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:195 (Diff revision 1) > + endDate: new Date(usSinceEpoch + (item.end * 60 * 60 * 1000 * 1000)) Looks like you're trying to pass microseconds to the Date constructor, that's not working as expected. You want to let msSinceEpoch = parseInt(usSinceEpoch / 1000); and remove a * 1000 from item.begin conversion. Same for endDate. ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:207 (Diff revision 1) > + }); we were not reporting errors before, and I don't feel like we should start now. let's keep this simple. Moreover the callback format here is not the same as updatePlaces, it's a simple callback that gets a pageInfo object. Just PlacesUtils.history.removeVisitsByFilter(filter).catch(ex => Logger.AssertTrue(false, "An error occured while deleting history: " + ex);).then(result => {cb(null, result)}, err => {cb(err)}); And you also need Async.waitForSyncCallback(cb); check what we did for clearHistory in the other bug. ::: toolkit/components/places/tests/unit/test_removeVisits.js:1 (Diff revision 1) > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ when moving or renaming files, you should always use hg mv, otherwise the file looks "new" to Mercurial and all its history gets lost. I can't review the test changes as-is cause I don't see the changes... You can save aapart this version, hg mv the old one and then copy over the new version. then hg should produce a proper diff ::: toolkit/components/places/tests/unit/test_removeVisits.js:5 (Diff revision 1) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ you can remove all the headers from here, included the licence ::: toolkit/components/places/tests/unit/test_removeVisits.js:18 (Diff revision 1) > +add_task(function remove_visits_outside_unbookmarked_uri() { when using add_task, you should always pass a generator. so function* instead of just function This happens multiple times. ::: toolkit/components/places/tests/unit/xpcshell.ini:137 (Diff revision 1) > -[test_removeVisitsByTimeframe.js] > +[test_removeVisits.js] you should move the test to the history/subfolder

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24615/diff/1-2/

(In reply to Marco Bonardo [::mak] from comment #6) > Comment on attachment 8684675 [details] > MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with > History.removeVisitsByFilter and deprecate it. r=mak > > https://reviewboard.mozilla.org/r/24615/#review22543 > > ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:195 > (Diff revision 1) > > + endDate: new Date(usSinceEpoch + (item.end * 60 * 60 * 1000 * 1000)) > > Looks like you're trying to pass microseconds to the Date constructor, > that's not working as expected. You want to > let msSinceEpoch = parseInt(usSinceEpoch / 1000); > and remove a * 1000 from item.begin conversion. > > Same for endDate. > > ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:207 > (Diff revision 1) > > + }); > > we were not reporting errors before, and I don't feel like we should start > now. let's keep this simple. > Moreover the callback format here is not the same as updatePlaces, it's a > simple callback that gets a pageInfo object. > > Just > PlacesUtils.history.removeVisitsByFilter(filter).catch(ex => > Logger.AssertTrue(false, "An error occured while deleting history: " + > ex);).then(result => {cb(null, result)}, err => {cb(err)}); > And you also need > Async.waitForSyncCallback(cb); > > check what we did for clearHistory in the other bug. > > ::: toolkit/components/places/tests/unit/test_removeVisits.js:1 > (Diff revision 1) > > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > > when moving or renaming files, you should always use hg mv, otherwise the > file looks "new" to Mercurial and all its history gets lost. > > I can't review the test changes as-is cause I don't see the changes... > You can save aapart this version, hg mv the old one and then copy over the > new version. then hg should produce a proper diff > > ::: toolkit/components/places/tests/unit/test_removeVisits.js:5 > (Diff revision 1) > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > you can remove all the headers from here, included the licence > > ::: toolkit/components/places/tests/unit/test_removeVisits.js:18 > (Diff revision 1) > > +add_task(function remove_visits_outside_unbookmarked_uri() { > > when using add_task, you should always pass a generator. so function* > instead of just function > > This happens multiple times. > > ::: toolkit/components/places/tests/unit/xpcshell.ini:137 > (Diff revision 1) > > -[test_removeVisitsByTimeframe.js] > > +[test_removeVisits.js] > > you should move the test to the history/subfolder Apologies for taking so long to get to this, I've been busy with a front-end dev class I'm teaching at General Assembly on top of the day job. Thanks for the detailed feedback! I've made updates and pushed another review and try.

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak https://reviewboard.mozilla.org/r/24615/#review24781 when fixing review comments, please mark them as fixed or dropped in mozreview. ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:193 (Diff revision 2) > - usSinceEpoch + (item.begin * 60 * 60 * 1000 * 1000), > + let msSinceEpoch = parseInt(usSinceEpoch /1000); missing space after / ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:199 (Diff revision 2) > + .catch(ex => Logger.AssertTrue(false, "An error occurred while deleting history: " + ex);) there should be no semicolon here, sorry for suggesting that, I didn't double check the code I posted... ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:200 (Diff revision 2) > + .then(result => {cb(null, result)}, err => {cb(err)}); please align .catch and .then to the first dot as: PlacesUtils.history.removeVisitsByFilter(filter) .catch(...) .then(...) ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:200 (Diff revision 2) > + .then(result => {cb(null, result)}, err => {cb(err)}); for readability, please add whitespace after { and before } ::: toolkit/components/places/tests/history/test_removeVisits.js:12 (Diff revision 2) > -add_task(function remove_visits_outside_unbookmarked_uri() { > +add_task(function* remove_visits_outside_unbookmarked_uri() { I think something happened in between and your changes replacesing removevisitsbyTimeframe with the new API got lost in the test file

(In reply to Marco Bonardo [::mak] (Away Dec 12-20) from comment #10) > Comment on attachment 8684675 [details] > MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with > History.removeVisitsByFilter and deprecate it. r=mak > > https://reviewboard.mozilla.org/r/24615/#review24781 > > when fixing review comments, please mark them as fixed or dropped in > mozreview. > > ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:193 > (Diff revision 2) > > - usSinceEpoch + (item.begin * 60 * 60 * 1000 * 1000), > > + let msSinceEpoch = parseInt(usSinceEpoch /1000); > > missing space after / > > ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:199 > (Diff revision 2) > > + .catch(ex => Logger.AssertTrue(false, "An error occurred while deleting history: " + ex);) > > there should be no semicolon here, sorry for suggesting that, I didn't > double check the code I posted... > > ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:200 > (Diff revision 2) > > + .then(result => {cb(null, result)}, err => {cb(err)}); > > please align .catch and .then to the first dot as: > > PlacesUtils.history.removeVisitsByFilter(filter) > .catch(...) > .then(...) > > ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:200 > (Diff revision 2) > > + .then(result => {cb(null, result)}, err => {cb(err)}); > > for readability, please add whitespace after { and before } I've updated the style adjustments and marked them as "Fixed" in mozreview. > ::: toolkit/components/places/tests/history/test_removeVisits.js:12 > (Diff revision 2) > > -add_task(function remove_visits_outside_unbookmarked_uri() { > > +add_task(function* remove_visits_outside_unbookmarked_uri() { > > I think something happened in between and your changes replacesing > removevisitsbyTimeframe with the new API got lost in the test file I'm not sure what to do about this last file. Your previous review comments said I should make all of those test functions into generator functions, and to move the file into the history subfolder. Can you advise what I should do to adjust for the new changes?

https://reviewboard.mozilla.org/r/24615/#review24781 > I think something happened in between and your changes replacesing removevisitsbyTimeframe with the new API got lost in the test file The only remaining problem is that we are deprecating removeVisitsByTimeframe, that means we must replace all usage of it in the code tree. You properly moved the test and fixed the generators, but now you should still replace all the removeVisitsByTimeframe calls in the test with removeVisitsByFilter calls, and test should pass.

sorry for the delay, please use the needinfo flag when you are blocked on me.

I'm having trouble updating the tests due to the differences in time precision between the old removeVisitsByTimeframe and new removeVisitsByFilter. removeVisitsByFilter expects the times to be in msecs but removeVisitsByTimeframe expects usecs. This is fine for most tests except something like http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_removeVisitsByTimeframe.js#106. Regardless of the times I use on the filter for beginDate and endDate, I can't seem to remove any items in the history. I've been looking around for help in the code, and this seems to be the only example I can find that handles the conversion http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/history/test_removeVisitsByFilter.js#35.

In general the conversion should be simple but there is precision in the middle, for example the test your linked does this: // Add 10 visits for the URI from now to 9 usecs in the past It's pretty clear we cannot use a 9us precision using milliseconds, so there you should instead replace usecs with millisecs. that means instead doing // Add 10 visits for the URI from now to 9 msecs in the past and use 2 dates that differ by 9 milliseconds to do the removing. If this still doesn't help, feel free to attach an updated version of the patch with the failing test commented out for review, so I could directly take a look at it. It's even possible there's a bug in RemoveVisitsByFilter, I can't tell off-hand.

(In reply to Marco Bonardo [::mak] from comment #16) > In general the conversion should be simple but there is precision in the > middle, for example the test your linked does this: > // Add 10 visits for the URI from now to 9 usecs in the past > It's pretty clear we cannot use a 9us precision using milliseconds, so there > you should instead replace usecs with millisecs. that means instead doing > // Add 10 visits for the URI from now to 9 msecs in the past > and use 2 dates that differ by 9 milliseconds to do the removing. > > If this still doesn't help, feel free to attach an updated version of the > patch with the failing test commented out for review, so I could directly > take a look at it. > It's even possible there's a bug in RemoveVisitsByFilter, I can't tell > off-hand. Thanks for the help, and that seems what I've been doing so I'm confused by the failing tests. I'm having trouble updating the review board with the error below, so I've added a patch attachment for the test file to this bug. > remote: abort: could not lock working directory of /repo/hg/mozilla/gecko: Permission denied

Comment on attachment 8714396 [details] [diff] [review] convert from usec to msec for remove visits by filter Review of attachment 8714396 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/history/test_removeVisits.js @@ +19,5 @@ > } > yield PlacesTestUtils.addVisits(visits); > > do_print("Remove visits using timerange outside the URI's visits."); > + PlacesUtils.history.removeVisitsByFilter({ one obvious mistake here is that you are not yielding, thus the test is not really waiting for the work to be done.

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24615/diff/2-3/

(In reply to Marco Bonardo [::mak] from comment #20) > Comment on attachment 8714396 [details] [diff] [review] > convert from usec to msec for remove visits by filter > > Review of attachment 8714396 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/places/tests/history/test_removeVisits.js > @@ +19,5 @@ > > } > > yield PlacesTestUtils.addVisits(visits); > > > > do_print("Remove visits using timerange outside the URI's visits."); > > + PlacesUtils.history.removeVisitsByFilter({ > > one obvious mistake here is that you are not yielding, thus the test is not > really waiting for the work to be done. Thanks for pointing that out, I've updated the tests to reflect your advice. I've successfully refactored the relevant tests except for 2, remove_all_visits_unbookmarked_place_uri and remove_all_visits_bookmarked_uri. They are failing on "URI should still exist in moz_places." and "Frecency should be negative.", respectively. I've updated the review with my changes and have commented out these 2 tests so you can follow my progress.

The first failure "makes sense". The old API was skipping place: uris that had no visits and no bookmark associated. I suspect the primary reason was to prevent us from removing a valuable place: url that was unassociated due to some kind of corruption. I don't think it has much value, it's unlikely we can recover that kind of corruption. I did a quick check on my db and I don't really see why we should have an unbookmarked place: entry anywhere. Thus I think you should remove the "remove_all_visits_unbookmarked_place_uri" test. Regarding test "remove_all_visits_bookmarked_uri", the reason it fails is that the old API was invalidating frecencies but not recalculating them, cause it was a synchronous API. the new API does recalculate and can wait since it's async. We may re-evaluate this in future for performance reasons, but I think for now we should update the test. I think you should store the frecency value before the call to removeVisitsByFilter, and then later check the new frecency value is smaller.

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak https://reviewboard.mozilla.org/r/24615/#review31539 ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:193 (Diff revision 3) > - usSinceEpoch + (item.begin * 60 * 60 * 1000 * 1000), > + let msSinceEpoch = parseInt(usSinceEpoch /1000); missing space after / ::: services/sync/tps/extensions/tps/resource/modules/history.jsm:199 (Diff revision 3) > + .catch(ex => Logger.AssertTrue(false, "An error occurred while deleting history: " + ex);) can remove the semicolon from here ::: toolkit/components/places/tests/history/test_removeVisits.js:26 (Diff revision 3) > + filter.endDate = new Date(JS_NOW); why not define these inline in the object? ::: toolkit/components/places/tests/history/test_removeVisits.js:76 (Diff revision 3) > + filter.endDate = new Date(JS_NOW); ditto ::: toolkit/components/places/tests/history/test_removeVisits.js:120 (Diff revision 3) > + filter.endDate = new Date(JS_NOW); ditto ::: toolkit/components/places/tests/history/test_removeVisits.js:170 (Diff revision 3) > + filter.endDate = new Date(JS_NOW); ditto ::: toolkit/components/places/tests/history/test_removeVisits.js:214 (Diff revision 3) > + filter.endDate = new Date(JS_NOW); ditto ::: toolkit/components/places/tests/history/test_removeVisits.js:237 (Diff revision 3) > -add_task(function* remove_all_visits_unbookmarked_place_uri() { > +// add_task(function* remove_all_visits_unbookmarked_place_uri() { an remove this test (see bugzilla comment) ::: toolkit/components/places/tests/history/test_removeVisits.js:276 (Diff revision 3) > - do_print("Add some visits for the URI."); > +// add_task(function* remove_all_visits_bookmarked_uri() { can be fixed, see bugzilla comment ::: toolkit/components/places/tests/history/test_removeVisits.js:337 (Diff revision 3) > + filter.endDate = new Date(JS_NOW); define inline

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24615/diff/3-4/

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24615/diff/4-5/

(In reply to Marco Bonardo [::mak] from comment #23) > The first failure "makes sense". The old API was skipping place: uris that > had no visits and no bookmark associated. I suspect the primary reason was > to prevent us from removing a valuable place: url that was unassociated due > to some kind of corruption. I don't think it has much value, it's unlikely > we can recover that kind of corruption. > I did a quick check on my db and I don't really see why we should have an > unbookmarked place: entry anywhere. > Thus I think you should remove the > "remove_all_visits_unbookmarked_place_uri" test. > > Regarding test "remove_all_visits_bookmarked_uri", the reason it fails is > that the old API was invalidating frecencies but not recalculating them, > cause it was a synchronous API. the new API does recalculate and can wait > since it's async. We may re-evaluate this in future for performance reasons, > but I think for now we should update the test. > I think you should store the frecency value before the call to > removeVisitsByFilter, and then later check the new frecency value is smaller. I've updated with your advice, thanks for the speedy feedback! I've also updated the review and pushed a try run with xpcshell tests.

Comment on attachment 8684675 [details] MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with History.removeVisitsByFilter and deprecate it. r?mak https://reviewboard.mozilla.org/r/24615/#review31889 Thank you, I will directly land this.

Just a couple hints for future contributions: Setting one flag is enough, you can request either needinfo, feedback or review, setting two flags won't add much value cause each of those flags already generates a visible and trackable request in the bugzilla dashboard. You can (when it's not broken) land to Try directly from mozreview, if you open Review summary and check at the top for the Automation menu. The results are tracked alongside the review.

(In reply to Marco Bonardo [::mak] from comment #29) > Comment on attachment 8684675 [details] > MozReview Request: Bug 1220654 - Replace removeVisitsTimeframe with > History.removeVisitsByFilter and deprecate it. r?mak > > https://reviewboard.mozilla.org/r/24615/#review31889 > > Thank you, I will directly land this. Great! (In reply to Marco Bonardo [::mak] from comment #31) > Just a couple hints for future contributions: > > Setting one flag is enough, you can request either needinfo, feedback or > review, setting two flags won't add much value cause each of those flags > already generates a visible and trackable request in the bugzilla dashboard. > > You can (when it's not broken) land to Try directly from mozreview, if you > open Review summary and check at the top for the Automation menu. The > results are tracked alongside the review. Good to know, thanks for the help!