Add link to Hangouts Remote Desktop in the It2Me section of the remoting app.

remoting/webapp/butter_bar.js

+// Copyright 2013 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.
+
+/**
+ * @fileoverview
+ * Functions and event handlers to invite the user to participate in a survey
+ * to help improve the product.

[Jamie, 2013/05/23 20:31:46]

Is this comment still correct?

[Sergey Ulanov, 2013/05/29 23:36:11]

Done.

+ */
+
+'use strict';
+
+/** @suppress {duplicate} */
+var remoting = remoting || {};
+
+var kButterBarId = 'butter-bar';
+
+var kSurveyInvitationId = 'survey-invitation';
+var kSurveyAcceptId = 'survey-accept';
+var kSurveyDismissId = 'survey-dismiss';
+
+var kHangoutsInvitationId = 'hangouts-invitation';
+var kHangoutsAcceptId = 'hangouts-accept';
+var kHangoutsDismissId = 'hangouts-dismiss';
+
+var kSurveyStorageKey = 'feedback-survey-dismissed';
+var kHangoutsStorageKey = 'hangouts-notice-dismissed';

[Jamie, 2013/05/23 20:31:46]

I realize this was my error originally, but by declaring these unscoped, we're
polluting the top-level 'window' namespace. I think it would make sense to make
the butter bar functions into a class and have these constants as members of it.

Alternatively, if you don't want to make this a class, you could just declare
them inside remoting.

[Sergey Ulanov, 2013/05/29 23:36:11]

Done.

+
+/** @enum {number} */
+remoting.ButterBarMode = {
+  NONE: 0,
+  SURVEY: 1,
+  HANGOUTS: 2
+};
+
+/**
+ * Hide the survey request and record some basic information about the current

[Jamie, 2013/05/23 20:31:46]

s/survey request/butter-bar/ (and similar elsewhere).

[Sergey Ulanov, 2013/05/29 23:36:11]

Done.

+ * state of the world in synced storage. This may be useful in the future if we
+ * want to show the request again. At the moment, the data itself is ignored;
+ * only its presence or absence is important.
+ *
+ * @param {string} storageKey Storage key used to store dismissal state.
+ * @param {boolean} optIn True if the user clicked the "Take the survey" link;
+ *     false if they clicked the close icon.

[Jamie, 2013/05/23 20:31:46]

|optIn| doesn't really make sense as a parameter name for the Hangouts case, and
this comment is out-of-date. I suggest calling the parameter something like
|accepted| or invert the meaning and call it something like
|dismissedViaCloseIcon|.

[Sergey Ulanov, 2013/05/29 23:36:11]

Done. Also changed the key that we use to store the result - let me know if it
is a problem.

[Jamie, 2013/05/30 01:10:16]

It will mean that users who have previously dismissed the butter-bar will start
seeing it again, which is not ideal. I think we should keep the key as it is and
add a comment explaining why it's called what it is.

[Sergey Ulanov, 2013/05/30 22:04:54]

No, the value is not actually used in any way. We just verify the presence of
the value, but never check optIn/accepted attribute.

[Jamie, 2013/05/31 00:12:41]

Ah yes, good point. Still, if we ever want to make use of it, we'd need to cope
with both variants if we change it. I still think that there's not a lot of
value in changing the name in storage, but if you feel strongly then I won't
insist. However, if you do change it, please add a comment in this function
explaining that some versions of the web-app use the "optIn" name.

[Sergey Ulanov, 2013/06/03 19:11:48]

Ok, renamed it back to optIn.

+ */
+remoting.dismissButterBar = function(storageKey, optIn) {
+  var value = {};
+  value[storageKey] = {
+    optIn: optIn,
+    date: new Date(),
+    version: chrome.runtime.getManifest().version
+  };
+  chrome.storage.sync.set(value);
+
+  remoting.showButterBar(remoting.ButterBarMode.NONE);
+};
+
+/**
+ * Show the butter bar if there is a message to show.
+ */
+remoting.initButterBar = function() {
+  document.getElementById(kHangoutsAcceptId).addEventListener(
+      'click', remoting.dismissButterBar.bind(null, kHangoutsStorageKey, true),
+      false);
+  document.getElementById(kHangoutsDismissId).addEventListener(
+      'click', remoting.dismissButterBar.bind(null, kHangoutsStorageKey, false),
+      false);
+
+  document.getElementById(kSurveyAcceptId).addEventListener(
+      'click', remoting.dismissButterBar.bind(null, kSurveyStorageKey, true),
+      false);
+  document.getElementById(kSurveyDismissId).addEventListener(
+      'click', remoting.dismissButterBar.bind(null, kSurveyStorageKey, false),
+      false);

[Jamie, 2013/05/23 20:31:46]

It's a shame to duplicate the action elements and this logic for the two
messages, and it doesn't really scale if we ever want to use this mechanism for
more messages.

I think a better design would be for this class just to manage a single
butter-bar message with one "yes please" link and one close icon. The storage
key can be a class member and you can have an |init| method that handles the
logic of picking which message to display, localizing the butter-bar UI with
that message and showing it.

One downside of that is that all messages need to conform to the same overall
structure w.r.t. having placeholders for the <a> and </a> elements. In
particular, you'd need to change the survey message so that the "Sign me up"
link is part of the message rather than a separate HTML element.

WDYT?

[Sergey Ulanov, 2013/05/29 23:36:11]

Done.

+
+  /** @param {boolean} surveyDismissed
+    * @param {boolean} hangoutsDismissed
+    * @param {boolean} it2meExpanded */
+  var onStateLoaded = function(surveyDismissed, hangoutsDismissed,
+                               it2meExpanded) {
+    var showSurvey = !surveyDismissed;
+    var showHangouts = it2meExpanded && !hangoutsDismissed;
+    if (showSurvey && !showHangouts) {
+      remoting.showButterBar(remoting.ButterBarMode.SURVEY);
+    } else if (showHangouts && !showSurvey) {
+      remoting.showButterBar(remoting.ButterBarMode.HANGOUTS);
+    } else if (showSurvey && showHangouts) {
+      if (Math.random() > 0.5) {
+        remoting.showButterBar(remoting.ButterBarMode.SURVEY);
+      } else {
+        remoting.showButterBar(remoting.ButterBarMode.HANGOUTS);
+      }
+    }
+  };
+  /** @param {Object} value */
+  var onSyncDataLoaded = function(value) {
+    /** @type {boolean} */
+    var surveyDismissed = !!value[kSurveyStorageKey];
+    /** @type {boolean} */
+    var hangoutsDismissed = !!value[kHangoutsStorageKey];
+    chrome.storage.local.get(remoting.kIT2MeVisitedStorageKey, function(value) {

[Jamie, 2013/05/23 20:31:46]

Optional: I'm not a big fan of anonymous functions, or nesting functions more
than two levels deep. Would you object to declaring an |onIT2MeVisitedLoaded|
function at the same level as |onSyncDataLoaded|?

[Sergey Ulanov, 2013/05/29 23:36:11]

Done

+      /** @type {boolean} */
+      var it2meExpanded = !!value[remoting.kIT2MeVisitedStorageKey];
+      onStateLoaded(surveyDismissed, hangoutsDismissed, it2meExpanded);
+    });
+  };
+  chrome.storage.sync.get(
+      [kSurveyStorageKey, kHangoutsStorageKey], onSyncDataLoaded);
+}
+
+/**
+ * Shows butter bar in the specified |mode|.
+ *
+ * @param {remoting.ButterBarMode} mode
+ */
+remoting.showButterBar = function(mode) {
+  if (mode == remoting.ButterBarMode.NONE) {
+    document.getElementById(kButterBarId).hidden = true;
+    return;
+  }
+
+  document.getElementById(kButterBarId).hidden = false;
+  if (mode == remoting.ButterBarMode.SURVEY) {
+    document.getElementById(kSurveyInvitationId).hidden = false;
+    document.getElementById(kHangoutsInvitationId).hidden = true;
+  } else {
+    document.getElementById(kSurveyInvitationId).hidden = true;
+    document.getElementById(kHangoutsInvitationId).hidden = false;
+  }

[Jamie, 2013/05/23 20:31:46]

This can be more simply expressed as three expressions of the form:

  element.hidden = (mode == xxx);

[Sergey Ulanov, 2013/05/29 23:36:11]

Removed most of this code.

+};