Merge pull request #1644 from dvoytenko/storage2

Using local storage for notification toggling

build-system/tasks/presubmit-checks.js

@@ -198,6 +198,7 @@ var forbiddenTerms = {
  message: requiresReviewPrivacy,
  whitelist: [
  'src/storage.js',
+ 'extensions/amp-user-notification/0.1/amp-user-notification.js',
  ],
  },
  'localStorage': {

extensions/amp-user-notification/0.1/amp-user-notification.js

@@ -19,7 +19,9 @@ import {assertHttpsUrl, addParamsToUrl} from '../../../src/url';
 import {assert} from '../../../src/asserts';
 import {cidFor} from '../../../src/cid';
 import {getService} from '../../../src/service';
+import {isExperimentOn} from '../../../src/experiments';
 import {log} from '../../../src/log';
+import {storageFor} from '../../../src/storage';
 import {urlReplacementsFor} from '../../../src/url-replacements';
 import {viewerFor} from '../../../src/viewer';
 import {whenDocumentReady} from '../../../src/document-state';
@@ -79,14 +81,27 @@ class NotificationInterface {
 export class AmpUserNotification extends AMP.BaseElement {
 
  /** @override */
- buildCallback() {
+ createdCallback() {
 
  /** @private @const {!Window} */
  this.win_ = this.getWin();
 
  /** @private @const {!UrlReplacements} */
  this.urlReplacements_ = urlReplacementsFor(this.win_);
 
+ /** @private @const {!UserNotificationManager} */
+ this.userNotificationManager_ = getUserNotificationManager_(this.win_);
+
+ /** @const @private {!Promise<!Storage>} */
+ this.storagePromise_ = storageFor(this.win_);
+
+ /** @const @private {boolean} */
+ this.isStorageEnabled_ = isExperimentOn(this.getWin(), 'amp-storage');
+ }
+
+ /** @override */
+ buildCallback() {
+
  /** @private {?string} */
  this.ampUserId_ = null;
 
@@ -98,26 +113,35 @@ export class AmpUserNotification extends AMP.BaseElement {
  this.dialogResolve_ = resolve;
  });
 
- /** @private @const {!UserNotificationManager} */
- this.userNotificationManager_ = getUserNotificationManager_(this.win_);
-
  this.elementId_ = assert(this.element.id,
  'amp-user-notification should have an id.');
 
- assert(this.element.hasAttribute('data-show-if-href'),
- `"amp-user-notification" (${this.elementId_}) ` +
- 'should have "data-show-if-href" attribute.');
- /** @private @const {string} */
- this.showIfHref_ = assertHttpsUrl(
- this.element.getAttribute('data-show-if-href'), this.element);
+ /** @private @const {?string} */
+ this.storageKey_ = this.isStorageEnabled_ ?
+ 'amp-user-notification:' + this.elementId_ : null;
 
- assert(this.element.hasAttribute('data-dismiss-href'),
- `"amp-user-notification" (${this.elementId_}) ` +
- 'should have "data-dismiss-href" attribute.');
+ /** @private @const {?string} */
+ this.showIfHref_ = this.element.getAttribute('data-show-if-href');
+ if (this.showIfHref_) {
+ assertHttpsUrl(this.showIfHref_, this.element);
+ }
 
- /** @private @const {string} */
- this.dismissHref_ = assertHttpsUrl(
- this.element.getAttribute('data-dismiss-href'), this.element);
+ /** @private @const {?string} */
+ this.dismissHref_ = this.element.getAttribute('data-dismiss-href');
+ if (this.dismissHref_) {
+ assertHttpsUrl(this.dismissHref_, this.element);
+ }
+
+ // TODO(dvoytenko): document storage behavior in these methods once
+ // experiment is removed.
+ if (!this.storageKey_) {
+ assert(this.showIfHref_,
+ `"amp-user-notification" (${this.elementId_}) ` +
+ 'should have "data-show-if-href" attribute.');
+ assert(this.dismissHref_,
+ `"amp-user-notification" (${this.elementId_}) ` +
+ 'should have "data-dismiss-href" attribute.');
+ }
 
  this.userNotificationManager_
  .registerUserNotification(this.elementId_, this);
@@ -133,7 +157,7 @@ export class AmpUserNotification extends AMP.BaseElement {
  * @private
  */
  buildGetHref_(ampUserId) {
- return this.urlReplacements_.expand(this.showIfHref_).then(href => {
+ return this.urlReplacements_.expand(assert(this.showIfHref_)).then(href => {
  return addParamsToUrl(href, {
  elementId: this.elementId_,
  ampUserId: ampUserId
@@ -164,7 +188,7 @@ export class AmpUserNotification extends AMP.BaseElement {
  * @return {!Promise}
  */
  postDismissEnpoint_() {
- return xhrFor(this.win_).fetchJson(this.dismissHref_, {
+ return xhrFor(this.win_).fetchJson(assert(this.dismissHref_), {
  method: 'POST',
  credentials: 'include',
  body: {
@@ -215,6 +239,36 @@ export class AmpUserNotification extends AMP.BaseElement {
 
  /** @override */
  shouldShow() {
+ if (this.storageKey_) {
+ return this.storagePromise_.then(storage => {
+ return storage.get(this.storageKey_);
+ }).then(value => {
+ if (value) {
+ // Consent has been accepted. Nothing more to do.
+ return false;
+ }
+ if (this.showIfHref_) {
+ // Ask remote endpoint if available.
+ return this.shouldShowViaXhr_();
+ }
+ // Otherwise, show the notification.
+ return true;
+ }).catch(reason => {
+ log.error('Failed to read storage', reason);
+ if (this.showIfHref_) {
+ return this.shouldShowViaXhr_();
+ }
+ return true;
+ });
+ }
+ return this.shouldShowViaXhr_();
+ }
+
+ /**
+ * @return {!Promise<boolean>}
+ * @private
+ */
+ shouldShowViaXhr_() {
  return this.getAsyncCid_()
  .then(this.getShowEndpoint_.bind(this))
  .then(this.onGetShowEndpointSuccess_.bind(this));
@@ -235,7 +289,16 @@ export class AmpUserNotification extends AMP.BaseElement {
  this.element.classList.remove('amp-active');
  this.element.classList.add('amp-hidden');
  this.dialogResolve_();
- this.postDismissEnpoint_();
+
+ // Store and post.
+ if (this.storageKey_) {
+ this.storagePromise_.then(storage => {
+ storage.set(this.storageKey_, true);
+ });
+ }
+ if (this.dismissHref_) {
+ this.postDismissEnpoint_();
+ }
  }
 }