Followup comment fix for bug 1409641

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: janv, Assigned: tt)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Assignee: nobody → shes050117
Depends on: 1409641
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
Priority: -- → P2
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)
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)
(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.
Will send it to review after re-checking one more time.
Attachment #9002404 - Attachment is obsolete: true
(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.
Address the comment and match comments between inside and outside of the function.
Attachment #9002420 - Attachment is obsolete: true
Attachment #9002454 - Flags: review?(jvarga)
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)
Posted patch patchSplinter Review
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+
(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
Comment on attachment 9002511 [details] [diff] [review]
patch

Review of attachment 9002511 [details] [diff] [review]:
-----------------------------------------------------------------

Tom, you can land this patch.
(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.
https://hg.mozilla.org/mozilla-central/rev/dcbb7762dc2f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.