984015 - GUIDHelper.getItemId should deliver `this` context to his internal executeAsync callback.

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

[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|.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: patch v1

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 2

[Marco Bonardo [:mak]]

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 => {")

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: patch v2

Updated!

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: check-in

(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!

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

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

Comment 7

[Phil Ringnalda (:philor)]

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

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

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.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: part1: GUIDHelper.getItemGUID() must return a GUID

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: part2: GUIDHelper should resolve promises after other operations are finished

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

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: part3: GUIDHelper.getItemId should deliver itself as |this|

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

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

Comment 13

[Marco Bonardo [:mak]]

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 14

[Marco Bonardo [:mak]]

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 15

[Marco Bonardo [:mak]]

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.

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[Tetsuharu OHZEKI [:tetsuharu] [UTC+9] (a.k.a saneyuki)]

Attachment: merged patch for check-in

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/85ca984dc8a2