Bug 110653 – IndexedDB: Properly refactor frontend/backend code by #includes

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

[Alec Flett]

IndexedDB: Properly refactor frontend/backend code by #includes

[Alec Flett]

Created attachment 189849 [details]
Patch

[Alec Flett]

jsbell/dgrogan - this is some much-needed code cleanup that you'll recognize the need for. I need to clean up the changelog, but mind taking a look at the rest of the patch?

[Joshua Bell]

Comment on attachment 189849 [details]
Patch

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

Overall lgtm...

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendInterface.h:81
> +        ReadOnly = 0,

Out of context, these don't read clearly: IDBDatabaseBackendInterface::ReadOnly doesn't imply "it's a read-only *transaction*". Although it'd be more verbose, how about including "Transaction" in the member names?

[Alec Flett]

Created attachment 190617 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190617 [details]
Patch

Attachment 190617 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16793153

[WebKit Review Bot]

Comment on attachment 190617 [details]
Patch

Attachment 190617 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16797128

[Alec Flett]

Created attachment 190638 [details]
Patch

This time with the right quoting style

[WebKit Review Bot]

Comment on attachment 190638 [details]
Patch

Attachment 190638 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16820090

[WebKit Review Bot]

Comment on attachment 190638 [details]
Patch

Attachment 190638 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16848023

[Alec Flett]

Created attachment 190644 [details]
Patch

This time with the tests fixed

[Joshua Bell]

Comment on attachment 190644 [details]
Patch

Transaction modes are more readable than in the previous patch. Thanks!

A few nits...

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

> Source/WebCore/ChangeLog:20
> +        (WebCore::IDBBackingStore::Cursor::continueFunction):

Remove or add details to the (parenthesized) lines.

> Source/WebCore/Modules/indexeddb/IndexedDB.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Isn't it 2013 ? :)

> Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:28
> +#include "IDBDatabase.h"

Sort/remove extra whitespace.

[Alec Flett]

Created attachment 190876 [details]
Patch

[Alec Flett]

Created attachment 190877 [details]
Patch

[WebKit Review Bot]

Attachment 190877 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp', u'Source/WebCore/Modules/indexeddb/IDBBackingStore.h', u'Source/WebCore/Modules/indexeddb/IDBCursor.cpp', u'Source/WebCore/Modules/indexeddb/IDBCursor.h', u'Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp', u'Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.h', u'Source/WebCore/Modules/indexeddb/IDBCursorBackendInterface.h', u'Source/WebCore/Modules/indexeddb/IDBCursorWithValue.cpp', u'Source/WebCore/Modules/indexeddb/IDBCursorWithValue.h', u'Source/WebCore/Modules/indexeddb/IDBDatabase.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabase.h', u'Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp', u'Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h', u'Source/WebCore/Modules/indexeddb/IDBDatabaseBackendInterface.h', u'Source/WebCore/Modules/indexeddb/IDBFactory.h', u'Source/WebCore/Modules/indexeddb/IDBIndex.cpp', u'Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp', u'Source/WebCore/Modules/indexeddb/IDBRequest.cpp', u'Source/WebCore/Modules/indexeddb/IDBRequest.h', u'Source/WebCore/Modules/indexeddb/IDBTransaction.cpp', u'Source/WebCore/Modules/indexeddb/IDBTransaction.h', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h', u'Source/WebCore/Modules/indexeddb/IDBTransactionCoordinator.cpp', u'Source/WebCore/Modules/indexeddb/IndexedDB.h', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/AssertMatchingEnums.cpp', u'Source/WebKit/chromium/src/IDBDatabaseBackendProxy.cpp', u'Source/WebKit/chromium/src/IDBDatabaseBackendProxy.h', u'Source/WebKit/chromium/src/WebIDBCallbacksImpl.cpp', u'Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp', u'Source/WebKit/chromium/tests/IDBAbortOnCorruptTest.cpp', u'Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp', u'Source/WebKit/chromium/tests/IDBFakeBackingStore.h']" exit_code: 1
Source/WebKit/chromium/tests/IDBDatabaseBackendTest.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alec Flett]

All nits addressed, except for the #include of IDBDatabase.h - webkit-patch started complaining because that test implements IDBDatabase. I couldn't figure out a way to shut it up. I'll keep trying while awaiting the final review.

dglazkov@ - r?

[WebKit Review Bot]

Comment on attachment 190877 [details]
Patch

Attachment 190877 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16824132

[WebKit Review Bot]

Comment on attachment 190877 [details]
Patch

Attachment 190877 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16778553

[Peter Beverloo (cr-android ews)]

Comment on attachment 190877 [details]
Patch

Attachment 190877 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16865098

[Alec Flett]

Created attachment 190980 [details]
Patch

[Alec Flett]

dglazkov@ - r?

[Alec Flett]

Created attachment 191529 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191529 [details]
Patch for landing

Clearing flags on attachment: 191529

Committed r144798: <http://trac.webkit.org/changeset/144798>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111478

[Dirk Pranke]

Looks like this breaks the windows build, so I'm rolling it out per dglazkov (since alec didn't appear to be around on #irc). Sorry!

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/builds/36731/steps/compile/logs/stdio

[Alec Flett]

Created attachment 191557 [details]
Patch for landing

[Alec Flett]

new patch should fix windows.

[WebKit Review Bot]

Comment on attachment 191557 [details]
Patch for landing

Clearing flags on attachment: 191557

Committed r144820: <http://trac.webkit.org/changeset/144820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.