Revert 255588 "Persist browseraction properties across restarts"

The new tests are failing:
http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%285%29/builds/25532/steps/browser_tests/logs/stdio

> Persist browseraction properties across restarts
> Persist browser action properties across restarts even if other properties are modified before the old ones are loaded. (Note: only properties which are NOT modified are persisted.)
> BUG=348775
> 
> Review URL: https://codereview.chromium.org/186013003

TBR=rdevlin.cronin@chromium.org

Review URL: https://codereview.chromium.org/189873005

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

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

-// Copyright 2014 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 "base/files/file_path.h"
-#include "base/message_loop/message_loop.h"
-#include "chrome/browser/extensions/extension_action.h"
-#include "chrome/browser/extensions/extension_action_manager.h"
-#include "chrome/browser/extensions/extension_browsertest.h"
-#include "chrome/browser/extensions/state_store.h"
-#include "content/public/test/test_utils.h"
-#include "extensions/browser/extension_registry.h"
-#include "extensions/browser/extension_system.h"
-#include "extensions/common/extension.h"
-#include "third_party/skia/include/core/SkColor.h"
-
-namespace extensions {
-
-namespace {
-
-// A key into the StateStore; we don't use any results, but need to know when
-// it's initialized.
-const char kBrowserActionStorageKey[] = "browser_action";
-// The name of the extension we add.
-const char kExtensionName[] = "Default Persistence Test Extension";
-
-void QuitMessageLoop(content::MessageLoopRunner* runner,
-                     scoped_ptr<base::Value> value) {
-  runner->Quit();
-}
-
-// We need to wait for the state store to initialize and respond to requests
-// so we can see if the preferences persist. Do this by posting our own request
-// to the state store, which should be handled after all others.
-void WaitForStateStore(Profile* profile, const std::string& extension_id) {
-  scoped_refptr<content::MessageLoopRunner> runner =
-      new content::MessageLoopRunner;
-  ExtensionSystem::Get(profile)->state_store()->GetExtensionValue(
-      extension_id,
-      kBrowserActionStorageKey,
-      base::Bind(&QuitMessageLoop, runner));
-  runner->Run();
-}
-
-}  // namespace
-
-// Setup for the test by loading an extension, which should set the browser
-// action background to blue.
-IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest,
-                       PRE_BrowserActionDefaultPersistence) {
-  const Extension* extension =
-      LoadExtension(test_data_dir_.AppendASCII("api_test")
-                        .AppendASCII("browser_action")
-                        .AppendASCII("default_persistence"));
-  ASSERT_TRUE(extension);
-  ASSERT_EQ(kExtensionName, extension->name());
-  WaitForStateStore(profile(), extension->id());
-
-  ExtensionAction* extension_action =
-      ExtensionActionManager::Get(profile())->GetBrowserAction(*extension);
-  ASSERT_TRUE(extension_action);
-  EXPECT_EQ(SK_ColorBLUE, extension_action->GetBadgeBackgroundColor(0));
-}
-
-// When Chrome restarts, the Extension will immediately update the browser
-// action, but will not modify the badge background color. Thus, the background
-// should remain blue (persisting the default set in onInstalled()).
-IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, BrowserActionDefaultPersistence) {
-  // Find the extension (it's a shame we don't have an ID for this, but it
-  // was generated in the last test).
-  const Extension* extension = NULL;
-  const ExtensionSet& extension_set =
-      ExtensionRegistry::Get(profile())->enabled_extensions();
-  for (ExtensionSet::const_iterator iter = extension_set.begin();
-       iter != extension_set.end();
-       ++iter) {
-    if ((*iter)->name() == kExtensionName) {
-      extension = *iter;
-      break;
-    }
-  }
-  ASSERT_TRUE(extension) << "Could not find extension in registry.";
-
-  // If this log becomes frequent, this test is losing its effectiveness, and
-  // we need to find a more invasive way of ensuring the test's StateStore
-  // initializes after extensions get their onStartup event.
-  if (ExtensionSystem::Get(profile())->state_store()->IsInitialized())
-    LOG(WARNING) << "State store already initialized; test guaranteed to pass.";
-
-  // Wait for the StateStore to load, and fetch the defaults.
-  WaitForStateStore(profile(), extension->id());
-
-  // Ensure the BrowserAction's badge background is still blue.
-  ExtensionAction* extension_action =
-      ExtensionActionManager::Get(profile())->GetBrowserAction(*extension);
-  ASSERT_TRUE(extension_action);
-  EXPECT_EQ(SK_ColorBLUE, extension_action->GetBadgeBackgroundColor(0));
-}
-
-}  // namespace extensions

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

@@ -138,48 +138,34 @@ std::string RepresentationToString(const gfx::ImageSkia& image, float scale) {
 // Set |action|'s default values to those specified in |dict|.
 void SetDefaultsFromValue(const base::DictionaryValue* dict,
                           ExtensionAction* action) {
-  const int kDefaultTabId = ExtensionAction::kDefaultTabId;
+  const int kTabId = ExtensionAction::kDefaultTabId;
   std::string str_value;
   int int_value;
   SkBitmap bitmap;
   gfx::ImageSkia icon;
 
-  // For each value, don't set it if it has been modified already.
-  if (dict->GetString(kPopupUrlStorageKey, &str_value) &&
-      !action->HasPopupUrl(kDefaultTabId)) {
-    action->SetPopupUrl(kDefaultTabId, GURL(str_value));
-  }
-  if (dict->GetString(kTitleStorageKey, &str_value) &&
-      !action->HasTitle(kDefaultTabId)) {
-    action->SetTitle(kDefaultTabId, str_value);
-  }
-  if (dict->GetString(kBadgeTextStorageKey, &str_value) &&
-      !action->HasBadgeText(kDefaultTabId)) {
-    action->SetBadgeText(kDefaultTabId, str_value);
-  }
-  if (dict->GetString(kBadgeBackgroundColorStorageKey, &str_value) &&
-      !action->HasBadgeBackgroundColor(kDefaultTabId)) {
-    action->SetBadgeBackgroundColor(kDefaultTabId,
-                                    RawStringToSkColor(str_value));
-  }
-  if (dict->GetString(kBadgeTextColorStorageKey, &str_value) &&
-      !action->HasBadgeTextColor(kDefaultTabId)) {
-    action->SetBadgeTextColor(kDefaultTabId, RawStringToSkColor(str_value));
-  }
-  if (dict->GetInteger(kAppearanceStorageKey, &int_value) &&
-      !action->HasIsVisible(kDefaultTabId)) {
+  if (dict->GetString(kPopupUrlStorageKey, &str_value))
+    action->SetPopupUrl(kTabId, GURL(str_value));
+  if (dict->GetString(kTitleStorageKey, &str_value))
+    action->SetTitle(kTabId, str_value);
+  if (dict->GetString(kBadgeTextStorageKey, &str_value))
+    action->SetBadgeText(kTabId, str_value);
+  if (dict->GetString(kBadgeBackgroundColorStorageKey, &str_value))
+    action->SetBadgeBackgroundColor(kTabId, RawStringToSkColor(str_value));
+  if (dict->GetString(kBadgeTextColorStorageKey, &str_value))
+    action->SetBadgeTextColor(kTabId, RawStringToSkColor(str_value));
+  if (dict->GetInteger(kAppearanceStorageKey, &int_value)) {
     switch (int_value) {
       case INVISIBLE:
       case OBSOLETE_WANTS_ATTENTION:
-        action->SetIsVisible(kDefaultTabId, false);
+        action->SetIsVisible(kTabId, false);
       case ACTIVE:
-        action->SetIsVisible(kDefaultTabId, true);
+        action->SetIsVisible(kTabId, true);
     }
   }
 
   const base::DictionaryValue* icon_value = NULL;
-  if (dict->GetDictionary(kIconStorageKey, &icon_value) &&
-      !action->HasIcon(kDefaultTabId)) {
+  if (dict->GetDictionary(kIconStorageKey, &icon_value)) {
     for (size_t i = 0; i < arraysize(kIconSizes); i++) {
       if (icon_value->GetString(kIconSizes[i].size_string, &str_value) &&
           StringToSkBitmap(str_value, &bitmap)) {
@@ -188,29 +174,27 @@ void SetDefaultsFromValue(const base::DictionaryValue* dict,
         icon.AddRepresentation(gfx::ImageSkiaRep(bitmap, scale));
       }
     }
-    action->SetIcon(kDefaultTabId, gfx::Image(icon));
+    action->SetIcon(kTabId, gfx::Image(icon));
   }
 }
 
 // Store |action|'s default values in a DictionaryValue for use in storing to
 // disk.
 scoped_ptr<base::DictionaryValue> DefaultsToValue(ExtensionAction* action) {
-  const int kDefaultTabId = ExtensionAction::kDefaultTabId;
+  const int kTabId = ExtensionAction::kDefaultTabId;
   scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
 
-  dict->SetString(kPopupUrlStorageKey,
-                  action->GetPopupUrl(kDefaultTabId).spec());
-  dict->SetString(kTitleStorageKey, action->GetTitle(kDefaultTabId));
-  dict->SetString(kBadgeTextStorageKey, action->GetBadgeText(kDefaultTabId));
-  dict->SetString(
-      kBadgeBackgroundColorStorageKey,
-      SkColorToRawString(action->GetBadgeBackgroundColor(kDefaultTabId)));
+  dict->SetString(kPopupUrlStorageKey, action->GetPopupUrl(kTabId).spec());
+  dict->SetString(kTitleStorageKey, action->GetTitle(kTabId));
+  dict->SetString(kBadgeTextStorageKey, action->GetBadgeText(kTabId));
+  dict->SetString(kBadgeBackgroundColorStorageKey,
+                  SkColorToRawString(action->GetBadgeBackgroundColor(kTabId)));
   dict->SetString(kBadgeTextColorStorageKey,
-                  SkColorToRawString(action->GetBadgeTextColor(kDefaultTabId)));
+                  SkColorToRawString(action->GetBadgeTextColor(kTabId)));
   dict->SetInteger(kAppearanceStorageKey,
-                   action->GetIsVisible(kDefaultTabId) ? ACTIVE : INVISIBLE);
+                   action->GetIsVisible(kTabId) ? ACTIVE : INVISIBLE);
 
-  gfx::ImageSkia icon = action->GetExplicitlySetIcon(kDefaultTabId);
+  gfx::ImageSkia icon = action->GetExplicitlySetIcon(kTabId);
   if (!icon.isNull()) {
     base::DictionaryValue* icon_value = new base::DictionaryValue();
     for (size_t i = 0; i < arraysize(kIconSizes); i++) {
@@ -456,6 +440,7 @@ void ExtensionActionStorageManager::Observe(
       if (profile != profile_)
         break;
 
+      extension_action->set_has_changed(true);
       WriteToStorage(extension_action);
       break;
     }
@@ -495,6 +480,13 @@ void ExtensionActionStorageManager::ReadFromStorage(
     return;
   }
 
+  // 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;

chrome/browser/extensions/extension_action.cc

@@ -50,19 +50,17 @@ class GetAttentionImageSource : public gfx::ImageSkiaSource {
   const gfx::ImageSkia icon_;
 };
 
-template <class T>
-bool HasValue(const std::map<int, T>& map, int tab_id) {
-  return map.find(tab_id) != map.end();
-}
-
 }  // namespace
 
 const int ExtensionAction::kDefaultTabId = -1;
 
-ExtensionAction::ExtensionAction(const std::string& extension_id,
-                                 extensions::ActionInfo::Type action_type,
-                                 const extensions::ActionInfo& manifest_data)
-    : extension_id_(extension_id), action_type_(action_type) {
+ExtensionAction::ExtensionAction(
+    const std::string& extension_id,
+    extensions::ActionInfo::Type action_type,
+    const extensions::ActionInfo& manifest_data)
+    : extension_id_(extension_id),
+      action_type_(action_type),
+      has_changed_(false) {
   // Page/script actions are hidden/disabled by default, and browser actions are
   // visible/enabled by default.
   SetIsVisible(kDefaultTabId,
@@ -207,34 +205,6 @@ gfx::ImageSkia ExtensionAction::GetIconWithBadge(
      icon.size());
 }
 
-bool ExtensionAction::HasPopupUrl(int tab_id) const {
-  return HasValue(popup_url_, tab_id);
-}
-
-bool ExtensionAction::HasTitle(int tab_id) const {
-  return HasValue(title_, tab_id);
-}
-
-bool ExtensionAction::HasBadgeText(int tab_id) const {
-  return HasValue(badge_text_, tab_id);
-}
-
-bool ExtensionAction::HasBadgeBackgroundColor(int tab_id) const {
-  return HasValue(badge_background_color_, tab_id);
-}
-
-bool ExtensionAction::HasBadgeTextColor(int tab_id) const {
-  return HasValue(badge_text_color_, tab_id);
-}
-
-bool ExtensionAction::HasIsVisible(int tab_id) const {
-  return HasValue(is_visible_, tab_id);
-}
-
-bool ExtensionAction::HasIcon(int tab_id) const {
-  return HasValue(icon_, tab_id);
-}
-
 // Determines which icon would be returned by |GetIcon|, and returns its width.
 int ExtensionAction::GetIconWidth(int tab_id) const {
   // If icon has been set, return its width.

chrome/browser/extensions/extension_action.h

@@ -68,6 +68,9 @@ class ExtensionAction {
   std::string id() const { return id_; }
   void set_id(const std::string& id) { id_ = id; }
 
+  bool has_changed() const { return has_changed_; }
+  void set_has_changed(bool value) { has_changed_ = value; }
+
   // Set the url which the popup will load when the user clicks this action's
   // icon.  Setting an empty URL will disable the popup for a given tab.
   void SetPopupUrl(int tab_id, const GURL& url);
@@ -179,16 +182,6 @@ class ExtensionAction {
                                   int tab_id,
                                   const gfx::Size& spacing) const;
 
-  // Determine whether or not the ExtensionAction has a value set for the given
-  // |tab_id| for each property.
-  bool HasPopupUrl(int tab_id) const;
-  bool HasTitle(int tab_id) const;
-  bool HasBadgeText(int tab_id) const;
-  bool HasBadgeBackgroundColor(int tab_id) const;
-  bool HasBadgeTextColor(int tab_id) const;
-  bool HasIsVisible(int tab_id) const;
-  bool HasIcon(int tab_id) const;
-
  private:
   // Returns width of the current icon for tab_id.
   // TODO(tbarzic): The icon selection is done in ExtensionActionIconFactory.
@@ -263,6 +256,10 @@ class ExtensionAction {
   // needed for compat with an older version of the page actions API.
   std::string id_;
 
+  // True if the ExtensionAction's settings have changed from what was
+  // specified in the manifest.
+  bool has_changed_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionAction);
 };
 

chrome/browser/extensions/state_store.cc

@@ -39,9 +39,6 @@ class StateStore::DelayedTaskQueue {
   // Marks us ready, and invokes all pending tasks.
   void SetReady();
 
-  // Return whether or not the DelayedTaskQueue is |ready_|.
-  bool ready() const { return ready_; }
-
  private:
   bool ready_;
   std::vector<base::Closure> pending_tasks_;
@@ -127,8 +124,6 @@ void StateStore::RemoveExtensionValue(const std::string& extension_id,
                  GetFullKey(extension_id, key)));
 }
 
-bool StateStore::IsInitialized() const { return task_queue_->ready(); }
-
 void StateStore::Observe(int type,
                          const content::NotificationSource& source,
                          const content::NotificationDetails& details) {

chrome/browser/extensions/state_store.h

@@ -52,9 +52,6 @@ class StateStore
   void RemoveExtensionValue(const std::string& extension_id,
                             const std::string& key);
 
-  // Return whether or not the StateStore has initialized itself.
-  bool IsInitialized() const;
-
  private:
   class DelayedTaskQueue;
 

chrome/chrome_tests.gypi

@@ -1119,7 +1119,6 @@
         'browser/extensions/api/dns/mock_host_resolver_creator.h',
         'browser/extensions/api/downloads/downloads_api_browsertest.cc',
         'browser/extensions/api/extension_action/browser_action_apitest.cc',
-        'browser/extensions/api/extension_action/browser_action_browsertest.cc',
         'browser/extensions/api/extension_action/page_action_apitest.cc',
         'browser/extensions/api/feedback_private/feedback_private_apitest.cc',
         'browser/extensions/api/feedback_private/feedback_browsertest.cc',

chrome/test/data/extensions/api_test/browser_action/default_persistence/background.js

-// Copyright 2014 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.
-
-// Set a background color for the badge at OnInstalled().
-chrome.runtime.onInstalled.addListener(function() {
-  chrome.browserAction.setBadgeBackgroundColor({color: [0, 0, 255, 255]});
-});
-
-// Every startup, change the badge text, but not the background color (which
-// should persist at the blue we set originally).
-chrome.runtime.onStartup.addListener(function() {
-  chrome.browserAction.setBadgeText({text: 'Hello'});
-});

chrome/test/data/extensions/api_test/browser_action/default_persistence/manifest.json

-{
-   "background": {
-      "persistent": false,
-      "scripts": [ "background.js" ]
-   },
-   "browser_action": {
-     "name": "browseraction"
-   },
-   "description": "An extension which sets properties of a browser action in onInstalled() to be persisted even when updating at startup. Inspired by Ben Kalman's Browser Clock extension.",
-   "manifest_version": 2,
-   "name": "Default Persistence Test Extension",
-   "version": "0.1.1"
-}