feature(perf): add `setFlushParams` method #1862

erwinmombay · 2016-02-09T10:08:23Z

No description provided.

erwinmombay · 2016-02-09T11:35:40Z

cramforce · 2016-02-09T16:17:42Z

src/performance.js

+      this.whenViewportLayoutComplete_()
+    ]).then(() => {
+      const params = Object.create(null);
+      params['canonicalUrl'] = documentInfoFor(this.win).canonicalUrl;


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

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)

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

cramforce · 2016-02-09T16:19:51Z

Looks great. A couple comments.

ericfs · 2016-02-09T20:08:14Z

src/runtime.js

@@ -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) => {


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

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

'tick'

'flush'

'setParams'

cramforce · 2016-02-10T01:58:03Z

src/performance.js

+      params['canonicalUrl'] = documentInfoFor(this.win).canonicalUrl;
+
+      /**
+       * @param {!Object<string, string>}


erwinmombay · 2016-02-10T10:07:37Z

src/service/viewer-impl.js

+   */
+  sendCsi() {
+    this.sendMessage_('sendCsi', undefined, false);
+  }


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

erwinmombay · 2016-02-10T10:08:59Z

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

ericfs · 2016-02-10T14:17:06Z

src/service/viewer-impl.js

+   * Triggers "sendCsi" event for the viewer.
+   */
+  sendCsi() {
+    this.sendMessage_('sendCsi', undefined, false);


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.

ericfs · 2016-02-10T14:18:52Z

src/service/viewer-impl.js

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


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

done. switched to setFlushParams

jridgewell · 2016-02-10T17:52:06Z

src/document-info.js

@@ -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),


Need to update the return doc comment above.

thanks! done.

ericfs · 2016-02-10T17:58:49Z

LGTM for viewer interface

erwinmombay · 2016-02-11T17:54:56Z

@cramforce ping pong

cramforce · 2016-02-11T19:09:16Z

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

erwinmombay · 2016-02-11T19:17:26Z

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

cramforce · 2016-02-11T19:19:37Z

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

ericfs · 2016-02-11T19:21:01Z

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

feature(perf): add `setParams` method

erwinmombay · 2016-02-11T22:36:04Z

@ericfs this is live now (canary)

erwinmombay · 2016-02-11T22:48:52Z

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 force-pushed the set-query-params branch from d743642 to 73e2fa7 Compare February 9, 2016 10:08

erwinmombay changed the title ~~Set query params~~ feature(perf): add setParams method Feb 9, 2016

erwinmombay changed the title ~~feature(perf): add setParams method~~ [WIP] feature(perf): add setParams method Feb 9, 2016

erwinmombay force-pushed the set-query-params branch 3 times, most recently from d7bab4d to 0a12fa5 Compare February 9, 2016 11:34

erwinmombay added the NEEDS REVIEW label Feb 9, 2016

erwinmombay assigned cramforce Feb 9, 2016

erwinmombay force-pushed the set-query-params branch 2 times, most recently from 899dfc4 to b732b36 Compare February 9, 2016 11:36

cramforce reviewed Feb 9, 2016
View reviewed changes

erwinmombay force-pushed the set-query-params branch from b732b36 to d0c3674 Compare February 9, 2016 19:11

erwinmombay changed the title ~~[WIP] feature(perf): add setParams method~~ feature(perf): add setParams method Feb 9, 2016

erwinmombay force-pushed the set-query-params branch from d0c3674 to e25a49d Compare February 9, 2016 19:31

ericfs reviewed Feb 9, 2016
View reviewed changes

erwinmombay force-pushed the set-query-params branch from e25a49d to c1a3e25 Compare February 9, 2016 20:20

cramforce reviewed Feb 10, 2016
View reviewed changes

erwinmombay force-pushed the set-query-params branch 4 times, most recently from 9ba5dbf to 2011b66 Compare February 10, 2016 10:07

erwinmombay reviewed Feb 10, 2016
View reviewed changes

erwinmombay force-pushed the set-query-params branch 2 times, most recently from ffd471d to 256c75a Compare February 10, 2016 10:11

ericfs reviewed Feb 10, 2016
View reviewed changes

erwinmombay force-pushed the set-query-params branch from 256c75a to 87f90b6 Compare February 10, 2016 16:34

jridgewell reviewed Feb 10, 2016
View reviewed changes

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

cramforce added LGTM and removed NEEDS REVIEW labels Feb 11, 2016

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

feature(perf): add setParams method

94e7951

erwinmombay force-pushed the set-query-params branch from 80be55e to 94e7951 Compare February 11, 2016 19:20

erwinmombay added a commit that referenced this pull request Feb 11, 2016

Merge pull request #1862 from erwinmombay/set-query-params

34c5b86

feature(perf): add `setParams` method

erwinmombay merged commit 34c5b86 into ampproject:master Feb 11, 2016

erwinmombay changed the title ~~feature(perf): add setParams method~~ feature(perf): add setFlushParams method Feb 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(perf): add `setFlushParams` method #1862

feature(perf): add `setFlushParams` method #1862

erwinmombay commented Feb 9, 2016

erwinmombay commented Feb 9, 2016

cramforce Feb 9, 2016

erwinmombay Feb 9, 2016

cramforce Feb 10, 2016

erwinmombay Feb 10, 2016

cramforce commented Feb 9, 2016

ericfs Feb 9, 2016

erwinmombay Feb 9, 2016

erwinmombay Feb 9, 2016

cramforce Feb 10, 2016

erwinmombay Feb 10, 2016

erwinmombay Feb 10, 2016

erwinmombay commented Feb 10, 2016

ericfs Feb 10, 2016

ericfs Feb 10, 2016

erwinmombay Feb 10, 2016

jridgewell Feb 10, 2016

erwinmombay Feb 10, 2016

ericfs commented Feb 10, 2016

erwinmombay commented Feb 11, 2016

cramforce commented Feb 11, 2016

erwinmombay commented Feb 11, 2016

cramforce commented Feb 11, 2016

ericfs commented Feb 11, 2016

erwinmombay commented Feb 11, 2016

erwinmombay commented Feb 11, 2016

feature(perf): add setFlushParams method #1862

feature(perf): add setFlushParams method #1862

Conversation

erwinmombay commented Feb 9, 2016

erwinmombay commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfs commented Feb 10, 2016

erwinmombay commented Feb 11, 2016

cramforce commented Feb 11, 2016

erwinmombay commented Feb 11, 2016

cramforce commented Feb 11, 2016

ericfs commented Feb 11, 2016

erwinmombay commented Feb 11, 2016

erwinmombay commented Feb 11, 2016

feature(perf): add `setFlushParams` method #1862

feature(perf): add `setFlushParams` method #1862