Re-sort preference update protocol between Chrome and ArcSupport.

chrome/browser/resources/chromeos/arc_support/background.js

 // Copyright 2016 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.
 
 /**
  * UI Pages. Note the order must be in sync with the ArcAuthService::UIPage
  * enum.
  * @type {Array<string>}
  */
 var UI_PAGES = ['none',

[Luis Héctor Chávez, 2016/10/17 02:26:01]

not your fault, but can this be const instead of var?

[hidehiko, 2016/10/17 09:27:56]

Unfortunately, the lint warns it. Instead @const is encouraged.
However, it is not a part of the focus of this CL, can we work on it in a
follow-up CL?

[Luis Héctor Chávez, 2016/10/17 10:44:07]

sgtm

                 'terms-loading',
                 'terms',
                 'lso-loading',
                 'lso',
                 'arc-loading',
                 'error',
                 'error-with-feedback'];
 
 /**
  * Chrome window that hosts UI. Only one window is allowed.
  * @type {chrome.app.window.AppWindow}
  */
 var appWindow = null;
 
 /**
  * Contains Web content provided by Google authorization server.
  * @type {WebView}
  */
 var lsoView = null;
 
 /**
  * Contains Play Store terms, loaded externally.
  * @type {WebView}
  */
 var termsView = null;
 
 /**
+ * @type {MetricsPreferenceCheckbox}
+ */
+var metricsCheckbox = null;
+
+/**
+ * @type {PreferenceCheckbox}
+ */
+var backupRestoreCheckbox = null;
+
+/**
+ * @type {PreferenceCheckbox}
+ */
+var locationServiceCheckbox = null;
+
+/**
  * Used for bidirectional communication with native code.
  * @type {chrome.runtime.Port}
  */
 var port = null;
 
 /**
  * Stores current device id.
  * @type {string}
  */
 var currentDeviceId = null;
 
 /**
  * Indicates that terms were accepted by user.
  * @type {boolean}
  */
 var termsAccepted = false;
 
 /**
  * Indicates that current user has managed Arc.
  * @type {boolean}
  */
 var arcManaged = false;
 
 /**
- * Tooltip text used in 'controlled by policy' indicator.
- * @type {boolean}
- */
-var controlledByPolicyText = '';
-
-/**
  * Host window inner default width.
  * @const {number}
  */
 var INNER_WIDTH = 960;

[Luis Héctor Chávez, 2016/10/17 02:26:00]

can this and the height be const as well?

[hidehiko, 2016/10/17 09:27:57]

Acknowledged.

 
 /**
  * Host window inner default height.
  * @const {number}
  */
 var INNER_HEIGHT = 688;
 
 
 /**
  * Sends a native message to ArcSupportHost.
  * @param {string} event The event type in message.
  * @param {Object=} opt_props Extra properties for the message.
  */
 function sendNativeMessage(event, opt_props) {
   var message = Object.assign({'event': event}, opt_props);
   port.postMessage(message);
 }
 
 /**
- * Helper function that sets inner content for an option which includes text,
- * link to 'learn more' section. This also creates an indicator showing that
- * option is controlled by policy and inserts it before link element.
- * @param {string} textId Id of the label element to process.
- * @param {string} learnMoreLinkId Id inner link to 'learn more' element.
- * @param {string} indicatorId Id of indicator to create.
- * @param {string} text Inner text to set. Includes link declaration.
- * @param {function} callback Callback to call on user action.
+ * Class to handle checkbox corresponding to a preference.
  */
-function createConsentOption(
-    textId, learnMoreLinkId, indicatorId, text, callback) {
-  var doc = appWindow.contentWindow.document;
-  var textElement = doc.getElementById(textId);
-  textElement.innerHTML = text;
-  var linkLearnMoreElement = doc.getElementById(learnMoreLinkId);
-  linkLearnMoreElement.addEventListener('click', callback);
+class PreferenceCheckbox {
 
-  // Create controlled by policy indicator.
-  var policyIndicator = new appWindow.contentWindow.cr.ui.ControlledIndicator();
-  policyIndicator.id = indicatorId;
-  policyIndicator.getBubbleText = function() {
-    return controlledByPolicyText;
-  };
-  textElement.insertBefore(policyIndicator, linkLearnMoreElement);
-}
+  /**
+   * Creates a Checkbox which handles the corresponding preference update.
+   * @param {Element} div The container this checkbox corresponds to.

[Luis Héctor Chávez, 2016/10/17 02:26:01]

nit: I'd rather call this |container| rather than |div|.

[hidehiko, 2016/10/17 09:27:57]

Done.

+   *     The element must have <input type="checkbox" class="checkbox-option">
+   *     for the checkbox itself, and <p class="checkbox-text"> for its label.
+   * @param {string} learnMoreContent I18n content which is shown when "Learn
+   *     More" link is clicked.
+   * @param {string?} learnMoreLinkId The ID for the "Learn More" link element.
+   *     TODO: Get rid of this. The element can have class so that it can be
+   *     identified easily. Also, it'd be better to extract the link element
+   *     (tag) from the i18n text, and let i18n focus on the content.
+   * @param {string?} policyText The content of the policy indicator.
+   */
+  constructor(div, learnMoreContent, learnMoreLinkId, policyText) {
+    this.div_ = div;
+    this.learnMoreContent_ = learnMoreContent;
+
+    this.checkbox_ = div.querySelector('.checkbox-option');
+    // TODO: Send the event to update the preference so that the extension's
+    // state and the native state can be in sync.

[khmel, 2016/10/13 15:12:43]

What do you mean by this? Do you want to change pref on checkbox click? If so
this may be confusing for user. Current behavior is update prefs/metrics mode
only if user presses Agree button. In case of cancel, prefs and metrics mode are
not updated.

[hidehiko, 2016/10/13 16:04:31]

Oh, is it intentional behavior? It looks a (small) bug to me, because I thought
we should keep the prev state, e.g., "open extension -> click checkbox -> close
by x-button (not cancel) -> reopen the extension"?

I'm ok if it is intentional. Then I'll update the following CLs.

[xiyuan, 2016/10/13 21:28:59]

Not a lawyer but think this is intentional similar to OOBE Eula screen where the
metrics etc is only enabled after user agrees to the terms. This TODO probably
is not needed.

[hidehiko, 2016/10/17 09:27:57]

Thank you for clarification. Done.

+
+    this.label_ = div.querySelector('.checkbox-text');
+
+    var learnMoreLink = this.label_.querySelector(learnMoreLinkId);
+    if (learnMoreLink) {
+      learnMoreLink.addEventListener(
+          'click', (event) => this.onLearnMoreLinkClicked(event));
+    }
+
+    // Create controlled indicator for policy if necessary.
+    if (policyText) {
+      this.policyIndicator_ =
+          new appWindow.contentWindow.cr.ui.ControlledIndicator();
+      this.policyIndicator_.setAttribute('textpolicy', policyText);
+      // TODO: better to have a dedicated element for this place.
+      this.label_.insertBefore(this.policyIndicator_, learnMoreLink);
+    } else {
+      this.policyIndicator_ = null;
+    }
+  }
+
+  isEnabled() { return this.checkbox_.checked; }

[Luis Héctor Chávez, 2016/10/17 02:26:01]

This was a bit confusing, since "enabled" means different things for preferences
and checkboxes. How about isChecked() so you don't overload the term and is
unambiguous? isPreferenceEnabled() is also acceptable, but more verbose.

[hidehiko, 2016/10/17 09:27:57]

Renamed to isChecked and added comments.

+
+  /**
+   * Called when the preference value in native code is updated.
+   */
+  onPreferenceChanged(isEnabled, isManaged) {
+    this.checkbox_.checked = isEnabled;
+    this.checkbox_.disabled = isManaged;
+    this.label_.disabled = isManaged;
+
+    if (this.policyIndicator_) {
+      if (isManaged) {
+        this.policyIndicator_.setAttribute('controlled-by', 'policy');
+      } else {
+        this.policyIndicator_.removeAttribute('controlled-by');
+      }
+    }
+  }
+
+  /**
+   * Called when the "Learn More" link is clicked.
+   */
+  onLearnMoreLinkClicked() {
+    showLearnMoreOverlay(this.learnMoreContent_);
+  }
+};
+
+/**
+ * Handles the checkbox action of metrics preference.
+ * This has special customization e.g. show/hide the checkbox based on
+ * the native preference.
+ */
+class MetricsPreferenceCheckbox extends PreferenceCheckbox {
+  constructor(
+      div, learnMoreContent, learnMoreLinkId, isOwned,
+      textDisabled, textEnabled, textManagedDisabled, textManagedEnabled) {
+    // Do not use policy indicator.
+    // Learn More link handling is done by this class.
+    // So pass |null| intentionally.
+    super(div, learnMoreContent, null, null);
+
+    this.learnMoreLinkId_ = learnMoreLinkId;
+    this.isOwned_ = isOwned;

[xiyuan, 2016/10/13 21:28:59]

nit: isOwned -> isOwner to be more clear

[hidehiko, 2016/10/17 09:27:57]

Done.

+
+    // Build a text array. The index is "isManaged * 2 + isEnabled". See

[Luis Héctor Chávez, 2016/10/17 02:26:01]

nit: Isn't it better to build a multi-dimensional array to avoid the index
arithmetic? Also |labels_| might be a slightly better name.

this.labels_ = [[textDisabled, textEnabled], [textManagedDisabled,
textManagedEnabled]];

[hidehiko, 2016/10/17 09:27:57]

Done.

+    // also onPreferenceChanged, too.
+    this.texts_ =
+        [textDisabled, textEnabled, textManagedDisabled, textManagedEnabled];
+  }
+
+  onPreferenceChanged(isEnabled, isManaged) {
+    isManaged = isManaged || !this.isOwned_;
+    super.onPreferenceChanged(isEnabled, isManaged);
+
+    // Hide the checkbox if it is not allowed to (re-)enable.
+    var canEnable = !isEnabled && !isManaged;
+    this.checkbox_.hidden = !canEnable;
+
+    // Update the label.
+    var index = 2 * isManaged + isEnabled;
+    this.label_.innerHTML = this.texts_[index];
+
+    // Work around for the current translation text.
+    // The translation text has tags for following links, although those
+    // tags are not the target of the translation (but those content text is
+    // the translation target).
+    // So, meanwhile, we set the link everytime we update the text.
+    // TODO: fix the translation text, and main html.
+    var learnMoreLink = this.label_.querySelector(this.learnMoreLinkId_);
+    learnMoreLink.addEventListener(
+        'click', (event) => this.onLearnMoreLinkClicked(event));
+    var settingsLink = this.label_.querySelector('#settings-link');
+    console.error(settingsLink);

[xiyuan, 2016/10/13 21:28:59]

nit: remove debugging log?

[hidehiko, 2016/10/17 09:27:57]

Oops. Removed.

+    settingsLink.addEventListener(
+        'click', (event) => this.onSettingsLinkClicked(event));
+
+    // Applying metrics mode changes page layout, update terms height.
+    updateTermsHeight();
+  }
+
+  /** Called when "settings" link is clicked. */
+  onSettingsLinkClicked(event) {
+    chrome.browser.openTab({'url': 'chrome://settings'}, function() {});
+    event.preventDefault();
+  }
+};
+
 
 /**
  * Applies localization for html content and sets terms webview.
  * @param {!Object} data Localized strings and relevant information.
  * @param {string} deviceId Current device id.
  */
 function initialize(data, deviceId) {
   currentDeviceId = deviceId;
   var doc = appWindow.contentWindow.document;
   var loadTimeData = appWindow.contentWindow.loadTimeData;
   loadTimeData.data = data;
   appWindow.contentWindow.i18nTemplate.process(doc, loadTimeData);
+
+  // Initialize preference connected checkboxes in terms of service page.
+  metricsCheckbox = new MetricsPreferenceCheckbox(
+      doc.getElementById('metrics-preference'),
+      data.learnMoreStatistics,
+      '#learn-more-link-metrics',
+      data.isOwnerProfile,
+      data.textMetricsDisabled,
+      data.textMetricsEnabled,
+      data.textMetricsManagedDisabled,
+      data.textMetricsManagedEnabled);
+  backupRestoreCheckbox = new PreferenceCheckbox(
+      doc.getElementById('backup-restore-preference'),
+      data.learnMoreBackupAndRestore,
+      '#learn-more-link-backup-restore',
+      data.controlledByPolicy);
+  locationServiceCheckbox = new PreferenceCheckbox(
+      doc.getElementById('location-service-preference'),
+      data.learnMoreLocationServices,
+      '#learn-more-link-location-service',
+      data.controlledByPolicy);
+
+  // Initialize terms of service view.
   var countryCode = data.countryCode.toLowerCase();
-  controlledByPolicyText = data.controlledByPolicy;
-  arcManaged = data.arcManaged;
-  setTermsVisible(!arcManaged);
-
-  createConsentOption('text-backup-restore',
-                      'learn-more-link-backup-restore',
-                      'policy-indicator-backup-restore',
-                      data.textBackupRestore,
-                      onLearnMoreBackupAndRestore);
-  createConsentOption('text-location-service',
-                      'learn-more-link-location-service',
-                      'policy-indicator-location-service',
-                      data.textLocationService,
-                      onLearnMoreLocationServices);
-
   var scriptSetCountryCode = 'document.countryCode = \'' + countryCode + '\';';
   termsView.addContentScripts([
       { name: 'preProcess',
         matches: ['https://play.google.com/*'],
         js: { code: scriptSetCountryCode },
         run_at: 'document_start'
       },
       { name: 'postProcess',
         matches: ['https://play.google.com/*'],
         css: { files: ['playstore.css'] },
         js: { files: ['playstore.js'] },
         run_at: 'document_end'
       }]);
+  arcManaged = data.arcManaged;
+  setTermsVisible(!arcManaged);
 }
 
 /**
- * Handles the event when the user clicks on a learn more metrics link. Opens
- * the pop up dialog with a help.
- */
-var onLearnMoreMetrics = function() {
-  var loadTimeData = appWindow.contentWindow.loadTimeData;
-  showLearnModeOverlay(loadTimeData.getString('learnMoreStatistics'));
-};
-
-/**
- * Handles the event when the user clicks on a learn more backup and restore
- * link. Opens the pop up dialog with a help.
- */
-var onLearnMoreBackupAndRestore = function() {
-  var loadTimeData = appWindow.contentWindow.loadTimeData;
-  showLearnModeOverlay(loadTimeData.getString('learnMoreBackupAndRestore'));
-};
-
-/**
- * Handles the event when the user clicks on a learn more location services
- * link. Opens the pop up dialog with a help.
- */
-var onLearnMoreLocationServices = function() {
-  var loadTimeData = appWindow.contentWindow.loadTimeData;
-  showLearnModeOverlay(loadTimeData.getString('learnMoreLocationServices'));
-};
-
-/**
- * Sets current metrics mode.
- * @param {string} text Describes current metrics state.
- * @param {boolean} canEnable Defines if user is allowed to change this metrics
- *                            option.
- * @param {boolean} on Defines if metrics are active currently.
- */
-function setMetricsMode(text, canEnable, on) {
-  var doc = appWindow.contentWindow.document;
-  var enableMetrics = doc.getElementById('enable-metrics');
-  enableMetrics.hidden = !canEnable;
-  enableMetrics.checked = on;
-
-  var onSettings = function(event) {
-    chrome.browser.openTab({'url': 'chrome://settings'}, function() {});
-    event.preventDefault();
-  };
-
-  doc.getElementById('text-metrics').innerHTML = text;
-  doc.getElementById('settings-link').addEventListener('click', onSettings);
-  doc.getElementById('learn-more-link-metrics').addEventListener('click',
-      onLearnMoreMetrics);
-
-  // Applying metrics mode changes page layout, update terms height.
-  updateTermsHeight();
-}
-
-/**
- * Sets current backup and restore mode.
- * @param {boolean} enabled Defines the value for backup and restore checkbox.
- * @param {boolean} managed Defines whether this setting is set by policy.
- */
-function setBackupRestoreMode(enabled, managed) {
-  var doc = appWindow.contentWindow.document;
-  doc.getElementById('enable-backup-restore').checked = enabled;
-  doc.getElementById('enable-backup-restore').disabled = managed;
-  doc.getElementById('text-backup-restore').disabled = managed;
-  var policyIconElement = doc.getElementById('policy-indicator-backup-restore');
-  if (managed) {
-    policyIconElement.setAttribute('controlled-by', 'policy');
-  } else {
-    policyIconElement.removeAttribute('controlled-by');
-  }
-}
-
-/**
- * Sets current usage of location service opt in mode.
- * @param {boolean} enabled Defines the value for location service opt in.
- * @param {boolean} managed Defines whether this setting is set by policy.
- */
-function setLocationServiceMode(enabled, managed) {
-  var doc = appWindow.contentWindow.document;
-  doc.getElementById('enable-location-service').checked = enabled;
-  doc.getElementById('enable-location-service').disabled = managed;
-  doc.getElementById('text-location-service').disabled = managed;
-  var policyIconElement = doc.getElementById(
-      'policy-indicator-location-service');
-  if (managed) {
-    policyIconElement.setAttribute('controlled-by', 'policy');
-  } else {
-    policyIconElement.removeAttribute('controlled-by');
-  }
-}
-
-/**
  * Sets visibility of Terms of Service.
  * @param {boolean} visible Whether the Terms of Service visible or not.
  */
 function setTermsVisible(visible) {
   var doc = appWindow.contentWindow.document;
   var styleVisibility = visible ? 'visible' : 'hidden';
   doc.getElementById('terms-title').style.visibility = styleVisibility;
   doc.getElementById('terms-container').style.visibility = styleVisibility;
 }
 
@@ skipping 25 matching lines @@
   }
 
   if (!appWindow) {
     console.warn('Received native message when window is not available.');
     return;
   }
 
   if (message.action == 'initialize') {
     initialize(message.data, message.deviceId);
   } else if (message.action == 'setMetricsMode') {
-    setMetricsMode(message.text, message.canEnable, message.on);
+    metricsCheckbox.onPreferenceChanged(message.enabled, message.managed);
   } else if (message.action == 'setBackupAndRestoreMode') {
-    setBackupRestoreMode(message.enabled, message.managed);
+    backupRestoreCheckbox.onPreferenceChanged(message.enabled, message.managed);
   } else if (message.action == 'setLocationServiceMode') {
-    setLocationServiceMode(message.enabled, message.managed);
+    locationServiceCheckbox.onPreferenceChanged(
+        message.enabled, message.managed);
   } else if (message.action == 'closeWindow') {
     if (appWindow) {
       appWindow.close();
     }
   } else if (message.action == 'showPage') {
     showPageWithStatus(message.page, message.status);
   } else if (message.action == 'setWindowBounds') {
     setWindowBounds();
   }
 }
@@ skipping 10 matching lines @@
 /**
  * Shows requested page and hide others. Show appWindow if it was hidden before.
  * 'none' hides all views.
  * @param {string} pageDivId id of divider of the page to show.
  */
 function showPage(pageDivId) {
   if (!appWindow) {
     return;
   }
 
-  hideLearnModeOverlay();
+  hideLearnMoreOverlay();
   var doc = appWindow.contentWindow.document;
   var pages = doc.getElementsByClassName('section');
   var sendFeedbackElement = doc.getElementById('button-send-feedback');
   if (pageDivId == 'error-with-feedback') {
     // Only show feedback button if the pageDivId is 'error-with-feedback'.
     sendFeedbackElement.hidden = false;
     pageDivId = 'error';
   } else {
     sendFeedbackElement.hidden = true;
   }
@@ skipping 25 matching lines @@
   }
   var doc = appWindow.contentWindow.document;
   var messageElement = doc.getElementById('error-message');
   messageElement.innerText = error;
 }
 
 /**
  * Sets learn more content text and shows it as overlay dialog.
  * @param {string} content HTML formatted text to show.
  */
-function showLearnModeOverlay(content) {
+function showLearnMoreOverlay(content) {
   var doc = appWindow.contentWindow.document;
   var learnMoreContainer = doc.getElementById('learn-more-container');
   var learnMoreContent = doc.getElementById('learn-more-content');
   learnMoreContent.innerHTML = content;
   learnMoreContainer.hidden = false;
 }
 
 /**
  * Hides learn more overlay dialog.
  */
-function hideLearnModeOverlay() {
+function hideLearnMoreOverlay() {
   var doc = appWindow.contentWindow.document;
   var learnMoreContainer = doc.getElementById('learn-more-container');
   learnMoreContainer.hidden = true;
 }
 
 /**
  * Shows requested page.
  * @param {int} pageId Index of the page to show. Must be in the array range of
  * UI_PAGES.
  * @param {string} status associated with page string status, error message for
@@ skipping 165 matching lines @@
     // webview is not allowed to open links in the new window. Hook these events
     // and open links in context of main page.
     termsView.addEventListener('newwindow', function(event) {
       event.preventDefault();
       chrome.browser.openTab({'url': event.targetUrl}, function() {});
     });
 
     var onAgree = function() {
       termsAccepted = true;
 
-      var enableMetrics = doc.getElementById('enable-metrics');
-      var enableBackupRestore = doc.getElementById('enable-backup-restore');
-      var enableLocationService = doc.getElementById('enable-location-service');
       sendNativeMessage('onAgreed', {
-        isMetricsEnabled: !enableMetrics.hidden && enableMetrics.checked,
-        isBackupRestoreEnabled: enableBackupRestore.checked,
-        isLocationServiceEnabled: enableLocationService.checked
+        isMetricsEnabled: metricsCheckbox.isEnabled(),
+        isBackupRestoreEnabled: backupRestoreCheckbox.isEnabled(),
+        isLocationServiceEnabled: locationServiceCheckbox.isEnabled()
       });
     };
 
     var onCancel = function() {
       if (appWindow) {
         appWindow.close();
       }
     };
 
     var onRetry = function() {
       if (termsAccepted) {
         // Reuse the onAgree() in case that the user has already accepted
         // the ToS.
         onAgree();
       } else {
         loadInitialTerms();
       }
     };
 
     var onSendFeedback = function() {
       sendNativeMessage('onSendFeedbackClicked');
     };
 
     doc.getElementById('button-agree').addEventListener('click', onAgree);
     doc.getElementById('button-cancel').addEventListener('click', onCancel);
     doc.getElementById('button-retry').addEventListener('click', onRetry);
     doc.getElementById('button-send-feedback')
         .addEventListener('click', onSendFeedback);
-    doc.getElementById('learn-more-close').addEventListener('click',
-        hideLearnModeOverlay);
+    doc.getElementById('learn-more-close').addEventListener(
+        'click', hideLearnMoreOverlay);
 
     var overlay = doc.getElementById('learn-more-container');
     appWindow.contentWindow.cr.ui.overlay.setupOverlay(overlay);
     appWindow.contentWindow.cr.ui.overlay.globalInitialization();
-    overlay.addEventListener('cancelOverlay', hideLearnModeOverlay);
+    overlay.addEventListener('cancelOverlay', hideLearnMoreOverlay);
 
     connectPort();
   };
 
   var onWindowCreated = function(createdWindow) {
     appWindow = createdWindow;
     appWindow.contentWindow.onload = onAppContentLoad;
     appWindow.onClosed.addListener(onWindowClosed);
 
     setWindowBounds();
@@ skipping 20 matching lines @@
       type: 'chrome',
       color: '#ffffff'
     },
     'innerBounds': {
       'width': INNER_WIDTH,
       'height': INNER_HEIGHT
     }
   };
   chrome.app.window.create('main.html', options, onWindowCreated);
 });