Persist browseraction properties across restarts

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

Index: chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc b/chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1d08ed607a91e60853710b083cb38991eee251f1
--- /dev/null
+++ b/chrome/browser/extensions/api/extension_action/browser_action_browsertest.cc
@@ -0,0 +1,88 @@
+// 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(scoped_ptr<base::Value> value) {
+  base::MessageLoopForUI::current()->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) {
+  ExtensionSystem::Get(profile)->state_store()->GetExtensionValue(
+      extension_id, kBrowserActionStorageKey, base::Bind(&QuitMessageLoop));
+  content::RunMessageLoop();

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

Use MessageLoopRunner when you need a Quit closure:
https://code.google.com/p/chromium/codesearch/#chromium/src/content/public/te...

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

I don't know that I can... The callback expected by
StateStore::GetExtensionValue() must take a parameter of a value - is there a
way to pass in the QuitClosure, even though it takes no params?

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

Oh, oops. We have an IgnoreResult, but no IgnoreArguments. You should still use
a MessageLoopRunner or RunLoop instead of a direct call to
MessageLoopForUI::current()->Quit(), but your helper function does look
necessary.

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

Done.

+}
+
+}  // 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);

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

Assert that the extension's name is kExtensionName in this test, so it's clear
that they match.

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

Done.

+  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.";
+
+  // Wait for the StateStore to load, and fetch the defaults.

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

Are you guaranteed that the StateStore has _not_ loaded before this call? Can
you assert that? This would make sure that if the behavior ever breaks, the test
will fail consistently instead of becoming flaky.

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

Per our discussion offline, there is no good way of ensuring that the StateStore
has not loaded by this point. But, there is a 1-second delay in the StateStore
which will _almost_ guarantee that it will not have. Instead, we'll insert a
warning, and use that to be able to trace back if the test is flaky.  Also added
a comment so that the log doesn't get taken out in the latest "Kill all the
logs" spree.

+  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