[webkit-reviews] review denied: [Bug 34619] Add a way to cancel/ignore transactions on a database : [Attachment 48269] patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Dumitru Daniliuc
<dumi at chromium.org>'s request for review:
Bug 34619: Add a way to cancel/ignore transactions on a database
https://bugs.webkit.org/show_bug.cgi?id=34619

Attachment 48269: patch
https://bugs.webkit.org/attachment.cgi?id=48269&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 54448)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,20 @@
> +2010-02-05  Dumitru Daniliuc  <dumi at 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::cre
ate(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.