WebCore:

        Reviewed by Alice

        Added a truth value to setIconURLForPageURL so WebKit can avoid sending a notification
        This is a win on the iBench

        * bridge/mac/WebCoreIconDatabaseBridge.h:
        * bridge/mac/WebCoreIconDatabaseBridge.mm:
        (-[WebCoreIconDatabaseBridge _setIconURL:forPageURL:]):
        * loader/icon/IconDatabase.cpp:
        (WebCore::IconDatabase::setIconURLForPageURL):
        * loader/icon/IconDatabase.h:

WebKit:

        Reviewed by Alice

        Added a truth value check for to setIconURL:forURL so WebKit can avoid sending a notification
        This is a win on the iBench

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

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

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index dd20953e21421927c644394a63e78b6be55f3220..0239265e6096062fbc79123f83753a6d58832952 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,17 @@
+2006-08-29  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Alice
+
+        Added a truth value to setIconURLForPageURL so WebKit can avoid sending a notification
+        This is a win on the iBench
+
+        * bridge/mac/WebCoreIconDatabaseBridge.h:
+        * bridge/mac/WebCoreIconDatabaseBridge.mm:
+        (-[WebCoreIconDatabaseBridge _setIconURL:forPageURL:]):
+        * loader/icon/IconDatabase.cpp:
+        (WebCore::IconDatabase::setIconURLForPageURL):
+        * loader/icon/IconDatabase.h:
+
 2006-08-29  Alice Liu  <alice.liu@apple.com>
 
         Reviewed by Brady.

diff --git a/WebCore/bridge/mac/WebCoreIconDatabaseBridge.h b/WebCore/bridge/mac/WebCoreIconDatabaseBridge.h
index a4b9a2204a4ded2f143a62b101c5882979b990ff..55e2ae011611eca26a56ead4c5081703d5ce86bd 100644 (file)
--- a/WebCore/bridge/mac/WebCoreIconDatabaseBridge.h
+++ b/WebCore/bridge/mac/WebCoreIconDatabaseBridge.h
@@ -59,7 +59,7 @@ typedef WebCore::IconDatabase WebCoreIconDatabase;
 
 - (void)_setIconData:(NSData *)data forIconURL:(NSString *)iconURL;
 - (void)_setHaveNoIconForIconURL:(NSString *)iconURL;
-- (void)_setIconURL:(NSString *)iconURL forPageURL:(NSString *)pageURL;
+- (BOOL)_setIconURL:(NSString *)iconURL forPageURL:(NSString *)pageURL;
 - (BOOL)_hasEntryForIconURL:(NSString *)iconURL;
 
 - (BOOL)_isEmpty;

diff --git a/WebCore/bridge/mac/WebCoreIconDatabaseBridge.mm b/WebCore/bridge/mac/WebCoreIconDatabaseBridge.mm
index a115e77e977c296c61e458e3f09e6ea4d19cd85a..c23aa85ca73763801fcb598186d6ab8dac4037b7 100644 (file)
--- a/WebCore/bridge/mac/WebCoreIconDatabaseBridge.mm
+++ b/WebCore/bridge/mac/WebCoreIconDatabaseBridge.mm
@@ -171,13 +171,13 @@ void WebCore::IconDatabase::loadIconFromURL(const String& url)
     _iconDB->setHaveNoIconForIconURL(String(iconURL));
 }
 
-- (void)_setIconURL:(NSString *)iconURL forPageURL:(NSString *)pageURL
+- (BOOL)_setIconURL:(NSString *)iconURL forPageURL:(NSString *)pageURL
 {
     ASSERT(_iconDB);
     ASSERT(iconURL);
     ASSERT(pageURL);
     
-    _iconDB->setIconURLForPageURL(String(iconURL), String(pageURL));
+    return _iconDB->setIconURLForPageURL(String(iconURL), String(pageURL));
 }
 
 - (BOOL)_hasEntryForIconURL:(NSString *)iconURL

diff --git a/WebCore/loader/icon/IconDatabase.cpp b/WebCore/loader/icon/IconDatabase.cpp
index 0112ecfb86cfaf380320e951e521935a42d1af67..206c4fefe8851394b032dca379b2f86ae81e9df8 100644 (file)
--- a/WebCore/loader/icon/IconDatabase.cpp
+++ b/WebCore/loader/icon/IconDatabase.cpp
@@ -568,7 +568,7 @@ void IconDatabase::setHaveNoIconForIconURL(const String& iconURL)
     setIconDataForIconURL(0, 0, iconURL);
 }
 
-void IconDatabase::setIconURLForPageURL(const String& iconURL, const String& pageURL)
+bool IconDatabase::setIconURLForPageURL(const String& iconURL, const String& pageURL)
 {
     ASSERT(!iconURL.isEmpty());
     ASSERT(!pageURL.isEmpty());
@@ -576,7 +576,7 @@ void IconDatabase::setIconURLForPageURL(const String& iconURL, const String& pag
     // If the urls already map to each other, bail.
     // This happens surprisingly often, and seems to cream iBench performance
     if (m_pageURLToIconURLMap.get(pageURL) == iconURL)
-        return;
+        return false;
 
     // If this pageURL is retained, we have some work to do on the IconURL retain counts
     if (m_pageURLToRetainCount.contains(pageURL)) {
@@ -605,6 +605,8 @@ void IconDatabase::setIconURLForPageURL(const String& iconURL, const String& pag
     // Then start the timer to commit this change - or further delay the timer if it
     // was already started
     m_updateTimer.startOneShot(updateTimerDelay);
+    
+    return true;
 }
 
 void IconDatabase::setIconURLForPageURLInDatabase(const String& iconURL, const String& pageURL)

diff --git a/WebCore/loader/icon/IconDatabase.h b/WebCore/loader/icon/IconDatabase.h
index 8cf9665430c43f2b009528228f9dce9ad9fa431b..9f5c2fb5bb90741446807bc28af6f19b4eb5527e 100644 (file)
--- a/WebCore/loader/icon/IconDatabase.h
+++ b/WebCore/loader/icon/IconDatabase.h
@@ -82,12 +82,11 @@ public:
 
     bool isIconExpiredForIconURL(const String&);
     
-    // TODO - The following 3 methods were considered private in WebKit - analyze the impact of making them
-    // public here in WebCore - I don't see any real badness with doing that...  after all if Chuck Norris wants to muck
-    // around with the icons in his database, he's going to anyway
     void setIconDataForIconURL(const void* data, int size, const String&);
     void setHaveNoIconForIconURL(const String&);
-    void setIconURLForPageURL(const String& iconURL, const String& pageURL);
+    
+    // Returns true if the set actually took place, false if the mapping already existed
+    bool setIconURLForPageURL(const String& iconURL, const String& pageURL);
 
     static const String& defaultDatabaseFilename();
     

diff --git a/WebKit/ChangeLog b/WebKit/ChangeLog
index a68f71bdd565c65d616eef84cd1c9338d6f7b54d..ea4aeac4f3439609c0eb466a8859755e6bc11b7f 100644 (file)
--- a/WebKit/ChangeLog
+++ b/WebKit/ChangeLog
@@ -1,3 +1,13 @@
+2006-08-29  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Alice
+
+        Added a truth value check for to setIconURL:forURL so WebKit can avoid sending a notification
+        This is a win on the iBench
+
+        * Misc/WebIconDatabase.m:
+        (-[WebIconDatabase _setIconURL:forURL:]):
+
 2006-08-29  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Tim Hatchers rubber stamp

diff --git a/WebKit/Misc/WebIconDatabase.m b/WebKit/Misc/WebIconDatabase.m
index 13ee75884b4a505f96fee206ae9534268eb7abde..06cb6cf558e12b05e39f8900c2a4badeca46059a 100644 (file)
--- a/WebKit/Misc/WebIconDatabase.m
+++ b/WebKit/Misc/WebIconDatabase.m
@@ -231,22 +231,11 @@ NSSize WebIconLargeSize = {128, 128};
     ASSERT(URL);
     ASSERT([self _isEnabled]);
     
-    // FIXME - The following comment is a holdover from the old icon DB, which handled missing icons
-    // differently than the new db.  It would return early if the icon is in the negative cache,
-    // avoiding the notification.  We should explore and see if a similar optimization can take place-
-        // If the icon is in the negative cache (ie, there is no icon), avoid the
-        // work of delivering a notification for it or saving it to disk. This is a significant
-        // win on the iBench HTML test.
-        
-    // FIXME - The following comment is also a holdover - if the iconURL->pageURL mapping was already the
-    // same, the notification would again be avoided - we should try to do this, too.
-        // Don't do any work if the icon URL is already bound to the site URL
-    
-    // A possible solution for both of these is to have the bridge method return a BOOL saying "Yes, notify" or
-    // "no, don't bother notifying"
-    
-    [_private->databaseBridge _setIconURL:iconURL forPageURL:URL];
-    [self _sendNotificationForURL:URL];
+    // If this iconURL already maps to this pageURL, don't bother sending the notification
+    // The WebCore::IconDatabase returns TRUE if we should send the notification, and false if we shouldn't.
+    // This is a measurable win on the iBench - about 1% worth on average
+    if ([_private->databaseBridge _setIconURL:iconURL forPageURL:URL])
+        [self _sendNotificationForURL:URL];
 }
 
 - (BOOL)_hasEntryForIconURL:(NSString *)iconURL;