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.

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

feature(perf): add setFlushParams method #1862

Merged
merged 1 commit into from
Feb 11, 2016

Conversation

erwinmombay
Copy link
Member

No description provided.

@erwinmombay erwinmombay changed the title Set query params feature(perf): add setParams method Feb 9, 2016
@erwinmombay erwinmombay changed the title feature(perf): add setParams method [WIP] feature(perf): add setParams method Feb 9, 2016
@erwinmombay erwinmombay force-pushed the set-query-params branch 3 times, most recently from d7bab4d to 0a12fa5 Compare February 9, 2016 11:34
@erwinmombay
Copy link
Member Author

@cramforce PTAL

@erwinmombay erwinmombay force-pushed the set-query-params branch 2 times, most recently from 899dfc4 to b732b36 Compare February 9, 2016 11:36
this.whenViewportLayoutComplete_()
]).then(() => {
const params = Object.create(null);
params['canonicalUrl'] = documentInfoFor(this.win).canonicalUrl;
Copy link
Member

Choose a reason for hiding this comment

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

Lets instead use getSourceUrl from url.js but expose that via documentInfo

Copy link
Member Author

Choose a reason for hiding this comment

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

do we use window.location.href to pass into getSourceUrl or the canonical url? (result unlikely to be different, but not sure which one is more trusted)

Copy link
Member

Choose a reason for hiding this comment

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

As discussed lets just use this.win.location.href without the fragment.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@cramforce
Copy link
Member

Looks great. A couple comments.

@erwinmombay erwinmombay changed the title [WIP] feature(perf): add setParams method feature(perf): add setParams method Feb 9, 2016
@@ -135,9 +135,9 @@ export function adopt(global) {
* @param {function()=} opt_flush
* @export
*/
global.AMP.setTickFunction = (fn, opt_flush) => {
global.AMP.setTickFunction = (fn, opt_flush, opt_setParams) => {
Copy link

Choose a reason for hiding this comment

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

Probably should have done this from the beginning, but could you use viewer.sendMessage() for all three of these messages to the viewer?

This is how all other communication with the viewer is done and it simpler for the viewer since it just receives whatever messages you send and we don't have to set up the callback by calling setTickFunction().
https://github.com/ampproject/amphtml/blob/master/src/service/viewer-impl.js#L703

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

Copy link
Member Author

Choose a reason for hiding this comment

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

@ericfs can you enumerate/confirm the actual event names you want for these?

  • 'tick'
  • 'flush'
  • 'setParams'

params['canonicalUrl'] = documentInfoFor(this.win).canonicalUrl;

/**
* @param {!Object<string, string>}
Copy link
Member

Choose a reason for hiding this comment

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

Document

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@erwinmombay erwinmombay force-pushed the set-query-params branch 4 times, most recently from 9ba5dbf to 2011b66 Compare February 10, 2016 10:07
*/
sendCsi() {
this.sendMessage_('sendCsi', undefined, false);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@ericfs can you review the changes to viewer-impl.js here. thanks

@erwinmombay
Copy link
Member Author

@cramforce PTAL. changed quite a bit with the deprecation of window.AMP.setTickFunction.

@erwinmombay erwinmombay force-pushed the set-query-params branch 2 times, most recently from ffd471d to 256c75a Compare February 10, 2016 10:11
* Triggers "sendCsi" event for the viewer.
*/
sendCsi() {
this.sendMessage_('sendCsi', undefined, false);
Copy link

Choose a reason for hiding this comment

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

This name should probably be changed to something more generic, but I think it makes sense to stick with this for now.

That is, the viewer can support both this and a new name, then the runtime can move to use the new name.

* @param {!JSONObject} message
*/
setParams(message) {
this.sendMessage_('setParams', message, false);
Copy link

Choose a reason for hiding this comment

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

Could we use a different name? "setParams" seems a bit vague to me. Maybe "setFlushParams" or "setInstrumentationParams"

Copy link
Member Author

Choose a reason for hiding this comment

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

done. switched to setFlushParams

@@ -33,6 +33,7 @@ export function documentInfoFor(win) {
'AMP files are required to have a <link rel=canonical> tag.')
.href).href,
pageViewId: getPageViewId(win),
sourceUrl: getSourceUrl(win.location.href),
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to update the return doc comment above.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks! done.

@ericfs
Copy link

ericfs commented Feb 10, 2016

LGTM for viewer interface

@erwinmombay erwinmombay force-pushed the set-query-params branch 5 times, most recently from 2462a86 to 798893a Compare February 10, 2016 19:06
@erwinmombay
Copy link
Member Author

@cramforce ping pong

@cramforce
Copy link
Member

LGTM. Please manually check that reporting works when this lands in canary.

@erwinmombay
Copy link
Member Author

@cramforce will do. whats the expected/estimated time frame that new data usually shows up, 24hrs?

@erwinmombay erwinmombay force-pushed the set-query-params branch 2 times, most recently from e4f23ec to 80be55e Compare February 11, 2016 19:19
@cramforce
Copy link
Member

What I meant is to check that the requests are made and that the values look good. The Dashboard is too slow :(

@ericfs
Copy link

ericfs commented Feb 11, 2016

Alex or I can help to check. Feel free to ping one of us when the canary is ready.

erwinmombay added a commit that referenced this pull request Feb 11, 2016
feature(perf): add `setParams` method
@erwinmombay erwinmombay merged commit 34c5b86 into ampproject:master Feb 11, 2016
@erwinmombay
Copy link
Member Author

@ericfs this is live now (canary)

@erwinmombay
Copy link
Member Author

tested on our demo viewer integration and that works there, need confirmation from @ericfs if format and data look good for tick and setFlushParams

@erwinmombay erwinmombay changed the title feature(perf): add setParams method feature(perf): add setFlushParams method Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants