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

chrome/test/data/extensions/api_test/preference/onchange_split/test.js

Index: chrome/test/data/extensions/api_test/preference/onchange_split/test.js
diff --git a/chrome/test/data/extensions/api_test/preference/onchange_split/test.js b/chrome/test/data/extensions/api_test/preference/onchange_split/test.js
new file mode 100644
index 0000000000000000000000000000000000000000..242f03d1c1a0e73365f75aacf66147c142122157
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/preference/onchange_split/test.js
@@ -0,0 +1,223 @@
+// 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.
+
+// Content settings API test
+// Run with browser_tests --gtest_filter=ExtensionApiTest.PreferenceOnChangeSplit

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

I think you may need to break this line.

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

I've seen this line not broken up elsewhere (see
chromium/src/chrome/test/data/extensions/api_test/proxy/individual/test.js)

But anyways, done!

+
+var inIncognitoContext = chrome.extension.inIncognitoContext;
+var pass = chrome.test.callbackPass;
+var sendMessage = chrome.test.sendMessage;
+var allowCookies = chrome.privacy.websites.thirdPartyCookiesAllowed;
+
+// Listen until |event| has fired with all of the values in |expected|.
+function listenUntil(event, expected) {
+  var done = chrome.test.listenForever(event, function(value) {
+    for (var i = 0; i < expected.length; i++) {
+      if (chrome.test.checkDeepEq(expected[i], value)) {
+        expected.splice(i, 1);
+        if (expected.length == 0)
+          done();
+        return;
+      }
+    }
+    chrome.test.fail("Unexpected event: " + JSON.stringify(value));
+  });
+}
+
+// Fail if |event| is fired (with any values). Because listenUntil stops
+// listening when |event| has fired with all the values in |expected|, it may
+// not capture superfluous unexpected events.
+function listenAndFailWhen(event) {
+  return chrome.test.listenForever(event, function(value) {
+    chrome.test.fail("Unexpected event: " + JSON.stringify(value));
+  });
+}
+
+// Context-to-string conversion for improved code readability on the C++ side.
+function contextToString(inIncognitoContext) {
+  if (inIncognitoContext)
+    return "incognito";
+  return "regular";
+}
+
+chrome.test.runTests([
+  // Changing the regular settings when no incognito-specific settings are
+  // defined should fire one event in the regular window, and two in the
+  // incognito.
+  function changeDefault() {
+    var expected = [{
+      'value': false,
+      'levelOfControl': 'controlled_by_this_extension'
+    }];
+
+    if (inIncognitoContext) {
+      expected[1] = {

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

I find expected.push() a bit nicer, as it doesn't have to rely on the fact that
there's already exactly one element in the array.

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

Done.

+        'value': false,
+        'incognitoSpecific': false,
+        'levelOfControl': 'controlled_by_this_extension'
+      };
+    }
+
+    listenUntil(allowCookies.onChange, expected);
+
+    sendMessage(
+        "changeDefault " + contextToString(inIncognitoContext) + " ready",

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

You could extract this to a variable to make the code a bit shorter.

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

Done.

+        pass(function() {
+          if (!inIncognitoContext) {
+            allowCookies.set({
+              'value': false
+            }, pass());
+          }
+        })
+    );
+  },
+
+  // Changing incognito-specific settings should only be visible to the
+  // incognito window.
+  function changeIncognitoOnly() {
+    var done;
+
+    if (!inIncognitoContext) {
+      done = listenAndFailWhen(allowCookies.onChange);
+      sendMessage("changeIncognitoOnly regular listening", pass(function() {

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

You could directly pass in done to pass().

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

Do you mean like this?

sendMessage("changeIncognitoOnly regular listening", pass(done());

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

just `done` -- you don't want to call |done| immediately. This is different from
pass(), because the latter tells the testing framework to keep waiting and
returns a suitable callback.

+        done();
+      }));
+    } else {
+      listenUntil(allowCookies.onChange, [{
+        'value': true,
+        'incognitoSpecific': true,
+        'levelOfControl': 'controlled_by_this_extension'
+      }]);
+    }
+
+    sendMessage(
+        "changeIncognitoOnly " + contextToString(inIncognitoContext) + " ready",
+        pass(function() {
+          if (inIncognitoContext) {
+            allowCookies.set({
+              'value': true,
+              'scope': 'incognito_session_only'
+            }, pass(sendMessage("changeIncognitoOnly incognito pref set",
+                                pass())));
+          }
+        })
+    );
+  },
+
+  // Changing the regular settings when incognito-specific settings are
+  // defined should only be visible to the regular window.
+  function changeDefaultOnly() {
+    if (!inIncognitoContext) {
+      listenUntil(allowCookies.onChange, [{
+        'value': true,
+        'levelOfControl': 'controlled_by_this_extension'
+      }]);
+    } else {
+      var done = listenAndFailWhen(allowCookies.onChange);
+      sendMessage("changeDefaultOnly incognito listening", pass(function() {
+        done();
+      }));
+    }
+
+    sendMessage(
+        "changeDefaultOnly " + contextToString(inIncognitoContext) +
+            " ready",

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

Nit: for string concatenation we usually align the continued line with the
previous one instead of indenting it four more spaces.

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

Done.

+        pass(function() {
+          if (!inIncognitoContext) {
+            allowCookies.set({
+              'value': true
+            }, pass(sendMessage("changeDefaultOnly pref set", pass())));
+          }
+        })
+    );
+  },
+
+  // Change the incognito setting back to false so that we get an event when
+  // clearing the value. Should not be visible to regular window.
+  function changeIncognitoOnlyBack() {
+    var done;

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

You could move |done| into the if-clause.

+
+    if (!inIncognitoContext) {
+      done = listenAndFailWhen(allowCookies.onChange);
+      sendMessage("changeIncognitoOnlyBack regular listening", pass(function() {
+        done();
+      }));
+    } else {
+      listenUntil(allowCookies.onChange, [{
+        'value': false,
+        'incognitoSpecific': true,
+        'levelOfControl': 'controlled_by_this_extension'
+      }]);
+    }
+
+    sendMessage(
+        "changeIncognitoOnlyBack " + contextToString(inIncognitoContext) +
+            " ready",
+        pass(function() {
+          if (inIncognitoContext) {
+            allowCookies.set({
+              'value': false,
+              'scope': 'incognito_session_only'
+            }, pass(sendMessage("changeIncognitoOnlyBack incognito pref set",
+                                pass())));
+          }
+        })
+    );
+  },
+
+  function clearIncognito() {
+    var done;
+
+    if (!inIncognitoContext) {
+      done = listenAndFailWhen(allowCookies.onChange);
+      sendMessage("clearIncognito regular listening", pass(function() {
+        done();
+      }));
+    } else {
+      listenUntil(allowCookies.onChange, [{
+        'value': true,
+        'incognitoSpecific': false,
+        'levelOfControl': 'controlled_by_this_extension'
+      }]);
+    }
+
+    sendMessage(
+        "clearIncognito " + contextToString(inIncognitoContext) + " ready",
+        pass(function() {
+          if (inIncognitoContext) {
+            allowCookies.clear({
+              'scope': 'incognito_session_only'
+            }, pass(sendMessage("clearIncognito incognito pref cleared",
+                                pass())));
+          }
+        })
+    );
+  },
+
+  function clearDefault() {
+    var expected = [{
+      'value': true,
+      'levelOfControl': 'controllable_by_this_extension'
+    }];
+
+    if (inIncognitoContext) {
+      expected[1] = {
+        'value': true,
+        'incognitoSpecific': false,
+        'levelOfControl': 'controllable_by_this_extension'
+      };
+    }
+
+    listenUntil(allowCookies.onChange, expected);
+
+    sendMessage(
+        "clearDefault " + contextToString(inIncognitoContext) + " ready",
+        pass(function() {
+          if (!inIncognitoContext) {

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

Nit: no braces for one-line statements.

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

Done.

+            allowCookies.clear({}, pass());
+          }
+        })
+    );
+  }
+]);