IndexedDB: IDBTransaction::abort() should throw DOMException if "finished"

[Xingnan Wang]

As description in http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBTransaction-abort-void

[Xingnan Wang]

Created attachment 153971 [details]
Patch

[Kentaro Hara]

Comment on attachment 153971 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Please add the spec link to this ChangeLog.

> Source/WebCore/ChangeLog:8
> +        Update storage/indexeddb/resources/transaction-abort.js to cover this case.

In WebKit convention, we write this line like this:

    Test: storage/indexeddb/transaction-abort.html

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:196
> +void IDBTransaction::abort(ExceptionCode& ec)

We do not want two abort() methods if possible. How about just changing the current 'void abort()' to 'void abort(ExceptionCode& ec)'?

[Xingnan Wang]

Comment on attachment 153971 [details]
Patch

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

Hi Kentaro,
    Thanks for your so quick response.

>> Source/WebCore/ChangeLog:7
>> +
> 
> Please add the spec link to this ChangeLog.

All right.

>> Source/WebCore/ChangeLog:8
>> +        Update storage/indexeddb/resources/transaction-abort.js to cover this case.
> 
> In WebKit convention, we write this line like this:
> 
>     Test: storage/indexeddb/transaction-abort.html

Got it.

>> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:196
>> +void IDBTransaction::abort(ExceptionCode& ec)
> 
> We do not want two abort() methods if possible. How about just changing the current 'void abort()' to 'void abort(ExceptionCode& ec)'?

abort() also needs to be called internally without throw exception, so I add another abort(ExceptionCode& ec) for external calling. If I just change abort() to abort(ExceptionCode& ec), all the points of call abort() need to be modified.

[Xingnan Wang]

Created attachment 153978 [details]
Patch

[Kentaro Hara]

(In reply to comment #3)
> abort() also needs to be called internally without throw exception, so I add another abort(ExceptionCode& ec) for external calling. If I just change abort() to abort(ExceptionCode& ec), all the points of call abort() need to be modified.

Makes sense.

The patch LGTM. If jsbell@ says OK, I'm happy to r+ it.

[Xingnan Wang]

(In reply to comment #5)
> (In reply to comment #3)
> > abort() also needs to be called internally without throw exception, so I add another abort(ExceptionCode& ec) for external calling. If I just change abort() to abort(ExceptionCode& ec), all the points of call abort() need to be modified.
> 
> Makes sense.
> 
> The patch LGTM. If jsbell@ says OK, I'm happy to r+ it.

Thank you.

[Joshua Bell]

Comment on attachment 153978 [details]
Patch

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

Another nice catch. LGTM with one minor tweak.

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:82
> +    bool abort();

Can you move this out of the "Implement the IDBTransaction IDL" set of methods, down with "setActive" and other non-IDL methods?

[Joshua Bell]

(In reply to comment #3)
> > We do not want two abort() methods if possible. How about just changing the current 'void abort()' to 'void abort(ExceptionCode& ec)'?
> 
> abort() also needs to be called internally without throw exception, so I add another abort(ExceptionCode& ec) for external calling. If I just change abort() to abort(ExceptionCode& ec), all the points of call abort() need to be modified.

We could name the new IDL method implementation something else (e.g. abortMethod ? I can't think of anything good right now) and add an IDL attribute like we do for methods where there's a C++ keyword collision, e.g. for IDBCursor.continue:

 [ImplementedAs=continueFunction] void continue( ... ) raises ( ... );

I'd also be fine with renaming the "internal" abort something else. (We have a foo/fooInternal naming convention, but that's usually for private static impls of callbacks.)

[Alec Flett]

I vote for a single abort() - change all the internal callers to use it, and ignore the result:

ExceptionCode unused;
transaction->abort(unused)

[Joshua Bell]

(In reply to comment #9)
> I vote for a single abort() - change all the internal callers to use it, and ignore the result:
> 
> ExceptionCode unused;
> transaction->abort(unused)

That sounds good to me as well, possibly with a comment in the abort() code that it is idempotent for internal callers if they ignore the exception code.

[Xingnan Wang]

Created attachment 154260 [details]
Patch

[Xingnan Wang]

Created attachment 154262 [details]
Patch

[Kentaro Hara]

Comment on attachment 154262 [details]
Patch

Looks OK

[WebKit Review Bot]

Comment on attachment 154262 [details]
Patch

Clearing flags on attachment: 154262

Committed r123698: <http://trac.webkit.org/changeset/123698>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.