Persist browseraction properties across restarts

chrome/browser/extensions/api/extension_action/extension_action_api.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/extensions/api/extension_action/extension_action_api.h"
 
 #include "base/base64.h"
 #include "base/lazy_instance.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ skipping 120 matching lines @@
   base::StringPiece raw_str(
       reinterpret_cast<const char*>(&data[0]), data.size());
   std::string base64_str;
   base::Base64Encode(raw_str, &base64_str);
   return base64_str;
 }
 
 // Set |action|'s default values to those specified in |dict|.
 void SetDefaultsFromValue(const base::DictionaryValue* dict,
                           ExtensionAction* action) {
   const int kTabId = ExtensionAction::kDefaultTabId;

[Jeffrey Yasskin, 2014/03/04 22:37:28]

Arguably, this should be named 'kDefaultTabId'.

[Devlin, 2014/03/05 18:27:17]

Done.

   std::string str_value;
   int int_value;
   SkBitmap bitmap;
   gfx::ImageSkia icon;
 
-  if (dict->GetString(kPopupUrlStorageKey, &str_value))
+  // For each value, don't set it if it has been modified already.
+  if (dict->GetString(kPopupUrlStorageKey, &str_value) &&
+      !action->HasDefaultPopupUrl()) {

[Jeffrey Yasskin, 2014/03/04 22:11:11]

I've gotten lost in the multiple-negatives. You have if (!HasDefault), which is
equivalent to if (HasBeenModified), but the comment says "don't set if has been
modified", which would make me expect the opposite condition.

Oh, HasBeenModified isn't the opposite of HasDefault. Default is just one of the
aspects of this key. I think you shouldn't add HasDefault*: you should call
action->HasPopupUrl(kTabId)

[Devlin, 2014/03/04 22:38:40]

Note that there is _only_ HasPopupUrl, not Has*.  Should I then add Has* (i.e.
s/HasDefault/Has/)?

[Jeffrey Yasskin, 2014/03/04 22:49:20]

I think that'll be more readable, so yes please.

[Devlin, 2014/03/05 18:27:17]

Done.

     action->SetPopupUrl(kTabId, GURL(str_value));
-  if (dict->GetString(kTitleStorageKey, &str_value))
+  }
+  if (dict->GetString(kTitleStorageKey, &str_value) &&
+      !action->HasDefaultTitle()) {
     action->SetTitle(kTabId, str_value);
-  if (dict->GetString(kBadgeTextStorageKey, &str_value))
+  }
+  if (dict->GetString(kBadgeTextStorageKey, &str_value) &&
+      !action->HasDefaultBadgeText()) {
     action->SetBadgeText(kTabId, str_value);
-  if (dict->GetString(kBadgeBackgroundColorStorageKey, &str_value))
+  }
+  if (dict->GetString(kBadgeBackgroundColorStorageKey, &str_value) &&
+      !action->HasDefaultBadgeBackgroundColor()) {
     action->SetBadgeBackgroundColor(kTabId, RawStringToSkColor(str_value));
-  if (dict->GetString(kBadgeTextColorStorageKey, &str_value))
+  }
+  if (dict->GetString(kBadgeTextColorStorageKey, &str_value) &&
+      !action->HasDefaultBadgeTextColor()) {
     action->SetBadgeTextColor(kTabId, RawStringToSkColor(str_value));
-  if (dict->GetInteger(kAppearanceStorageKey, &int_value)) {
+  }
+  if (dict->GetInteger(kAppearanceStorageKey, &int_value) &&
+      !action->HasDefaultIsVisible()) {
     switch (int_value) {
       case INVISIBLE:
       case OBSOLETE_WANTS_ATTENTION:
         action->SetIsVisible(kTabId, false);
       case ACTIVE:
         action->SetIsVisible(kTabId, true);
     }
   }
 
   const base::DictionaryValue* icon_value = NULL;
-  if (dict->GetDictionary(kIconStorageKey, &icon_value)) {
+  if (dict->GetDictionary(kIconStorageKey, &icon_value) &&
+      !action->HasDefaultIcon()) {
     for (size_t i = 0; i < arraysize(kIconSizes); i++) {
       if (icon_value->GetString(kIconSizes[i].size_string, &str_value) &&
           StringToSkBitmap(str_value, &bitmap)) {
         CHECK(!bitmap.isNull());
         float scale = ui::GetImageScale(kIconSizes[i].scale);
         icon.AddRepresentation(gfx::ImageSkiaRep(bitmap, scale));
       }
     }
     action->SetIcon(kTabId, gfx::Image(icon));
   }
@@ skipping 254 matching lines @@
       }
       break;
     }
     case chrome::NOTIFICATION_EXTENSION_BROWSER_ACTION_UPDATED: {
       ExtensionAction* extension_action =
           content::Source<ExtensionAction>(source).ptr();
       Profile* profile = content::Details<Profile>(details).ptr();
       if (profile != profile_)
         break;
 
-      extension_action->set_has_changed(true);
       WriteToStorage(extension_action);
       break;
     }
     default:
       NOTREACHED();
       break;
   }
 }
 
 void ExtensionActionStorageManager::WriteToStorage(
@@ skipping 14 matching lines @@
   const Extension* extension =
       ExtensionSystem::Get(profile_)->extension_service()->
       extensions()->GetByID(extension_id);
   if (!extension)
     return;
 
   ExtensionAction* browser_action =
       ExtensionActionManager::Get(profile_)->GetBrowserAction(*extension);
   CHECK(browser_action);
 
-  // Don't load values from storage if the extension has updated a value
-  // already. The extension may have only updated some of the values, but
-  // this is a good first approximation. If the extension is doing stuff
-  // to the browser action, we can assume it is ready to take over.
-  if (browser_action->has_changed())
-    return;
-
   const base::DictionaryValue* dict = NULL;
   if (!value.get() || !value->GetAsDictionary(&dict))
     return;
 
   SetDefaultsFromValue(dict, browser_action);
 }
 
 //
 // ExtensionActionFunction
 //
@@ skipping 443 matching lines @@
   return true;
 }
 
 bool EnablePageActionsFunction::RunImpl() {
   return SetPageActionEnabled(true);
 }
 
 bool DisablePageActionsFunction::RunImpl() {
   return SetPageActionEnabled(false);
 }