Bug 100700 - IndexedDB: Remove unused error handling clauses when writing to transaction
Summary: IndexedDB: Remove unused error handling clauses when writing to transaction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 94861
  Show dependency treegraph
 
Reported: 2012-10-29 14:45 PDT by Joshua Bell
Modified: 2012-11-08 13:38 PST (History)
6 users (show)

See Also:


Attachments
Patch (17.00 KB, patch)
2012-10-29 14:49 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (25.11 KB, patch)
2012-10-30 11:15 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (25.07 KB, patch)
2012-10-30 12:05 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (25.06 KB, patch)
2012-11-05 16:57 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (27.67 KB, patch)
2012-11-08 12:44 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-10-29 14:45:57 PDT
IndexedDB: Remove unused error handling clauses when writing to transaction
Comment 1 Joshua Bell 2012-10-29 14:49:12 PDT
Created attachment 171315 [details]
Patch
Comment 2 Joshua Bell 2012-10-29 14:50:41 PDT
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.
Comment 3 WebKit Review Bot 2012-10-29 15:13:13 PDT
Comment on attachment 171315 [details]
Patch

Attachment 171315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14627288
Comment 4 Peter Beverloo (cr-android ews) 2012-10-29 15:17:15 PDT
Comment on attachment 171315 [details]
Patch

Attachment 171315 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14618878
Comment 5 Joshua Bell 2012-10-30 11:15:49 PDT
Created attachment 171486 [details]
Patch
Comment 6 Joshua Bell 2012-10-30 11:20:35 PDT
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?
Comment 7 Peter Beverloo (cr-android ews) 2012-10-30 11:58:54 PDT
Comment on attachment 171486 [details]
Patch

Attachment 171486 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14632595
Comment 8 Joshua Bell 2012-10-30 12:05:54 PDT
Created attachment 171492 [details]
Patch
Comment 9 David Grogan 2012-11-05 15:41:55 PST
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?
Comment 10 Alec Flett 2012-11-05 16:46:25 PST
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?
Comment 11 Joshua Bell 2012-11-05 16:56:37 PST
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.
Comment 12 Joshua Bell 2012-11-05 16:57:11 PST
Created attachment 172443 [details]
Patch
Comment 13 Joshua Bell 2012-11-05 16:57:30 PST
tony@ - r?
Comment 14 Joshua Bell 2012-11-07 09:41:53 PST
dglazkov@ - tony's out, would you mind r?
Comment 15 Joshua Bell 2012-11-08 12:44:04 PST
Created attachment 173098 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-11-08 13:38:28 PST
Comment on attachment 173098 [details]
Patch for landing

Clearing flags on attachment: 173098

Committed r133940: <http://trac.webkit.org/changeset/133940>
Comment 17 WebKit Review Bot 2012-11-08 13:38:32 PST
All reviewed patches have been landed.  Closing bug.