Add Finch Checks to the State Machine

chrome/browser/resources/google_now/background.js

Index: chrome/browser/resources/google_now/background.js
diff --git a/chrome/browser/resources/google_now/background.js b/chrome/browser/resources/google_now/background.js
index aa68162313ed48f53b2c64666c6a21d938af991c..4bb1fcbf7cf2f89e1c610710e5207c5e78b1d7aa 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -709,6 +709,9 @@ function setShouldPollCards(shouldPollCardsRequest, onSuccess) {
         startPollingCards();
       else
         stopPollingCards();
+    } else {
+      console.log(
+          'Action Ignored setShouldPollCards=' + shouldPollCardsRequest);
     }
     onSuccess();
   });
@@ -734,6 +737,8 @@ function setToastVisible(visibleRequest, onSuccess) {
         showWelcomeToast();
       else
         hideWelcomeToast();
+    } else {
+      console.log('Action Ignored setToastVisible=' + visibleRequest);
     }
 
     onSuccess();
@@ -741,6 +746,36 @@ function setToastVisible(visibleRequest, onSuccess) {
 }
 
 /**
+ * Enables or disables the Google Now background permission.
+ * @param {boolean} backgroundEnable true to run in the background.
+ *     false to not run in the background.
+ * @param {function} onSuccess Called on completion.
+ */
+function setBackgroundEnable(backgroundEnable, onSuccess) {

[vadimt, 2013/08/02 21:54:43]

onSuccess -> callback

[robliao, 2013/08/03 01:21:18]

Kept this way to remain consistent with the above. We can change this in another
CL.
On 2013/08/02 21:54:43, vadimt wrote:

+  chrome.permissions.contains({permissions: ['background']},

[vadimt, 2013/08/02 21:54:43]

Please instrument new APIs.

[robliao, 2013/08/03 01:21:18]

Done.

+      function(hasPermission) {
+        if (backgroundEnable != hasPermission) {
+          console.log('Action Taken setBackgroundEnable=' + backgroundEnable);
+          if (backgroundEnable)
+            chrome.permissions.request(
+                {permissions: ['background']},
+                function() {
+                  onSuccess();
+                });
+          else
+            chrome.permissions.remove(
+                {permissions: ['background']},
+                function() {
+                  onSuccess();
+                });
+        } else {
+          console.log('Action Ignored setBackgroundEnable=' + backgroundEnable);
+          onSuccess();
+        }
+      });
+}
+
+/**
  * Does the actual work of deciding what Google Now should do
  * based off of the current state of Chrome.
  * @param {boolean} signedIn true if the user is signed in.
@@ -748,23 +783,28 @@ function setToastVisible(visibleRequest, onSuccess) {
  *     the geolocation option is enabled.
  * @param {boolean} userRespondedToToast true if
  *     the user has responded to the toast.
+ * @param {boolean} enableExperiment true if
+ *     this extension should be running.

[vadimt, 2013/08/02 21:54:43]

Actually, we need a flag that conditionally turns on background mode while we
are polling. So, we may have 2 states: polling & background, polling &
!background.

[robliao, 2013/08/03 01:21:18]

We will want to update the doc:
S & L => poll + background permission; TS = true; no showing toast

Do we want to only set this on responding to the toast?
On 2013/08/02 21:54:43, vadimt wrote:

[vadimt, 2013/08/03 02:07:53]

The ultimate goal is to implement as described in the doc.
However, we want to first run the experiment to make sure that background mode
is safe. Then, presumably, we'll switch all users to background mode.
So, I see this conditional background mode as a temporary technical detail that
doesn't need to be documented. However, I've added this as a note in the doc.

[robliao, 2013/08/09 21:46:44]

Clarified with
Modes without Background:
EnableWithoutBackground

Modes with Background
EnableWithBackground
EnabledViaFlag
''

On 2013/08/03 02:07:53, vadimt wrote:

  * @param {function()} callback Call this function on completion.
  */
 function updateRunningState(
     signedIn,
     geolocationEnabled,
     userRespondedToToast,
+    enableExperiment,
     callback) {

[vadimt, 2013/08/02 21:54:43]

Are you going to get rid of callback and debugSetStepName in a separate CL?
Sorry if I already asked this.

[robliao, 2013/08/03 01:21:18]

That's the plan. I didn't know if you were doing this or I was. I can set this
up.
On 2013/08/02 21:54:43, vadimt wrote:

[vadimt, 2013/08/03 02:07:53]

Please do this only for your task. I'll take care of others.

[robliao, 2013/08/09 21:46:44]

Done. There was no debugSetStepName added in.
On 2013/08/03 02:07:53, vadimt wrote:

[vadimt, 2013/08/09 22:13:39]

OK :), but please plan a separate CL where you'll get rid of debugSetStepName
and callback ONLY IN STATE_CHANGED_TASK_NAME functions.

 
   console.log(
       'State Update signedIn=' + signedIn + ' ' +
       'geolocationEnabled=' + geolocationEnabled + ' ' +
-      'userRespondedToToast=' + userRespondedToToast);
+      'userRespondedToToast=' + userRespondedToToast + ' ' +
+      'enableExperiment=' + enableExperiment);
 
   var shouldSetToastVisible = false;
   var shouldPollCards = false;
+  var shouldSetBackground = false;
 
-  if (signedIn) {
+  if (signedIn && enableExperiment) {
     if (geolocationEnabled) {
       if (!userRespondedToToast) {
         // If the user enabled geolocation independently of Google Now,
@@ -774,6 +814,7 @@ function updateRunningState(
       }
 
       shouldPollCards = true;
+      shouldSetBackground = true;
     } else {
       if (userRespondedToToast) {
         recordEvent(GoogleNowEvent.USER_SUPPRESSED);
@@ -786,11 +827,14 @@ function updateRunningState(
   }
 
   console.log(
-      'Requested Actions setToastVisible=' + shouldSetToastVisible + ' ' +
+      'Requested Actions shouldSetBackground=' + shouldSetBackground + ' ' +
+      'setToastVisible=' + shouldSetToastVisible + ' ' +
       'setShouldPollCards=' + shouldPollCards);
 
-  setToastVisible(shouldSetToastVisible, function() {
-    setShouldPollCards(shouldPollCards, callback);
+  setBackgroundEnable(shouldSetBackground, function() {
+    setToastVisible(shouldSetToastVisible, function() {
+      setShouldPollCards(shouldPollCards, callback);
+    });
   });
 }
 
@@ -803,19 +847,26 @@ function onStateChange() {
     tasks.debugSetStepName('onStateChange-isSignedIn');
     authenticationManager.isSignedIn(function(token) {
       var signedIn = !!token && !!NOTIFICATION_CARDS_URL;
-      tasks.debugSetStepName(
-          'onStateChange-get-googleGeolocationAccessEnabledPref');
-      googleGeolocationAccessEnabledPref.get({}, function(prefValue) {
-        var geolocationEnabled = !!prefValue.value;
+      chrome.metricsPrivate.getFieldTrial('GoogleNow', function(response) {

[vadimt, 2013/08/02 21:54:43]

How did you test this?

[robliao, 2013/08/03 01:21:18]

Tested with my personal finch server.
On 2013/08/02 21:54:43, vadimt wrote:

+        // '' means we were enabled via the flags but not the experiment.

[vadimt, 2013/08/02 21:54:43]

More precisely:
// '' means we were enabled via the flags, but the experiment is not running.

Explanation:
If experiment is running, turning on the flag will force EnableWithFlag group,
otherwise, the value will be ''.

ALSO:
Please modify C++ code that looks at the experiment code per that email.

[robliao, 2013/08/03 01:21:18]

Done.

+        var enableExperiment = (response == 'EnableWithBackground' ||
+                               (response == 'EnableWithFlag') ||
+                               (response == ''));
         tasks.debugSetStepName(
-          'onStateChange-get-userRespondedToToast');
-        storage.get('userRespondedToToast', function(items) {
-          var userRespondedToToast = !!items.userRespondedToToast;
-          updateRunningState(
-              signedIn,
-              geolocationEnabled,
-              userRespondedToToast,
-              callback);
+            'onStateChange-get-googleGeolocationAccessEnabledPref');
+        googleGeolocationAccessEnabledPref.get({}, function(prefValue) {
+          var geolocationEnabled = !!prefValue.value;
+          tasks.debugSetStepName(
+            'onStateChange-get-userRespondedToToast');
+          storage.get('userRespondedToToast', function(items) {
+            var userRespondedToToast = !!items.userRespondedToToast;
+            updateRunningState(
+                signedIn,
+                geolocationEnabled,
+                userRespondedToToast,
+                enableExperiment,
+                callback);
+          });
         });
       });
     });