Re-sort preference update protocol between Chrome and ArcSupport.

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

Index: chrome/browser/resources/chromeos/arc_support/background.js
diff --git a/chrome/browser/resources/chromeos/arc_support/background.js b/chrome/browser/resources/chromeos/arc_support/background.js
index a4a54dd4f0986f15adda654ec98fdd8a54ca3a9e..bba1ce551e70f5c67a3e53d7f2a194e3948e1444 100644
--- a/chrome/browser/resources/chromeos/arc_support/background.js
+++ b/chrome/browser/resources/chromeos/arc_support/background.js
@@ -35,6 +35,21 @@ var lsoView = null;
 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}
  */
@@ -59,12 +74,6 @@ var termsAccepted = false;
 var arcManaged = false;
 
 /**
- * Tooltip text used in 'controlled by policy' indicator.
- * @type {boolean}
- */
-var controlledByPolicyText = '';
-
-/**
  * Host window inner default width.
  * @const {number}
  */
@@ -88,31 +97,138 @@ function sendNativeMessage(event, opt_props) {
 }
 
 /**
- * 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);
-
-  // 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);
-}
+class PreferenceCheckbox {
+
+  /**
+   * 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.
@@ -125,22 +241,30 @@ function initialize(data, deviceId) {
   var loadTimeData = appWindow.contentWindow.loadTimeData;
   loadTimeData.data = data;
   appWindow.contentWindow.i18nTemplate.process(doc, loadTimeData);
-  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);
 
+  // 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();
   var scriptSetCountryCode = 'document.countryCode = \'' + countryCode + '\';';
   termsView.addContentScripts([
       { name: 'preProcess',
@@ -154,97 +278,8 @@ function initialize(data, deviceId) {
         js: { files: ['playstore.js'] },
         run_at: 'document_end'
       }]);
-}
-
-/**
- * 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');
-  }
+  arcManaged = data.arcManaged;
+  setTermsVisible(!arcManaged);
 }
 
 /**
@@ -293,11 +328,12 @@ function onNativeMessage(message) {
   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();
@@ -328,7 +364,7 @@ function showPage(pageDivId) {
     return;
   }
 
-  hideLearnModeOverlay();
+  hideLearnMoreOverlay();
   var doc = appWindow.contentWindow.document;
   var pages = doc.getElementsByClassName('section');
   var sendFeedbackElement = doc.getElementById('button-send-feedback');
@@ -374,7 +410,7 @@ function setErrorMessage(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');
@@ -385,7 +421,7 @@ function showLearnModeOverlay(content) {
 /**
  * Hides learn more overlay dialog.
  */
-function hideLearnModeOverlay() {
+function hideLearnMoreOverlay() {
   var doc = appWindow.contentWindow.document;
   var learnMoreContainer = doc.getElementById('learn-more-container');
   learnMoreContainer.hidden = true;
@@ -571,13 +607,10 @@ chrome.app.runtime.onLaunched.addListener(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()
       });
     };
 
@@ -606,13 +639,13 @@ chrome.app.runtime.onLaunched.addListener(function() {
     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();
   };