updatePlaces doesn't have a great API for javascript, thus I'd like to convert its consumers to the new PlacesUtils.history.insert(Many) APIs. alternatively, tests code can use PlacesTestUtils.addVisits, that has an ever more streamlined API. These are the code points http://searchfox.org/mozilla-central/search?q=.updatePlaces%28&path= Though, the following files should not be changed: toolkit/components/places/History.jsm (implementation) toolkit/components/places/tests/PlacesTestUtils.jsm (covered by bug 1272686) toolkit/components/places/tests/unit/test_async_history_api.js (own test) Would also be great to remove the addVisits helper from toolkit/components/places/tests/browser/head.js and replace its usage in those tests with PlacesTestUtils.addVisits Finally, we should add a comment in mozIAsyncHistory.idl stating that it is intended as a low-level API, and javascript consumers should rather look into History.jsm APIs.

Hi Marco, I'm interested to work on this bug. Could you please point me to the setup instructions and how to reproduce, please? Thank you for your time.

Hi, you can start from here to create your build: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction Once you have a build, comment 0 points out most of the code that should be touched. Basically we have this updatePlaces API, that should be considered a low level API. We have 2 abstractions over it, PlacesUtils.history.insert (or insertMany) (from History.jsm) and PlacesTestUtils.addVisits (usable only by tests). The scope is to remove the unneded calls to the low level updatePlaces API and replace them with the higher level abstractions. For any question feel free to set the needinfo flag that is at the bottom of this bug report.

H(In reply to Marco Bonardo [::mak] from comment #2) > Hi, you can start from here to create your build: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction > On this URL, I have an option to setup a desktop Firefox build, Firefox for Android or b2g OS. I'm supposed to create the build for desktop Firefox right? Please excuse me being naive.

yes, Firefox Desktop.

Hi Marco, I was able to setup the build and run it for Firefox today. This got delayed because of poor internet connectivity, I'll reproduce the bug at the earliest and get back to you. This is an update, thank you for your time!

Hi! I would like to work on this bug.

I think abhi12ravi@gmail.com planned to work on it, so let's see if that's still the case first.

Waiting for abhi12ravi@gmail.com. BTW I am all set to make the patch.

Hi Marco/Divyanshu, Please go ahead. I might take some more time to understand and send in a patch. The Tech Speakers program is taking up all my time.

Hi Marco, Out of the two abstractions, how to decide which one to use in order to replace UpdatePlaces in each case. eg: In browser/components/migration/ChromeProfileMigrator.js PlacesUtils.asyncHistory.updatePlaces(places, { Thanks

(In reply to divyanshu.divix from comment #10) > Out of the two abstractions, how to decide which one to use in order to > replace UpdatePlaces in each case. eg: > In browser/components/migration/ChromeProfileMigrator.js > PlacesUtils.asyncHistory.updatePlaces(places, { If it's in a test, better to use PlacesTestUtils.addVisits, tests are in folders that are descendants od a test/ or tests/ folder. If it's in product code, insert should be used when only one url is added, insertMany when many urls are added at once. Do you plan to work on this bug?

(In reply to Marco Bonardo [::mak] from comment #11) > (In reply to divyanshu.divix from comment #10) > > Out of the two abstractions, how to decide which one to use in order to > > replace UpdatePlaces in each case. eg: > > In browser/components/migration/ChromeProfileMigrator.js > > PlacesUtils.asyncHistory.updatePlaces(places, { > > If it's in a test, better to use PlacesTestUtils.addVisits, tests are in > folders that are descendants od a test/ or tests/ folder. > If it's in product code, insert should be used when only one url is added, > insertMany when many urls are added at once. > Do you plan to work on this bug? Yes, I want to work on this bug.

Replaces UpdatePlaces to PlacesUtils.history.insert or PlacesTestUtils.addVisits Removes addVisits helper from toolkit/components/places/tests/browser/head.js Adds a comment in mozIAsyncHistory.idl about UpdatePlaces.

you don't need to set more than one flag, one between needinfo, feedback or review is enough to track a request.

Comment on attachment 8786448 [details] [diff] [review] Convert js consumers of UpdatePlaces to PlacesUtils.history.insert or PlacesTestUtils.addVisits Review of attachment 8786448 [details] [diff] [review]: ----------------------------------------------------------------- Ok, unfornately there is more to do than just a plain search&replace, I have probably been unclear on the scope here. Since the methods we are replacing have different signatures, each replament may require code adaptation, and they could slightly differ. Since that would make up a too wide patch, I'd suggest to take the problems one-by-one (either consumer-by-consumer or folder-by-folder) and make one patch per piece, after the first 2 or 3 patches you may gain more knoledge about later patches. You can queue patches using Mercurial Queues (https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues, or you could use real commits and mutable-history if you're more used to git, but that may be a bit more complex at the beginning if you don't know git). alternatively, let's just complete one of the patches, land it and proceed to the next one. As you prefer. ::: addon-sdk/source/test/addons/places/lib/places-helper.js @@ +93,4 @@ > > function addVisits (urls) { > var deferred = defer(); > + PlacesTestUtils.addVisits([].concat(urls).map(createVisit), { The PlacetTestUtils module is not imported in this scope, you must import it first. Please check at the top of this file what we do for PlacesUtils.jsm for example The address is "resource://testing-common/PlacesTestUtils.jsm" @@ +97,3 @@ > handleResult: function () {}, > handleError: deferred.reject, > handleCompletion: deferred.resolve You cannot just replace the name of the function, cause the 2 methods have different signatures (they take different input and have different behavior). You can check the PlacesTestUtils.addVisits documentation here: http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/toolkit/components/places/tests/PlacesTestUtils.jsm#24 It resolves to a promise already, so you may not need a defer() at all. So every replacement needs some code adaptation to the different behavior. When you modify a test, you should compile it and run it to ensure it passes yet https://developer.mozilla.org/en-US/docs/Mozilla/QA/Developing_tests These addon-sdk tests are a bit more complex to run, it could be easier to run the other tests using ./mach test path_to_the_test ::: browser/base/content/test/general/browser_sanitize-timespans.js @@ +477,4 @@ > let lastYear = new Date(); > lastYear.setFullYear(lastYear.getFullYear() - 1); > addPlace(makeURI("http://before-today.com/"), "Before Today", lastYear.getTime() * 1000); > + PlacesTestUtils.addVisits(places, { here you can use PlacesTestUtils directly because it's imported by the head.js file that is in the same folder (browser-chrome tests, who begins with browser_, automatically import an head.js header file) You clearly still need to update the behavior (and I'm not going to repeat this, since it's valid for eachg change). ::: browser/components/migration/ChromeProfileMigrator.js @@ +349,4 @@ > > if (places.length > 0) { > yield new Promise((resolve, reject) => { > + PlacesUtils.history.insert(places, { Also history.insert has a different signature than updatePlaces. please check http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/toolkit/components/places/History.jsm#135 ::: browser/components/migration/tests/marionette/test_refresh_firefox.py @@ +47,4 @@ > error = self.runAsyncCode(""" > // Copied from PlacesTestUtils, which isn't available in Marionette tests. > let didReturn; > + PlacesTestUtils.addVisits( Notice the comment above, we may have to leave this alone for now. ::: toolkit/components/places/mozIAsyncHistory.idl @@ +151,4 @@ > /** > * Adds a set of visits for one or more mozIPlaceInfo objects, and updates > * each mozIPlaceInfo's title or guid. > + * you are introducing a trailing space here @@ +156,2 @@ > * > + * It is intended as a low-level API, and javascript consumers should rather look into History.jsm APIs. I'd make this a @note ::: toolkit/components/places/tests/browser/head.js @@ +124,4 @@ > * @param [optional] aStack > * The stack frame used to report errors. > */ > +PlacesTestUtils.addVisits( this is wrong and can't work. the addvisits method should rather be completely removed and then it's consumers should be updated to use PlacesTestUtils.

Hi, are you still working on this?

Hi Marco, I would like to start working on this bug. Ill go through the above comments and will get back to you if I need some clarifications. Please let me know if its fine. Regards Ganesh

This is the list of the files that CAN use updatePlaces and thus should NOT be changed: toolkit/components/places/History.cpp toolkit/components/places/History.jsm toolkit/components/places/mozIAsyncHistory.idl test_async_history_api.js anything else in this list should use PlacesTestUtils.addVisits if it's a test, PlacesUtils.history.insertMany if it's passing true as the coalesceNotifications option to updatePlaces (currently just Migration), or PlacesUtils.history.insert in the other cases.

Oh I'm also about to add test_updatePlaces_embed.js that should also NOT be changed.

ehr in comment 18 I didn't post the link to the search: http://searchfox.org/mozilla-central/search?q=updatePlaces%28&path=

Hi Marco, I just started with the first file in the search list you have provided. I changed the code as below for http://searchfox.org/mozilla-central/source/browser/components/migration/MigrationUtils.jsm#1013 let infos = []; // Create a PageInfo for each entry. for (let place of places) { let info = {url: place.uri}; info.title = place.title ; if (typeof place.referrer == "string") { place.referrer = NetUtil.newURI(place.referrer); } else if (place.referrer && place.referrer instanceof URL) { place.referrer = NetUtil.newURI(place.referrer.href); } let visitDate = place.visitDate; if (visitDate) { if (!(visitDate instanceof Date)) { visitDate = PlacesUtils.toDate(visitDate); } } else { visitDate = new Date(); } info.visits = [{ transition: place.transition, date: visitDate, referrer: place.referrer }]; infos.push(info); } return PlacesUtils.history.insertMany(infos); Let me know if this is the way to go, where we have to build the info's object from the places argument and pass to PlacesUtils.history.insertMany. And also, I have a query like, we are not using the options parameter when we change to insertMany. Is it expected or I have to pass it somewhere ?

(In reply to ganesh2583 from comment #21) > Let me know if this is the way to go, where we have to build the info's > object from the places argument and pass to PlacesUtils.history.insertMany. > And also, I have a query like, we are not using the options parameter when > we change to insertMany. Is it expected or I have to pass it somewhere ? The best path forward would be to change the insertVisitsWrapper callers. That is, the various migrators using it. They could collect page info objects with the right format already, rather than passing the old format and then having insertVisitsWrapper convert it to the new format. It should be easy to do. These are the current consumers: http://searchfox.org/mozilla-central/search?q=insertVisitsWrapper&path= Regarding the options object, you are right, it is no more needed, because insertMany does that already. But, they use handleCompletion of the old options object to move on (either resolve a promise or invoke a callback. For that you should use the .then() callback of the promise returned by insertVisitsWrapper, and remove the options arguments.

(In reply to Marco Bonardo [::mak] from comment #22) > The best path forward would be to change the insertVisitsWrapper callers. > That is, the various migrators using it. > They could collect page info objects with the right format already, rather > than passing the old format and then having insertVisitsWrapper convert it > to the new format. It should be easy to do. > These are the current consumers: > http://searchfox.org/mozilla-central/search?q=insertVisitsWrapper&path= Hi Marco, If we change the insertVisitsWrapper callers we need to update the 4 migrators, with the code of converting the places object to info object which would be the same code. Instead if we have the conversion logic inside insertVisitsWrapper wouldn't it be easier to maintain with minimal code duplication. Let me know If I'm wrong. My basic assumption is we have to convert the places object to info object and change the signature of insertVisitsWrapper to take in the info objects as parameters and pass it to insertMany. Regards, Ganesh

(In reply to ganesh2583 from comment #23) > If we change the insertVisitsWrapper callers we need to update the 4 > migrators, with the code of converting the places object to info object > which would be the same code. The 4 migrators are building place objects just to pass them to insertVisitsWrapper, so changing their format should not be a big deal, they are built with the purpose to be used just there. > Instead if we have the conversion logic inside > insertVisitsWrapper wouldn't it be easier to maintain with minimal code > duplication. In other cases you could be right, for example if in the future the format may change again, or even go back to what it was before, it would be simpler to write a converter. But the reality in our case is that we are deprecating an old format and moving to a new one. Thus it doesn't make sense to keep conversion code around for a deprecated input format. > Let me know If I'm wrong. My basic assumption is we have to > convert the places object to info object and change the signature of > insertVisitsWrapper to take in the info objects as parameters and pass it to > insertMany. Yes, but you don't need a converter, just change the code creating the object to create the "right" new format. For example http://searchfox.org/mozilla-central/rev/fcd9f1480d65f1a5df2acda97eb07a7e133f6ed4/browser/components/migration/ChromeProfileMigrator.js#332 (change uri to url, change transitionType to transition, ...)

(In reply to Marco Bonardo [::mak] from comment #24) > (In reply to ganesh2583 from comment #23) > > Instead if we have the conversion logic inside > > insertVisitsWrapper wouldn't it be easier to maintain with minimal code > > duplication. > > In other cases you could be right, for example if in the future the format > may change again, or even go back to what it was before, it would be simpler > to write a converter. But the reality in our case is that we are deprecating > an old format and moving to a new one. Thus it doesn't make sense to keep > conversion code around for a deprecated input format. Yup. Plus converting 2000-odd objects isn't exactly free, so we should just create them the right way to begin with. :-)

Hi Marco, I added a initial version of patch, where I modified the MigrationUtils module.I Changed the function call inside insertVisitsWrapper from updatePlacesChanged to insertMany and changed the function insertVisitsWrapper to take pageInfos as parameters and pass them directly to PlacesUtils.history.insertMany. Modified the consumers of insertVisitsWrapper to prepare the pageInfos object instead of places and pass it to insertVisitsWrapper. I had to change the _updateHistoryUndo function as well to keep up with the latest changes. Made changes to test_automigration unit test as well to take it new format of pageInfos. Let me know if any changes are required, if its fine Ill proceed with changes to other files as well. Regards, Ganesh

Comment on attachment 8856218 [details] [diff] [review] Bug 1282770 - Changed updatePlacesChanged to insertMany and parameters to pageInfos in insertVisitsWrapper. Consumers of insertVisitsWrapper to prepare the pageInfos and pass. r=mak Review of attachment 8856218 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/migration/ChromeProfileMigrator.js @@ +325,5 @@ > try { > // if having typed_count, we changes transition type to typed. > let transType = PlacesUtils.history.TRANSITION_LINK; > if (row.getResultByName("typed_count") > 0) > transType = PlacesUtils.history.TRANSITION_TYPED; nit: if you rename transType to transition, then later you can just use { transition, ... in the object @@ +333,3 @@ > visits: [{ > + transition: transType, > + date: chromeTimeToDate(row.getResultByName("last_visit_time")) * 1000; date should be a Date object, here you are converting it to a PRTime by multiplying per 1000. that was needed before because visitData was a PRTime, but date is just a Date object. Just remove the * 1000. @@ +343,4 @@ > yield new Promise((resolve, reject) => { > + MigrationUtils.insertVisitsWrapper(pageInfos); > + }).then((updatedCount) => { > + if(updatedCount > 0){ All of this machinery is no more needed, insertMany will already do this and insertVisitsWrapper will return a promise already. Just: yield MigrationUtils.insertVisitsWrapper(pageInfos); ::: browser/components/migration/EdgeProfileMigrator.js @@ +128,4 @@ > // Note that the time will be in microseconds (PRTime), > // and Date.now() returns milliseconds. Places expects PRTime, > // so we multiply the Date.now return value to make up the difference. > + let date = time || (Date.now() * 1000); PlacesUtils.toDate(time) and no * 1000 Also the comment needs to be updated. @@ +131,5 @@ > + let date = time || (Date.now() * 1000); > + pageInfos.push({ > + url, > + visits: [{ transition: Ci.nsINavHistoryService.TRANSITION_LINK, > + date}] please add whitespace after date @@ +140,5 @@ > aCallback(typedURLs.size == 0); > return; > } > > + MigrationUtils.insertVisitsWrapper(pageInfos); you should invoke aCallback here, the migrators use aCallback to communicate if the migration succeeded or not, through a bool. MigrationUtils.insertVisitsWrapper(pageInfos).then(() => aCallback(true), () => aCallback(false)); ::: browser/components/migration/IEProfileMigrator.js @@ +75,2 @@ > // so we multiply the Date.now return value to make up the difference. > let lastVisitTime = entry.get("time") || (Date.now() * 1000); Same as the Edge migrator, use toDate on time, new Date() instead of Date.now and don't multiply by 1000. @@ +89,5 @@ > aCallback(true); > return; > } > > + MigrationUtils.insertVisitsWrapper(pageInfos); same as Edge migrator ::: browser/components/migration/MigrationUtils.jsm @@ +1071,3 @@ > let first, last; > if (visitCount > 1) { > + let dates = pageInfo.visits.map(v => v.date); dates were PRTime, now they are Date objects... this will break automigration undo, because it passes these to PlacesUtils.toDate http://searchfox.org/mozilla-central/rev/eace920a0372051a11d8d275bd9b8f14f3024ecd/browser/components/migration/AutoMigrate.jsm#600 We should do 2 things: 1. remove the toDate calls from AutoMigrate.jsm since they are no more needed 2. change toPRTime and toDate in PlacesUtils.jsm to check their input, in particular if toDate receives a Date object, it should Cu.reportError the fact, and then return the unchanged object. It should not try to convert it! toPRTime should instead check that the input is a Date object or a Number and if it's a Number it should be smaller or equal than Date.now(), otherwise Cu.reportError and don't touch it. @@ +1078,2 @@ > } > + let url = pageInfo.url.spec; We should not assume these are nsIURI let's do: let url = pageInfo.url instanceof Ci.nsIURI ? pageInfo.url.spec : typeof pageInfo.url == "string" ? pageInfo.url : pageInfo.url.href; Or add an util to PlacesUtils to do the same, something like PlacesUtils.hrefForURILike(uri) that always returns the string and then use that here. ::: browser/components/migration/SafariProfileMigrator.js @@ +216,1 @@ > let transType = PlacesUtils.history.TRANSITION_LINK; rename to transition, and just pass transition in the visits object @@ +226,2 @@ > title: entry.get("title"), > + visits: [{ transition: transType, this needs reindentation @@ +233,5 @@ > } > } > } > + if (pageInfos.length > 0) { > + MigrationUtils.insertVisitsWrapper(pageInfos); again, this should invoke aCallback once done. ::: browser/components/migration/tests/unit/test_automigration.js @@ +263,4 @@ > visits: [ > { > + transition: PlacesUtils.history.TRANSITION_LINK, > + date: now_uSec, cannot be microseconds, you should change let now_uSec to let now = new Date(); @@ +267,4 @@ > }, > { > + transition: PlacesUtils.history.TRANSITION_LINK, > + date: now_uSec - 100 * kUsecPerMin, also change kUsecPerMin to kMsPerMin (and update its value, should be / 1000) @@ +486,3 @@ > title: "Example", > visits: [{ > + date: new Date("2015-07-10").getTime() * 1000, All the .getTime() * 1000 should go away

note, since this patch is large enough, it would be great to fix other consumers in separate patches or clones bugs. For example the Sync consumers could be done separately, the remaining tests as well...

Hi Marco, I implemented code review comments. I have taken a combined patch of both my change lists(i.e the initial changes and code review implementation ones)by using the below command: hg export -r 351733:351734 > "<file path>" Please let me know If I have to create the patch in any different way. And do provide the feedback on the code review implementation. Regards, Ganesh

Comment on attachment 8858065 [details] [diff] [review] Bug 1282770 - Changed updatePlacesChanged to insertMany and parameters to pageInfos in insertVisitsWrapper. Consumers of insertVisitsWrapper to prepare the pageInfos and pass. r=mak Review of attachment 8858065 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Ganesh Chaitanya Kale from comment #29) > I implemented code review comments. I have taken a combined patch of both my > change lists(i.e the initial changes and code review implementation ones)by > using the below command: > hg export -r 351733:351734 > "<file path>" Ugh, I spent time reviewing the patch without figuring out it was the old one with the new one appended. So my comments were pointless :( The patch exported like that doesn't work for a review, since I don't see the final code changes. Depending on your workflow, the right way to address review comments may vary: 1. if you use the mozreview workflow (doesn't look like), you need the Evolve extension, address comments, then use hg amend && hg evolve --all, then push again to review 2. if you mercurial queues (MQ) you can use hg qfold to merge comments into the original patch, then attach the patch 3. if you just use heads with committed changes, you need to merge the changesets. To do that you can use the histedit mercurial extension, it will allow you to fold 2 draft changesets into one, then just export the single changeset and attach it

Taking this on to get the patch landed, we'll finish the migration patch in this bug, and get new bugs filed for the remaining parts.

Comment on attachment 8960535 [details] Bug 1282770 - Convert uses of PU.asyncHistory.updatePlaces in browser/components/migration to PU.history.insertMany. https://reviewboard.mozilla.org/r/229266/#review235394 Please, move this bug to Firefox::migration so it's visible to the right devs ::: browser/components/migration/AutoMigrate.jsm:623 (Diff revision 1) > continue; > } > let visitData = { > url: urlObj, > - beginDate: PlacesUtils.toDate(urlVisits.first), > - endDate: PlacesUtils.toDate(urlVisits.last), > + beginDate: new Date(urlVisits.first), > + endDate: new Date(urlVisits.last), Let's add a comment here at the top that it's necessary to recreate URL and Date objects because the data can be serialized through JSON, that destroys those objects ::: browser/components/migration/ChromeProfileMigrator.js:282 (Diff revision 1) > for (let row of rows) { > try { > // if having typed_count, we changes transition type to typed. > - let transType = PlacesUtils.history.TRANSITION_LINK; > + let transition = PlacesUtils.history.TRANSITION_LINK; > if (row.getResultByName("typed_count") > 0) > - transType = PlacesUtils.history.TRANSITION_TYPED; > + transition = PlacesUtils.history.TRANSITION_TYPED; please use PlacesUtils.history.TRANSITIONS.LINK/TYPED so we don't cross XPCOM ::: browser/components/migration/ChromeProfileMigrator.js:286 (Diff revision 1) > - transType = PlacesUtils.history.TRANSITION_TYPED; > + transition = PlacesUtils.history.TRANSITION_TYPED; > > - places.push({ > + pageInfos.push({ > - uri: NetUtil.newURI(row.getResultByName("url")), > title: row.getResultByName("title"), > + url: NetUtil.newURI(row.getResultByName("url")), just use new URL, no reason for nsIURI ::: browser/components/migration/EdgeProfileMigrator.js:116 (Diff revision 1) > let typedURLs = this._typedURLs; > - let places = []; > + let pageInfos = []; > for (let [urlString, time] of typedURLs) { > - let uri; > + let url; > try { > - uri = Services.io.newURI(urlString); > + url = Services.io.newURI(urlString); new URL ::: browser/components/migration/EdgeProfileMigrator.js:128 (Diff revision 1) > } > > - // Note that the time will be in microseconds (PRTime), > - // and Date.now() returns milliseconds. Places expects PRTime, > - // so we multiply the Date.now return value to make up the difference. > - let visitDate = time || (Date.now() * 1000); > + pageInfos.push({ > + url, > + visits: [{ > + transition: Ci.nsINavHistoryService.TRANSITION_TYPED, ditto on using history.TRANSITIONS. ::: browser/components/migration/EdgeProfileMigrator.js:129 (Diff revision 1) > > - // Note that the time will be in microseconds (PRTime), > - // and Date.now() returns milliseconds. Places expects PRTime, > - // so we multiply the Date.now return value to make up the difference. > - let visitDate = time || (Date.now() * 1000); > - places.push({ > + pageInfos.push({ > + url, > + visits: [{ > + transition: Ci.nsINavHistoryService.TRANSITION_TYPED, > + date: time ? PlacesUtils.toDate(time) : Date.now(), should be new Date(), not Date.now(), to have date be a consistent type ::: browser/components/migration/IEProfileMigrator.js:64 (Diff revision 1) > } > > // The typed urls are already fixed-up, so we can use them for comparison. > - let transitionType = typedURLs.has(uri.spec) ? > + let transition = typedURLs.has(url.spec) ? > - Ci.nsINavHistoryService.TRANSITION_TYPED : > + Ci.nsINavHistoryService.TRANSITION_TYPED : > - Ci.nsINavHistoryService.TRANSITION_LINK; > + Ci.nsINavHistoryService.TRANSITION_LINK; use .TRANSITIONS. ::: browser/components/migration/IEProfileMigrator.js:73 (Diff revision 1) > - places.push( > - { uri, > + pageInfos.push({ > + url, > - title, > + title, > - visits: [{ transitionType, > - visitDate: lastVisitTime }], > - } > + visits: [{ > + transition, > + date: time ? PlacesUtils.toDate(entry.get("time")) : Date.now(), new Date(), not Date.now() ::: browser/components/migration/MigrationUtils.jsm:1129 (Diff revision 1) > try { > new URL(url); > } catch (ex) { > // This won't save and we won't need to 'undo' it, so ignore this URL. > continue; > } ideally it should be possible to just do: let url = pageInfo.url; try { url = new URL(url instanceof Ci.nsIURI ? url.spec : url); } catch (ex) { // This won't save and we won't need to 'undo' it, so ignore this URL. continue; } I think visitMap will work fine with URL objects, as well as JSON.stringify (that will just .toString it). Please check anyway. ::: browser/components/migration/SafariProfileMigrator.js:198 (Diff revision 1) > // reference date of NSDate. > let date = new Date("1 January 2001, GMT"); > date.setMilliseconds(asDouble * 1000); > - return date * 1000; > + return date; > } > return 0; hmm, I'm not totally convinced it's ok to assume the visit was in 1970... what about we just return new Date()? It's wrong regardless, but it may be less wrong. ::: browser/components/migration/SafariProfileMigrator.js:216 (Diff revision 1) > for (let entry of entries) { > if (entry.has("lastVisitedDate")) { > - let visitDate = this._parseCocoaDate(entry.get("lastVisitedDate")); > + let date = this._parseCocoaDate(entry.get("lastVisitedDate")); > try { > - places.push({ uri: NetUtil.newURI(entry.get("")), > + pageInfos.push({ > + url: NetUtil.newURI(entry.get("")), new URL (please check if we can remove some of the NetUtil imports in these files when we stop using nsIURI) ::: browser/components/migration/SafariProfileMigrator.js:221 (Diff revision 1) > + url: NetUtil.newURI(entry.get("")), > - title: entry.get("title"), > + title: entry.get("title"), > - visits: [{ transitionType: transType, > - visitDate }] }); > + visits: [{ > + // Safari's History file contains only top-level urls. It does not > + // distinguish between typed urls and linked urls. > + transition: PlacesUtils.history.TRANSITION_LINK, .TRANSITIONS. ::: browser/components/migration/SafariProfileMigrator.js:233 (Diff revision 1) > - handleCompletion(updatedCount) { > - aCallback(updatedCount > 0); > - }, > - }); > - } else { > aCallback(false); This looks wrong, if there's nothing to import the migration should be successful, unless we skipped all the entries. Maybe we should store whether we catched at least once, if we did then it's failure. ::: browser/components/migration/tests/unit/test_automigration.js:284 (Diff revision 1) > - transitionType: PlacesUtils.history.TRANSITION_LINK, > - visitDate: now_uSec, > + transition: PlacesUtils.history.TRANSITION_LINK, > + date: now, > }, > { > - transitionType: PlacesUtils.history.TRANSITION_LINK, > - visitDate: now_uSec - 100 * kUsecPerMin, > + transition: PlacesUtils.history.TRANSITION_LINK, > + date: PlacesUtils.toDate((now * 1000) - 100 * kUsecPerMin), just change kUsecPerMin to kMsecPerMin and drop the * 1000? ::: browser/components/migration/tests/unit/test_automigration.js:512 (Diff revision 1) > }, { > - visitDate: new Date("2015-08-10").getTime() * 1000, > - transitionType: Ci.nsINavHistoryService.TRANSITION_LINK, > + date: new Date("2015-08-10"), > + transition: Ci.nsINavHistoryService.TRANSITION_LINK, > }], > }, { > - uri: NetUtil.newURI("http://www.example.org/"), > + url: NetUtil.newURI("http://www.example.org/"), to increase testing coverage you could mixup nsIURI, URL and href.

Comment on attachment 8960535 [details] Bug 1282770 - Convert uses of PU.asyncHistory.updatePlaces in browser/components/migration to PU.history.insertMany. https://reviewboard.mozilla.org/r/229266/#review235394 > ideally it should be possible to just do: > let url = pageInfo.url; > try { > url = new URL(url instanceof Ci.nsIURI ? url.spec : url); > } catch (ex) { > // This won't save and we won't need to 'undo' it, so ignore this URL. > continue; > } > > I think visitMap will work fine with URL objects, as well as JSON.stringify (that will just .toString it). Please check anyway. I've tested this an a Map object unfortunately doesn't work with URL objects, so we can't do this.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/82e4db446286 Convert uses of PU.asyncHistory.updatePlaces in browser/components/migration to PU.history.insertMany. r=mak

FWIW, this changed some consumers to produce new URL() instead of Services.io.newURI(). But those objects don't have the same properties. So the code that continued to access url.scheme now never works, because it's always undefined - you want url.protocol instead. ( https://hg.mozilla.org/mozilla-central/rev/82e4db446286#l3.18 ) Mark: is there something we can do to prevent this type of mistake with eslint, or is this only possible if we use something like Flow so that there's type information? What's the state of doing something like that? Marco: I'm fixing this instance in bug 1448920, as Edge history import is also being broken by the new win10 update, but please can you check if there are other instances in this and/or other insertMany() migration patches? Thanks.

(In reply to :Gijs (he/him) from comment #38) > Marco: I'm fixing this instance in bug 1448920, as Edge history import is > also being broken by the new win10 update, but please can you check if there > are other instances in this and/or other insertMany() migration patches? I should have (and I will) paid more attention, the horrible testing situation in migration unfortunately doesn't help at all. The problem is more general though, since I suspect we'll convert more and more nsIURI to URL (that I suspect has less xpcom overhead?) I'm not sure what's the exact request, but afaik this was the only patch touching history migration. And I'm not sure how to check if this mistake is spread in the codebase, without checking all .scheme accesses one by one. Also note that protocol is not the same as scheme, since it also includes the colon.

Fwiw, this code is also unnecessary and can be removed + else if (typeof url != "string") { + pageInfo.url.href; + }

(In reply to :Gijs (he/him) from comment #38) > Mark: is there something we can do to prevent this type of mistake with > eslint, or is this only possible if we use something like Flow so that > there's type information? What's the state of doing something like that? We could only do this with something like flow + type information. Last Mossop looked at Flow & other items, there was nothing really that would easily apply to our codebase & be able to work with it.

(In reply to Marco Bonardo [::mak] from comment #39) > The problem is more general though, since I suspect we'll convert more and > more nsIURI to URL (that I suspect has less xpcom overhead?) I think this would be true if it wasn't for the fact that AFAICT we then later construct an nsIURI anyway (once we're inside places). :-\ > I'm not sure what's the exact request, but afaik this was the only patch > touching history migration. I assumed that there was an effort to switch to insertMany elsewhere. Indeed, the other bugs under bug 1448041 seem to be about this. I checked the 2 other commits from the 2 other closed deps, and they don't seem to switch to `new URL` anywhere new, so I guess this is the only case where the uri/url switch was made in a way where it broke things. \o/ > And I'm not sure how to check if this mistake is > spread in the codebase, without checking all .scheme accesses one by one. > Also note that protocol is not the same as scheme, since it also includes > the colon. Thanks for this tip, it saved me some time in bug 1448920...

(In reply to :Gijs (he/him) from comment #42) > I think this would be true if it wasn't for the fact that AFAICT we then > later construct an nsIURI anyway (once we're inside places). :-\ Yes, for some APIs like updatePlaces we do. But now that legacy add-ons are gone, we can drop that and just use strings. I'll file a bug to investigate that attached to bug 1354531, updatePlaces will become a Places internal API, and as such it can avoid some input validation.