Bug 100700 – IndexedDB: Remove unused error handling clauses when writing to transaction

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

[Joshua Bell]

IndexedDB: Remove unused error handling clauses when writing to transaction

[Joshua Bell]

Created attachment 171315 [details]
Patch

[Joshua Bell]

alecflett@, dgrogan@ - what do you think?

This simplifies some of the code in IDBLevelDBBackingStore but it's not a significant win overall. I also worry that if we ever let transactions spill to disk we'd need to resurrect the error handlers.

[WebKit Review Bot]

Comment on attachment 171315 [details]
Patch

Attachment 171315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14627288

[Peter Beverloo (cr-android ews)]

Comment on attachment 171315 [details]
Patch

Attachment 171315 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14618878

[Joshua Bell]

Created attachment 171486 [details]
Patch

[Joshua Bell]

New patch:
* Removes unused non-template functions (which failed compile)
* static function deleteRange() only writes to transaction so can be void
* Made setUpMetadata, getNewDatabaseId, and createIDBDatabaseMetaData write via transactions - now all writing is via transactions, which simplifies error cases.

This should make webkit.org/b/94861 and related cases where a file system error occurs easier handle properly.

So... I'm now in favor of going forward with this.

alecflett@, dgrogan@ - thoughts?

[Peter Beverloo (cr-android ews)]

Comment on attachment 171486 [details]
Patch

Attachment 171486 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14632595

[Joshua Bell]

Created attachment 171492 [details]
Patch

[David Grogan]

Comment on attachment 171492 [details]
Patch

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

LGTM

> Source/WebCore/ChangeLog:15
> +        (WebCore::putBool): Fork into "put to database" vs. "put to transaction" variants

Seems like "put to database" was removed altogether?

[Alec Flett]

Comment on attachment 171492 [details]
Patch

generally lgtm, one minor nit:

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

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:972
> +    return true;

Is it worth simplifying this return path too?

[Joshua Bell]

Comment on attachment 171492 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        (WebCore::putBool): Fork into "put to database" vs. "put to transaction" variants
> 
> Seems like "put to database" was removed altogether?

Yeah, older iteration. Comment updated.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:972
>> +    return true;
> 
> Is it worth simplifying this return path too?

That API is exposed in IDBBackingStore so I left it alone for this first pass.

[Joshua Bell]

Created attachment 172443 [details]
Patch

[Joshua Bell]

tony@ - r?

[Joshua Bell]

dglazkov@ - tony's out, would you mind r?

[Joshua Bell]

Created attachment 173098 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173098 [details]
Patch for landing

Clearing flags on attachment: 173098

Committed r133940: <http://trac.webkit.org/changeset/133940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.