Closed
Bug 984015
Opened 7 years ago
Closed 7 years ago
GUIDHelper.getItemId should deliver `this` context to his internal executeAsync callback.
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: tetsuharu, Assigned: tetsuharu)
References
Details
Attachments
(4 files, 3 obsolete files)
930 bytes,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
1.79 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
Details | Diff | Splinter Review |
[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|.
Assignee | ||
Comment 1•7 years ago
|
||
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
Attachment #8391738 -
Flags: review?(mak77)
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
Updated!
Attachment #8391738 -
Attachment is obsolete: true
Attachment #8394137 -
Flags: review?(mak77)
Comment 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
(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 | ||
Comment 6•7 years ago
|
||
land to fx-team: https://hg.mozilla.org/integration/fx-team/rev/4774010c55bd
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
Comment 7•7 years ago
|
||
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
Assignee | ||
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8395374 -
Attachment is obsolete: true
Attachment #8395424 -
Flags: review?(mak77)
Assignee | ||
Comment 10•7 years ago
|
||
I think that It's better we should resolve the returned promise after all needed operations are finished.
Attachment #8395425 -
Flags: review?(mak77)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8395426 -
Flags: review?(mak77)
Assignee | ||
Comment 12•7 years ago
|
||
try: https://tbpl.mozilla.org/?tree=Try&rev=c625f3d17dec
Comment 13•7 years ago
|
||
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 14•7 years ago
|
||
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 15•7 years ago
|
||
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+
Comment 16•7 years ago
|
||
please, coalesce the patches into one before landing, to make tree management easier for sheriffs.
Assignee | ||
Comment 17•7 years ago
|
||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/85ca984dc8a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•