Closed Bug 984015 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: tetsuharu, Assigned: tetsuharu)

References

Details

Attachments

(4 files, 3 obsolete files)

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 => {")
Attachment #8391738 - Flags: review?(mak77) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Updated!
Attachment #8391738 - Attachment is obsolete: true
Attachment #8394137 - Flags: review?(mak77)
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
Attachment #8394137 - Flags: review?(mak77) → review+
Attached patch check-in (obsolete) — Splinter Review
(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!
Attachment #8394137 - Attachment is obsolete: true
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
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.
Attachment #8395425 - Flags: review?(mak77)
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 :/
Attachment #8395424 - Flags: review?(mak77) → review+
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.
Attachment #8395425 - Flags: review?(mak77) → review+
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.
Attachment #8395426 - Flags: review?(mak77) → review+
please, coalesce the patches into one before landing, to make tree management easier for sheriffs.
https://hg.mozilla.org/mozilla-central/rev/85ca984dc8a2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 993084
No longer depends on: 993084
You need to log in before you can comment on or make changes to this bug.