I'm not sure whether distribution is async-friendly...

Now that DistributionCustomizer.applyBookmarks() is async, should nsBrowserGlue.ensurePlacesDefaultQueriesInitialized() only be called after that is finished?

Comment on attachment 8577986 [details] [diff] [review] 0001-Bug-1094886-Make-distribution.js-use-the-new-Bookmar.patch Review of attachment 8577986 [details] [diff] [review]: ----------------------------------------------------------------- Yes, I think it's worth to yield in nsBrowserGlue where we have .applyBookmarks() calls. Luckily I just changed that code to yield sequentially in the keywords patch and looks like it's working (or at least I hope so!). please ensure to run the tests in browser/components/places, some of them go through this code path. better to get a green Try run. ::: browser/components/distribution.js @@ +90,4 @@ > let keys = []; > for (let i in enumerate(this._ini.getKeys(section))) > keys.push(i); > keys.sort(); while here, probably this could use a comprehension like keys = [for (i in enumerate(this._ini.getKeys(section)))].sort(); @@ +93,5 @@ > keys.sort(); > > let items = {}; > let defaultItemId = -1; > let maxItemId = -1; I know it's out of scope for this bug, but this code is so unreadable that makes my eyes bleed. Could you please rename iid to itemIndex, defaultItemId to defaultIndex and maxItemId to maxIndex? I really can't read this code with such terrible naming. Anything would be an improvement here. @@ +178,1 @@ > PlacesUtils.livemarks.addLivemark({ title: items[iid]["title"] I'll file a bug to add parentGuid support to livemarks and deprecate passing parentId (bug 1145653) For now this is ok. Annotations is a different story since there ideally we want a new implementation (bug 699844)

Made all the requested changes. Tests are passing.

Comment on attachment 8586750 [details] [diff] [review] 0001-Bug-1094886-Make-distribution.js-use-the-new-Bookmar.patch, v2 Review of attachment 8586750 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/distribution.js @@ +170,5 @@ > index = prependIndex++; > > // Don't bother updating the livemark contents on creation. > + let parentId = yield PlacesUtils.promiseItemId(parentGuid); > + PlacesUtils.livemarks.addLivemark({ title: items[itemIndex]["title"] rather than catching I'd probably yield now that we can. @@ +374,5 @@ > } > > function enumToObject(UTF8Enumerator) { > let ret = {}; > + for (let i of enumerate(UTF8Enumerator)) I didn't verify if this is correct for whatever UTF8Enumerator is, I assume you did.

(In reply to Marco Bonardo [::mak] from comment #4) > > // Don't bother updating the livemark contents on creation. > > + let parentId = yield PlacesUtils.promiseItemId(parentGuid); > > + PlacesUtils.livemarks.addLivemark({ title: items[itemIndex]["title"] > > rather than catching I'd probably yield now that we can. Good point, totally missed that. > > function enumToObject(UTF8Enumerator) { > > let ret = {}; > > + for (let i of enumerate(UTF8Enumerator)) > > I didn't verify if this is correct for whatever UTF8Enumerator is, I assume > you did. Well I changed enumerate() to be real generator now and so I had to change enumToObject() to use for...of with enumerate(). BUT I just realized I'll have to change all the other places where we still use for...in, will do before landing. Looks like we have no real tests for those parts.

Tim, please provide a bit of context for this fix, so that QA can verify it.

Marco, do we really need manual verification here? We have a few tests covering most of the code. If we do, how would we do that? Would we need to customize distribution.ini and package it with an official build?

bookmarks import in ditribution has a test, considered the difficulty in doing that manually we can avoid the qe. sorry for setting the flag wrongly.