When the user wants to clear all browsing data, Chromium needs to be able to delete all DB files. In order to do that, we need to have a way to cancel/ignore transactions on a database, and close the handle to that DB file. This could lead to canceled/ignored transactions and undefined app behavior, but we believe the user's request to clear all browsing data is more important.
Created attachment 48269 [details] patch
Comment on attachment 48269 [details] patch > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 54448) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,20 @@ > +2010-02-05 Dumitru Daniliuc <dumi@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Adding a way to get the set of all open database handles pointing > + to a given database. > + > + https://bugs.webkit.org/show_bug.cgi?id=34619 > + Maybe add a few more details? > =================================================================== > --- WebCore/storage/DatabaseTracker.h (revision 54448) > +++ WebCore/storage/DatabaseTracker.h (working copy) > @@ -69,12 +70,20 @@ public: > > void addOpenDatabase(Database*); > void removeOpenDatabase(Database*); > + void getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases); This probably should be a hash set of ref pointers. > Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp > =================================================================== > --- WebCore/storage/chromium/DatabaseTrackerChromium.cpp (revision 54448) > +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp (working copy) > @@ -38,6 +38,7 @@ > #include "QuotaTracker.h" > #include "ScriptExecutionContext.h" > #include "SecurityOrigin.h" > +#include "SecurityOriginHash.h" > #include "SQLiteFileSystem.h" > #include <wtf/HashSet.h> > #include <wtf/MainThread.h> > @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab > void DatabaseTracker::addOpenDatabase(Database* database) > { Don't we need this code in the non-Chromium version too? If so, maybe there's even some way to share more code? For example have a common parent class? Just a thought. > @@ -102,14 +126,55 @@ private: > > void DatabaseTracker::removeOpenDatabase(Database* database) > { > + if (!database) > + return; > + > if (!database->scriptExecutionContext()->isContextThread()) { > database->scriptExecutionContext()->postTask(TrackerRemoveOpenDatabaseTask::create(database)); > return; > } > > + MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard); > + ASSERT(m_openDatabaseMap); > + DatabaseNameMap* nameMap = m_openDatabaseMap->get(database->securityOrigin()); > + ASSERT(nameMap); > + String name(database->stringIdentifier()); > + DatabaseSet* databaseSet = nameMap->get(name); > + ASSERT(databaseSet); > + databaseSet->remove(database); > + > + if (databaseSet->isEmpty()) { > + nameMap->remove(name); > + delete databaseSet; > + if (nameMap->isEmpty()) { > + m_openDatabaseMap->remove(database->securityOrigin()); > + delete nameMap; I wish there was some way to avoid this, but I can't think of any that wouldn't be more complex. > + } > + } > + > DatabaseObserver::databaseClosed(database); > } > > + > +void DatabaseTracker::getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases) This method should probably take in a SecurityOrigin pointer. > +{ Maybe you should clear the databases hash set or at least assert it's empty? > Index: WebKit/chromium/public/WebDatabase.h > =================================================================== > --- WebKit/chromium/public/WebDatabase.h (revision 54448) > +++ WebKit/chromium/public/WebDatabase.h (working copy) > @@ -72,6 +72,8 @@ public: > WEBKIT_API static void updateDatabaseSize( > const WebString& originIdentifier, const WebString& databaseName, > unsigned long long databaseSize, unsigned long long spaceAvailable); > + WEBKIT_API static void closeDatabaseImmediately( > + const WebString& originIdentifier, const WebString& databaseName); Instead of an origin identifier, it should probably take in a "const WebSecurityOrigin&". > > #if WEBKIT_IMPLEMENTATION > WebDatabase(const WTF::PassRefPtr<WebCore::Database>&); > Index: WebKit/chromium/src/WebDatabase.cpp > =================================================================== > --- WebKit/chromium/src/WebDatabase.cpp (revision 54448) > +++ WebKit/chromium/src/WebDatabase.cpp (working copy) > @@ -32,7 +32,9 @@ > #include "WebDatabase.h" > > #include "Database.h" > +#include "DatabaseTask.h" > #include "DatabaseThread.h" > +#include "DatabaseTracker.h" > #include "Document.h" > #include "KURL.h" > #include "QuotaTracker.h" > @@ -106,6 +108,21 @@ void WebDatabase::updateDatabaseSize( > originIdentifier, databaseName, databaseSize, spaceAvailable); > } > > +void WebDatabase::closeDatabaseImmediately( > + const WebString& originIdentifier, const WebString& databaseName) Seems like those lines could be combined.
Oops, lost the context for this one: > > +{ > > Maybe you should clear the databases hash set or at least assert it's empty? > It was supposed to be this: > +void DatabaseTracker::getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases) > +{ Maybe you should clear the databases hash set or at least assert it's empty? > + MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard); > + if (!m_openDatabaseMap) > + return;
> +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp (working copy) > @@ -38,6 +38,7 @@ > #include "QuotaTracker.h" > #include "ScriptExecutionContext.h" > #include "SecurityOrigin.h" > +#include "SecurityOriginHash.h" > #include "SQLiteFileSystem.h" > #include <wtf/HashSet.h> > #include <wtf/MainThread.h> > @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab > void DatabaseTracker::addOpenDatabase(Database* database) > { > ASSERT(database->scriptExecutionContext()->isContextThread()); This should never be called with null database; note the line just above. > + if (!database) > + return; > void DatabaseTracker::removeOpenDatabase(Database* database) > { Can this ever be called with null database? > + if (!database) > + return; > +
Created attachment 48278 [details] patch (In reply to comment #2) > (From update of attachment 48269 [details]) > > Index: WebCore/ChangeLog > > =================================================================== > > --- WebCore/ChangeLog (revision 54448) > > +++ WebCore/ChangeLog (working copy) > > @@ -1,3 +1,20 @@ > > +2010-02-05 Dumitru Daniliuc <dumi@chromium.org> > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + Adding a way to get the set of all open database handles pointing > > + to a given database. > > + > > + https://bugs.webkit.org/show_bug.cgi?id=34619 > > + > > Maybe add a few more details? done. > > =================================================================== > > --- WebCore/storage/DatabaseTracker.h (revision 54448) > > +++ WebCore/storage/DatabaseTracker.h (working copy) > > @@ -69,12 +70,20 @@ public: > > > > void addOpenDatabase(Database*); > > void removeOpenDatabase(Database*); > > + void getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases); > > This probably should be a hash set of ref pointers. done. > > Index: WebCore/storage/chromium/DatabaseTrackerChromium.cpp > > =================================================================== > > --- WebCore/storage/chromium/DatabaseTrackerChromium.cpp (revision 54448) > > +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp (working copy) > > @@ -38,6 +38,7 @@ > > #include "QuotaTracker.h" > > #include "ScriptExecutionContext.h" > > #include "SecurityOrigin.h" > > +#include "SecurityOriginHash.h" > > #include "SQLiteFileSystem.h" > > #include <wtf/HashSet.h> > > #include <wtf/MainThread.h> > > @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab > > void DatabaseTracker::addOpenDatabase(Database* database) > > { > > Don't we need this code in the non-Chromium version too? > > If so, maybe there's even some way to share more code? For example have a > common parent class? Just a thought. i need to refactor DatabaseTracker.{h|cpp} to remove the #ifdefs. i can try to share some of this code then. for nwo though, i don't think there's a much better way to do this. > > + } > > + } > > + > > DatabaseObserver::databaseClosed(database); > > } > > > > + > > +void DatabaseTracker::getOpenDatabases(const String& originIdentifier, const String& name, HashSet<Database*>* databases) > > This method should probably take in a SecurityOrigin pointer. done. > > +{ > > Maybe you should clear the databases hash set or at least assert it's empty? i don't think we need to do this. for example, the caller might want to get all handles for DBs A, B and C. i don't think we should force it to use 3 different hash sets. > > Index: WebKit/chromium/public/WebDatabase.h > > =================================================================== > > --- WebKit/chromium/public/WebDatabase.h (revision 54448) > > +++ WebKit/chromium/public/WebDatabase.h (working copy) > > @@ -72,6 +72,8 @@ public: > > WEBKIT_API static void updateDatabaseSize( > > const WebString& originIdentifier, const WebString& databaseName, > > unsigned long long databaseSize, unsigned long long spaceAvailable); > > + WEBKIT_API static void closeDatabaseImmediately( > > + const WebString& originIdentifier, const WebString& databaseName); > > Instead of an origin identifier, it should probably take in a "const > WebSecurityOrigin&". the caller operates on strings and is unaware of WebSecurityOrigins. i think it might be better to leave it this way, especially since updateDatabaseSize() uses strings too. > > #if WEBKIT_IMPLEMENTATION > > WebDatabase(const WTF::PassRefPtr<WebCore::Database>&); > > Index: WebKit/chromium/src/WebDatabase.cpp > > =================================================================== > > --- WebKit/chromium/src/WebDatabase.cpp (revision 54448) > > +++ WebKit/chromium/src/WebDatabase.cpp (working copy) > > @@ -32,7 +32,9 @@ > > #include "WebDatabase.h" > > > > #include "Database.h" > > +#include "DatabaseTask.h" > > #include "DatabaseThread.h" > > +#include "DatabaseTracker.h" > > #include "Document.h" > > #include "KURL.h" > > #include "QuotaTracker.h" > > @@ -106,6 +108,21 @@ void WebDatabase::updateDatabaseSize( > > originIdentifier, databaseName, databaseSize, spaceAvailable); > > } > > > > +void WebDatabase::closeDatabaseImmediately( > > + const WebString& originIdentifier, const WebString& databaseName) > > Seems like those lines could be combined. done. (In reply to comment #4) > > +++ WebCore/storage/chromium/DatabaseTrackerChromium.cpp (working copy) > > @@ -38,6 +38,7 @@ > > #include "QuotaTracker.h" > > #include "ScriptExecutionContext.h" > > #include "SecurityOrigin.h" > > +#include "SecurityOriginHash.h" > > #include "SQLiteFileSystem.h" > > #include <wtf/HashSet.h> > > #include <wtf/MainThread.h> > > @@ -76,6 +77,29 @@ String DatabaseTracker::fullPathForDatab > > void DatabaseTracker::addOpenDatabase(Database* database) > > { > > ASSERT(database->scriptExecutionContext()->isContextThread()); > > This should never be called with null database; note the line just above. > > > + if (!database) > > + return; removed. > > void DatabaseTracker::removeOpenDatabase(Database* database) > > { > > Can this ever be called with null database? > > > + if (!database) > > + return; i don't think so. removed.
Attachment 48278 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/238526
Comment on attachment 48278 [details] patch All your comments seem reasonable. > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 54456) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,22 @@ > +2010-02-05 Dumitru Daniliuc <dumi@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Adding a way to get the set of all open database handles pointing > + to a given database. Sometimes we need to be able to close all > + handles to a database as soon as possible (to delete the DB file, > + for example). > + > + https://bugs.webkit.org/show_bug.cgi?id=34619 Please use the change log style of Headline. http://bug.link Details and more details.
Changed the format of the comments in ChangeLogs. Also, fixed the chromium build problem. Landed as r54506.