Split mode incognito extension can get misleading pref changed events from regular mode

chrome/browser/extensions/extension_preference_apitest.cc

Index: chrome/browser/extensions/extension_preference_apitest.cc
diff --git a/chrome/browser/extensions/extension_preference_apitest.cc b/chrome/browser/extensions/extension_preference_apitest.cc
index 59ffdcd09ed3d70a2a5907d369f35ef8c19abbdc..80efffd43f34c5b247a4226b417a47c398e46a17 100644
--- a/chrome/browser/extensions/extension_preference_apitest.cc
+++ b/chrome/browser/extensions/extension_preference_apitest.cc
@@ -4,11 +4,13 @@
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_test_message_listener.h"

[Bernhard Bauer, 2012/06/26 08:44:29]

I think you forgot to add this file to the CL?

[mitchellwrosen, 2012/06/26 17:32:52]

I didn't write it!

[Bernhard Bauer, 2012/06/26 20:09:55]

Oh, sorry, I thought it was new 0:-)

 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "webkit/plugins/npapi/mock_plugin_list.h"
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PreferenceApi) {
@@ -110,9 +112,127 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PreferenceClear) {
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PreferenceOnChange) {
-  PrefService* prefs = browser()->profile()->GetPrefs();
-  prefs->SetBoolean(prefs::kBlockThirdPartyCookies, false);
-
   EXPECT_TRUE(RunExtensionTestIncognito("preference/onchange")) <<
       message_;
 }
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PreferenceOnChangeSplit) {
+  ResultCatcher catcher;
+  catcher.RestrictToProfile(browser()->profile());
+  ResultCatcher catcher_incognito;
+  catcher_incognito.RestrictToProfile(
+      browser()->profile()->GetOffTheRecordProfile());
+
+  // Open an incognito window.
+  ui_test_utils::OpenURLOffTheRecord(browser()->profile(),
+                                     GURL("chrome://newtab/"));
+
+  // changeDefault listeners.
+  ExtensionTestMessageListener listener1("changeDefault regular ready", true);
+  ExtensionTestMessageListener listener_incognito1(
+      "changeDefault incognito ready", true);
+
+  // changeIncognitoOnly listeners.
+  ExtensionTestMessageListener listener2(
+      "changeIncognitoOnly regular ready", true);
+  ExtensionTestMessageListener listener_incognito2(
+      "changeIncognitoOnly incognito ready", true);
+  ExtensionTestMessageListener listener3(
+      "changeIncognitoOnly regular listening", true);
+  ExtensionTestMessageListener listener_incognito3(
+      "changeIncognitoOnly incognito pref set", false);
+
+  // changeDefaultOnly listeners.
+  ExtensionTestMessageListener listener4(
+      "changeDefaultOnly regular ready", true);
+  ExtensionTestMessageListener listener_incognito4(
+      "changeDefaultOnly incognito ready", true);
+  ExtensionTestMessageListener listener5(
+      "changeDefaultOnly pref set", false);
+  ExtensionTestMessageListener listener_incognito5(
+      "changeDefaultOnly incognito listening", true);
+
+  // changeIncognitoOnlyBack listeners.
+  ExtensionTestMessageListener listener6(
+      "changeIncognitoOnlyBack regular ready", true);
+  ExtensionTestMessageListener listener_incognito6(
+      "changeIncognitoOnlyBack incognito ready", true);
+  ExtensionTestMessageListener listener7(
+      "changeIncognitoOnlyBack regular listening", true);
+  ExtensionTestMessageListener listener_incognito7(
+      "changeIncognitoOnlyBack incognito pref set", false);
+
+  // clearIncognito listeners.
+  ExtensionTestMessageListener listener8(
+      "clearIncognito regular ready", true);
+  ExtensionTestMessageListener listener_incognito8(
+      "clearIncognito incognito ready", true);
+  ExtensionTestMessageListener listener9(
+      "clearIncognito regular listening", true);
+  ExtensionTestMessageListener listener_incognito9(
+      "clearIncognito incognito pref cleared", false);
+
+  // clearDefault listeners.
+  ExtensionTestMessageListener listener10(
+      "clearDefault regular ready", true);
+  ExtensionTestMessageListener listener_incognito10(
+      "clearDefault incognito ready", true);
+
+  ASSERT_TRUE(LoadExtensionIncognito(test_data_dir_

[Bernhard Bauer, 2012/06/26 08:44:29]

Nit: This is a bit awkwardly broken. Could you extract the extension directory
to a local variable?

[mitchellwrosen, 2012/06/26 17:32:52]

Done.

+      .AppendASCII("preference").AppendASCII("onchange_split")));
+
+  // Test 1 - changeDefault
+  EXPECT_TRUE(listener1.WaitUntilSatisfied()); // Regular ready
+  EXPECT_TRUE(listener_incognito1.WaitUntilSatisfied()); // Incognito ready
+  listener1.Reply("ok");
+  listener_incognito1.Reply("ok");
+
+  // Test 2 - changeIncognitoOnly
+  EXPECT_TRUE(listener2.WaitUntilSatisfied()); // Regular ready
+  EXPECT_TRUE(listener_incognito2.WaitUntilSatisfied()); // Incognito ready
+  EXPECT_TRUE(listener3.WaitUntilSatisfied()); // Regular listening
+  listener2.Reply("ok");
+  listener_incognito2.Reply("ok");
+  // Incognito preference set -- notify the regular listener
+  EXPECT_TRUE(listener_incognito3.WaitUntilSatisfied());
+  listener3.Reply("ok");
+
+  // Test 3 - changeDefaultOnly
+  EXPECT_TRUE(listener4.WaitUntilSatisfied()); // Regular ready
+  EXPECT_TRUE(listener_incognito4.WaitUntilSatisfied()); // Incognito ready
+  EXPECT_TRUE(listener_incognito5.WaitUntilSatisfied()); // Incognito listening
+  listener4.Reply("ok");
+  listener_incognito4.Reply("ok");
+  // Regular preference set - notify the incognito listener
+  EXPECT_TRUE(listener5.WaitUntilSatisfied());
+  listener_incognito5.Reply("done");

[Bernhard Bauer, 2012/06/26 08:44:29]

Why are you replying with "done" here instead of "ok"?

[mitchellwrosen, 2012/06/26 17:32:52]

Ehh, the words don't actually mean anything, since the JS never actually looks
at the message in the callback. I chose "ok" to mean, "ok, I see you're ready",
and "done" to mean, "you're done listening". They could even all be null
strings.

[Bernhard Bauer, 2012/06/26 20:09:55]

Would you mind still changing it to "ok" (or change all strings), just so that
future readers of the code don't assume it does something special? Thanks!

+
+  // Test 4 - changeIncognitoOnlyBack
+  EXPECT_TRUE(listener6.WaitUntilSatisfied()); // Regular ready
+  EXPECT_TRUE(listener_incognito6.WaitUntilSatisfied()); // Incognito ready
+  EXPECT_TRUE(listener7.WaitUntilSatisfied()); // Regular listening
+  listener6.Reply("ok");
+  listener_incognito6.Reply("ok");
+  // Incognito preference set -- notify the regular listener
+  EXPECT_TRUE(listener_incognito7.WaitUntilSatisfied());
+  listener7.Reply("ok");
+
+  // Test 5 - clearIncognito
+  EXPECT_TRUE(listener8.WaitUntilSatisfied()); // Regular ready
+  EXPECT_TRUE(listener_incognito8.WaitUntilSatisfied()); // Incognito ready
+  EXPECT_TRUE(listener9.WaitUntilSatisfied()); // Regular listening
+  listener8.Reply("ok");
+  listener_incognito8.Reply("ok");
+  // Incognito preference cleared -- notify the regular listener
+  EXPECT_TRUE(listener_incognito9.WaitUntilSatisfied());
+  listener9.Reply("ok");
+
+  // Test 6 - clearDefault
+  EXPECT_TRUE(listener10.WaitUntilSatisfied()); // Regular ready
+  EXPECT_TRUE(listener_incognito10.WaitUntilSatisfied()); // Incognito ready
+  listener10.Reply("ok");
+  listener_incognito10.Reply("ok");
+
+  EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
+  EXPECT_TRUE(catcher_incognito.GetNextResult()) << catcher.message();
+}