1484615 - Followup comment fix for bug 1409641

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, enhancement, P2)

Description

[Jan Varga [:janv]]

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.

Comment 1

[Jan Varga [:janv]]

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

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1484615: Add comments to IndexedDBHelper.jsm to clarify how is txn.result set.

Add comments to IndexedDB.jsm based on comment 1. Please note that I add one line more and modify the comment's format.

Comment 3

[Jan Varga [:janv]]

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

Comment 4

[Tom Tung [:tt, :ttung]]

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

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1484615 - Followup comment fix for bug 1409641; r=janv

Will send it to review after re-checking one more time.

Comment 6

[Jan Varga [:janv]]

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

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1484615 - Followup comment fix for bug 1409641; r=janv

Address the comment and match comments between inside and outside of the function.

Comment 8

[Jan Varga [:janv]]

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.

Comment 9

[Jan Varga [:janv]]

Attachment: patch

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.

Comment 10

[Tom Tung [:tt, :ttung]]

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

[Jan Varga [:janv]]

Comment on attachment 9002511 [details] [diff] [review]
patch

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

Tom, you can land this patch.

Comment 12

[Tom Tung [:tt, :ttung]]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcbb7762dc2f
Followup comment fix for bug 1409641; r=janv

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/dcbb7762dc2f