Bug 60186 – Port Mozilla's IndexedDB tests: transaction lifetimes

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Mark Pilgrim (Google)]

http://mxr.mozilla.org/mozilla2.0/source/dom/indexedDB/test/test_transaction_lifetimes.html?force=1

This is a port of a test from Mozilla's IndexedDB test suite. It checks that transactions expire upon completion, i.e. that they can not be reused once they are complete.

WebKit passes this test.

[Mark Pilgrim (Google)]

Created attachment 92284 [details]
Patch

[David Grogan]

Comment on attachment 92284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92284&action=review

> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:56
> +    transaction = evalAndLog("transaction = db.transaction('foo');");

I think a transaction with nothing to do aborts; could you add an onabort handler with testPassed("empty transaction aborted") or something similar?

[Mark Pilgrim (Google)]

I don't see anything in the spec to back up that assertion. Also, WebKit does not appear to trigger onabort in this test.

[David Grogan]

(In reply to comment #2)
> (From update of attachment 92284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92284&action=review
> 
> > LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:56
> > +    transaction = evalAndLog("transaction = db.transaction('foo');");
> 
> I think a transaction with nothing to do aborts; could you add an onabort handler with testPassed("empty transaction aborted") or something similar?

(In reply to comment #3)
> I don't see anything in the spec to back up that assertion. Also, WebKit does not appear to trigger onabort in this test.

I was referring to behavior described here:
http://trac.webkit.org/browser/trunk/LayoutTests/storage/indexeddb/tutorial.html#L209

If onabort doesn't get called, could you add onabort = unexpectedAbortCalled (or whatever its name is)?

[David Grogan]

Comment on attachment 92284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92284&action=review

> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:53
> +    evalAndLog("event.target.transaction.oncomplete = checkTransaction;");

What is event.target?  db?

[Mark Pilgrim (Google)]

(In reply to comment #3)
> I don't see anything in the spec to back up that assertion. Also, WebKit does not appear to trigger onabort in this test.

I was wrong on both counts. Updating test as you originally suggested.

[Mark Pilgrim (Google)]

(In reply to comment #5)
> (From update of attachment 92284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92284&action=review
> 
> > LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:53
> > +    evalAndLog("event.target.transaction.oncomplete = checkTransaction;");
> 
> What is event.target?  db?

Yes.

[Mark Pilgrim (Google)]

Created attachment 93985 [details]
Patch

[David Grogan]

Comment on attachment 93985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=93985&action=review

informal r+, pending additional comment about transaction inside setVersion transaction

> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:56
> +    transaction = evalAndLog("transaction = db.transaction('foo');");

Sorry, one last request.  Can you add a comment that this test might have to be refactored when http://www.w3.org/Bugs/Public/show_bug.cgi?id=11401 is resolved?  It seems as though creating a transaction inside a setVersion transaction will be forbidden, based on the latest discussion: http://lists.w3.org/Archives/Public/public-webapps/2011AprJun/0608.html

[Mark Pilgrim (Google)]

Created attachment 94209 [details]
Patch

[Mark Pilgrim (Google)]

(In reply to comment #10)
> Created an attachment (id=94209) [details]
> Patch

This adds a debug statement linking to the unresolved WG bug.

[David Grogan]

r+

Thanks

[David Levin]

Comment on attachment 94209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94209&action=review

> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:1
> +<!DOCTYPE html>

Where is the layoutTestController.dumpAsText() call?

> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:6
> +      http://creativecommons.org/publicdomain/zero/1.0/ "

This license isn't one of the two that is allowed. Perhaps you can ask if this is ok on webkit-dev.

[David Levin]

Comment on attachment 94209 [details]
Patch

r- pending resolution of my two comments.

[Tony Chang]

Comment on attachment 94209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94209&action=review

>> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:6
>> +      http://creativecommons.org/publicdomain/zero/1.0/ "
> 
> This license isn't one of the two that is allowed. Perhaps you can ask if this is ok on webkit-dev.

It's public domain, so it has no copyright/license (e.g., like sqlite).

[Mark Pilgrim (Google)]

(In reply to comment #13)
> (From update of attachment 94209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94209&action=review
> 
> > LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:1
> > +<!DOCTYPE html>
> 
> Where is the layoutTestController.dumpAsText() call?

None of the other IndexedDB tests (mine or existing ones) have this call.

> 
> > LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:6
> > +      http://creativecommons.org/publicdomain/zero/1.0/ "
> 
> This license isn't one of the two that is allowed. Perhaps you can ask if this is ok on webkit-dev.

Is Tony's comment sufficient?

[Tony Chang]

Comment on attachment 94209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94209&action=review

>>> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:1
>>> +<!DOCTYPE html>
>> 
>> Where is the layoutTestController.dumpAsText() call?
> 
> None of the other IndexedDB tests (mine or existing ones) have this call.

It's in js-test-pre.js.

[David Grogan]

Mark, it looks like this patch was forgotten.  Can it be committed if Dave Levin is satisfied?

[David Levin]

(In reply to comment #15)
> (From update of attachment 94209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94209&action=review
> 
> >> LayoutTests/storage/indexeddb/mozilla/transaction-lifetimes.html:6
> >> +      http://creativecommons.org/publicdomain/zero/1.0/ "
> > 
> > This license isn't one of the two that is allowed. Perhaps you can ask if this is ok on webkit-dev.
> 
> It's public domain, so it has no copyright/license (e.g., like sqlite).

As I suggested, perhaps you can ask on webkit-dev because this doesn't seem complaint to me.

When one becomes a committer, one signs a document saying that they will only let in code that is one of those two licenses, It also says:

  "If a contributed source file does not have any licensing terms attached to it, do not commit it to the 
repository. Instead, please contact the author to determine what the intended licensing terms are.  If you need assistance with this, contact the WebKit team."

So this code doesn't seem acceptable. I'll admit that these license issues simply confuse me -- personally I wish it was all simpler -- which is why I mentioned emailing webkit-dev previously for clarification because I believe others there can help clarify this (while I cannot).

[Joshua Bell]

This will be redundant with https://bugs.webkit.org/show_bug.cgi?id=90412 (which also changes the semantics of an empty transaction from aborting to completing, which makes this test invalid)