Bug 101415 - IndexedDB: Combine IDBBackingStore and IDBLevelDBBackingStore
Summary: IndexedDB: Combine IDBBackingStore and IDBLevelDBBackingStore
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: Alec Flett
URL:
Keywords:
Depends on: 101488 101790
Blocks: 101668
  Show dependency treegraph
 
Reported: 2012-11-06 17:50 PST by Alec Flett
Modified: 2012-11-09 16:02 PST (History)
6 users (show)

See Also:


Attachments
Patch (190.35 KB, patch)
2012-11-06 18:02 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (109.30 KB, patch)
2012-11-07 12:10 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (118.70 KB, patch)
2012-11-08 12:38 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (88.61 KB, patch)
2012-11-08 13:01 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (88.17 KB, patch)
2012-11-08 17:05 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (88.38 KB, patch)
2012-11-09 13:52 PST, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (88.34 KB, patch)
2012-11-09 14:19 PST, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-11-06 17:50:12 PST
IndexedDB: Combine IDBBackingStore and IDBLevelDBBackingStore
Comment 1 Alec Flett 2012-11-06 18:02:38 PST
Created attachment 172687 [details]
Patch
Comment 2 Alec Flett 2012-11-06 18:03:51 PST
jsbell/dgrogan - this is a decent sized refactor, but do you mind taking a look? It's mechanical, but check the changelog for the exact details. Beyond the reduction in code complexity, this also removes roughly 200 lines of code.
Comment 3 Alec Flett 2012-11-06 18:07:31 PST
oh, and jsbell this will most certainly conflict with bug 100700 - I'm happy to wait for that to land and deal with the fallout.
Comment 4 WebKit Review Bot 2012-11-06 18:19:06 PST
Comment on attachment 172687 [details]
Patch

Attachment 172687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14660522
Comment 5 Peter Beverloo (cr-android ews) 2012-11-06 18:38:45 PST
Comment on attachment 172687 [details]
Patch

Attachment 172687 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14732979
Comment 6 David Grogan 2012-11-06 18:40:51 PST
Comment on attachment 172687 [details]
Patch

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

Just a cursory look

> Source/WebCore/ChangeLog:23
> +        owner, IDBCursorBackendImpl.

I think you mean IDBTransactionBackendImpl?

> Source/WebCore/ChangeLog:25
> +        4) Shortened the name of a few classes/methods (e.g.

Could you do 4, 5, and 6 in separate patches?  Maybe 3 too?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:-408
> -        backingStoreCursor->close();

This line seems scary, why don't we have to close the backingStoreCursor anymore?
Comment 7 Build Bot 2012-11-06 19:31:38 PST
Comment on attachment 172687 [details]
Patch

Attachment 172687 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14760198
Comment 8 EFL EWS Bot 2012-11-06 20:23:24 PST
Comment on attachment 172687 [details]
Patch

Attachment 172687 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14731952
Comment 9 Alec Flett 2012-11-07 12:10:25 PST
Created attachment 172856 [details]
Patch
Comment 10 Alec Flett 2012-11-07 12:13:41 PST
ok, I've broken this up into 3 patches. The first 2 are reasonably simple, and this bug has the "final" bit that couldn't be split up any further... but as you'll see it is way easier to read now.

Regarding closing the cursor - so IDBLevelDBBackingStore::Cursor::close() was a no-op - so I nuked it - if anything it demonstrates why having so many layers of abstraction and interfaces obfuscate and clutter up other code - it's hard for us to figure out what parts of what interfaces are actually dead or not!
Comment 11 Alec Flett 2012-11-08 12:14:28 PST
Comment on attachment 172856 [details]
Patch

jsbell/dgrogan - care to take a look?
Comment 12 Alec Flett 2012-11-08 12:14:52 PST
(FYI, this patch won't apply cleanly to chromium until the next webkit roll)
Comment 13 Alec Flett 2012-11-08 12:38:47 PST
Created attachment 173096 [details]
Patch
Comment 14 Alec Flett 2012-11-08 13:01:49 PST
Created attachment 173101 [details]
Patch
Comment 15 Alec Flett 2012-11-08 13:02:33 PST
my changelogs went haywire in the last patch. I also fixed the histogram enums back the way they were. Patch is otherwise the same.
Comment 16 Alec Flett 2012-11-08 17:05:46 PST
Created attachment 173153 [details]
Patch

Update after bug 100700 landed
Comment 17 David Grogan 2012-11-08 18:15:20 PST
LGTM
Comment 18 Alec Flett 2012-11-08 18:30:36 PST
tony@ - another refactor for ya. r?
Comment 19 Tony Chang 2012-11-09 09:45:59 PST
Comment on attachment 173153 [details]
Patch

rs=me
Comment 20 WebKit Review Bot 2012-11-09 12:47:43 PST
Comment on attachment 173153 [details]
Patch

Clearing flags on attachment: 173153

Committed r134101: <http://trac.webkit.org/changeset/134101>
Comment 21 WebKit Review Bot 2012-11-09 12:47:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Review Bot 2012-11-09 13:27:12 PST
Re-opened since this is blocked by bug 101790
Comment 23 Alec Flett 2012-11-09 13:52:26 PST
Created attachment 173357 [details]
Patch for landing
Comment 24 WebKit Review Bot 2012-11-09 14:05:59 PST
Comment on attachment 173357 [details]
Patch for landing

Rejecting attachment 173357 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14680479
Comment 25 Alec Flett 2012-11-09 14:19:05 PST
Created attachment 173362 [details]
Patch for landing
Comment 26 WebKit Review Bot 2012-11-09 15:24:37 PST
Comment on attachment 173362 [details]
Patch for landing

Rejecting attachment 173362 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
Updating OpenSource
From http://git.chromium.org/external/Webkit
   955bc47..a388f5d  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 134117 = 5609c0bf5278e86109c3d178cfb12786bf7f48d5
last_rev is higher!: 134117 >= 134113 at /usr/lib/git-core/git-svn line 1523

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/14775735
Comment 27 Alec Flett 2012-11-09 15:30:59 PST
Comment on attachment 173362 [details]
Patch for landing

trying again - I think the git mirror got wedged or something
Comment 28 WebKit Review Bot 2012-11-09 16:02:06 PST
Comment on attachment 173362 [details]
Patch for landing

Clearing flags on attachment: 173362

Committed r134129: <http://trac.webkit.org/changeset/134129>
Comment 29 WebKit Review Bot 2012-11-09 16:02:12 PST
All reviewed patches have been landed.  Closing bug.