Persist browseraction properties across restarts

chrome/browser/extensions/api/extension_action/extension_action_api.cc

Index: chrome/browser/extensions/api/extension_action/extension_action_api.cc
diff --git a/chrome/browser/extensions/api/extension_action/extension_action_api.cc b/chrome/browser/extensions/api/extension_action/extension_action_api.cc
index a34dc7e3f6324748e4c81c5da0b78df2554d2c54..a84535e5f171aa64bb3af86bbbef6a3be9d73043 100644
--- a/chrome/browser/extensions/api/extension_action/extension_action_api.cc
+++ b/chrome/browser/extensions/api/extension_action/extension_action_api.cc
@@ -144,17 +144,29 @@ void SetDefaultsFromValue(const base::DictionaryValue* dict,
   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:
@@ -165,7 +177,8 @@ void SetDefaultsFromValue(const base::DictionaryValue* dict,
   }
 
   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)) {
@@ -440,7 +453,6 @@ void ExtensionActionStorageManager::Observe(
       if (profile != profile_)
         break;
 
-      extension_action->set_has_changed(true);
       WriteToStorage(extension_action);
       break;
     }
@@ -475,13 +487,6 @@ void ExtensionActionStorageManager::ReadFromStorage(
       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;