Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor observables in viewer-impl into a map object #7004

Merged
merged 4 commits into from
Jan 13, 2017
Merged

Refactor observables in viewer-impl into a map object #7004

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec0a4b1
Refactor observables in viewer-impl into a map object
muxin Jan 12, 2017
c43eb90
Fix check-types
muxin Jan 12, 2017
e5191c9
Keep broadcastObservable and rename paddingTop_ in viewer
muxin Jan 13, 2017
14d3f39
No need to store paddingTop value in viewer-impl
muxin Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/service/history-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ export class HistoryBindingVirtual_ {
this.onStackIndexUpdated_ = null;

/** @private {!UnlistenDef} */
this.unlistenOnHistoryPopped_ = this.viewer_.onHistoryPoppedEvent(
this.unlistenOnHistoryPopped_ = this.viewer_.onMessage('historyPopped',
this.onHistoryPopped_.bind(this));
}

Expand Down Expand Up @@ -764,11 +764,11 @@ export class HistoryBindingVirtual_ {
}

/**
* @param {!./viewer-impl.ViewerHistoryPoppedEventDef} event
* @param {!JSONType} data
* @private
*/
onHistoryPopped_(event) {
this.updateStackIndex_(event.newStackIndex);
onHistoryPopped_(data) {
this.updateStackIndex_(data['newStackIndex']);
}

/**
Expand Down
74 changes: 25 additions & 49 deletions src/service/viewer-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import {Observable} from '../observable';
import {findIndex} from '../utils/array';
import {map} from '../utils/object';
import {documentStateFor} from './document-state';
import {getServiceForDoc} from '../service';
import {dev} from '../log';
Expand Down Expand Up @@ -70,7 +71,6 @@ const TRUSTED_VIEWER_HOSTS = [
/(^|\.)google\.(com?|[a-z]{2}|com?\.[a-z]{2}|cat)$/,
];


/**
* An AMP representation of the Viewer. This class doesn't do any work itself
* but instead delegates everything to the actual viewer. This class and the
Expand Down Expand Up @@ -113,20 +113,17 @@ export class Viewer {
this.prerenderSize_ = 1;

/** @private {number} */
this.paddingTop_ = 0;
this.initPaddingTop_ = 0;

/** @private {!Object<string, !Observable<!JSONType>>} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a special object for these messages. Let's call it a ViewerMessage and put this class in this file. It will be similar to ActionInvocation class in https://github.com/ampproject/amphtml/blob/master/src/service/action-impl.js#L69

E.g.

export class ViewerMessage {
  constructor(type, data) {
     /** @const {string} */
     this.type = type;
     /** @const {!JSONType} */
     this.data = data;
  }
}

Thus, this declaration will become:

/** @private {!Object<string, !Observable<!ViewerMessage>>} */

this.messageObservables_ = map();

/** @private {!Observable<boolean>} */
this.runtimeOnObservable_ = new Observable();

/** @private {!Observable} */
this.visibilityObservable_ = new Observable();

/** @private {!Observable<!JSONType>} */
this.viewportObservable_ = new Observable();

/** @private {!Observable<!ViewerHistoryPoppedEventDef>} */
this.historyPoppedObservable_ = new Observable();

/** @private {!Observable<!JSONType>} */
this.broadcastObservable_ = new Observable();

Expand Down Expand Up @@ -204,9 +201,9 @@ export class Viewer {
this.prerenderSize_;
dev().fine(TAG_, '- prerenderSize:', this.prerenderSize_);

this.paddingTop_ = parseInt(this.params_['paddingTop'], 10) ||
this.paddingTop_;
dev().fine(TAG_, '- padding-top:', this.paddingTop_);
this.initPaddingTop_ = parseInt(this.params_['paddingTop'], 10) ||
this.initPaddingTop_;
dev().fine(TAG_, '- initial padding-top:', this.initPaddingTop_);

/**
* Whether the AMP document is embedded in a webview.
Expand Down Expand Up @@ -600,8 +597,8 @@ export class Viewer {
* Returns the top padding requested by the viewer.
* @return {number}
*/
getPaddingTop() {
return this.paddingTop_;
getInitPaddingTop() {
return this.initPaddingTop_;
}

/**
Expand Down Expand Up @@ -704,21 +701,18 @@ export class Viewer {
}

/**
* Adds a "viewport" event listener for viewer events.
* @param {function(!JSONType)} handler
* @return {!UnlistenDef}
*/
onViewportEvent(handler) {
return this.viewportObservable_.add(handler);
}

/**
* Adds a "history popped" event listener for viewer events.
* @param {function(ViewerHistoryPoppedEventDef)} handler
* Adds a eventType listener for viewer events.
* @param {string} eventType
* @param {!function(!JSONType)} handler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

functions are already ! by default. remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* @return {!UnlistenDef}
*/
onHistoryPoppedEvent(handler) {
return this.historyPoppedObservable_.add(handler);
onMessage(eventType, handler) {
let observable = this.messageObservables_[eventType];
if (!observable) {
observable = new Observable();
this.messageObservables_[eventType] = observable;
}
return observable.add(handler);
}

/**
Expand Down Expand Up @@ -763,21 +757,6 @@ export class Viewer {
* @export
*/
receiveMessage(eventType, data, unusedAwaitResponse) {
if (eventType == 'viewport') {
if (data['paddingTop'] !== undefined) {
this.paddingTop_ = data['paddingTop'];
this.viewportObservable_.fire(
/** @type {!JSONType} */ (data));
return Promise.resolve();
}
return undefined;
}
if (eventType == 'historyPopped') {
this.historyPoppedObservable_.fire({
newStackIndex: data['newStackIndex'],
});
return Promise.resolve();
}
if (eventType == 'visibilitychange') {
if (data['prerenderSize'] !== undefined) {
this.prerenderSize_ = data['prerenderSize'];
Expand All @@ -791,6 +770,11 @@ export class Viewer {
/** @type {!JSONType|undefined} */ (data));
return Promise.resolve();
}
const observable = this.messageObservables_[eventType];
if (observable) {
observable.fire(/** @type {!JSONType} */ (data));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already !JSONType. Why cast it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

return Promise.resolve();
}
dev().fine(TAG_, 'unknown message:', eventType);
return undefined;
}
Expand Down Expand Up @@ -991,14 +975,6 @@ function getChannelError(opt_reason) {
return new Error('No messaging channel: ' + opt_reason);
}

/**
* @typedef {{
* newStackIndex: number
* }}
*/
export let ViewerHistoryPoppedEventDef;


/**
* Sets the viewer visibility state. This calls is restricted to runtime only.
* @param {!VisibilityState} state
Expand Down
16 changes: 8 additions & 8 deletions src/service/viewport-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class Viewport {
this./*OK*/scrollLeft_ = null;

/** @private {number} */
this.paddingTop_ = viewer.getPaddingTop();
this.paddingTop_ = viewer.getInitPaddingTop();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here: this.paddingTop_ = Number(viewer.getParam('paddingTop') || 0). Should work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks for the simpler solution


/** @private {number} */
this.lastPaddingTop_ = 0;
Expand Down Expand Up @@ -151,7 +151,7 @@ export class Viewport {
/** @private @const (function()) */
this.boundThrottledScroll_ = this.throttledScroll_.bind(this);

this.viewer_.onViewportEvent(this.updateOnViewportEvent_.bind(this));
this.viewer_.onMessage('viewport', this.updateOnViewportEvent_.bind(this));
this.binding_.updatePaddingTop(this.paddingTop_);

this.binding_.onScroll(this.scroll_.bind(this));
Expand Down Expand Up @@ -584,15 +584,15 @@ export class Viewport {
}

/**
* @param {!JSONType} event
* @param {!JSONType} data
* @private
*/
updateOnViewportEvent_(event) {
const paddingTop = event['paddingTop'];
const duration = event['duration'] || 0;
const curve = event['curve'];
updateOnViewportEvent_(data) {
const paddingTop = data['paddingTop'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to old code, it's possible that paddingTop is undefined. I.e. see this old code snippet:

    if (eventType == 'viewport') {
      if (data['paddingTop'] !== undefined) {
        this.paddingTop_ = data['paddingTop'];
        this.viewportObservable_.fire(
          /** @type {!JSONType} */ (data));

I don't know if it's a real situation here. If it is, we may want to add the following in this method:

const paddingTop = data['paddingTop'];
if (paddingTop == undefined) {
  return;
}

Please check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, fixed

const duration = data['duration'] || 0;
const curve = data['curve'];
/** @const {boolean} */
const transient = event['transient'];
const transient = data['transient'];

if (paddingTop != this.paddingTop_) {
this.lastPaddingTop_ = this.paddingTop_;
Expand Down
8 changes: 4 additions & 4 deletions test/functional/test-history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ describes.sandboxed('History install', {}, () => {
beforeEach(() => {
viewer = {
isOvertakeHistory: () => false,
onHistoryPoppedEvent: () => function() {},
onMessage: () => function() {},
};

win = {
Expand Down Expand Up @@ -374,7 +374,7 @@ describe('HistoryBindingVirtual', () => {
onStackIndexUpdated = sandbox.spy();
viewerHistoryPoppedHandler = undefined;
const viewer = {
onHistoryPoppedEvent: handler => {
onMessage: (eventType, handler) => {
viewerHistoryPoppedHandler = handler;
return () => {};
},
Expand Down Expand Up @@ -484,7 +484,7 @@ describes.fakeWin('Local Hash Navigation', {

it('should push a new state and replace it for target on Virtual', () => {
const viewer = {
onHistoryPoppedEvent: () => {
onMessage: () => {
return () => {};
},
postPushHistory: unusedStackIndex => {},
Expand Down Expand Up @@ -524,7 +524,7 @@ describes.fakeWin('Get and update fragment', {}, env => {
installTimerService(env.win);
sandbox = env.sandbox;
viewer = {
onHistoryPoppedEvent: () => {
onMessage: () => {
return () => {};
},
hasCapability: () => {},
Expand Down
9 changes: 4 additions & 5 deletions test/functional/test-viewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,14 @@ describe('Viewer', () => {
});

it('should configure as 0 padding top by default', () => {
expect(viewer.getPaddingTop()).to.equal(0);
expect(viewer.getInitPaddingTop()).to.equal(0);
});

it('should configure correctly based on window name and hash', () => {
windowApi.name = '__AMP__viewportType=natural';
windowApi.location.hash = '#paddingTop=17&other=something';
const viewer = new Viewer(ampdoc);
expect(viewer.getPaddingTop()).to.equal(17);
expect(viewer.getInitPaddingTop()).to.equal(17);

// All of the startup params are also available via getParam.
expect(viewer.getParam('paddingTop')).to.equal('17');
Expand All @@ -116,7 +116,7 @@ describe('Viewer', () => {
windowApi.name = '__AMP__other=something';
windowApi.location.hash = '#paddingTop=17';
const viewer = new Viewer(ampdoc, params);
expect(viewer.getPaddingTop()).to.equal(171);
expect(viewer.getInitPaddingTop()).to.equal(171);

// All of the startup params are also available via getParam.
expect(viewer.getParam('paddingTop')).to.equal('171');
Expand Down Expand Up @@ -200,14 +200,13 @@ describe('Viewer', () => {

it('should receive viewport event', () => {
let viewportEvent = null;
viewer.onViewportEvent(event => {
viewer.onMessage('viewport', event => {
viewportEvent = event;
});
viewer.receiveMessage('viewport', {
paddingTop: 19,
});
expect(viewportEvent).to.not.equal(null);
expect(viewer.getPaddingTop()).to.equal(19);
});

describe('should receive the visibilitychange event', () => {
Expand Down
12 changes: 6 additions & 6 deletions test/functional/test-viewport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ describes.fakeWin('Viewport', {}, env => {
viewerViewportHandler = undefined;
viewer = {
isEmbedded: () => false,
getPaddingTop: () => 19,
onViewportEvent: handler => {
getInitPaddingTop: () => 19,
onMessage: (eventType, handler) => {
viewerViewportHandler = handler;
},
sendMessage: sandbox.spy(),
Expand Down Expand Up @@ -845,8 +845,8 @@ describe('Viewport META', () => {
clock = sandbox.useFakeTimers();
viewer = {
isEmbedded: () => false,
getPaddingTop: () => 0,
onViewportEvent: () => {},
getInitPaddingTop: () => 0,
onMessage: () => {},
isVisible: () => true,
onVisibilityChanged: () => {},
};
Expand Down Expand Up @@ -1009,8 +1009,8 @@ describe('ViewportBindingNatural', () => {
installPlatformService(windowApi);
viewer = {
isEmbedded: () => false,
getPaddingTop: () => 19,
onViewportEvent: () => {},
getInitPaddingTop: () => 19,
onMessage: () => {},
};
viewerMock = sandbox.mock(viewer);
binding = new ViewportBindingNatural_(windowApi, viewer);
Expand Down