Bug 26054 - Need a new abstraction layer between the DB classes and the file system
Summary: Need a new abstraction layer between the DB classes and the file system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-27 18:15 PDT by Dumitru Daniliuc
Modified: 2009-07-02 14:43 PDT (History)
6 users (show)

See Also:


Attachments
Adds the required abstraction layer (21.65 KB, patch)
2009-05-27 18:17 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
Adds the required abstraction layer (21.65 KB, patch)
2009-05-27 18:17 PDT, Dumitru Daniliuc
dglazkov: review-
Details | Formatted Diff | Diff
new patch (22.16 KB, patch)
2009-05-28 17:36 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
same patch with a minor change (21.85 KB, patch)
2009-06-04 17:58 PDT, Dumitru Daniliuc
dglazkov: review-
Details | Formatted Diff | Diff
same patch (with all comments addressed) (21.54 KB, patch)
2009-06-10 18:59 PDT, Dumitru Daniliuc
dglazkov: review-
Details | Formatted Diff | Diff
patch (21.89 KB, patch)
2009-06-15 16:22 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (21.92 KB, patch)
2009-06-15 20:08 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (21.92 KB, patch)
2009-06-26 11:44 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (22.70 KB, patch)
2009-06-26 18:35 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
new patch with a bug fix + xcode changes (26.99 KB, patch)
2009-07-01 13:02 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
final version (27.03 KB, patch)
2009-07-01 14:15 PDT, Dumitru Daniliuc
dglazkov: review-
Details | Formatted Diff | Diff
patch (26.89 KB, patch)
2009-07-01 15:21 PDT, Dumitru Daniliuc
dglazkov: review-
Details | Formatted Diff | Diff
patch (26.89 KB, patch)
2009-07-01 15:42 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch (26.90 KB, patch)
2009-07-01 15:55 PDT, Dumitru Daniliuc
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2009-05-27 18:15:41 PDT
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.
Comment 1 Dumitru Daniliuc 2009-05-27 18:17:05 PDT
Created attachment 30724 [details]
Adds the required abstraction layer
Comment 2 Dumitru Daniliuc 2009-05-27 18:17:23 PDT
Created attachment 30725 [details]
Adds the required abstraction layer
Comment 3 Dimitri Glazkov (Google) 2009-05-27 20:29:58 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2009-05-28 08:33:23 PDT
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.
Comment 5 Dumitru Daniliuc 2009-05-28 17:36:49 PDT
Created attachment 30760 [details]
new patch
Comment 6 Dimitri Glazkov (Google) 2009-05-29 13:24:15 PDT
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?
Comment 7 Dumitru Daniliuc 2009-06-04 17:58:21 PDT
Created attachment 30977 [details]
same patch with a minor change

removed SQLiteFileSystem::allDatabasesClosed().
Comment 8 Michael Nordman 2009-06-08 22:46:28 PDT
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).
Comment 9 Dimitri Glazkov (Google) 2009-06-10 09:12:57 PDT
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 :)
Comment 10 Dimitri Glazkov (Google) 2009-06-10 12:41:34 PDT
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?
Comment 11 Dumitru Daniliuc 2009-06-10 18:59:46 PDT
Created attachment 31151 [details]
same patch (with all comments addressed)
Comment 12 Dumitru Daniliuc 2009-06-10 19:02:29 PDT
> 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.
Comment 13 Dimitri Glazkov (Google) 2009-06-11 13:24:58 PDT
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.
Comment 14 Dumitru Daniliuc 2009-06-15 16:22:50 PDT
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).
Comment 15 Darin Fisher (:fishd, Google) 2009-06-15 16:47:10 PDT
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
Comment 16 Dumitru Daniliuc 2009-06-15 20:08:04 PDT
Created attachment 31328 [details]
patch
Comment 17 Dumitru Daniliuc 2009-06-15 20:09:35 PDT
> > 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.
Comment 18 Dimitri Glazkov (Google) 2009-06-16 08:59:42 PDT
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+.
Comment 19 Brady Eidson 2009-06-16 10:02:56 PDT
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  :)
Comment 20 Dimitri Glazkov (Google) 2009-06-22 14:45:10 PDT
Any luck with finding time to review this? :)
Comment 21 Brady Eidson 2009-06-22 16:03:46 PDT
I am actually mopping up things today, Jeremy beat you out for LocalStorage though.  Gimme a bit.  ;)
Comment 22 Dimitri Glazkov (Google) 2009-06-26 10:13:18 PDT
Dumi, can you rebase the patch? It's bit-rotten and I can't apply (damn WebCore.vcproj!) locally.
Comment 23 Dumitru Daniliuc 2009-06-26 11:44:55 PDT
Created attachment 31940 [details]
patch
Comment 24 Dumitru Daniliuc 2009-06-26 11:47:13 PDT
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.
Comment 25 Brady Eidson 2009-06-26 15:02:15 PDT
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.
Comment 26 Brady Eidson 2009-06-26 15:03:00 PDT
(and nuke the windows.h include, which was discussed on IRC)  :)
Comment 27 Dumitru Daniliuc 2009-06-26 18:35:30 PDT
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.
Comment 28 Dimitri Glazkov (Google) 2009-06-29 11:31:20 PDT
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.
Comment 29 Eric Seidel (no email) 2009-06-29 13:21:44 PDT
Comment on attachment 31940 [details]
patch

Clearing r+ on this obsolete patch.
Comment 30 Dumitru Daniliuc 2009-07-01 13:02:42 PDT
Created attachment 32136 [details]
new patch with a bug fix + xcode changes
Comment 31 Dumitru Daniliuc 2009-07-01 14:15:58 PDT
Created attachment 32139 [details]
final version

forgot to add project.pbxproj to the ChangeLog.
Comment 32 Dimitri Glazkov (Google) 2009-07-01 14:49:52 PDT
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 @@
> +2009-07-01  Dumitru Daniliuc  <dumi@chromium.org>
> +
> +        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.
Comment 33 Dimitri Glazkov (Google) 2009-07-01 14:53:50 PDT
Also, please set your EMAIL_ADDRESS environment variable to make sure the prepare-ChangeLog script populates the top line of the entry correctly.
Comment 34 Dimitri Glazkov (Google) 2009-07-01 15:02:18 PDT
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)
Comment 35 Dumitru Daniliuc 2009-07-01 15:21:02 PDT
Created attachment 32148 [details]
patch

addressing all dimitri's comments.
Comment 36 Adam Roben (:aroben) 2009-07-01 15:34:29 PDT
(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);

Comment 37 Dimitri Glazkov (Google) 2009-07-01 15:35:19 PDT
Comment on attachment 32148 [details]
patch

Just one more thing! I promise!

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

4 spaces?
Comment 38 Dumitru Daniliuc 2009-07-01 15:42:55 PDT
Created attachment 32151 [details]
patch
Comment 39 Dumitru Daniliuc 2009-07-01 15:55:25 PDT
Created attachment 32154 [details]
patch
Comment 40 Dimitri Glazkov (Google) 2009-07-01 16:04:03 PDT
Comment on attachment 32154 [details]
patch

r=me for realz. Landing now.
Comment 41 Dimitri Glazkov (Google) 2009-07-02 14:43:01 PDT
Landed as http://trac.webkit.org/changeset/45487.