Closed
Bug 1484615
Opened 4 years ago
Closed 4 years ago
Followup comment fix for bug 1409641
Categories
(Core :: Storage: IndexedDB, enhancement, P2)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: janv, Assigned: tt)
References
Details
Attachments
(1 file, 3 obsolete files)
2.83 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
The commit message for bug 1409641 was a bit confusing. I think we should add a comment to IndexedDBHelper.jsm to explain how txn.result is being set.
Reporter | ||
Updated•4 years ago
|
Assignee: nobody → shes050117
Reporter | ||
Comment 1•4 years ago
|
||
Tom, we should add these comments to newTxn() in IndexedDBHelper.jsm: 1. before |if (successCb) {| in txn.oncomplete /* * txn.result is not part of the transaction object returned by * this._db.transaction method called above. * The attribute is expected to be set by callers of newTxn method. * It may be null. */ 2. before |if (failureCb) {| in txn.onabort /* * txn.error is part of the transaction object returned by * this._db.transaction method called above. * The attribute is defined in IDBTranscation WebIDL interface. * It may be null. */ Commit message: Bug 1484615 - Followup comment and commit message fix for bug 1409641
Updated•4 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•4 years ago
|
||
Add comments to IndexedDB.jsm based on comment 1. Please note that I add one line more and modify the comment's format.
Attachment #9002404 -
Flags: review?(jvarga)
Reporter | ||
Comment 3•4 years ago
|
||
Comment on attachment 9002404 [details] [diff] [review] Bug 1484615: Add comments to IndexedDBHelper.jsm to clarify how is txn.result set. Review of attachment 9002404 [details] [diff] [review]: ----------------------------------------------------------------- Your commit message is: "Bug 1484615: Add comments to IndexedDBHelper.jsm to clarify how is txn.result set." However, I wanted to mention that this patch somehow tries to fix previous commit message which was: "Get the result only when it's defined in IDBtranscation" That can be confusing since IDBTransaction doesn't contain any result attribute. So I suggested this commit message: Bug 1484615 - Followup comment and commit message fix for bug 1409641 which explicitly mentions bug 1409641 What about this multi line commit message: Bug 1484615 - Followup comment fix for bug 1409641; r=janv This patch add comments to IndexedDBHelper.jsm to clarify how txn.result is set. The comments should clear up any confusion caused by previous commit message (fix for bug 1409641). ::: dom/base/IndexedDBHelper.jsm @@ +161,5 @@ > } > > txn.oncomplete = function () { > if (DEBUG) debug("Transaction complete. Returning to callback."); > + // txn.result is not part of the transaction object returned by I wonder why you changed it to "//" when everything else in the file uses: /* * */ @@ +164,5 @@ > if (DEBUG) debug("Transaction complete. Returning to callback."); > + // txn.result is not part of the transaction object returned by > + // this._db.transaction method called above. > + // The attribute is expected to be set by callers of newTxn method. > + // It may be null. I'm going to correct myself here. "It may be null" should be replaced with "It may not be defined on txn object."
Attachment #9002404 -
Flags: review?(jvarga)
Assignee | ||
Comment 4•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #3) Thanks for the review! I will address the comment. > Comment on attachment 9002404 [details] [diff] [review] > Bug 1484615: Add comments to IndexedDBHelper.jsm to clarify how is > txn.result set. > > Review of attachment 9002404 [details] [diff] [review]: > ----------------------------------------------------------------- > > Your commit message is: > "Bug 1484615: Add comments to IndexedDBHelper.jsm to clarify how is > txn.result set." > > However, I wanted to mention that this patch somehow tries to fix previous > commit message which was: > "Get the result only when it's defined in IDBtranscation" > > That can be confusing since IDBTransaction doesn't contain any result > attribute. > > So I suggested this commit message: > Bug 1484615 - Followup comment and commit message fix for bug 1409641 > > which explicitly mentions bug 1409641 > > > What about this multi line commit message: > Bug 1484615 - Followup comment fix for bug 1409641; r=janv > > This patch add comments to IndexedDBHelper.jsm to clarify how txn.result is > set. > The comments should clear up any confusion caused by previous commit message > (fix for bug 1409641). Will do and I'll try to make my commit message succinct next time. > ::: dom/base/IndexedDBHelper.jsm > @@ +161,5 @@ > > } > > > > txn.oncomplete = function () { > > if (DEBUG) debug("Transaction complete. Returning to callback."); > > + // txn.result is not part of the transaction object returned by > > I wonder why you changed it to "//" when everything else in the file uses: > /* > * > */ I checked other .jsm files under dom (e.g. https://searchfox.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm) and I found some of them uses "// " for their comment inside the function while having "/**/" above the function. Besides, I thought it's better to differentiate comments above for explaining the overall concept of a function and comments for explaining details of the code. However, I don't have a strong preference for that, so I will change that to "/* */". > @@ +164,5 @@ > > if (DEBUG) debug("Transaction complete. Returning to callback."); > > + // txn.result is not part of the transaction object returned by > > + // this._db.transaction method called above. > > + // The attribute is expected to be set by callers of newTxn method. > > + // It may be null. > > I'm going to correct myself here. "It may be null" should be replaced with > "It may not be defined on txn object." Will do.
Assignee | ||
Comment 5•4 years ago
|
||
Will send it to review after re-checking one more time.
Attachment #9002404 -
Attachment is obsolete: true
Reporter | ||
Comment 6•4 years ago
|
||
(In reply to Tom Tung [:tt] from comment #4) > I checked other .jsm files under dom (e.g. > https://searchfox.org/mozilla-central/source/dom/base/DOMRequestHelper.jsm) > and I found some of them uses "// " for their comment inside the function > while having "/**/" above the function. Besides, I thought it's better to > differentiate comments above for explaining the overall concept of a > function and comments for explaining details of the code. However, I don't > have a strong preference for that, so I will change that to "/* */". Yeah, DOMRequestHelper.jsm looks like that, but most of other JS files use both "//" and "/**/" depending on patch author probably. IndexedDBHelper.jsm clearly had /* */ inside functions too: https://hg.mozilla.org/mozilla-central/rev/3f0e23762df5 It's not a big deal, we're not going to fix all other files for sure. It just looked natural to me, since we removed a comment in the previous commit and it had /**/ which was consistent wit rest of the file.
Assignee | ||
Comment 7•4 years ago
|
||
Address the comment and match comments between inside and outside of the function.
Attachment #9002420 -
Attachment is obsolete: true
Attachment #9002454 -
Flags: review?(jvarga)
Reporter | ||
Comment 8•4 years ago
|
||
Comment on attachment 9002454 [details] [diff] [review] Bug 1484615 - Followup comment fix for bug 1409641; r=janv Review of attachment 9002454 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/IndexedDBHelper.jsm @@ +182,5 @@ > + /* > + * txn.error is part of the transaction object returned by > + * this._db.transaction method called above. > + * The attribute is defined in IDBTranscation WebIDL interface. > + * It may not be defined on txn object. Tom, can you check the last block of comment 3 ? I wrote: """ @@ +164,5 @@ > if (DEBUG) debug("Transaction complete. Returning to callback."); > + // txn.result is not part of the transaction object returned by > + // this._db.transaction method called above. > + // The attribute is expected to be set by callers of newTxn method. > + // It may be null. I'm going to correct myself here. "It may be null" should be replaced with "It may not be defined on txn object." """ That was clearly meant for the oncomplete function, but you replaced it in the onabort function. I'll send a new patch with exact wording.
Attachment #9002454 -
Flags: review?(jvarga)
Reporter | ||
Comment 9•4 years ago
|
||
I reworked the comments to make it even clearer how txn.result is set and what is the difference between undefined object property and null property.
Attachment #9002454 -
Attachment is obsolete: true
Attachment #9002511 -
Flags: review+
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #8) > the onabort function. > > I'll send a new patch with exact wording. Oh, I'm really sorry for not finding that in advance ... I'll make sure that won't happen again
Reporter | ||
Comment 11•4 years ago
|
||
Comment on attachment 9002511 [details] [diff] [review] patch Review of attachment 9002511 [details] [diff] [review]: ----------------------------------------------------------------- Tom, you can land this patch.
Assignee | ||
Comment 12•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #11) Sure. Since it only modifies comments, I will only test it locally without pushing to try before landing it.
Comment 13•4 years ago
|
||
Pushed by shes050117@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dcbb7762dc2f Followup comment fix for bug 1409641; r=janv
Comment 14•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dcbb7762dc2f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•