Google Now Code Cleanup

BUG=306613
R=isherman@chromium.org, rgustafson@chromium.org, skare@chromium.org, vadimt@chromium.org

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

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

6 files changed, 28 insertions, 224 deletions

diff --git a/chrome/browser/resources/google_now/background.js b/chrome/browser/resources/google_now/background.js
index b5718f8..056aa7f 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -10,11 +10,10 @@
  * them as Chrome notifications.
  * The service performs periodic updating of Google Now cards.
  * Each updating of the cards includes 4 steps:
- * 1. Obtaining the location of the machine;
- * 2. Processing requests for cards dismissals that are not yet sent to the
- *    server;
- * 3. Making a server request based on that location;
- * 4. Showing the received cards as notifications.
+ * 1. Processing requests for cards dismissals that are not yet sent to the
+ *    server.
+ * 2. Making a server request.
+ * 3. Showing the received cards as notifications.
  */
 
 // TODO(vadimt): Decide what to do in incognito mode.
@@ -98,8 +97,6 @@ var STATE_CHANGED_TASK_NAME = 'state-changed';
 var SHOW_ON_START_TASK_NAME = 'show-cards-on-start';
 var ON_PUSH_MESSAGE_START_TASK_NAME = 'on-push-message';
 
-var LOCATION_WATCH_NAME = 'location-watch';
-
 /**
  * Group as received from the server.
  *
@@ -180,7 +177,6 @@ function areTasksConflicting(newTaskName, scheduledTaskName) {
 var tasks = buildTaskManager(areTasksConflicting);
 
 // Add error processing to API calls.
-wrapper.instrumentChromeApiFunction('location.onLocationUpdate.addListener', 0);
 wrapper.instrumentChromeApiFunction('metricsPrivate.getVariationParams', 1);
 wrapper.instrumentChromeApiFunction('notifications.clear', 1);
 wrapper.instrumentChromeApiFunction('notifications.create', 2);
@@ -195,12 +191,6 @@ wrapper.instrumentChromeApiFunction(
     'notifications.onPermissionLevelChanged.addListener', 0);
 wrapper.instrumentChromeApiFunction(
     'notifications.onShowSettings.addListener', 0);
-wrapper.instrumentChromeApiFunction(
-    'preferencesPrivate.googleGeolocationAccessEnabled.get',
-    1);
-wrapper.instrumentChromeApiFunction(
-    'preferencesPrivate.googleGeolocationAccessEnabled.onChange.addListener',
-    0);
 wrapper.instrumentChromeApiFunction('permissions.contains', 1);
 wrapper.instrumentChromeApiFunction('pushMessaging.onMessage.addListener', 0);
 wrapper.instrumentChromeApiFunction('runtime.onInstalled.addListener', 0);
@@ -210,7 +200,7 @@ wrapper.instrumentChromeApiFunction('storage.local.get', 1);
 
 var updateCardsAttempts = buildAttemptManager(
     'cards-update',
-    requestLocation,
+    requestCards,
     INITIAL_POLLING_PERIOD_SECONDS,
     MAXIMUM_POLLING_PERIOD_SECONDS);
 var dismissalAttempts = buildAttemptManager(
@@ -233,7 +223,7 @@ var GoogleNowEvent = {
   DISMISS_REQUEST_TOTAL: 3,
   DISMISS_REQUEST_SUCCESS: 4,
   LOCATION_REQUEST: 5,
-  LOCATION_UPDATE: 6,
+  DELETED_LOCATION_UPDATE: 6,
   EXTENSION_START: 7,
   DELETED_SHOW_WELCOME_TOAST: 8,
   STOPPED: 9,
@@ -330,8 +320,6 @@ function showNotificationCards(
 
 /**
  * Removes all cards and card state on Google Now close down.
- * For example, this occurs when the geolocation preference is unchecked in the
- * content settings.
  */
 function removeAllCards() {
   console.log('removeAllCards');
@@ -634,10 +622,9 @@ function requestOptedIn(optedInCallback) {
 
 /**
  * Requests notification cards from the server.
- * @param {Location=} position Location of this computer.
  */
-function requestNotificationCards(position) {
-  console.log('requestNotificationCards ' + JSON.stringify(position));
+function requestNotificationCards() {
+  console.log('requestNotificationCards');
 
   instrumented.storage.local.get(
       ['notificationGroups', 'googleNowEnabled'], function(items) {
@@ -668,58 +655,23 @@ function requestNotificationCards(position) {
 }
 
 /**
- * Starts getting location for a cards update.
+ * Requests and shows notification cards.
  */
-function requestLocation() {
-  console.log('requestLocation');
+function requestCards() {
+  console.log('requestCards @' + new Date());
+  // LOCATION_REQUEST is a legacy histogram value when we requested location.
+  // This corresponds to the extension attempting to request for cards.
+  // We're keeping the name the same to keep our histograms in order.
   recordEvent(GoogleNowEvent.LOCATION_REQUEST);
-  // TODO(vadimt): Figure out location request options.
-  instrumented.metricsPrivate.getVariationParams('GoogleNow', function(params) {
-    var minDistanceInMeters =
-        parseInt(params && params.minDistanceInMeters, 10) ||
-        100;
-    var minTimeInMilliseconds =
-        parseInt(params && params.minTimeInMilliseconds, 10) ||
-        180000;  // 3 minutes.
-
-    // TODO(vadimt): Uncomment/remove watchLocation and remove invoking
-    // updateNotificationsCards once state machine design is finalized.
-//    chrome.location.watchLocation(LOCATION_WATCH_NAME, {
-//      minDistanceInMeters: minDistanceInMeters,
-//      minTimeInMilliseconds: minTimeInMilliseconds
-//    });
-    // We need setTimeout to avoid recursive task creation. This is a temporary
-    // code, and it will be removed once we finally decide to send or not send
-    // client location to the server.
-    setTimeout(wrapper.wrapCallback(updateNotificationsCards, true), 0);
-  });
-}
-
-/**
- * Stops getting the location.
- */
-function stopRequestLocation() {
-  console.log('stopRequestLocation');
-  chrome.location.clearWatch(LOCATION_WATCH_NAME);
-}
-
-/**
- * Obtains new location; requests and shows notification cards based on this
- * location.
- * @param {Location=} position Location of this computer.
- */
-function updateNotificationsCards(position) {
-  console.log('updateNotificationsCards ' + JSON.stringify(position) +
-      ' @' + new Date());
   tasks.add(UPDATE_CARDS_TASK_NAME, function() {
-    console.log('updateNotificationsCards-task-begin');
+    console.log('requestCards-task-begin');
     updateCardsAttempts.isRunning(function(running) {
       if (running) {
         updateCardsAttempts.planForNext(function() {
           processPendingDismissals(function(success) {
             if (success) {
               // The cards are requested only if there are no unsent dismissals.
-              requestNotificationCards(position);
+              requestNotificationCards();
             }
           });
         });
@@ -958,22 +910,22 @@ function onNotificationClosed(chromeNotificationId, byUser) {
 }
 
 /**
- * Initializes the polling system to start monitoring location and fetching
- * cards.
+ * Initializes the polling system to start fetching cards.
  */
 function startPollingCards() {
-  // Create an update timer for a case when for some reason location request
-  // gets stuck.
+  console.log('startPollingCards');
+  // Create an update timer for a case when for some reason requesting
+  // cards gets stuck.
   updateCardsAttempts.start(MAXIMUM_POLLING_PERIOD_SECONDS);
 
-  requestLocation();
+  requestCards();
 }
 
 /**
  * Stops all machinery in the polling system.
  */
 function stopPollingCards() {
-  stopRequestLocation();
+  console.log('stopPollingCards');
   updateCardsAttempts.stop();
   removeAllCards();
   // Mark the Google Now as disabled to start with checking the opt-in state
@@ -1033,8 +985,6 @@ function setBackgroundEnable(backgroundEnable) {
  * 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.
- * @param {boolean} geolocationEnabled true if
- *     the geolocation option is enabled.
  * @param {boolean} canEnableBackground true if
  *     the background permission can be requested.
  * @param {boolean} notificationEnabled true if
@@ -1044,30 +994,23 @@ function setBackgroundEnable(backgroundEnable) {
  */
 function updateRunningState(
     signedIn,
-    geolocationEnabled,
     canEnableBackground,
     notificationEnabled,
     googleNowEnabled) {
   console.log(
       'State Update signedIn=' + signedIn + ' ' +
-      'geolocationEnabled=' + geolocationEnabled + ' ' +
       'canEnableBackground=' + canEnableBackground + ' ' +
       'notificationEnabled=' + notificationEnabled + ' ' +
       'googleNowEnabled=' + googleNowEnabled);
 
-  // TODO(vadimt): Remove this line once state machine design is finalized.
-  geolocationEnabled = true;
-
   var shouldPollCards = false;
   var shouldSetBackground = false;
 
   if (signedIn && notificationEnabled) {
-    if (geolocationEnabled) {
-      if (canEnableBackground && googleNowEnabled)
-        shouldSetBackground = true;
+    if (canEnableBackground && googleNowEnabled)
+      shouldSetBackground = true;
 
-      shouldPollCards = true;
-    }
+    shouldPollCards = true;
   } else {
     recordEvent(GoogleNowEvent.STOPPED);
   }
@@ -1088,7 +1031,6 @@ function onStateChange() {
   tasks.add(STATE_CHANGED_TASK_NAME, function() {
     Promise.all([
         authenticationManager.isSignedIn(),
-        isGeolocationEnabled(),
         canEnableBackground(),
         isNotificationsEnabled(),
         isGoogleNowEnabled()])
@@ -1099,20 +1041,6 @@ function onStateChange() {
 }
 
 /**
- * Gets the geolocation enabled preference.
- * @return {Promise} A promise to get the geolocation enabled preference.
- */
-function isGeolocationEnabled() {
-  return new Promise(function(resolve) {
-    instrumented.preferencesPrivate.googleGeolocationAccessEnabled.get(
-        {},
-        function(prefValue) {
-          resolve(!!prefValue.value);
-        });
-  });
-}
-
-/**
  * Determines if background mode should be requested.
  * @return {Promise} A promise to determine if background can be enabled.
  */
@@ -1181,16 +1109,6 @@ instrumented.runtime.onStartup.addListener(function() {
   initialize();
 });
 
-instrumented.
-    preferencesPrivate.
-    googleGeolocationAccessEnabled.
-    onChange.
-    addListener(function(prefValue) {
-      console.log('googleGeolocationAccessEnabled Pref onChange ' +
-          prefValue.value);
-      onStateChange();
-});
-
 authenticationManager.addListener(function() {
   console.log('signIn State Change');
   onStateChange();
@@ -1227,11 +1145,6 @@ instrumented.notifications.onShowSettings.addListener(function() {
   openUrl(SETTINGS_URL);
 });
 
-instrumented.location.onLocationUpdate.addListener(function(position) {
-  recordEvent(GoogleNowEvent.LOCATION_UPDATE);
-  updateNotificationsCards(position);
-});
-
 instrumented.pushMessaging.onMessage.addListener(function(message) {
   // message.payload will be '' when the extension first starts.
   // Each time after signing in, we'll get latest payload for all channels.
@@ -1266,7 +1179,7 @@ instrumented.pushMessaging.onMessage.addListener(function(message) {
             notificationGroups: items.notificationGroups
           });
 
-          updateNotificationsCards();
+          requestCards();
         }
       });
     });
diff --git a/chrome/browser/resources/google_now/background_test_util.js b/chrome/browser/resources/google_now/background_test_util.js
index f68356c..6303781 100644
--- a/chrome/browser/resources/google_now/background_test_util.js
+++ b/chrome/browser/resources/google_now/background_test_util.js
@@ -18,14 +18,11 @@ var buildAttemptManager = emptyMock;
 var buildCardSet = emptyMock;
 
 var instrumented = {};
-mockChromeEvent(instrumented, 'location.onLocationUpdate');
 mockChromeEvent(instrumented, 'notifications.onButtonClicked');
 mockChromeEvent(instrumented, 'notifications.onClicked');
 mockChromeEvent(instrumented, 'notifications.onClosed');
 mockChromeEvent(instrumented, 'notifications.onPermissionLevelChanged');
 mockChromeEvent(instrumented, 'notifications.onShowSettings');
-mockChromeEvent(
-    instrumented, 'preferencesPrivate.googleGeolocationAccessEnabled.onChange');
 mockChromeEvent(instrumented, 'pushMessaging.onMessage');
 mockChromeEvent(instrumented, 'runtime.onInstalled');
 mockChromeEvent(instrumented, 'runtime.onStartup');
diff --git a/chrome/browser/resources/google_now/background_unittest.gtestjs b/chrome/browser/resources/google_now/background_unittest.gtestjs
index 5061584..325d171 100644
--- a/chrome/browser/resources/google_now/background_unittest.gtestjs
+++ b/chrome/browser/resources/google_now/background_unittest.gtestjs
@@ -63,12 +63,10 @@ function mockInitializeDependencies(fixture) {
   ]);
   fixture.makeAndRegisterMockApis([
     'authenticationManager.isSignedIn',
-    'chrome.location.clearWatch',
     'chrome.storage.local.remove',
     'instrumented.metricsPrivate.getVariationParams',
     'instrumented.notifications.getAll',
     'instrumented.notifications.getPermissionLevel',
-    'instrumented.preferencesPrivate.googleGeolocationAccessEnabled.get',
     'instrumented.storage.local.get',
     'instrumented.webstorePrivate.getBrowserLogin',
     'tasks.add',
@@ -85,7 +83,6 @@ function mockInitializeDependencies(fixture) {
  * expects() calls cannot be chained with the same argument matchers.
  * @param {object} fixture Test fixture.
  * @param {string} testIdentityToken getAuthToken callback token.
- * @param {boolean} testGeolocationPref Geolocation Preference callback value.
  * @param {object} testExperimentVariationParams Response of
  *     metricsPrivate.getVariationParams.
  * @param {string} testExperimentVariationParams Response of
@@ -96,7 +93,6 @@ function mockInitializeDependencies(fixture) {
 function expectStateMachineCalls(
     fixture,
     testIdentityToken,
-    testGeolocationPref,
     testExperimentVariationParams,
     testNotificationPermissionLevel,
     testGoogleNowEnabled) {
@@ -114,14 +110,6 @@ function expectStateMachineCalls(
       will(invokeCallback(
           getVariationParamsSavedArgs, 1, testExperimentVariationParams));
 
-  var googleGeolocationPrefGetSavedArgs = new SaveMockArguments();
-  fixture.mockApis.expects(once()).
-      instrumented_preferencesPrivate_googleGeolocationAccessEnabled_get(
-          googleGeolocationPrefGetSavedArgs.match(eqJSON({})),
-          googleGeolocationPrefGetSavedArgs.match(ANYTHING)).
-      will(invokeCallback(
-          googleGeolocationPrefGetSavedArgs, 1, {value: testGeolocationPref}));
-
   var notificationGetPermissionLevelSavedArgs = new SaveMockArguments();
   fixture.mockApis.expects(once()).
       instrumented_notifications_getPermissionLevel(
@@ -176,7 +164,6 @@ TEST_F(
 
       // Setup and expectations.
       var testIdentityToken = undefined;
-      var testGeolocationPref = false;
       var testExperimentVariationParams = {};
       var testNotificationPermissionLevel = 'denied';
       var testGoogleNowEnabled = undefined;
@@ -194,53 +181,10 @@ TEST_F(
       expectStateMachineCalls(
           this,
           testIdentityToken,
-          testGeolocationPref,
-          testExperimentVariationParams,
-          testNotificationPermissionLevel,
-          testGoogleNowEnabled);
-
-      // Invoking the tested function.
-      initialize();
-    });
-
-TEST_F(
-    'GoogleNowBackgroundUnitTest',
-    'DISABLED_Initialize_ToastStateEmpty2',
-    function() {
-      // Tests the case when getAuthToken succeeds, and the user has never
-      // responded to the toast.
-      // In this case, the function should invoke showWelcomeToast().
-
-      // Setup and expectations.
-      var testIdentityToken = 'some identity token';
-      var testGeolocationPref = false;
-      var testExperimentVariationParams = {};
-      var testNotificationPermissionLevel = 'denied';
-      var testGoogleNowEnabled = undefined;
-
-      mockInitializeDependencies(this);
-
-      this.mockGlobals.expects(once()).recordEvent(
-          GoogleNowEvent.EXTENSION_START);
-
-      expectInitialization(this.mockApis);
-
-      expectStateMachineCalls(
-          this,
-          testIdentityToken,
-          testGeolocationPref,
           testExperimentVariationParams,
           testNotificationPermissionLevel,
           testGoogleNowEnabled);
 
-      var chromeNotificationGetAllSavedArgs = new SaveMockArguments();
-      this.mockApis.expects(exactly(1)).
-          instrumented_notifications_getAll(
-              chromeNotificationGetAllSavedArgs.match(ANYTHING)).
-          will(
-              invokeCallback(chromeNotificationGetAllSavedArgs, 0, {}),
-              invokeCallback(chromeNotificationGetAllSavedArgs, 0, {}));
-
       // Invoking the tested function.
       initialize();
     });
@@ -251,7 +195,6 @@ TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_RunGoogleNow', function() {
 
   // Setup and expectations.
   var testIdentityToken = 'some identity token';
-  var testGeolocationPref = true;
   var testExperimentVariationParams = {};
   var testNotificationPermissionLevel = 'granted';
   var testGoogleNowEnabled = true;
@@ -266,7 +209,6 @@ TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_RunGoogleNow', function() {
   expectStateMachineCalls(
       this,
       testIdentityToken,
-      testGeolocationPref,
       testExperimentVariationParams,
       testNotificationPermissionLevel,
       testGoogleNowEnabled);
@@ -277,52 +219,6 @@ TEST_F('GoogleNowBackgroundUnitTest', 'Initialize_RunGoogleNow', function() {
   initialize();
 });
 
-TEST_F(
-    'GoogleNowBackgroundUnitTest',
-    'DISABLED_Initialize_NoGeolocation',
-    function() {
-  // Tests the case where everything is in place except for the
-  // Geolocation Preference after the user responded to the toast.
-
-  // Setup and expectations.
-  var testIdentityToken = 'some identity token';
-  var testGeolocationPref = false;
-  var testExperimentVariationParams = {};
-  var testNotificationPermissionLevel = 'denied';
-  var testGoogleNowEnabled = undefined;
-
-
-  mockInitializeDependencies(this);
-
-  this.mockGlobals.expects(once()).recordEvent(
-        GoogleNowEvent.EXTENSION_START);
-
-  this.mockGlobals.expects(once()).recordEvent(
-      GoogleNowEvent.USER_SUPPRESSED);
-
-  expectInitialization(this.mockApis);
-
-  expectStateMachineCalls(
-      this,
-      testIdentityToken,
-      testGeolocationPref,
-      testExperimentVariationParams,
-      testNotificationPermissionLevel,
-      testNotificationPermissionLevel,
-      testGoogleNowEnabled);
-
-  var chromeNotificationGetAllSavedArgs = new SaveMockArguments();
-  this.mockApis.expects(exactly(2)).
-      instrumented_notifications_getAll(
-          chromeNotificationGetAllSavedArgs.match(ANYTHING)).
-      will(
-          invokeCallback(chromeNotificationGetAllSavedArgs, 0, {}),
-          invokeCallback(chromeNotificationGetAllSavedArgs, 0, {}));
-
-  // Invoking the tested function.
-  initialize();
-});
-
 /**
  * Mocks global functions and APIs that onNotificationClicked() depends upon.
  * @param {Test} fixture Test fixture.
diff --git a/chrome/browser/resources/google_now/manifest.json b/chrome/browser/resources/google_now/manifest.json
index dc1adae4..8ee0ae2 100644
--- a/chrome/browser/resources/google_now/manifest.json
+++ b/chrome/browser/resources/google_now/manifest.json
@@ -12,10 +12,8 @@
   "permissions": [
     "alarms",
     "identity",
-    "location",
     "metricsPrivate",
     "notifications",
-    "preferencesPrivate",
     "pushMessaging",
     "storage",
     "tabs",
diff --git a/chrome/browser/resources/google_now/utility.js b/chrome/browser/resources/google_now/utility.js
index e246fbd..b913911 100644
--- a/chrome/browser/resources/google_now/utility.js
+++ b/chrome/browser/resources/google_now/utility.js
@@ -15,7 +15,7 @@
  *     otherwise, we generate an error. Chrome may unload event pages waiting
  *     for an event. When the event fires, Chrome will reload the event page. We
  *     don't require event listeners to fire because they are generally not
- *     predictable (like a location change event).
+ *     predictable (like a button clicked event).
  * (2) Task Manager (built with buildTaskManager() call) provides controlling
  *     mutually excluding chains of callbacks called tasks. Task Manager uses
  *     WrapperPlugins to add instrumentation code to 'wrapper' to determine
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index b602867..668bf14 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -27327,7 +27327,7 @@ other types of suffix sets.
   <int value="3" label="DISMISS_REQUEST_TOTAL"/>
   <int value="4" label="DISMISS_REQUEST_SUCCESS"/>
   <int value="5" label="LOCATION_REQUEST"/>
-  <int value="6" label="LOCATION_UPDATE"/>
+  <int value="6" label="DELETED_LOCATION_UPDATE"/>
   <int value="7" label="EXTENSION_START"/>
   <int value="8" label="DELETED_SHOW_WELCOME_TOAST"/>
   <int value="9" label="STOPPED"/>