[Environment] - https://hg.mozilla.org/mozilla-central/rev/82c90c17fc95 By http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1705, This |this| should be GUIDHelper, but this code don't delivered `GUIDHelper` as |this|.

These method should deliver itself as |this|: - GUIDHelper.getItemId: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1705 - GUIDHelper.getItemGUID: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1739

Comment on attachment 8391738 [details] [diff] [review] patch v1 Review of attachment 8391738 [details] [diff] [review]: ----------------------------------------------------------------- ugh! thanks for catching that. please use an arrow function instead of bind (i.e. "handleCompletion: aReason => {")

Updated!

Comment on attachment 8394137 [details] [diff] [review] patch v2 Review of attachment 8394137 [details] [diff] [review]: ----------------------------------------------------------------- you don't need a further review, the following comments are not important, if you want to fix them fine, otherwise it's still ok :) just attach a patch with proper check-in comment (missing r=mak) and proceed. Thank you. ::: toolkit/components/places/PlacesUtils.jsm @@ +1697,5 @@ > let row = aResultSet.getNextRow(); > if (row) > itemId = row.getResultByIndex(0); > }, > + handleCompletion: (aReason) => { nit: when you have only one argument you can omit the rounded parentheses handleCompletion: aReason => { @@ +1731,5 @@ > if (row) { > guid = row.getResultByIndex(1); > } > }, > + handleCompletion: (aReason) => { ditto

(In reply to Marco Bonardo [:mak] from comment #4) > you don't need a further review, the following comments are not important, > if you want to fix them fine, otherwise it's still ok :) > > just attach a patch with proper check-in comment (missing r=mak) and proceed. Ok. Thank you!

land to fx-team: https://hg.mozilla.org/integration/fx-team/rev/4774010c55bd

Backed out in https://hg.mozilla.org/integration/fx-team/rev/40439a58acfd for xpcshell bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=36573400&tree=Fx-Team

I found that the xpcshell bustage is caused by the bug which GUIDHelper.getItemGUID() doesn't return a guid (https://hg.mozilla.org/integration/fx-team/file/40439a58acfd/toolkit/components/places/PlacesUtils.jsm#l1722). So I have written some patches & I'll attach them.

I think that It's better we should resolve the returned promise after all needed operations are finished.

try: https://tbpl.mozilla.org/?tree=Try&rev=c625f3d17dec

Comment on attachment 8395424 [details] [diff] [review] part1: GUIDHelper.getItemGUID() must return a GUID Review of attachment 8395424 [details] [diff] [review]: ----------------------------------------------------------------- yeah, this path was not tested before cause we were never adding guids to the map... this was definitely a typo :/

Comment on attachment 8395425 [details] [diff] [review] part2: GUIDHelper should resolve promises after other operations are finished Review of attachment 8395425 [details] [diff] [review]: ----------------------------------------------------------------- this is not mandatory, cause the promise regardless is resolved on the next tick (so it would still run after that code). Though I agree this way we can ensure the previous code works fine, increasting test coverage.

Comment on attachment 8395426 [details] [diff] [review] part3: GUIDHelper.getItemId should deliver itself as |this| Review of attachment 8395426 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! sorry for not having noticed the typo earlier.

please, coalesce the patches into one before landing, to make tree management easier for sheriffs.