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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(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 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?

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.