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

chrome/browser/protector/protector_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/protector/protector_service.h"
 
 #include "base/logging.h"
 #include "chrome/browser/prefs/pref_service.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/protector/protected_prefs_watcher.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_source.h"
 
 namespace protector {
 
 ProtectorService::ProtectorService(Profile* profile)
     : profile_(profile),
       has_active_change_(false) {
   // Start observing pref changes.
   prefs_watcher_.reset(new ProtectedPrefsWatcher(profile));
 }
 
 ProtectorService::~ProtectorService() {
   DCHECK(!IsShowingChange());  // Should have been dismissed by Shutdown.
 }
 
 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";
   if (!change->Init(profile_)) {
     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 {
   return !items_.empty();
 }
 
 void ProtectorService::ApplyChange(BaseSettingChange* change,
                                    Browser* browser) {
   change->Apply(browser);
   DismissChange(change);
 }
 
 void ProtectorService::DiscardChange(BaseSettingChange* change,
                                      Browser* browser) {
   change->Discard(browser);
   DismissChange(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();
 }
 
 void ProtectorService::OpenTab(const GURL& url, Browser* browser) {
   DCHECK(browser);
   browser->ShowSingletonTab(url);
 }
 
 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();
 }
 
 void ProtectorService::OnApplyChange(SettingsChangeGlobalError* error,
                                      Browser* browser) {
   DVLOG(1) << "Apply change";
   error->change()->Apply(browser);
   has_active_change_ = false;
 }
 
 void ProtectorService::OnDiscardChange(SettingsChangeGlobalError* error,
                                        Browser* browser) {
   DVLOG(1) << "Discard change";
   error->change()->Discard(browser);
   has_active_change_ = false;
 }
 
 void ProtectorService::OnDecisionTimeout(SettingsChangeGlobalError* error) {
   DVLOG(1) << "Timeout";
   error->change()->Timeout();
 }
 
 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();
         has_active_change_ = true;
         return;
       }
     }
   }
 }
 
 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() {
 }
 
 ProtectorService::MatchItemByChange::MatchItemByChange(
     const BaseSettingChange* other) : other_(other) {
 }
 
 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(
     const SettingsChangeGlobalError* other) : other_(other) {
 }
 
 bool ProtectorService::MatchItemByError::operator()(
     const ProtectorService::Item& item) {
   return other_ == item.error.get();
 }
 
 }  // namespace protector