WebCore:
authorbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Aug 2006 21:05:23 +0000 (21:05 +0000)
committerbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Aug 2006 21:05:23 +0000 (21:05 +0000)
        Reviewed by John

        -Defers writing to the database via a timer and handles starting the timer
         intelligently

        * loader/icon/IconDatabase.cpp:
        (WebCore::IconDatabase::IconDatabase):
        (WebCore::IconDatabase::open): Don't start the update timer by default
        (WebCore::IconDatabase::close): Call syncDatabase()
        (WebCore::IconDatabase::setPrivateBrowsingEnabled): Call syncDatabase()
        (WebCore::IconDatabase::setIconURLForPageURL): Setup the cached url, but defer the DB call
        (WebCore::IconDatabase::setIconURLForPageURLInDatabase): Actually commit the url to the DB
        (WebCore::IconDatabase::pruneUnretainedIconsOnStartup):
        (WebCore::IconDatabase::updateDatabase): The updateTimer calls this, which just calls syncDatabase()
        (WebCore::IconDatabase::syncDatabase): Add and remove pending pageURLs and iconURLs, and stop the updateTimer
        * loader/icon/IconDatabase.h:

WebKit:

        Reviewed by John

        Quick ICONDEBUG flag fix

        * Misc/WebIconDatabase.m:
        (-[WebIconDatabase _applicationWillTerminate:]):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@15953 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/loader/icon/IconDatabase.cpp
WebCore/loader/icon/IconDatabase.h
WebKit/ChangeLog
WebKit/Misc/WebIconDatabase.m

index 513646dd447a8cf1638b7cd15690c306dda9a403..0814c95270a4b13cbbda401fe529ff0e56e50c51 100644 (file)
@@ -1,3 +1,22 @@
+2006-08-21  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by John
+
+        -Defers writing to the database via a timer and handles starting the timer
+         intelligently
+
+        * loader/icon/IconDatabase.cpp:
+        (WebCore::IconDatabase::IconDatabase):
+        (WebCore::IconDatabase::open): Don't start the update timer by default
+        (WebCore::IconDatabase::close): Call syncDatabase()
+        (WebCore::IconDatabase::setPrivateBrowsingEnabled): Call syncDatabase()
+        (WebCore::IconDatabase::setIconURLForPageURL): Setup the cached url, but defer the DB call
+        (WebCore::IconDatabase::setIconURLForPageURLInDatabase): Actually commit the url to the DB
+        (WebCore::IconDatabase::pruneUnretainedIconsOnStartup):
+        (WebCore::IconDatabase::updateDatabase): The updateTimer calls this, which just calls syncDatabase()
+        (WebCore::IconDatabase::syncDatabase): Add and remove pending pageURLs and iconURLs, and stop the updateTimer
+        * loader/icon/IconDatabase.h:
+
 2006-08-21  Alexey Proskuryakov  <ap@nypop.com>
 
         Reviewed by Eric.
index bcfade519065c9c552e4b7c28bbc29de8e88f8f3..69e3c6ef5f2959afd135bd36399f79d561c4a188 100644 (file)
@@ -35,8 +35,6 @@
 #include <time.h>
 
 
-// FIXME - Make sure we put a private browsing consideration in that uses the temporary tables anytime private browsing would be an issue.
-
 // FIXME - One optimization to be made when this is no longer in flux is to make query construction smarter - that is queries that are created from
 // multiple strings and numbers should be handled differently than with String + String + String + etc.
 
@@ -56,6 +54,8 @@ const int IconDatabase::iconExpirationTime = 60*60*24;
 // Absent icons are rechecked once a week
 const int IconDatabase::missingIconExpirationTime = 60*60*24*7; 
 
+const int IconDatabase::updateTimerDelay = 5; 
+
 const String& IconDatabase::defaultDatabaseFilename()
 {
     static String defaultDatabaseFilename = "icon.db";
@@ -93,7 +93,7 @@ IconDatabase::IconDatabase()
     : m_currentDB(&m_mainDB)
     , m_privateBrowsingEnabled(false)
     , m_startupTimer(this, &IconDatabase::pruneUnretainedIconsOnStartup)
-    , m_pruneTimer(this, &IconDatabase::pruneIconsPendingDeletion)
+    , m_updateTimer(this, &IconDatabase::updateDatabase)
 {
     
 }
@@ -143,16 +143,15 @@ bool IconDatabase::open(const String& databasePath)
     // rdar://4690949 - when we have deferred reads and writes all the way in, the prunetimer
     // will become "deferredTimer" or something along those lines, and will be set only when
     // a deferred read/write is queued
-    if (isOpen()) {
+    if (isOpen())
         m_startupTimer.startOneShot(0);
-        m_pruneTimer.startRepeating(10);
-    }
     
     return isOpen();
 }
 
 void IconDatabase::close()
 {
+    syncDatabase();
     m_mainDB.close();
     m_privateBrowsingDB.close();
 }
@@ -243,6 +242,9 @@ void IconDatabase::setPrivateBrowsingEnabled(bool flag)
     if (m_privateBrowsingEnabled == flag)
         return;
     
+    // Sync any deferred DB changes before we change the active DB
+    syncDatabase();
+    
     m_privateBrowsingEnabled = flag;
     
     if (m_privateBrowsingEnabled) {
@@ -526,12 +528,31 @@ void IconDatabase::setIconURLForPageURL(const String& iconURL, const String& pag
         if (!oldIconURL.isEmpty())
             releaseIconURL(oldIconURL);
         retainIconURL(iconURL);
+    } else {
+        // If this pageURL is *not* retained, then we may be marking it for deletion, as well!
+        // As counterintuitive as it seems to mark it for addition and for deletion at the same time,
+        // it's valid because when we do a new pageURL->iconURL mapping we *have* to mark it for addition,
+        // no matter what, as there is no efficient was to determine if the mapping is in the DB already.
+        // But, if the iconURL is marked for deletion, we'll also mark this pageURL for deletion - if a 
+        // client comes along and retains it before the timer fires, the "pendingDeletion" lists will
+        // be manipulated appopriately and new pageURL will be brought back from the brink
+        if (m_iconURLsPendingDeletion.contains(iconURL))
+            m_pageURLsPendingDeletion.add(pageURL);
     }
     
     // Cache the pageURL->iconURL map
     m_pageURLToIconURLMap.set(pageURL, iconURL);
+    
+    // And mark this mapping to be added to the database
+    m_pageURLsPendingAddition.set(pageURL, iconURL);
+    
+    // Then start the timer to commit this change - or further delay the timer if it
+    // was already started
+    m_updateTimer.startOneShot(updateTimerDelay);
+}
 
-    // Store it in the database
+void IconDatabase::setIconURLForPageURLInDatabase(const String& iconURL, const String& pageURL)
+{
     int64_t iconID = establishIconIDForIconURL(*m_currentDB, iconURL);
     if (!iconID) {
         LOG_ERROR("Failed to establish an ID for iconURL %s", iconURL.ascii().data());
@@ -573,7 +594,6 @@ void IconDatabase::pruneUnretainedIconsOnStartup(Timer<IconDatabase>*)
     if (!isOpen())
         return;
         
-// FIXME - The PageURL delete and the pruneunreferenced icons need to be in an atomic transaction
 #ifndef NDEBUG
     CFTimeInterval start = CFAbsoluteTimeGetCurrent();
 #endif
@@ -590,15 +610,29 @@ void IconDatabase::pruneUnretainedIconsOnStartup(Timer<IconDatabase>*)
 #endif
 }
 
-void IconDatabase::pruneIconsPendingDeletion(Timer<IconDatabase>*)
+void IconDatabase::updateDatabase(Timer<IconDatabase>*)
+{
+    syncDatabase();
+}
+
+void IconDatabase::syncDatabase()
 {
 #ifndef NDEBUG
     CFTimeInterval start = CFAbsoluteTimeGetCurrent();
 #endif
 
+    // First we'll do the pending additions
+    for (HashMap<String, String>::iterator i = m_pageURLsPendingAddition.begin(); i != m_pageURLsPendingAddition.end(); ++i) {
+        // The HashMap maps PageURL->IconURL, but the method takes IconURL then PageURL - so this ordering is correct
+        setIconURLForPageURLInDatabase(i->second, i->first);
+        LOG(IconDatabase, "Commited IconURL %s for PageURL %s to database", (i->second).ascii().data(), (i->first).ascii().data());
+    }
+    m_pageURLsPendingAddition.clear();
+    
+    // Then we'll do the pending deletions
     // First lets wipe all the pageURLs
     HashSet<String>::iterator i = m_pageURLsPendingDeletion.begin();
-    for (;i != m_pageURLsPendingDeletion.end(); ++i) {    
+    for (; i != m_pageURLsPendingDeletion.end(); ++i) {    
         forgetPageURL(*i);
         LOG(IconDatabase, "Deleted PageURL %s", (*i).ascii().data());
     }
@@ -606,7 +640,7 @@ void IconDatabase::pruneIconsPendingDeletion(Timer<IconDatabase>*)
 
     // Then get rid of all traces of the icons and IconURLs
     SiteIcon* icon;    
-    for (i = m_iconURLsPendingDeletion.begin();i != m_iconURLsPendingDeletion.end(); ++i) {
+    for (i = m_iconURLsPendingDeletion.begin(); i != m_iconURLsPendingDeletion.end(); ++i) {
         // Forget the SiteIcon
         icon = m_iconURLToSiteIcons.get(*i);
         m_iconURLToSiteIcons.remove(*i);
@@ -618,11 +652,14 @@ void IconDatabase::pruneIconsPendingDeletion(Timer<IconDatabase>*)
     }
     m_iconURLsPendingDeletion.clear();
     
+    // If the timer was running to cause this update, we can kill the timer as its firing would be redundant
+    m_updateTimer.stop();
+    
 #ifndef NDEBUG
     CFTimeInterval duration = CFAbsoluteTimeGetCurrent() - start;
-    LOG(IconDatabase, "Wiping the items pending-deletion took %.4f seconds", duration);
+    LOG(IconDatabase, "Updating the database took %.4f seconds", duration);
     if (duration > 1.0) 
-        LOG_ERROR("Wiping the items pending-deletion took %.4f seconds - this is much too long!", duration);
+        LOG_ERROR("Updating the database took %.4f seconds - this is much too long!", duration);
 #endif
 }
 
@@ -653,7 +690,6 @@ IconDatabase::~IconDatabase()
     close();
 }
 
-
 // Query helper functions
 bool pageURLTableIsEmptyQuery(SQLDatabase& db)
 {
index 360a507faf3c9bf4023aef346ece15f2d91d5bcf..f4c0b31c01420651b1dba6090dbbceebd7ada451 100644 (file)
@@ -121,6 +121,7 @@ public:
     static const int currentDatabaseVersion;    
     static const int iconExpirationTime;
     static const int missingIconExpirationTime;
+    static const int updateTimerDelay;
 private:
     IconDatabase();
     ~IconDatabase();
@@ -135,6 +136,8 @@ private:
     // Remove the current database entry for this IconURL
     void forgetIconForIconURLFromDatabase(const String&);
     
+    void setIconURLForPageURLInDatabase(const String&, const String&);
+    
     // Wipe all icons from the DB
     void removeAllIcons();
     
@@ -148,7 +151,10 @@ private:
     
     // Called by the prune timer, this method periodically removes all the icons in the pending-deletion
     // queue
-    void pruneIconsPendingDeletion(Timer<IconDatabase>*);
+    void updateDatabase(Timer<IconDatabase>*);
+    
+    // This is called by updateDatabase and when private browsing shifts, and when the DB is closed down
+    void syncDatabase();
     
     // Determine if an IconURL is still retained by anyone
     bool isIconURLRetained(const String&);
@@ -186,17 +192,18 @@ private:
     bool m_privateBrowsingEnabled;
     
     Timer<IconDatabase> m_startupTimer;
-    Timer<IconDatabase> m_pruneTimer;
+    Timer<IconDatabase> m_updateTimer;
     
     typedef HashMap<String, SiteIcon*> SiteIconMap;
     SiteIconMap m_iconURLToSiteIcons;
     
-    HashMap<String,String> m_pageURLToIconURLMap;
-    
+    HashMap<String, String> m_pageURLToIconURLMap;
+    HashMap<String, String> m_pageURLsPendingAddition;
+
     // This will keep track of the retaincount for each pageURL
-    HashMap<String,int> m_pageURLToRetainCount;
+    HashMap<String, int> m_pageURLToRetainCount;
     // This will keep track of the retaincount for each iconURL (ie - the number of pageURLs retaining this icon)
-    HashMap<String,int> m_iconURLToRetainCount;
+    HashMap<String, int> m_iconURLToRetainCount;
 
     HashSet<String> m_pageURLsPendingDeletion;
     HashSet<String> m_iconURLsPendingDeletion;
index f23e43c04758ed70a09f0cdbb0234ba666382290..aaa80c27bb4110831cda7f60f20f9f163ae1ccbf 100644 (file)
@@ -1,3 +1,12 @@
+2006-08-21  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by John
+
+        Quick ICONDEBUG flag fix
+
+        * Misc/WebIconDatabase.m:
+        (-[WebIconDatabase _applicationWillTerminate:]):
+
 2006-08-21  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Anders
index 3d5672bf6f5fc676403bc8cdf6d1a4e319250a71..811343d9e4c5397b8eb69a4c6b0bd4570cfa2391 100644 (file)
@@ -698,9 +698,11 @@ NSSize WebIconLargeSize = {128, 128};
     // Should only cause a write if user quit before 3 seconds after the last _updateFileDatabase
     [_private->fileDatabase sync];
     
+#ifdef ICONDEBUG
     [_private->databaseBridge closeSharedDatabase];
     [_private->databaseBridge release];
     _private->databaseBridge = nil;
+#endif
     _isClosing = YES;
 }