Make us save favicon in incognito mode if the url is bookmarked. This

way the bookmark bar/manager have a favicon for the page.

BUG=22670
TEST=see bug

Review URL: http://codereview.chromium.org/5753007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69345 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 86 insertions, 5 deletions

diff --git a/chrome/browser/fav_icon_helper.cc b/chrome/browser/fav_icon_helper.cc
index bcc3e90..544b4e8 100644
--- a/chrome/browser/fav_icon_helper.cc
+++ b/chrome/browser/fav_icon_helper.cc
@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/ref_counted_memory.h"
+#include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
@@ -78,7 +79,7 @@ void FavIconHelper::SetFavIcon(
       (image.width() == kFavIconSize && image.height() == kFavIconSize)
       ? image : ConvertToFavIconSize(image);
 
-  if (GetFaviconService() && !profile()->IsOffTheRecord()) {
+  if (GetFaviconService() && ShouldSaveFavicon(url)) {
     std::vector<unsigned char> image_data;
     gfx::PNGCodec::EncodeBGRASkBitmap(sized_image, false, &image_data);
     GetFaviconService()->SetFavicon(url, image_url, image_data);
@@ -296,3 +297,12 @@ SkBitmap FavIconHelper::ConvertToFavIconSize(const SkBitmap& image) {
   }
   return image;
 }
+
+bool FavIconHelper::ShouldSaveFavicon(const GURL& url) {
+  if (!profile()->IsOffTheRecord())
+    return true;
+
+  // Otherwise store the favicon if the page is bookmarked.
+  BookmarkModel* bookmark_model = profile()->GetBookmarkModel();
+  return bookmark_model && bookmark_model->IsBookmarked(url);
+}
diff --git a/chrome/browser/fav_icon_helper.h b/chrome/browser/fav_icon_helper.h
index 8eb7f17..a64eba2 100644
--- a/chrome/browser/fav_icon_helper.h
+++ b/chrome/browser/fav_icon_helper.h
@@ -156,6 +156,9 @@ class FavIconHelper : public RenderViewHostDelegate::FavIcon {
   // wide. Does nothing if the image is empty.
   SkBitmap ConvertToFavIconSize(const SkBitmap& image);
 
+  // Returns true if the favicon should be saved.
+  bool ShouldSaveFavicon(const GURL& url);
+
   // Hosting TabContents. We callback into this when done.
   TabContents* tab_contents_;
 
diff --git a/chrome/browser/history/history.cc b/chrome/browser/history/history.cc
index 73efc91..0736fc2 100644
--- a/chrome/browser/history/history.cc
+++ b/chrome/browser/history/history.cc
@@ -357,6 +357,14 @@ void HistoryService::AddPage(const history::HistoryAddPageArgs& add_page_args) {
                         add_page_args.Clone()));
 }
 
+void HistoryService::AddPageNoVisitForBookmark(const GURL& url) {
+  if (!CanAddURL(url))
+    return;
+
+  ScheduleAndForget(PRIORITY_NORMAL,
+                    &HistoryBackend::AddPageNoVisitForBookmark, url);
+}
+
 void HistoryService::SetPageTitle(const GURL& url,
                                   const string16& title) {
   ScheduleAndForget(PRIORITY_NORMAL, &HistoryBackend::SetPageTitle, url, title);
diff --git a/chrome/browser/history/history.h b/chrome/browser/history/history.h
index 3fe34ba..3e604e6 100644
--- a/chrome/browser/history/history.h
+++ b/chrome/browser/history/history.h
@@ -212,6 +212,11 @@ class HistoryService : public CancelableRequestProvider,
   // All AddPage variants end up here.
   void AddPage(const history::HistoryAddPageArgs& add_page_args);
 
+  // Adds an entry for the specified url without creating a visit. This should
+  // only be used when bookmarking a page, otherwise the row leaks in the
+  // history db (it never gets cleaned).
+  void AddPageNoVisitForBookmark(const GURL& url);
+
   // Sets the title for the given page. The page should be in history. If it
   // is not, this operation is ignored. This call will not update the full
   // text index. The last title set when the page is indexed will be the
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 0bf6045..cd0a40b 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -870,6 +870,23 @@ void HistoryBackend::SetPageTitle(const GURL& url,
     ScheduleCommit();
 }
 
+void HistoryBackend::AddPageNoVisitForBookmark(const GURL& url) {
+  if (!db_.get())
+    return;
+
+  URLRow url_info(url);
+  URLID url_id = db_->GetRowForURL(url, &url_info);
+  if (url_id) {
+    // URL is already known, nothing to do.
+    return;
+  }
+  url_info.set_last_visit(Time::Now());
+  // Mark the page hidden. If the user types it in, it'll unhide.
+  url_info.set_hidden(true);
+
+  db_->AddURL(url_info);
+}
+
 void HistoryBackend::IterateURLs(HistoryService::URLEnumerator* iterator) {
   if (db_.get()) {
     HistoryDatabase::URLEnumerator e;
diff --git a/chrome/browser/history/history_backend.h b/chrome/browser/history/history_backend.h
index b8c27d6..4ce7aea 100644
--- a/chrome/browser/history/history_backend.h
+++ b/chrome/browser/history/history_backend.h
@@ -121,6 +121,7 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
 
   void AddPage(scoped_refptr<HistoryAddPageArgs> request);
   virtual void SetPageTitle(const GURL& url, const string16& title);
+  void AddPageNoVisitForBookmark(const GURL& url);
 
   // Indexing ------------------------------------------------------------------
 
diff --git a/chrome/browser/history/history_types.cc b/chrome/browser/history/history_types.cc
index 8a41ae8..05d9a72 100644
--- a/chrome/browser/history/history_types.cc
+++ b/chrome/browser/history/history_types.cc
@@ -60,8 +60,8 @@ void URLRow::Swap(URLRow* other) {
 
 void URLRow::Initialize() {
   id_ = 0;
-  visit_count_ = false;
-  typed_count_ = false;
+  visit_count_ = 0;
+  typed_count_ = 0;
   last_visit_ = Time();
   hidden_ = false;
   favicon_id_ = 0;
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index a5cf474..3aeb4e0 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -40,6 +40,7 @@
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/external_protocol_handler.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/history/history.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/host_zoom_map.h"
@@ -99,6 +100,7 @@
 #include "chrome/common/render_messages_params.h"
 #include "chrome/common/renderer_preferences.h"
 #include "chrome/common/url_constants.h"
+#include "gfx/codec/png_codec.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
@@ -971,6 +973,33 @@ void TabContents::ShowPageInfo(const GURL& url,
   delegate_->ShowPageInfo(profile(), url, ssl, show_history);
 }
 
+void TabContents::SaveFavicon() {
+  NavigationEntry* entry = controller_.GetActiveEntry();
+  if (!entry || entry->url().is_empty())
+    return;
+
+  // Make sure the page is in history, otherwise adding the favicon does
+  // nothing.
+  HistoryService* history = profile()->GetOriginalProfile()->GetHistoryService(
+      Profile::IMPLICIT_ACCESS);
+  if (!history)
+    return;
+  history->AddPageNoVisitForBookmark(entry->url());
+
+  FaviconService* service = profile()->GetOriginalProfile()->GetFaviconService(
+      Profile::IMPLICIT_ACCESS);
+  if (!service)
+    return;
+  const NavigationEntry::FaviconStatus& favicon(entry->favicon());
+  if (!favicon.is_valid() || favicon.url().is_empty() ||
+      favicon.bitmap().empty()) {
+    return;
+  }
+  std::vector<unsigned char> image_data;
+  gfx::PNGCodec::EncodeBGRASkBitmap(favicon.bitmap(), false, &image_data);
+  service->SetFavicon(entry->url(), favicon.url(), image_data);
+}
+
 ConstrainedWindow* TabContents::CreateConstrainedDialog(
       ConstrainedWindowDelegate* delegate) {
   ConstrainedWindow* window =
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 3ae7941..c5448c8 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -403,6 +403,9 @@ class TabContents : public PageNavigator,
                     const NavigationEntry::SSLStatus& ssl,
                     bool show_history);
 
+  // Saves the favicon for the current page.
+  void SaveFavicon();
+
   // Window management ---------------------------------------------------------
 
   // Create a new window constrained to this TabContents' clip and visibility.
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 8f8c281..895e945 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -1501,9 +1501,14 @@ void Browser::BookmarkCurrentPage() {
 
   GURL url;
   string16 title;
-  bookmark_utils::GetURLAndTitleToBookmark(GetSelectedTabContents(), &url,
-                                           &title);
+  TabContents* tab = GetSelectedTabContents();
+  bookmark_utils::GetURLAndTitleToBookmark(tab, &url, &title);
   bool was_bookmarked = model->IsBookmarked(url);
+  if (!was_bookmarked && profile_->IsOffTheRecord()) {
+    // If we're off the record the favicon may not have been saved. Save it now
+    // so that bookmarks have an icon for the page.
+    tab->SaveFavicon();
+  }
   model->SetURLStarred(url, title, true);
   // Make sure the model actually added a bookmark before showing the star. A
   // bookmark isn't created if the url is invalid.