[webkit-reviews] review requested: [Bug 31482] Refactor DatabaseTracker.h and eliminate all #ifdefs : [Attachment 52454] patch #1: remove the dependency on OriginQuotaManager from DatabaseTracker.h

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	for review:
Bug 31482: Refactor DatabaseTracker.h and eliminate all #ifdefs
https://bugs.webkit.org/show_bug.cgi?id=31482

Attachment 52454: patch #1: remove the dependency on OriginQuotaManager from
DatabaseTracker.h
https://bugs.webkit.org/attachment.cgi?id=52454&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
Brady: this entire bug will be about refactoring. The idea is to keep in
DatabaseTracker.h only the fields/methods used by all platforms and get rid of
the #if !PLATFORM(CHROMIUM) conditionals.

(In reply to comment #5)
> (From update of attachment 52090 [details])
> > Index: WebCore/storage/DatabaseTracker.cpp
> > ===================================================================
> > --- WebCore/storage/DatabaseTracker.cpp	(revision 56810)
> > +++ WebCore/storage/DatabaseTracker.cpp	(working copy)
> > @@ -49,20 +49,13 @@
> >  
> >  using namespace std;
> >  
> > -namespace WebCore {
> > -
> > -OriginQuotaManager& DatabaseTracker::originQuotaManagerNoLock()
> > +static WebCore::OriginQuotaManager& originQuotaManager()
> >  {
> > -	 ASSERT(m_quotaManager);
> > -	 return *m_quotaManager;
> > +	 DEFINE_STATIC_LOCAL(WebCore::OriginQuotaManager, quotaManager, ());
> 
> Does this have to be a static local?	Could we just create it in the
> DatabaseTracker constructor now?
> If it's always accessed from inside non-static DatabaseTracker methods, that
> should be simple.
> And then we don't even need the accessor function; it can just be a data
> member.

Chromium's DatabaseTracker implementation doesn't need this field, so I would
like to remove it from DatabaseTracker.h. I removed the accessor method though:
quotaManager is initialized in DatabaseTracker's constructor and destroyed in
its destructor.

> > @@ -504,8 +501,6 @@ void DatabaseTracker::removeOpenDatabase
> >	 if (!database)
> >	     return;
> >  
> > -	 Locker<OriginQuotaManager> quotaManagerLocker(originQuotaManager());
> > -	 MutexLocker lockDatabase(m_databaseGuard);
> >	 MutexLocker openDatabaseMapLock(m_openDatabaseMapGuard);
> >  
> >	 if (!m_openDatabaseMap) {
> > @@ -541,7 +536,9 @@ void DatabaseTracker::removeOpenDatabase
> >  
> >	 m_openDatabaseMap->remove(database->securityOrigin());
> >	 delete nameMap;
> > -	 originQuotaManagerNoLock().removeOrigin(database->securityOrigin());
> 
> You're now locking originQuotaManager after m_openDatabaseMapGuard [it used
to
> have to go before], and the header file no longer specifies the ordering of
> those two locks.  If this code is right, please specify the ordering in the
> header file.	The reordering is probably doable, though, as
> m_openDatabaseMapGuard isn't used in many places.

Updated the expectations in DatabaseTracker.h. The new expectation for
m_openDatabaseMapGuard is that the code locked on it should not acquire any
other lock.

> >  unsigned long long DatabaseTracker::usageForOriginNoLock(SecurityOrigin*
origin)
> >  {
> > -	 ASSERT(!originQuotaManagerNoLock().tryLock());
> > +	 ASSERT(!originQuotaManager().tryLock());
> >  
> >	 // Use the OriginQuotaManager mechanism to calculate the usage
> > -	 if (originQuotaManagerNoLock().tracksOrigin(origin))
> > -	     return originQuotaManagerNoLock().diskUsage(origin);
> > +	 if (originQuotaManager().tracksOrigin(origin))
> > +	     return originQuotaManager().diskUsage(origin);
> >  
> >	 // If the OriginQuotaManager doesn't track this origin already, prime
it to do so
> > -	 originQuotaManagerNoLock().trackOrigin(origin);
> > +	 originQuotaManager().trackOrigin(origin);
> >  
> >	 Vector<String> names;
> > -	 databaseNamesForOriginNoLock(origin, names);
> > +	 {
> > +	     MutexLocker lockDatabase(m_databaseGuard);
> 
> You're taking a lock in a function whose name ends in NoLock.  Don't do that.

> The whole point of the NoLock suffix is that you can trust it to mean
> something.  See if you can maintain an invariant of all locks being taken in
> public functions, and all functions called by DatabaseTracker methods being
> private NoLock functions.

Oops, fixed. Acquiring the lock on m_databaseGuard in usageForOrigin().

> >	 {
> > -	 MutexLocker lockDatabase(m_databaseGuard);
> > -	 ASSERT(deletingDatabase(origin, name) || deletingOrigin(origin));
> > +	     MutexLocker lockDatabase(m_databaseGuard);
> > +	     ASSERT(deletingDatabase(origin, name) || deletingOrigin(origin));
> 
> Accidental tabs.

No, intentional. The code in this {} block should be indented by 4 spaces like
all other {} blocks.