[protector] Support for collapsing multiple changes into a single one.

chrome/browser/protector/protector_service.cc

Index: chrome/browser/protector/protector_service.cc
diff --git a/chrome/browser/protector/protector_service.cc b/chrome/browser/protector/protector_service.cc
index ce8b20a85cb2e331655130d13bba30920718fad4..eeedcbebaea52110059cf3afc3ea17419e8dd7c5 100644
--- a/chrome/browser/protector/protector_service.cc
+++ b/chrome/browser/protector/protector_service.cc
@@ -6,8 +6,10 @@
 
 #include "base/logging.h"
 #include "chrome/browser/prefs/pref_service.h"
-#include "chrome/browser/protector/settings_change_global_error.h"
+#include "chrome/browser/protector/composite_settings_change.h"
+#include "chrome/browser/protector/keys.h"
 #include "chrome/browser/protector/protected_prefs_watcher.h"
+#include "chrome/browser/protector/settings_change_global_error.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
@@ -28,6 +30,7 @@ ProtectorService::~ProtectorService() {
 
 void ProtectorService::ShowChange(BaseSettingChange* change) {
   DCHECK(change);
+
   Item new_item;
   new_item.change.reset(change);

[whywhat, 2012/04/06 16:28:32]

Maybe not do it here since we call release later in true branch of the if?

[Ivan Korotkov, 2012/04/09 14:46:47]

Done.

   DVLOG(1) << "Init change";
@@ -35,13 +38,27 @@ void ProtectorService::ShowChange(BaseSettingChange* change) {
     LOG(WARNING) << "Error while initializing, dismissing change";
     return;
   }
-  SettingsChangeGlobalError* error =
-      new SettingsChangeGlobalError(change, this);
-  new_item.error.reset(error);
-  items_.push_back(new_item);
-  // Do not show the bubble immediately if another one is active.
-  error->AddToProfile(profile_, !has_active_change_);
-  has_active_change_ = true;
+
+  Item* merger_item = FindItemToMergeWith(change);

[whywhat, 2012/04/06 16:28:32]

merger_item -> item_to_merge_with?

[Ivan Korotkov, 2012/04/09 14:46:47]

Done.

+  if (merger_item) {
+    CompositeSettingsChange* merged_change =
+        merger_item->change.release()->MergeWith(new_item.change.release());

[whywhat, 2012/04/06 16:28:32]

Would split this into two statements with a comment why each release is needed.

[Ivan Korotkov, 2012/04/09 14:46:47]

Done.

+    merger_item->change.reset(merged_change);
+    merger_item->is_merging = true;
+    if (merger_item->error->GetBubbleView())
+      merger_item->show_when_merged = true;
+    // Remove old GlobalError instance. Later in OnRemovedFromProfile() a new
+    // GlobalError instance will be created for the composite change.
+    merger_item->error->RemoveFromProfile();
+  } else {
+    SettingsChangeGlobalError* error =
+        new SettingsChangeGlobalError(change, this);
+    new_item.error.reset(error);
+    items_.push_back(new_item);
+    // Do not show the bubble immediately if another one is active.
+    error->AddToProfile(profile_, !has_active_change_);
+    has_active_change_ = true;
+  }
 }
 
 bool ProtectorService::IsShowingChange() const {
@@ -61,8 +78,8 @@ void ProtectorService::DiscardChange(BaseSettingChange* change,
 }
 
 void ProtectorService::DismissChange(BaseSettingChange* change) {
-  std::vector<Item>::iterator item =
-      std::find_if(items_.begin(), items_.end(), MatchItemByChange(change));
+  Items::iterator item = std::find_if(items_.begin(), items_.end(),
+                                      MatchItemByChange(change));
   DCHECK(item != items_.end());
   item->error->RemoveFromProfile();
 }
@@ -76,6 +93,18 @@ ProtectedPrefsWatcher* ProtectorService::GetPrefsWatcher() {
   return prefs_watcher_.get();
 }
 
+ProtectorService::Item* ProtectorService::FindItemToMergeWith(
+    const BaseSettingChange* change) {
+  std::string keyword = change->GetApplyKeyword();
+  if (keyword.empty())
+    return NULL;
+  for (Items::iterator item = items_.begin(); item != items_.end(); item++) {
+    if (item->change->GetApplyKeyword() == keyword)
+      return &*item;
+  }
+  return NULL;
+}
+
 void ProtectorService::Shutdown() {
   while (IsShowingChange())
     items_[0].error->RemoveFromProfile();
@@ -101,13 +130,27 @@ void ProtectorService::OnDecisionTimeout(SettingsChangeGlobalError* error) {
 }
 
 void ProtectorService::OnRemovedFromProfile(SettingsChangeGlobalError* error) {
-  std::vector<Item>::iterator item =
-      std::find_if(items_.begin(), items_.end(), MatchItemByError(error));
+  Items::iterator item = std::find_if(items_.begin(), items_.end(),
+                                      MatchItemByError(error));
   DCHECK(item != items_.end());
+
+  if (item->is_merging) {
+    bool show_new_error = !has_active_change_ || item->show_when_merged;
+    item->is_merging = false;

[whywhat, 2012/04/06 16:28:32]

why do we have to false the fields here?

[Ivan Korotkov, 2012/04/09 14:46:47]

So that when this error is really removed from profile, this path is not
executed.

+    item->show_when_merged = false;
+    // Item was merged with another change instance and error has been removed,
+    // create a new one for the composite change.
+    item->error.reset(new SettingsChangeGlobalError(item->change.get(), this));
+    item->error->AddToProfile(profile_, show_new_error);
+    has_active_change_ = true;
+    return;
+  }
+
   items_.erase(item);
 
+  // If no other change is shown and there are changes that haven't been shown
+  // yet, show the first one.
   if (!has_active_change_) {
-    // If there are changes that haven't been shown yet, show the first one.
     for (item = items_.begin(); item != items_.end(); ++item) {
       if (!item->error->HasShownBubbleView()) {
         item->error->ShowBubble();
@@ -122,7 +165,8 @@ BaseSettingChange* ProtectorService::GetLastChange() {
   return items_.empty() ? NULL : items_.back().change.get();
 }
 
-ProtectorService::Item::Item() {
+ProtectorService::Item::Item()
+    : is_merging(false), show_when_merged(false) {

[whywhat, 2012/04/06 16:28:32]

each field on a separate line, please.

[Ivan Korotkov, 2012/04/09 14:46:47]

Done.

 }
 
 ProtectorService::Item::~Item() {
@@ -134,7 +178,8 @@ ProtectorService::MatchItemByChange::MatchItemByChange(
 
 bool ProtectorService::MatchItemByChange::operator()(
     const ProtectorService::Item& item) {
-  return other_ == item.change.get();
+  return item.change->Contains(other_);
+  // return item.change.get() == other_;

[whywhat, 2012/04/06 16:28:32]

remove the commented line?

[Ivan Korotkov, 2012/04/09 14:46:47]

Done.

 }
 
 ProtectorService::MatchItemByError::MatchItemByError(