Bug 61000 - Control Indexeddb backends from LayoutTestController
Summary: Control Indexeddb backends from LayoutTestController
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: Nobody
URL:
Keywords:
Depends on: 61457
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-17 15:45 PDT by Greg Simon
Modified: 2011-06-08 10:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (17.57 KB, patch)
2011-05-17 15:47 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.24 MB, application/zip)
2011-05-17 16:20 PDT, WebKit Review Bot
no flags Details
Patch (17.52 KB, patch)
2011-05-17 22:21 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Patch (32.35 KB, patch)
2011-05-22 22:46 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Patch (33.55 KB, patch)
2011-05-23 11:12 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.54 MB, application/zip)
2011-05-23 11:54 PDT, WebKit Review Bot
no flags Details
Patch (33.73 KB, patch)
2011-05-23 16:18 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Patch (33.19 KB, patch)
2011-05-24 12:09 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Patch (34.14 KB, patch)
2011-06-06 16:02 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Patch (30.16 KB, patch)
2011-06-07 10:27 PDT, Greg Simon
no flags Details | Formatted Diff | Diff
Patch (30.56 KB, patch)
2011-06-08 09:15 PDT, Greg Simon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Simon 2011-05-17 15:45:57 PDT
Control Indexeddb backends from LayoutTestController
Comment 1 Greg Simon 2011-05-17 15:47:37 PDT
Created attachment 93837 [details]
Patch
Comment 2 Greg Simon 2011-05-17 15:50:17 PDT
This is the first in a two-part change to get migration in IndexedDB into the build. This specific change is adds an API to LayoutTestController on chromium so a unit test can be written (attached) that will exercise the migration of data from the SQLite backend to the LeveLDB backend.
Comment 3 WebKit Review Bot 2011-05-17 16:20:25 PDT
Comment on attachment 93837 [details]
Patch

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

New failing tests:
storage/indexeddb/migrate-basics.html
Comment 4 WebKit Review Bot 2011-05-17 16:20:31 PDT
Created attachment 93842 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Greg Simon 2011-05-17 22:21:04 PDT
Created attachment 93872 [details]
Patch
Comment 6 Hans Wennborg 2011-05-18 10:25:28 PDT
Comment on attachment 93872 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        to be used for IndexedDB so a unit test can be written

s/unit test/layout test/ throughout; we have unit tests too, e.g. in Source/WebKit/chromium/tests/

> LayoutTests/storage/indexeddb/migrate-basics.html:96
> +    } catch( ex ) {

i suppose you get an exception here because trans.objectStore("store") returns null, and then store.openCursor(keyRange) fails. I think it would be nicer to do an explicit check that "store" exists in the database. That would make it easier to see why it fails.

> LayoutTests/storage/indexeddb/migrate-basics.html:105
> +    if(!cursor) {

ultra nit: space between if and (

> LayoutTests/storage/indexeddb/migrate-basics.html:110
> +    if (cursor.value.text != "testing 1,2,3")

use the shouldBe() function that we use in the other layout tests

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90
> +        if (backingStoreType == DefaultBackingStore

no need for a newline here

> Source/WebKit/chromium/public/WebIDBFactory.h:65
> +    // Used for DumpRenderTree tests

ultra nit: period at end of comment.

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:36
> +#include <base/memory/scoped_temp_dir.h>

can we really use that here? isn't this a header in Chromium?

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:49
> +static ScopedTempDir tempLevelDBDir;

ScopedTempDir is a non-POD type, so I don't think it's allowed as a static variable

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:64
> +        LOG_ERROR("Failed to clear LevelDB temporary folder.");

doesn't ScopedTempDir delete the dir on destruction?

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:83
> +            && backingStoreType == LevelDBBackingStore)

no need for newline

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:84
> +        {

curly brace goes on same line as the if

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:36
> +#include "WebIDBFactory.h"

alpha order

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:158
> +    bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore);

these statements look like they should be sorted
Comment 7 Greg Simon 2011-05-22 22:46:34 PDT
Created attachment 94368 [details]
Patch
Comment 8 Greg Simon 2011-05-22 22:49:13 PDT
Comment on attachment 94368 [details]
Patch

This patch is waiting for the DEPS roll of chromium

https://bugs.webkit.org/show_bug.cgi?id=61236
Comment 9 WebKit Review Bot 2011-05-22 23:05:21 PDT
Comment on attachment 94368 [details]
Patch

Attachment 94368 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8723727
Comment 10 Hans Wennborg 2011-05-23 08:51:07 PDT
Comment on attachment 94368 [details]
Patch

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

> LayoutTests/ChangeLog:7
> +        can be implemented and tested.

I think this descriptive text is usually put after the bugzilla url below.

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=61000

Should probably have a description here too

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:80
> +        // Also check that backing store types match: this is important for migration

period.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82
> +            && (backingStoreType == it->second->backingStore()->backingStoreType())) {

no need for newline

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:96
> +            bool hasSqlBackend = IDBSQLiteBackingStore::backingStoreExists(securityOrigin.get(), dataDir);

i think this should be named hasSQLBackend.. same for hasLevelDBBackend below

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:97
> +            bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir);

hmm, now we're doing LevelDB stuff outside of the #if ENABLE(LEVELDB) guard.. i'm not sure that's a good idea

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102
> +            // Migration: if the database exists and is SQLite we want to migrate it to LevelDB

period.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1276
> +    if (!makeAllDirectories(pathBase)) {

we probably don't need this?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1281
> +    // FIXME: We should eventually use the same LevelDB database for all origins

period.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1284
> +    // FIXME: It would be more thorough to open the database here but also more expensive

yes, i think this is good enough. period after comment.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> +    if (!makeAllDirectories(pathBase)) {

we probably don't need this?

> Source/WebKit/chromium/public/WebIDBFactory.h:65
> +    // Used for DumpRenderTree tests

period.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:81
> +{

hmm, shouldn't this get passed on somewhere?

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:163
> +    bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore);

one line further down to keep it sorted i believe

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:233
> +    delete m_tempFolder;

can we use an OwnPtr instead so we don't have to call delete explicitly?
Comment 11 Greg Simon 2011-05-23 11:12:28 PDT
Created attachment 94446 [details]
Patch
Comment 12 Greg Simon 2011-05-23 11:13:53 PDT
(In reply to comment #10)
> (From update of attachment 94368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94368&action=review
> 
> > LayoutTests/ChangeLog:7
> > +        can be implemented and tested.
> 
> I think this descriptive text is usually put after the bugzilla url below.
Fixed

> > Source/WebCore/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=61000
> 
> Should probably have a description here too
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:80
> > +        // Also check that backing store types match: this is important for migration
> 
> period.
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82
> > +            && (backingStoreType == it->second->backingStore()->backingStoreType())) {
> 
> no need for newline
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:96
> > +            bool hasSqlBackend = IDBSQLiteBackingStore::backingStoreExists(securityOrigin.get(), dataDir);
> 
> i think this should be named hasSQLBackend.. same for hasLevelDBBackend below
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:97
> > +            bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir);
> 
> hmm, now we're doing LevelDB stuff outside of the #if ENABLE(LEVELDB) guard.. i'm not sure that's a good idea

Added ENABLE(LEVELDB) guards

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102
> > +            // Migration: if the database exists and is SQLite we want to migrate it to LevelDB
> 
> period.
Fixed

> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1276
> > +    if (!makeAllDirectories(pathBase)) {
> 
> we probably don't need this?
Removed

> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1281
> > +    // FIXME: We should eventually use the same LevelDB database for all origins
> 
> period.
Fixed

> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1284
> > +    // FIXME: It would be more thorough to open the database here but also more expensive
> 
> yes, i think this is good enough. period after comment.
Fixed

> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> > +    if (!makeAllDirectories(pathBase)) {
> 
> we probably don't need this?
Removed

> > Source/WebKit/chromium/public/WebIDBFactory.h:65
> > +    // Used for DumpRenderTree tests
> 
> period.
> 
> > Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:81
> > +{
> 
> hmm, shouldn't this get passed on somewhere?
This path is not used for LayoutTests but I plumbed it through for chrome side of the world.

> > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:163
> > +    bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore);
> 
> one line further down to keep it sorted i believe
The "addMockSpeecInputResult" is the item out of order so I moved it instead.

> > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:233
> > +    delete m_tempFolder;
> 
> can we use an OwnPtr instead so we don't have to call delete explicitly?
Replaced with OwnPtr
Comment 13 WebKit Review Bot 2011-05-23 11:54:30 PDT
Comment on attachment 94446 [details]
Patch

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

New failing tests:
storage/indexeddb/mozilla/versionchange.html
storage/indexeddb/mozilla/global-data.html
storage/indexeddb/set_version_queue.html
storage/indexeddb/set_version_blocked.html
Comment 14 WebKit Review Bot 2011-05-23 11:54:35 PDT
Created attachment 94451 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 15 Greg Simon 2011-05-23 16:18:04 PDT
Created attachment 94514 [details]
Patch
Comment 16 Hans Wennborg 2011-05-24 03:55:30 PDT
Comment on attachment 94514 [details]
Patch

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


LGTM barring some final nits. Is there a reviewer on the CC list who would like to take a look?

> LayoutTests/ChangeLog:10
> +        from sqlite to leveldb can be tested.

There's no plumbing in the LayoutTests.. this message should probably be in the Source/WebCore/ChangeLog.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:136
> +    return false;

Add a FIXME that this should be implemented?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1279
> +//    }

Just delete these lines?

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> +    if (!makeAllDirectories(pathBase)) {

I still don't see any need to create the directory? db.open() will fail if it's not there, right?

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1145
> +        m_tempFolder.clear();

i don't think you need the to clear() the OwnPtr before assigning to it.. if it has a non-null value, it will just free that, and take ownership of the new pointer.
Comment 17 Greg Simon 2011-05-24 12:09:35 PDT
Created attachment 94659 [details]
Patch
Comment 18 Dimitri Glazkov (Google) 2011-05-24 12:13:30 PDT
Comment on attachment 94659 [details]
Patch

rs=me.
Comment 19 WebKit Commit Bot 2011-05-24 20:01:10 PDT
Comment on attachment 94659 [details]
Patch

Clearing flags on attachment: 94659

Committed r87257: <http://trac.webkit.org/changeset/87257>
Comment 20 WebKit Commit Bot 2011-05-24 20:01:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Greg Simon 2011-06-06 16:02:38 PDT
Created attachment 96144 [details]
Patch
Comment 23 Adam Barth 2011-06-06 17:32:23 PDT
Comment on attachment 96144 [details]
Patch

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

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1317
> +//    if (!makeAllDirectories(pathBase)) {
> +//        LOG_ERROR("Unable to create IndexedDB database path %s", pathBase.utf8().data());
> +//        return false;
> +//    }

Commented out code.
Comment 24 Hans Wennborg 2011-06-07 07:03:24 PDT
Comment on attachment 96144 [details]
Patch

Hi Greg! Again, thank you very much for working on this!

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

> Source/WebCore/storage/IDBBackingStore.h:32
> +#include "IDBFactoryBackendImpl.h"

Why is this being included?

> Source/WebCore/storage/IDBBackingStore.h:111
> +    virtual IDBFactoryBackendInterface::BackingStoreType backingStoreType() = 0;

let's make this const

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:91
> +        if (m_migrateEnabled) {

i'm trying to think why we're making this conditional on m_migrateEnabled, rather than on backingStoreType...

shouldn't the logic of this function be more like

"if backingStoreType is LevelDB and the origin has a SQLiteBackingStore, and the origin doesn't have a LevelDBBackingStore, then do migration?"

as it is now, if m_migrateEnabled is true (which is always is??), the backingStoreType gets ignored and we migrate to LevelDB anyway?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:93
> +            bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir);

i'd prefer if these were named hasSQLBackingStore and hasLevelDBBackingStore

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> +    if (!makeAllDirectories(pathBase)) {

I think I've commented five times that we shouldn't be creating directories should to see if the database exists
Comment 25 Greg Simon 2011-06-07 10:27:11 PDT
Created attachment 96256 [details]
Patch
Comment 26 Greg Simon 2011-06-07 10:33:24 PDT
(In reply to comment #24)
> (From update of attachment 96144 [details])
> Hi Greg! Again, thank you very much for working on this!
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=96144&action=review
> 
> > Source/WebCore/storage/IDBBackingStore.h:32
> > +#include "IDBFactoryBackendImpl.h"
> 
> Why is this being included?

Refactored to remove this include.

> > Source/WebCore/storage/IDBBackingStore.h:111
> > +    virtual IDBFactoryBackendInterface::BackingStoreType backingStoreType() = 0;
> 
> let's make this const
Done.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:91
> > +        if (m_migrateEnabled) {
> 
> i'm trying to think why we're making this conditional on m_migrateEnabled, rather than on backingStoreType...
> 
> shouldn't the logic of this function be more like
> 
> "if backingStoreType is LevelDB and the origin has a SQLiteBackingStore, and the origin doesn't have a LevelDBBackingStore, then do migration?"
> 
> as it is now, if m_migrateEnabled is true (which is always is??), the backingStoreType gets ignored and we migrate to LevelDB anyway?

Agreed -- removed the enable/disable.

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:93
> > +            bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir);
> 
> i'd prefer if these were named hasSQLBackingStore and hasLevelDBBackingStore
> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> > +    if (!makeAllDirectories(pathBase)) {
> 
> I think I've commented five times that we shouldn't be creating directories should to see if the database exists
yes... I had a local git merge failure from when it was fixed before :-) Fixed (again)!
Comment 27 Hans Wennborg 2011-06-08 03:32:52 PDT
Comment on attachment 96256 [details]
Patch

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

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90
> +        // Migrationp

this comment doesn't add much value

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95
> +            backingStoreType = LevelDBBackingStore;

I don't like that we override the backingStoreType. I think we should only migrate if backingStoreType == LevelDBBackingStore.

This in fact will break the layout tests. After the migration test, hasLeveLDBBackingStore will be true, so any subsequent layout tests run in the same process will use LevelDB, which is not intended (they don't all pass with LevelDB at the moment).
Comment 28 Greg Simon 2011-06-08 07:14:31 PDT
(In reply to comment #27)
> (From update of attachment 96256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96256&action=review
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90
> > +        // Migrationp
> 
> this comment doesn't add much value
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95
> > +            backingStoreType = LevelDBBackingStore;
> 
> I don't like that we override the backingStoreType. I think we should only migrate if backingStoreType == LevelDBBackingStore.
> 
> This in fact will break the layout tests. After the migration test, hasLeveLDBBackingStore will be true, so any subsequent layout tests run in the same process will use LevelDB, which is not intended (they don't all pass with LevelDB at the moment).

This will not break the layout tests because the backend can be controlled from LayoutTestController. This is the point of this patch actually -- so you can set things up from individual layout tests to force migrations. 

If you look at the changes to WebIDBFactoryImpl.cpp there is a comment explaining how this works.

With this patch and the migration code patch I am able to run through the (unchanged) indexeddb tests.
Comment 29 Greg Simon 2011-06-08 09:15:43 PDT
Created attachment 96431 [details]
Patch
Comment 30 Hans Wennborg 2011-06-08 09:36:48 PDT
Comment on attachment 96431 [details]
Patch

Yay, the tests now pass. 

LGTM


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

> LayoutTests/storage/indexeddb/migrate-basics.html:189
> +    layoutTestController.clearAllDatabases();

Just for future reference, this is the major difference from the previous patch.
Comment 31 Dimitri Glazkov (Google) 2011-06-08 09:57:51 PDT
I don't think cq will take patches from a closed bug.
Comment 32 WebKit Review Bot 2011-06-08 10:50:42 PDT
Comment on attachment 96431 [details]
Patch

Clearing flags on attachment: 96431

Committed r88358: <http://trac.webkit.org/changeset/88358>
Comment 33 WebKit Review Bot 2011-06-08 10:50:53 PDT
All reviewed patches have been landed.  Closing bug.