Few changes to the NTP remove thumbnails:

- some minor string changes
- there is now a Cancel button next to the Done button to restore the thumbnails removed during the current editing session
- link and favico are opacified as well in remove mode
- now using a dictionary instead of a list to store the URL blacklist for faster access
- now storing URL hashes instead of URLs (for privacy and also because the DictionaryValue breaks down keys on . characters)

BUG=None
TEST=Make sure removing thumbnails works as expected. Start removing thumbnails then click Cancel, verify that the thumbnails just removed are restored.
Review URL: http://codereview.chromium.org/69007

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

4 files changed, 101 insertions, 47 deletions

diff --git a/base/values.h b/base/values.h
index cad1311..fa5b1017 100644
--- a/base/values.h
+++ b/base/values.h
@@ -204,6 +204,9 @@ class DictionaryValue : public Value {
   // Returns true if the current dictionary has a value for the given key.
   bool HasKey(const std::wstring& key) const;
 
+  // Returns the number of Values in this dictionary.
+  size_t GetSize() const { return dictionary_.size(); }
+
   // Clears any current contents of this dictionary.
   void Clear();
 
@@ -214,8 +217,8 @@ class DictionaryValue : public Value {
   // If the key at any step of the way doesn't exist, or exists but isn't
   // a DictionaryValue, a new DictionaryValue will be created and attached
   // to the path in that location.
-  // Note that the dictionary takes ownership of the value
-  // referenced by in_value.
+  // Note that the dictionary takes ownership of the value referenced by
+  // |in_value|.
   bool Set(const std::wstring& path, Value* in_value);
 
   // Convenience forms of Set().  These methods will replace any existing
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index b24edeb..bd22eaa 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -3040,13 +3040,17 @@ each locale. -->
                desc="The 'Show history' link on the new tab page">
         Show full history
       </message>
-      <message name="IDS_NEW_TAB_EDIT_THUMBNAILS"
-         desc="The 'Edit thumbnails' link on the new tab page">
-        Edit thumbnails
+      <message name="IDS_NEW_TAB_REMOVE_THUMBNAILS"
+         desc="The 'Remove thumbnails' link on the new tab page">
+        Remove thumbnails
       </message>
-      <message name="IDS_NEW_TAB_MOST_VISITED_DONE_EDITING_BUTTON"
-         desc="The caption of the button used to exit the edit thumbnails mode">
-        Done editing
+      <message name="IDS_NEW_TAB_MOST_VISITED_DONE_REMOVING_BUTTON"
+         desc="The caption of the button used to validate the removal of thumbnails">
+        Done
+      </message>
+      <message name="IDS_NEW_TAB_MOST_VISITED_CANCEL_REMOVING_BUTTON"
+         desc="The caption of the button used to cancel the current removal thumbnails mode">
+        Cancel
       </message>
       <message name="IDS_NEW_TAB_RESTORE_THUMBNAILS_LINK"
          desc="The link that restores previously removed thumbnails">
diff --git a/chrome/browser/dom_ui/new_tab_ui.cc b/chrome/browser/dom_ui/new_tab_ui.cc
index c4da817..cd9b5d9 100644
--- a/chrome/browser/dom_ui/new_tab_ui.cc
+++ b/chrome/browser/dom_ui/new_tab_ui.cc
@@ -246,13 +246,15 @@ void NewTabHTMLSource::StartDataRequest(const std::string& path,
   localized_strings.SetString(L"showhistoryurl",
       chrome::kChromeUIHistoryURL);
   localized_strings.SetString(L"editthumbnails",
-      l10n_util::GetString(IDS_NEW_TAB_EDIT_THUMBNAILS));
+      l10n_util::GetString(IDS_NEW_TAB_REMOVE_THUMBNAILS));
   localized_strings.SetString(L"restorethumbnails",
       l10n_util::GetString(IDS_NEW_TAB_RESTORE_THUMBNAILS_LINK));
   localized_strings.SetString(L"editmodeheading",
       l10n_util::GetString(IDS_NEW_TAB_MOST_VISITED_EDIT_MODE_HEADING));
   localized_strings.SetString(L"doneediting",
-      l10n_util::GetString(IDS_NEW_TAB_MOST_VISITED_DONE_EDITING_BUTTON));
+      l10n_util::GetString(IDS_NEW_TAB_MOST_VISITED_DONE_REMOVING_BUTTON));
+  localized_strings.SetString(L"cancelediting",
+      l10n_util::GetString(IDS_NEW_TAB_MOST_VISITED_CANCEL_REMOVING_BUTTON));
   localized_strings.SetString(L"searchhistory",
       l10n_util::GetString(IDS_NEW_TAB_HISTORY_SEARCH));
   localized_strings.SetString(L"recentlyclosed",
@@ -354,6 +356,9 @@ class MostVisitedHandler : public DOMMessageHandler,
   // Callback for the "blacklistURLFromMostVisited" message.
   void HandleBlacklistURL(const Value* url);
 
+  // Callback for the "removeURLsFromMostVisitedBlacklist" message.
+  void HandleRemoveURLsFromBlacklist(const Value* url);
+
   // Callback for the "clearMostVisitedURLsBlacklist" message.
   void HandleClearBlacklist(const Value* url);
 
@@ -376,11 +381,8 @@ class MostVisitedHandler : public DOMMessageHandler,
   // Puts the passed URL in the blacklist (so it does not show as a thumbnail).
   void BlacklistURL(const GURL& url);
 
-  // Returns true if the passed URL has been blacklisted.
-  bool IsURLBlacklisted(const GURL& url);
-
-  // Returns the URL blacklist.
-  ListValue* GetURLBlacklist();
+  // Returns the key used in url_blacklist_ for the passed |url|.
+  std::wstring GetBlacklistKeyForURL(const std::string& url);
 
   // Our consumer for the history service.
   CancelableRequestConsumerTSimple<PageUsageData*> cancelable_consumer_;
@@ -390,6 +392,11 @@ class MostVisitedHandler : public DOMMessageHandler,
   // was clicked on for metrics purposes.
   std::vector<GURL> most_visited_urls_;
 
+  // The URL blacklist: URLs we do not want to show in the thumbnails list.  It
+  // is a dictionary for quick access (it associates a dummy boolean to the URL
+  // string).
+  DictionaryValue* url_blacklist_;
+
   DISALLOW_COPY_AND_ASSIGN(MostVisitedHandler);
 };
 
@@ -403,9 +410,14 @@ MostVisitedHandler::MostVisitedHandler(DOMUI* dom_ui)
   // Also register ourselves for any most-visited item blacklisting.
   dom_ui_->RegisterMessageCallback("blacklistURLFromMostVisited",
       NewCallback(this, &MostVisitedHandler::HandleBlacklistURL));
+  dom_ui_->RegisterMessageCallback("removeURLsFromMostVisitedBlacklist",
+      NewCallback(this, &MostVisitedHandler::HandleRemoveURLsFromBlacklist));
   dom_ui_->RegisterMessageCallback("clearMostVisitedURLsBlacklist",
       NewCallback(this, &MostVisitedHandler::HandleClearBlacklist));
 
+  url_blacklist_ = dom_ui_->GetProfile()->GetPrefs()->
+      GetMutableDictionary(prefs::kNTPMostVisitedURLsBlacklist);
+
   // Set up our sources for thumbnail and favicon data. Since we may be in
   // testing mode with no I/O thread, only add our handler when an I/O thread
   // exists. Ownership is passed to the ChromeURLDataManager.
@@ -436,7 +448,7 @@ void MostVisitedHandler::HandleGetMostVisited(const Value* value) {
   const int kMostVisitedCount = 9;
   // Let's query for the number of items we want plus the blacklist size as
   // we'll be filtering-out the returned list with the blacklist URLs.
-  int result_count = kMostVisitedCount + GetURLBlacklist()->GetSize();
+  int result_count = kMostVisitedCount + url_blacklist_->GetSize();
   HistoryService* hs =
       dom_ui_->GetProfile()->GetHistoryService(Profile::EXPLICIT_ACCESS);
   hs->QuerySegmentUsageSince(
@@ -462,8 +474,35 @@ void MostVisitedHandler::HandleBlacklistURL(const Value* value) {
   HandleGetMostVisited(NULL);
 }
 
+void MostVisitedHandler::HandleRemoveURLsFromBlacklist(const Value* urls) {
+  if (!urls->IsType(Value::TYPE_LIST)) {
+    NOTREACHED();
+    return;
+  }
+  const ListValue* list = static_cast<const ListValue*>(urls);
+  if (list->GetSize() == 0) {
+    NOTREACHED();
+    return;
+  }
+
+  for (ListValue::const_iterator iter = list->begin();
+       iter != list->end(); ++iter) {
+    std::wstring url;
+    bool r = (*iter)->GetAsString(&url);
+    if (!r) {
+      NOTREACHED();
+      return;
+    }
+    r = url_blacklist_->Remove(GetBlacklistKeyForURL(WideToUTF8(url)), NULL);
+    DCHECK(r) << "Unknown URL removed from the NTP Most Visited blacklist.";
+  }
+
+  // Force a refresh of the thumbnails.
+  HandleGetMostVisited(NULL);
+}
+
 void MostVisitedHandler::HandleClearBlacklist(const Value* value) {
-  GetURLBlacklist()->Clear();
+  url_blacklist_->Clear();
   // Force a refresh of the thumbnails.
   HandleGetMostVisited(NULL);
 }
@@ -477,7 +516,8 @@ void MostVisitedHandler::OnSegmentUsageAvailable(
   for (size_t i = 0; i < data->size(); ++i) {
     const PageUsageData& page = *(*data)[i];
     GURL url = page.GetURL();
-    if (IsURLBlacklisted(url))
+
+    if (url_blacklist_->HasKey(GetBlacklistKeyForURL(url.spec())))
       continue;
     DictionaryValue* page_value = new DictionaryValue;
     SetURLTitleAndDirection(page_value, page.GetTitle(), page.GetURL());
@@ -502,33 +542,19 @@ void MostVisitedHandler::Observe(NotificationType type,
 }
 
 void MostVisitedHandler::BlacklistURL(const GURL& url) {
-  if (IsURLBlacklisted(url))
+  std::wstring key = GetBlacklistKeyForURL(url.spec());
+  if (url_blacklist_->HasKey(key))
     return;
-  GetURLBlacklist()->Append(Value::CreateStringValue(url.spec()));
-}
-
-bool MostVisitedHandler::IsURLBlacklisted(const GURL& url) {
-  std::string url_spec = url.spec();
-  ListValue* blacklist = GetURLBlacklist();
-  for (ListValue::const_iterator iter = blacklist->begin();
-       iter != blacklist->end(); ++iter) {
-    std::string blacklisted_url;
-    bool success = (*iter)->GetAsString(&blacklisted_url);
-    DCHECK(success);
-    if (url_spec == blacklisted_url)
-      return true;
-  }
-  return false;
+  url_blacklist_->SetBoolean(key, true);
 }
 
-ListValue* MostVisitedHandler::GetURLBlacklist() {
-  return dom_ui_->GetProfile()->GetPrefs()->
-      GetMutableList(prefs::kNTPMostVisitedURLsBlacklist);
+std::wstring MostVisitedHandler::GetBlacklistKeyForURL(const std::string& url) {
+  return ASCIIToWide(MD5String(url));
 }
 
 // static
 void MostVisitedHandler::RegisterUserPrefs(PrefService* prefs) {
-  prefs->RegisterListPref(prefs::kNTPMostVisitedURLsBlacklist);
+  prefs->RegisterDictionaryPref(prefs::kNTPMostVisitedURLsBlacklist);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/resources/new_tab.html b/chrome/browser/resources/new_tab.html
index 56a0ff1..249c002 100644
--- a/chrome/browser/resources/new_tab.html
+++ b/chrome/browser/resources/new_tab.html
@@ -36,6 +36,11 @@ var renderFunctionsDefined = false;
 // TODO(glen): Merge into common file used by this and history.
 var localStrings;
 
+// The list of URLs that have been blacklisted (so that thumbnails are not
+// shown) in the last set of change.  Used to revert changes when the Cancel
+// button is pressed.
+var blacklistedURLs = [];
+
 ///////////////////////////////////////////////////////////////////////////////
 // localStrings:
 /**
@@ -404,7 +409,7 @@ html[dir='rtl'] #searches input {
   left: 0px;
   top: 0px;
 }
-.edit-mode a.most-visited-item {
+.edit-mode .disabled-on-edit {
   opacity: 0.5;
   pointer-events: none;  /* Disable clicks */
 }
@@ -474,6 +479,8 @@ document.addEventListener('DOMContentLoaded', handleDOMContentLoaded);
       <span jscontent="editthumbnails"></span></a>
     <button type="button" class="edit-visible" onClick="exitEditMode();"
 	    jscontent="doneediting"></button>
+    <button type="button" class="edit-visible" onClick="cancelEdits();"
+	    jscontent="cancelediting"></button>
     <a href="#"
        class="manage edit-visible"
        onClick="restoreThumbnails(); return false">
@@ -560,15 +567,16 @@ function bind(fn, selfObj, var_args) {
    |page| should be an object with "title", "url", and "direction" fields. */
 function makeMostVisitedDOM(page, number) {
   /* The HTML we want looks like this:
-    <a class="most-visited-item" href="URL" title="gmail.com">
-      <div class="thumbnail-title"
+    <a class="disabled-on-edit" href="URL" title="gmail.com">
+      <div class="thumbnail-title disabled-on-edit"
            style="background-image:url(faviconurl);direction:ltr">gmail.com</div>
-      <img class="thumbnail" style="background-image:url(thumbnailurl);" />
+      <img class="thumbnail disabled-on-edit"
+	   style="background-image:url(thumbnailurl);" />
     </a>
   */
   var root;
   if (page.url) {
-    root = DOM('a', {className:'most-visited-item',
+    root = DOM('a', {className:'disabled-on-edit',
                      href:page.url,
                      title:page.title});
     root.addEventListener("mousedown", function(event) {
@@ -580,12 +588,12 @@ function makeMostVisitedDOM(page, number) {
   }
 
   /* Create the thumbnail */
-  var img_thumbnail = DOM('img', {className:'thumbnail'});
+  var img_thumbnail = DOM('img', {className:'thumbnail disabled-on-edit'});
   img_thumbnail.setAttribute('onload', "logEvent('image loaded');");
   img_thumbnail.src = 'chrome-ui://thumb/' + page.url;
 
   /* Create the title */
-  var div_title = DOM('div', {className:'thumbnail-title'});
+  var div_title = DOM('div', {className:'thumbnail-title disabled-on-edit'});
   div_title.style.backgroundImage =
       'url("chrome-ui://favicon/' + page.url + '")';
   /* Set the title's directionality independently of the overall page
@@ -619,8 +627,9 @@ function makeMostVisitedDOM(page, number) {
    over a "most visited" entry. */
 function makeCrossImageDOM(url) {
   var cross = DOM('div', {className:'edit-cross'});
-  cross.addEventListener("mousedown", function(event) {
-    chrome.send("blacklistURLFromMostVisited", [url])}, false);
+  cross.addEventListener("mousedown",
+                         function(event) { blacklistURL(url); },
+                         false);
   return cross;
 }
 
@@ -977,6 +986,18 @@ function enterEditMode() {
 
 function exitEditMode() {
   document.getElementById('main').className = 'visible';
+  blacklistedURLs = [];
+}
+
+function cancelEdits() {
+  if (blacklistedURLs.length > 0)
+    chrome.send("removeURLsFromMostVisitedBlacklist", blacklistedURLs);
+  exitEditMode();
+}
+
+function blacklistURL(url) {
+ blacklistedURLs.push(url);
+ chrome.send("blacklistURLFromMostVisited", [url]);
 }
 
 function restoreThumbnails() {