Bug 26054 – Need a new abstraction layer between the DB classes and the file system

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

We need an abstraction layer between the DB classes and the file system. It would remove the assumption that each DB is stored in a file on the hard drive, and would allow us to implement our own logic of storing/opening/deleting/etc. databases.

[Dumitru Daniliuc]

Created attachment 30724 [details]
Adds the required abstraction layer

[Dumitru Daniliuc]

Created attachment 30725 [details]
Adds the required abstraction layer

[Dimitri Glazkov (Google)]

Comment on attachment 30725 [details]
Adds the required abstraction layer

Need XCode project changes.

The ChangeLog entry needs a description of the bug and URL of the bug.

Also, I would really like Brady or Anders to review this.

[Dimitri Glazkov (Google)]

Since I'll be the one landing this once reviewed, I'll go ahead and commit to adding the XCode build changes to the patch, since Dumi doesn't have a Mac.

Dumi, can you resubmit after addressing the ChangeLog entries? Leave reviewer field blank.

[Dumitru Daniliuc]

Created attachment 30760 [details]
new patch

[Dimitri Glazkov (Google)]

Comment on attachment 30760 [details]
new patch

Adding review flag to make sure this is in the review queue. Anders, Adam, Brady, could you guys review?

[Dumitru Daniliuc]

Created attachment 30977 [details]
same patch with a minor change

removed SQLiteFileSystem::allDatabasesClosed().

[Michael Nordman]

FWIW, this code looks reasonable to me.

nit: Maybe use c++ style comments instead of c style in SQLiteFileSystem.h, the c style looks out of place to me for webkit (maybe I'm wrong?). 

Dimitry said...
> Also, I would really like Brady or Anders to review this.

I would really like Brady or Anders (or whomever is guilty of the existing Database code) to have a decent understanding of where we're heading with this refactoring. The direction is not really apparent with the modest changes in this initial patch.

Part of our plan is to have a chromium specific impl of the SQLiteFileSystem class (in platform/chromium). No surprises there I'm sure.

The more interesting parts of our plans are around the functionality provided by the central DatabaseTracker class. In chrome, access to the tracker database file will by constrained to code executing in the main browser process. The main browser process will also serve as a 'transaction coordinator' in chrome. That 'coordinator' is something the current Database impl has no notion of at this point (afaik).

[Dimitri Glazkov (Google)]

Dumi, one of the reasons why your patch hasn't gotten any attention could be because you haven't marked it for review. Please do :)

[Dimitri Glazkov (Google)]

Comment on attachment 30977 [details]
same patch with a minor change

I looked over existing WebKit code and it looks like the practice of just defining functions, rather than a static class is preferred. I don't have hard feelings either way.
Overall, this looks fine. There are a couple of style nits below:


Now that I think about it, since the code in SQLiteFileSystem.* is reused from other files, we should take the copyright header from those files and add (C) Google blah line to it.

> Property changes on: WebCore/platform/sql/SQLiteFileSystem.cpp
> ___________________________________________________________________
> Added: svn:executable
>    + *

No need for this svn prop.

> +    /*
> +     * Registers a user-defined SQLite VFS.
> +     */
> +    static void registerSQLiteVFS();

Use C++ style comments here and elsewhere. No need for extra lines.

>      if (result == SQLResultRow)
> -        return pathByAppendingComponent(originPath, statement.getColumnText(0));
> +      return SQLiteFileSystem::appendDatabaseFileNameToPath(originPath, statement.getColumnText(0));

A spacing issue?

[Dumitru Daniliuc]

Created attachment 31151 [details]
same patch (with all comments addressed)

[Dumitru Daniliuc]

> I looked over existing WebKit code and it looks like the practice of just
> defining functions, rather than a static class is preferred. I don't have hard
> feelings either way.

i left it as a class with all static methods. it seems to me like Class::Method makes the code a bit clearer than just methodThatCameOutOfNoWhere().

> Now that I think about it, since the code in SQLiteFileSystem.* is reused from
> other files, we should take the copyright header from those files and add (C)
> Google blah line to it.

done (if i understood correctly what you meant). please take another look.

> > Property changes on: WebCore/platform/sql/SQLiteFileSystem.cpp
> > ___________________________________________________________________
> > Added: svn:executable
> >    + *

removed. never meant to have +x added to these files...

> > +    /*
> > +     * Registers a user-defined SQLite VFS.
> > +     */
> > +    static void registerSQLiteVFS();
> 
> Use C++ style comments here and elsewhere. No need for extra lines.

changed all comments to //.

> >      if (result == SQLResultRow)
> > -        return pathByAppendingComponent(originPath, statement.getColumnText(0));
> > +      return SQLiteFileSystem::appendDatabaseFileNameToPath(originPath, statement.getColumnText(0));
> 
> A spacing issue?

fixed.

[Dimitri Glazkov (Google)]

Comment on attachment 31151 [details]
same patch (with all comments addressed)

Are you sure you uploaded the right patch? The C-style comments are still there.

Also, I just saw that changes to WebCore.pro (Qt build) are missing.

[Dumitru Daniliuc]

Created attachment 31317 [details]
patch

added the changes to WebCore.pro and changed all /* */ comments to // (not sure how i missed that in the last patch).

[Darin Fisher (:fishd, Google)]

Comment on attachment 31317 [details]
patch

> Index: WebCore/platform/sql/SQLiteFileSystem.cpp
...
> +String SQLiteFileSystem::getFileNameForNewDatabase(const String& dbDir, const String& dbName,
...
> +    if (result == SQLResultRow) {
> +        seq = sequenceStatement.getColumnInt64(0);
> +    } else if (result != SQLResultDone)

nit: no brackets around single statement body


> Index: WebCore/platform/sql/SQLiteFileSystem.h
...
> +class SQLiteFileSystem {
...
> +    static String getFileNameForNewDatabase(const String& dbDir, const String& dbName,
> +					    const String& originIdentifier, SQLiteDatabase* db);

nit: probably want to indent this second line by 2 more chars


> Index: WebCore/storage/DatabaseTracker.cpp
...
>      if (result == SQLResultRow)
> -        return pathByAppendingComponent(originPath, statement.getColumnText(0));
> +      return SQLiteFileSystem::appendDatabaseFileNameToPath(originPath, statement.getColumnText(0));

nit: indent by 4 white spaces

[Dumitru Daniliuc]

Created attachment 31328 [details]
patch

[Dumitru Daniliuc]

> > Index: WebCore/platform/sql/SQLiteFileSystem.cpp
> ...
> > +String SQLiteFileSystem::getFileNameForNewDatabase(const String& dbDir, const String& dbName,
> ...
> > +    if (result == SQLResultRow) {
> > +        seq = sequenceStatement.getColumnInt64(0);
> > +    } else if (result != SQLResultDone)
> 
> nit: no brackets around single statement body

removed.

> > Index: WebCore/platform/sql/SQLiteFileSystem.h
> ...
> > +class SQLiteFileSystem {
> ...
> > +    static String getFileNameForNewDatabase(const String& dbDir, const String& dbName,
> > +					    const String& originIdentifier, SQLiteDatabase* db);
> 
> nit: probably want to indent this second line by 2 more chars

this looked ok in emacs because of tabs. replaced all tabs with empty spaces.

> > Index: WebCore/storage/DatabaseTracker.cpp
> ...
> >      if (result == SQLResultRow)
> > -        return pathByAppendingComponent(originPath, statement.getColumnText(0));
> > +      return SQLiteFileSystem::appendDatabaseFileNameToPath(originPath, statement.getColumnText(0));
> 
> nit: indent by 4 white spaces

done.

[Dimitri Glazkov (Google)]

Comment on attachment 31328 [details]
patch

Looks fine to me. I'll add xcodeproj changes when landing. beidson, andersca, unless you have any objections, I will r+.

[Brady Eidson]

I've been busy with conference stuff as of late - sorry for ignoring this!  I hope to give a full review today, please hold off till then  :)

[Dimitri Glazkov (Google)]

Any luck with finding time to review this? :)

[Brady Eidson]

I am actually mopping up things today, Jeremy beat you out for LocalStorage though.  Gimme a bit.  ;)

[Dimitri Glazkov (Google)]

Dumi, can you rebase the patch? It's bit-rotten and I can't apply (damn WebCore.vcproj!) locally.

[Dumitru Daniliuc]

Created attachment 31940 [details]
patch

[Dumitru Daniliuc]

Brady, we are submitting this patch because we need to move forward. However, if you get to reviewing this patch and have comments, I'll be happy to address them in a separate patch.

[Brady Eidson]

Comment on attachment 31940 [details]
patch

This looks fine in general, but...

> +#if PLATFORM(WIN)
> +#define PATH_SEPARATOR '\\'
> +#else
> +#define PATH_SEPARATOR '/'
> +#endif
...
> +bool SQLiteFileSystem::ensureDatabaseFileExists(const String& fileName, bool checkPathOnly)
...
> +        int lastIndexOfBackSlash = dir.reverseFind(PATH_SEPARATOR);
> +        if (lastIndexOfBackSlash > 0) {
> +            dir.truncate(lastIndexOfBackSlash);
> +            return ensureDatabaseDirectoryExists(dir);
> +        }
...

*Please* don't add more platform specific ifdefs where not necessary - There is already a FileSystem.h abstraction for handling the difference in filesystem paths on Windows and non-Windows platforms.  
Use pathGetFileName() or directoryName() here instead!

r+ with that change.

[Brady Eidson]

(and nuke the windows.h include, which was discussed on IRC)  :)

[Dumitru Daniliuc]

Created attachment 31967 [details]
patch

Dimitri, Brady, please take one more quick look at this patch before we land it. I've only made a few minor changes to address Brady's comments.

[Dimitri Glazkov (Google)]

Comment on attachment 31967 [details]
patch

Looks ok, needs XCode changes. Since this is going to break the canary and it needs the XCode project changes, I won't be able to land it until tomorrow. Not settting r+ to avoid accidental landing.

[Eric Seidel (no email)]

Comment on attachment 31940 [details]
patch

Clearing r+ on this obsolete patch.

[Dumitru Daniliuc]

Created attachment 32136 [details]
new patch with a bug fix + xcode changes

[Dumitru Daniliuc]

Created attachment 32139 [details]
final version

forgot to add project.pbxproj to the ChangeLog.

[Dimitri Glazkov (Google)]

Comment on attachment 32136 [details]
new patch with a bug fix + xcode changes

r=me, will land.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 45444)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,47 @@
> [phone]  Dumitru Daniliuc  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Adds an abstraction layer between the DB classes and the file
> +        system, which allows us to add our own logic for storing, opening,
> +        deleting, etc. databases.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=26054
> +
> +        * GNUmakefile.am:
> +        * WebCore.pro:
> +        * WebCore.vcproj/WebCore.vcproj:
> +        * platform/sql/SQLiteDatabase.cpp:
> +        (WebCore::SQLiteDatabase::open):
> +        * platform/sql/SQLiteFileSystem.cpp: Added.
> +        (WebCore::SQLiteFileSystem::SQLiteFileSystem):
> +        (WebCore::SQLiteFileSystem::registerSQLiteVFS):
> +        (WebCore::SQLiteFileSystem::openDatabase):
> +        (WebCore::SQLiteFileSystem::getFileNameForNewDatabase):
> +        (WebCore::SQLiteFileSystem::appendDatabaseFileNameToPath):
> +        (WebCore::SQLiteFileSystem::ensureDatabaseDirectoryExists):
> +        (WebCore::SQLiteFileSystem::ensureDatabaseFileExists):
> +        (WebCore::SQLiteFileSystem::deleteEmptyDatabaseDirectory):
> +        (WebCore::SQLiteFileSystem::deleteDatabaseFile):
> +        (WebCore::SQLiteFileSystem::getDatabaseFileSize):

When adding new files, you can remove entries listing new methods/funcs that are auto-generated by the prepare-ChangeLog script.

> +        * platform/sql/SQLiteFileSystem.h: Added.
> +        * platform/win/FileSystemWin.cpp:
> +        (WebCore::directoryName):
> +        * storage/Database.cpp:
> +        (WebCore::Database::databaseSize):
> +        * storage/DatabaseTracker.cpp:
> +        (WebCore::DatabaseTracker::DatabaseTracker):
> +        (WebCore::DatabaseTracker::trackerDatabasePath):
> +        (WebCore::DatabaseTracker::openTrackerDatabase):
> +        (WebCore::DatabaseTracker::originPath):
> +        (WebCore::DatabaseTracker::fullPathForDatabase):
> +        (WebCore::DatabaseTracker::usageForDatabase):
> +        (WebCore::DatabaseTracker::deleteOrigin):
> +        (WebCore::DatabaseTracker::deleteDatabaseFile):
> +        * storage/OriginUsageRecord.cpp:
> +        (WebCore::OriginUsageRecord::diskUsage):

It's a very good idea (though not required) to list what was done in each method. This auto-generated list is just for that purpose.

Also, you should mention which tests are affected, added, or used to ensure validity of patch.

> +  String fileName = pathGetFileName(path);
> +  String dirName = String(path);
> +  dirName.truncate(dirName.length() - pathGetFileName(path).length());
> +  return dirName;

4 spaces indent.

>  #endif
>  {
> +  SQLiteFileSystem::registerSQLiteVFS();
>  }

Ditto.

[Dimitri Glazkov (Google)]

Also, please set your EMAIL_ADDRESS environment variable to make sure the prepare-ChangeLog script populates the top line of the entry correctly.

[Dimitri Glazkov (Google)]

Comment on attachment 32139 [details]
final version


> +String SQLiteFileSystem::getFileNameForNewDatabase(const String& dbDir, const String& dbName,
> +                                                   const String& originIdentifier, SQLiteDatabase* db)
> +{

> +    // dbName and originIdentifier not used in the default WebKit implementation
> +    // touch them so gcc doesn't complain about that when building on Mac
> +    String unused = dbName;
> +    unused = originIdentifier;

Oops, this is wrong. Let's not create more work. You can just do:

String SQLiteFileSystem::getFileNameForNewDatabase(const String& dbDir, const String&, const String&, SQLiteDatabase* db)

[Dumitru Daniliuc]

Created attachment 32148 [details]
patch

addressing all dimitri's comments.

[Adam Roben (:aroben)]

(In reply to comment #34)
> (From update of attachment 32139 [details] [review])
> 
> > +String SQLiteFileSystem::getFileNameForNewDatabase(const String& dbDir, const String& dbName,
> > +                                                   const String& originIdentifier, SQLiteDatabase* db)
> > +{
> 
> > +    // dbName and originIdentifier not used in the default WebKit implementation
> > +    // touch them so gcc doesn't complain about that when building on Mac
> > +    String unused = dbName;
> > +    unused = originIdentifier;
> 
> Oops, this is wrong. Let's not create more work. You can just do:
> 
> String SQLiteFileSystem::getFileNameForNewDatabase(const String& dbDir, const
> String&, const String&, SQLiteDatabase* db)

If you *really* had to name the parameters, you could use UNUSED_PARAM from <wtf/UnusedParam.h>:

UNUSED_PARAM(dbName);
UNUSED_PARAM(originIdentifier);

[Dimitri Glazkov (Google)]

Comment on attachment 32148 [details]
patch

Just one more thing! I promise!

> +	    SQLiteFileSystem::deleteDatabaseFile(trackerDatabasePath());
> +	    SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_databaseDirectoryPath);

4 spaces?

[Dumitru Daniliuc]

Created attachment 32151 [details]
patch

[Dumitru Daniliuc]

Created attachment 32154 [details]
patch

[Dimitri Glazkov (Google)]

Comment on attachment 32154 [details]
patch

r=me for realz. Landing now.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/45487.