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

Jump to bottom

Refactor methods in viewer into performance-impl #6252

Merged
merged 3 commits into from
Nov 23, 2016
Merged

Refactor methods in viewer into performance-impl #6252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dedca42
Refactor methods in viewer into performance-impl
muxin Nov 18, 2016
c599ba2
Addressed comments
muxin Nov 21, 2016
8e5478c
Refine tests
muxin Nov 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions build-system/tasks/presubmit-checks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,19 @@ var forbiddenTerms = {
whitelist: [
'src/service/viewer-impl.js',
'src/service/storage-impl.js',
'src/service/performance-impl.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erwinmombay I saw we have whitelist rule for sendMessage but not the sendMessageCancelUnsent (previously sendMessageUnreliable). Do we need to add rules for sendMessageCancelUnsent too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dvoytenko WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

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.

'examples/viewer-integr-messaging.js',
'extensions/amp-access/0.1/login-dialog.js',
'extensions/amp-access/0.1/signin.js',
],
},
'sendMessageCancelUnsent': {
message: 'Usages must be reviewed.',
whitelist: [
'src/service/viewer-impl.js',
'src/service/performance-impl.js',
],
},
// Privacy sensitive
'cidFor': {
message: requiresReviewPrivacy,
Expand Down
19 changes: 10 additions & 9 deletions src/service/performance-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ export class Performance {
* @private
*/
setFlushParams_(params) {
this.viewer_.setFlushParams(params);
this.viewer_.sendMessageCancelUnsent('setFlushParams', params,
/* awaitResponse */false);
}

/**
Expand All @@ -239,11 +240,11 @@ export class Performance {
opt_value = opt_value == undefined ? Date.now() : opt_value;

if (this.isMessagingReady_ && this.isPerformanceTrackingOn_) {
this.viewer_.tick({
this.viewer_.sendMessage('tick', {
label,
from: opt_from,
value: opt_value,
});
}, /* awaitResponse */false);
} else {
this.queueTick_(label, opt_from, opt_value);
}
Expand Down Expand Up @@ -282,11 +283,12 @@ export class Performance {


/**
* Calls the "flushTicks" function on the viewer.
* Ask the viewer to flush the ticks
*/
flush() {
if (this.isMessagingReady_ && this.isPerformanceTrackingOn_) {
this.viewer_.flushTicks();
this.viewer_.sendMessageCancelUnsent('sendCsi', undefined,
/* awaitResponse */false);
}
}

Expand Down Expand Up @@ -332,7 +334,7 @@ export class Performance {
}

this.events_.forEach(tickEvent => {
this.viewer_.tick(tickEvent);
this.viewer_.sendMessage('tick', tickEvent, /* awaitResponse */false);
});
this.events_.length = 0;
}
Expand Down Expand Up @@ -370,9 +372,8 @@ export class Performance {
*/
prerenderComplete_(value) {
if (this.viewer_) {
this.viewer_.prerenderComplete({
value,
});
this.viewer_.sendMessageCancelUnsent('prerenderComplete', {value},
/* awaitResponse */false);
}
}

Expand Down
35 changes: 0 additions & 35 deletions src/service/viewer-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,41 +882,6 @@ export class Viewer {
'fragment', {fragment}, true));
}

/**
* Triggers "tick" event for the viewer.
* @param {!Object} message
* TODO: move this to performance-impl, and use sendMessage()
*/
tick(message) {
this.sendMessageCancelUnsent('tick', message, false);
}

/**
* Triggers "sendCsi" event for the viewer.
* TODO: move this to performance-impl
*/
flushTicks() {
this.sendMessageCancelUnsent('sendCsi', undefined, false);
}

/**
* Triggers "setFlushParams" event for the viewer.
* @param {!Object} message
* TODO: move this to performance-impl
*/
setFlushParams(message) {
this.sendMessageCancelUnsent('setFlushParams', message, false);
}

/**
* Triggers "prerenderComplete" event for the viewer.
* @param {!Object} message
* TODO: move this to performance-impl
*/
prerenderComplete(message) {
this.sendMessageCancelUnsent('prerenderComplete', message, false);
}

/**
* Requests AMP document to receive a message from Viewer.
* @param {string} eventType
Expand Down