these APIs are synchronous and used in browser may not be an easy task.

I need some guidance here, since the code is compiling without errors, but it's crashing when using: > PlacesUtils.asyncHistory.setCharsetForURI(URI, charset); Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (nsStandardURL not thread-safe), at /netwerk/base/src/nsStandardURL.cpp:921

Comment on attachment 716250 [details] [diff] [review] Part I (Set charset) Review of attachment 716250 [details] [diff] [review]: ----------------------------------------------------------------- Since these are basically just UI hints used by browser and some backup toolkit code, it's very unlikely we need to access them from cpp, thus I think it would be much simpler (for everyone) to just expose a couple helpers in PlacesUtils, that for now will internally use annotations (but expose the value through a callback (and maybe a promise so that the consumer can choose?). If in future we should find the need to do that in cpp, probably we should do that directly when adding the visit... But for now I couldn't find any consumer in need of that. So I'd actually remove the APIs from history and provide them as async PlacesUtils helpers. Btw, just for information your bug here was probably the fact you create a nsIURI on main-thread, then assign it a member variable of a runnable on another thread, and I think it doesn't support thread-safe refcounting, thus you should have rather stored the spec, or used .swap to avoid changing refcounting

The first part implements the PlacesUtils async helpers and updates the unit tests.

Comment on attachment 718486 [details] [diff] [review] Part I - Async helpers and unit tests Review of attachment 718486 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesUtils.jsm @@ +2136,5 @@ > + * Sets the character-set for a URI. > + * If aCharset is empty remove character-set annotation for aURI. > + * > + * @param aURI nsIURI > + * @param aCharset character-set value. please document the return value @@ +2148,5 @@ > + Ci.nsIAnnotationService.EXPIRE_NEVER); > + } else { > + PlacesUtils.annotations.removePageAnnotation(aURI, this.CHARSET_ANNO); > + } > + deferred.resolve(); to be sure to catch bogus behaviors with an async setter all of this should be enqueued on main-thread, otherwise it would still be synchronous and would be hard in future to make it properly async. @@ +2158,5 @@ > + * Gets the last saved character-set for a URI. > + * > + * @param aURI nsIURI > + * @param aCallback the callback method. > + * @return the character-set value if found. this is wrong @@ +2175,5 @@ > + if (aCallback) { > + aCallback(); > + } > + deferred.resolve(); > + } ditto, all of this should be enqueued on main-thread so we can start fixing consumers to an async behavior ::: toolkit/components/places/tests/unit/test_317472.js @@ +22,5 @@ > yield promiseAddVisits(TEST_BOOKMARKED_URI); > > // create bookmarks on TEST_BOOKMARKED_URI > + var bm1 = PlacesUtils.bookmarks.insertBookmark( > + PlacesUtils.bookmarks.unfiledBookmarksFolder, for roots always use the PlacesUtils getters (PlacesUtils.unfiledBookmarksFolderId in this case) not the .bookmarks. getters ::: toolkit/components/places/tests/unit/test_384370.js @@ +188,5 @@ > PlacesUtils.annotations.getItemAnnotation(testBookmark1.itemId, POST_DATA_ANNO)); > > // last charset > var testURI = PlacesUtils._uri(testBookmark1.uri); > + PlacesUtils.getCharsetForURI(testURI, function(aCharset) { this is wrong, testCanonicalBookmarks should wait for this to be complete since now it's async ::: toolkit/components/places/tests/unit/test_bookmarks_html.js @@ +362,5 @@ > case "charset": > + let testURI = NetUtil.newURI(aNode.uri); > + Task.spawn(function() { > + do_check_eq((yield PlacesUtils.getCharsetForURI(testURI)), aExpected.charset); > + }); also this is wrong, checkItem doesn't seem to wait for these checks... indeed I think also the getLivemark in the feedUrl case is totally wrong and should be fixed ::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js @@ +130,5 @@ > // last charset > var testURI = uri(testBookmark1.uri); > + Task.spawn(function() { > + do_check_eq((yield PlacesUtils.getCharsetForURI(testURI)), "ISO-8859-1"); > + }); nothing is waiting for the task

Comment on attachment 718590 [details] [diff] [review] Part IV - Remove cpp methods Review of attachment 718590 [details] [diff] [review]: ----------------------------------------------------------------- this will require SR (and clearly all of the other patches should have been pushed already). I found about 6 add-ons using these helpers, we can probably contact them directly once we push this, or we may file a bug apart to do the removal and here just mark the deprecation.

Comment on attachment 718588 [details] [diff] [review] Part II - Update use of setCharsetForURI Review of attachment 718588 [details] [diff] [review]: ----------------------------------------------------------------- this looks ok, there is some involved risk with moving to async since adding a page, calling the setter, removing a page would likely throw, but it's likely the setter will just fail there.

Comment on attachment 718589 [details] [diff] [review] Part III - Update use of getCharsetForURI Review of attachment 718589 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately Task doesn't have any magic that ensures the code stops there, that means most of this patch is wrong (the only right piece is BookmarkHTMLUtils.jsm change). we can't just replace a synchronous getter with an asynchronous one like that, we need separate patches for each consumer and figure out how to change each consumer to accept an async getter

(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #7) > Comment on attachment 718486 [details] [diff] [review] > Part I - Async helpers and unit tests > > > ::: toolkit/components/places/tests/unit/test_bookmarks_html.js > @@ +362,5 @@ > > case "charset": > > + let testURI = NetUtil.newURI(aNode.uri); > > + Task.spawn(function() { > > + do_check_eq((yield PlacesUtils.getCharsetForURI(testURI)), aExpected.charset); > > + }); > > also this is wrong, checkItem doesn't seem to wait for these checks... > indeed I think also the getLivemark in the feedUrl case is totally wrong and > should be fixed > I am not sure how to tackle this because checkItem() is called inside loop which makes it hard to wait for these checks and then carry on.... Any suggestions?

See comment 11 please

(In reply to Raymond Lee [:raymondlee] from comment #11) > > I am not sure how to tackle this because checkItem() is called inside loop > which makes it hard to wait for these checks and then carry on.... Any > suggestions? It will have to return a promise, and the same should then happen in its callers up to the add_task... I don't think there is an easier solution

Simplified the patch

There are some complicated cases so created some dependency bugs.

I'm a bit worried by how complicate the getShortcutOrURI patches are, and the fact they convert something that depends on "synchronous" user input to an asynchronous behavior. Those reviews may take a bit more time to ensure safety.

So likely we should move the "remove cpp" part to a separate bug that depends on them being fixed.

Comment on attachment 719843 [details] [diff] [review] Part I - Async helpers and unit tests v4 Review of attachment 719843 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesUtils.jsm @@ +49,5 @@ > + > +XPCOMUtils.defineLazyGetter(this, "Promise", function() { > + Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); > + return Promise; > +}); why not defineLazyModuleGetter? If you just took inspiration from the other getters, those are not using defineLazyModuleGetter cause likely at that time it didn't exist (Well, I actually verified and it existed the week before I made that patch, so it's possible I was just not aware of that, the week after). I'm happy if you also fix those :) @@ +2133,5 @@ > + }, > + > + /** > + * Sets the character-set for a URI. > + * If aCharset is empty remove character-set annotation for aURI. s/annotation// that's an implementation detail that should not matter here @@ +2149,5 @@ > + Ci.nsIAnnotationService.EXPIRE_NEVER); > + } else { > + PlacesUtils.annotations.removePageAnnotation(aURI, this.CHARSET_ANNO); > + } > + Services.tm.mainThread.dispatch(function() { you should delay the whole thing (apart let deferred and return deferred.promise clearly), if we set it synchronously and just resolve asynchronously a test may still pass expecting it to be set as soon as the call completes. please also add a comment stating we are delaying this to catch issues with asynchronous behavior while waiting to implement asynchronous annotations in bug 699844. @@ +2160,5 @@ > + /** > + * Gets the last saved character-set for a URI. > + * > + * @param aURI nsIURI > + * @param aCallback the callback method. should state it gets the character-set as argument, or null otherwise @@ +2162,5 @@ > + * > + * @param aURI nsIURI > + * @param aCallback the callback method. > + * @return {Promise} > + * @resolve a character-set should also state which value the character-set has if it doesn't exist, I suppose null may be fine. @@ +2179,5 @@ > + deferred.resolve(charset); > + } catch (ex) { > + if (aCallback) { > + aCallback(); > + } just let charset = null; outside of the try/catch, and invoke aCallback(charset) and resolve(charset) after it? @@ +2182,5 @@ > + aCallback(); > + } > + deferred.resolve(); > + } > + }.bind(this), Ci.nsIThread.DISPATCH_NORMAL); may avoid bind by useing full PlacesUtils, also cause you do PlacesUtils.annotations and this.CHARSET_ANNO, so it's not coherent atm. (ditto for setCharsetForURI) ::: toolkit/components/places/tests/unit/test_384370.js @@ +55,5 @@ > populate(); > > // 2. run the test-suite > + Task.spawn(validate).then(function() { > + promiseAsyncUpdates().then(function testJsonExport() { if we could just yield validate(); and yield promiseAsyncUpdates(); here and below, the code could be a bit more readable ::: toolkit/components/places/tests/unit/test_bookmarks_html.js @@ +272,5 @@ > root.containerOpen = false; > } > } > > +/* this looks wrong debug commenting ::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js @@ +186,5 @@ > // and comparing it, we just check the data size. > do_check_eq(TEST_FAVICON_DATA_SIZE, aDataLen); > deferred.resolve(); > }); > + yield deferred.promise; instead of this I think you may change the test to do yield Task.spawn(database_check) instead of yield database_check(), and here you may not need anymore the deferred

(In reply to Marco Bonardo [:mak] from comment #19) > Comment on attachment 719843 [details] [diff] [review] > Part I - Async helpers and unit tests v4 > > Review of attachment 719843 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/places/PlacesUtils.jsm > @@ +49,5 @@ > > + > > +XPCOMUtils.defineLazyGetter(this, "Promise", function() { > > + Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); > > + return Promise; > > +}); > > why not defineLazyModuleGetter? > If you just took inspiration from the other getters, those are not using > defineLazyModuleGetter cause likely at that time it didn't exist (Well, I > actually verified and it existed the week before I made that patch, so it's > possible I was just not aware of that, the week after). I'm happy if you > also fix those :) Updated > > @@ +2133,5 @@ > > + }, > > + > > + /** > > + * Sets the character-set for a URI. > > + * If aCharset is empty remove character-set annotation for aURI. > > s/annotation// > that's an implementation detail that should not matter here > Removed that comment. > @@ +2149,5 @@ > > + Ci.nsIAnnotationService.EXPIRE_NEVER); > > + } else { > > + PlacesUtils.annotations.removePageAnnotation(aURI, this.CHARSET_ANNO); > > + } > > + Services.tm.mainThread.dispatch(function() { > > you should delay the whole thing (apart let deferred and return > deferred.promise clearly), if we set it synchronously and just resolve > asynchronously a test may still pass expecting it to be set as soon as the > call completes. > please also add a comment stating we are delaying this to catch issues with > asynchronous behavior while waiting to implement asynchronous annotations in > bug 699844. delayed the whole thing and added comment > > @@ +2160,5 @@ > > + /** > > + * Gets the last saved character-set for a URI. > > + * > > + * @param aURI nsIURI > > + * @param aCallback the callback method. > > should state it gets the character-set as argument, or null otherwise Updated > > @@ +2162,5 @@ > > + * > > + * @param aURI nsIURI > > + * @param aCallback the callback method. > > + * @return {Promise} > > + * @resolve a character-set > > should also state which value the character-set has if it doesn't exist, I > suppose null may be fine. > Updated > @@ +2179,5 @@ > > + deferred.resolve(charset); > > + } catch (ex) { > > + if (aCallback) { > > + aCallback(); > > + } > > just let charset = null; outside of the try/catch, and invoke > aCallback(charset) and resolve(charset) after it? Done. > > @@ +2182,5 @@ > > + aCallback(); > > + } > > + deferred.resolve(); > > + } > > + }.bind(this), Ci.nsIThread.DISPATCH_NORMAL); > > may avoid bind by useing full PlacesUtils, also cause you do > PlacesUtils.annotations and this.CHARSET_ANNO, so it's not coherent atm. > (ditto for setCharsetForURI) Use PlacesUtils.CHARSET_ANNO instead > > ::: toolkit/components/places/tests/unit/test_384370.js > @@ +55,5 @@ > > populate(); > > > > // 2. run the test-suite > > + Task.spawn(validate).then(function() { > > + promiseAsyncUpdates().then(function testJsonExport() { > > if we could just yield validate(); and yield promiseAsyncUpdates(); here and > below, the code could be a bit more readable Done. > > ::: toolkit/components/places/tests/unit/test_bookmarks_html.js > @@ +272,5 @@ > > root.containerOpen = false; > > } > > } > > > > +/* > > this looks wrong debug commenting > The testImportedBookmarksToFolder() is actually being used. Shall we remove that method or leave it as it was? > ::: toolkit/components/places/tests/unit/test_bookmarks_html_corrupt.js > @@ +186,5 @@ > > // and comparing it, we just check the data size. > > do_check_eq(TEST_FAVICON_DATA_SIZE, aDataLen); > > deferred.resolve(); > > }); > > + yield deferred.promise; > > instead of this I think you may change the test to do > yield Task.spawn(database_check) instead of yield database_check(), and here > you may not need anymore the deferred Removed.

Comment on attachment 722086 [details] [diff] [review] Part I - Async helpers and unit tests v5 Review of attachment 722086 [details] [diff] [review]: ----------------------------------------------------------------- it looks fine now ::: toolkit/components/places/PlacesUtils.jsm @@ +45,5 @@ > + "resource://gre/modules/Task.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, > + "Promise", > + "resource://gre/modules/commonjs/sdk/core/promise.js"); nit: please keep the 2nd argumento on the first line, it's a bit more compact and the most common style around, afaik @@ +2095,5 @@ > + setCharsetForURI: function PU_setCharsetForURI(aURI, aCharset) { > + let deferred = Promise.defer(); > + > + // delaying to catch issues with asynchronous behavior while waiting > + // to implement asynchronous annotations in bug 699844 nit: ucfirst and end with period @@ +2115,5 @@ > + /** > + * Gets the last saved character-set for a URI. > + * > + * @param aURI nsIURI > + * @param aCallback the callback method that retuns a character-set or null. please add [optional] @@ +2128,5 @@ > + > + try { > + charset = > + PlacesUtils.annotations.getPageAnnotation( > + aURI, PlacesUtils.CHARSET_ANNO); too much indentation

Updated based on comments in comment 20

mak: do you think we should land the part I, II and III patches first and then fix other dependency bugs?

This patch couldn't apply clearly so updated it.

Part I, II and III passed try https://tbpl.mozilla.org/?tree=Try&rev=e6b11d76a7bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/13ab0d3009cf https://hg.mozilla.org/integration/mozilla-inbound/rev/7c24f1f46721 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5deb8171b04

https://hg.mozilla.org/mozilla-central/rev/13ab0d3009cf https://hg.mozilla.org/mozilla-central/rev/7c24f1f46721 https://hg.mozilla.org/mozilla-central/rev/f5deb8171b04

please close this bug and move part IV elsewhere Also, please file a new bug to remove the callback from getCharsetForURI, in the end we don't need both the callback and the promise, since with the promise you may do getCharsetForURI(...).then(callback)

(In reply to Marco Bonardo [:mak] (Away Mar 27 - Apr 2) from comment #31) > please close this bug and move part IV elsewhere > Also, please file a new bug to remove the callback from getCharsetForURI, in > the end we don't need both the callback and the promise, since with the > promise you may do getCharsetForURI(...).then(callback) Filed bug 854925 and bug 854927