add version parameter to AMP.extension signature #5989

erwinmombay · 2016-11-02T22:47:01Z

erwinmombay · 2016-11-02T22:47:38Z

zhouyx · 2016-11-02T22:48:54Z

extensions/amp-sticky-ad/1.0/amp-sticky-ad.js

@@ -235,6 +235,6 @@ class AmpStickyAd extends AMP.BaseElement {
  }
 }

-AMP.extension('amp-sticky-ad', AMP => {
+AMP.extension('amp-sticky-ad', 0.1, AMP => {


zhouyx · 2016-11-02T22:49:24Z

src/runtime.js

@@ -179,10 +179,11 @@ function adoptShared(global, opts, callback) {
  if (!global.AMP.extension) {
    /**
     * @param {string} unusedName
+     * @param {number|string} version


can we make it to be 0.1 by default? and make the version optional?

id rather this be explicit

then you probably want to search global.AMP.extension in our codebase. I think there are other elements that use this function.

zhouyx · 2016-11-02T22:50:56Z

test/_init_tests.js

-global.AMP.extension = function(name, installer) {
-  describes.bufferExtension(name, installer);
+global.AMP.extension = function(name, version, installer) {
+  describes.bufferExtension(`${name}:${version}`, installer);


I think we also need to change the describes.js file?

can you explain what needs to be updated there?

Don't worry about describes. I'll pick that refactoring up.

@dvoytenko do you want me to remove this change? or just leave as is?

Leave as is.

zhouyx · 2016-11-02T23:41:28Z

testing/describes.js

+        if (extensionId.indexOf(':') == -1) {
+            // We default to 0.1 if the user did not give an explicit
+            // version.
+            extensionId = `${extensionId}:0.1`;


we can't overwrite extensionId here if we can't find an installer. We would need the extension name w/o version number to call resetScheduledElementForTesting(win, extensionId);

renamed to bufferId

zhouyx · 2016-11-02T23:42:38Z

src/runtime.js

     * @param {function(!Object)} installer
     * @const
     */
-    global.AMP.extension = function(unusedName, installer) {
+    global.AMP.extension = function(unusedName, version, installer) {


It would be good to combine unusedName and version to a single object, and make the version to be 0.1 as default. But this can be done afterwards.

zhouyx · 2016-11-03T02:04:22Z

testing/describes.js

@@ -429,7 +429,13 @@ class AmpFixture {
    }
    if (spec.extensions) {
      spec.extensions.forEach(extensionId => {
-        const installer = extensionsBuffer[extensionId];
+          let bufferId = extensionId;


zhouyx · 2016-11-03T02:04:47Z

LGTM, defer to @dvoytenko for final approval.

dvoytenko · 2016-11-03T16:54:49Z

build-system/runner/src/org/ampproject/AmpPass.java

@@ -48,6 +48,8 @@
  private final Map<String, Node> prodAssignmentReplacements;
  final boolean isProd;

+  final Integer ampExtensionCallbackPosition = 4;


Add javadoc here to describe what the signature looks like.

Also, why not just go with the "last" arg?

went with the last arg.

dvoytenko · 2016-11-03T16:56:23Z

src/runtime.js

@@ -179,10 +179,11 @@ function adoptShared(global, opts, callback) {
  if (!global.AMP.extension) {


Can we go ahead and replace this if with minified or whatever if that will strip this function from PROD version?

done. switched to minified. will double check

dvoytenko · 2016-11-03T16:56:54Z

test/_init_tests.js

-global.AMP.extension = function(name, installer) {
-  describes.bufferExtension(name, installer);
+global.AMP.extension = function(name, version, installer) {
+  describes.bufferExtension(`${name}:${version}`, installer);


Don't worry about describes. I'll pick that refactoring up.

dvoytenko · 2016-11-03T17:01:45Z

testing/describes.js

@@ -429,7 +429,13 @@ class AmpFixture {
    }
    if (spec.extensions) {
      spec.extensions.forEach(extensionId => {
-        const installer = extensionsBuffer[extensionId];
+        let bufferId = extensionId;


Not quite. See extensionId usage lower in this method. I recommend to do this:

Rename extensionId => arg to bufferId or extensionWithVersion or such.

First thing in the method, parse it out into extensionId and version.

Use ${extensionId}:${version} to lookup installer in the extensionsBuffer.

For everything else, use extensionId by itself.

dvoytenko · 2016-11-03T22:22:40Z

src/runtime.js

@@ -176,13 +176,14 @@ function adoptShared(global, opts, callback) {
  // as `AMP.push()` in production.
  // TODO(dvoytenko, #5507): Only expose this method for `!getMode().minified`
  // once the compile-time inlining is done.
-  if (!global.AMP.extension) {
+  if (!getMode().minified) {


Remove TODO above. Is !minified already DCE'd?

yes, double checked. code isn't there in production binary.

dvoytenko · 2016-11-03T22:23:16Z

test/_init_tests.js

-global.AMP.extension = function(name, installer) {
-  describes.bufferExtension(name, installer);
+global.AMP.extension = function(name, version, installer) {
+  describes.bufferExtension(`${name}:${version}`, installer);


Leave as is.

dvoytenko · 2016-11-03T22:23:55Z

LGTM with one small request

erwinmombay · 2016-11-03T22:28:23Z

@cramforce apologies :D bug in bot.

erwinmombay · 2016-11-03T22:29:54Z

@dvoytenko done.

dvoytenko · 2016-11-03T22:53:49Z

LGTM

…tion rewrite (ampproject#5982) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples Introduce amp-auto-id attribute to AMP element. (ampproject#5973) * Introduce amp-auto-id attribute to AMP element. * Fix tests * Address comments * Address comments * revert amp-live-list.md polling clarification (ampproject#5991) Only report 1% of errors if a page has non-AMP JS (ampproject#5994) Fixes ampproject#5732 Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests Remove unsupported query selector feature (ampproject#5999) Workaround for misbehaving webview viewer (ampproject#6001) A webview viewer from Google is seing origins of the form `www.google.com`. For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage. A4A integration tests (ampproject#5812) * First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error. Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003) * Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property. * Added check for existence and changed to plain old 'transform'. * Now asserting the existence of the container. amp-sticky-ad close button new style (ampproject#5979) * unit style * fix css * fix test * rebase * fix z-index Other JS errors: Use startsWith (ampproject#6006) Fixes ampproject#5994 (comment). Fix log calls without TAGNAME (ampproject#6005) My presubmit was a bit too lenient, and too strict at the same time. refactor amp-ad.css (ampproject#5992) Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576) * Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 * Updated non-supported vars + format changes for amp-analytics vars * Updates per Avi's feedback * Updated descriptions for amp-carousel vars add version parameter to AMP.extension signature (ampproject#5989) * add version parameter to AMP.extension signature * apply recommendations * remove TODO Improve error reporting (ampproject#6019) - Include the current `location.hash`. We are missing it, because it is not included in the referrer. - Include previously recorded errors, so it is easier to identify follow on errors. - Kill the 2000 char limit as it isn't important for AMP's target browsers. Include the originalHash (ampproject#6020) We remove the hash in embedding mode, so the original code doesn't actually work. Fix error: "Can't find variable: TextDecoder" (ampproject#6011) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples * Include self (window) when verifying TextDecoder/TextEncoder existence * fix upstream conflict * PR feedback * PR feedback

Update ads.amp.html Modify A4A AMP Creative to use ampRuntimeUtf16CharOffsets from validation rewrite (ampproject#5982) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples Introduce amp-auto-id attribute to AMP element. (ampproject#5973) * Introduce amp-auto-id attribute to AMP element. * Fix tests * Address comments * Address comments * revert amp-live-list.md polling clarification (ampproject#5991) Only report 1% of errors if a page has non-AMP JS (ampproject#5994) Fixes ampproject#5732 Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890) * Allow for hosted testing to override where third party frame is retrieved from * test my own site * fix ava tests Remove unsupported query selector feature (ampproject#5999) Workaround for misbehaving webview viewer (ampproject#6001) A webview viewer from Google is seing origins of the form `www.google.com`. For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage. A4A integration tests (ampproject#5812) * First draft of full integration tests for A4A. * First draft of full integration tests for A4A. * Got tests of exception conditions working. Refactored some common stuff to utils.js. * Refactored XDom and friendly iframe tests to helpers. * Added tests. * Resolved merge conflicts. * Centralized visibility check and updated visibility test. * Moved all A4A tests to using central visibility test. * Lint fixes. * Changed custom expectations to local functions. * Replaced stringToArrayBuffer with utf8Encode. * Fixed visibility assertion. * Lint fixes. * Added a comment (partly in order to get Travis to re-run). * Changes in response to reviews. * Rebased and fixed small rebase error. Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003) * Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property. * Added check for existence and changed to plain old 'transform'. * Now asserting the existence of the container. amp-sticky-ad close button new style (ampproject#5979) * unit style * fix css * fix test * rebase * fix z-index Other JS errors: Use startsWith (ampproject#6006) Fixes ampproject#5994 (comment). Fix log calls without TAGNAME (ampproject#6005) My presubmit was a bit too lenient, and too strict at the same time. refactor amp-ad.css (ampproject#5992) Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576) * Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 * Updated non-supported vars + format changes for amp-analytics vars * Updates per Avi's feedback * Updated descriptions for amp-carousel vars add version parameter to AMP.extension signature (ampproject#5989) * add version parameter to AMP.extension signature * apply recommendations * remove TODO Improve error reporting (ampproject#6019) - Include the current `location.hash`. We are missing it, because it is not included in the referrer. - Include previously recorded errors, so it is easier to identify follow on errors. - Kill the 2000 char limit as it isn't important for AMP's target browsers. Include the originalHash (ampproject#6020) We remove the hash in embedding mode, so the original code doesn't actually work. Fix error: "Can't find variable: TextDecoder" (ampproject#6011) * Update amp-ad-network-doubleclick-impl.md * Reverting doc changes The previous change was made in error (wrong branch). * Initial changes, need to update tests * Start fixing tests * Fix unit test coverage * revert a4a examples * Include self (window) when verifying TextDecoder/TextEncoder existence * fix upstream conflict * PR feedback * PR feedback

* add version parameter to AMP.extension signature * apply recommendations * remove TODO

erwinmombay assigned dvoytenko and zhouyx Nov 2, 2016

zhouyx suggested changes Nov 2, 2016

View reviewed changes

erwinmombay force-pushed the add-version-to-amp-extension branch 3 times, most recently from b9b372d to 6139ba4 Compare November 2, 2016 23:27

zhouyx suggested changes Nov 2, 2016

View reviewed changes

erwinmombay force-pushed the add-version-to-amp-extension branch from 6139ba4 to eeed48d Compare November 3, 2016 01:14

zhouyx reviewed Nov 3, 2016

View reviewed changes

zhouyx approved these changes Nov 3, 2016

View reviewed changes

erwinmombay force-pushed the add-version-to-amp-extension branch from eeed48d to e389c69 Compare November 3, 2016 06:30

dvoytenko suggested changes Nov 3, 2016

View reviewed changes

erwinmombay added 2 commits November 3, 2016 10:17

add version parameter to AMP.extension signature

9f6e95e

apply recommendations

5f8d2e5

erwinmombay force-pushed the add-version-to-amp-extension branch from e389c69 to 5f8d2e5 Compare November 3, 2016 17:32

dvoytenko approved these changes Nov 3, 2016

View reviewed changes

remove TODO

65c0d83

erwinmombay merged commit 8b89349 into ampproject:master Nov 3, 2016

erwinmombay mentioned this pull request Nov 7, 2016

Meeting Notes 2016-11-07 #6067

Closed

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

add version parameter to AMP.extension signature (ampproject#5989)

dc0932a

* add version parameter to AMP.extension signature * apply recommendations * remove TODO

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

add version parameter to AMP.extension signature (ampproject#5989)

ecc2027

* add version parameter to AMP.extension signature * apply recommendations * remove TODO

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add version parameter to AMP.extension signature #5989

add version parameter to AMP.extension signature #5989

erwinmombay commented Nov 2, 2016 •

edited by dvoytenko

erwinmombay commented Nov 2, 2016

zhouyx Nov 2, 2016

erwinmombay Nov 2, 2016

zhouyx Nov 2, 2016

erwinmombay Nov 2, 2016

zhouyx Nov 2, 2016

zhouyx Nov 2, 2016

erwinmombay Nov 2, 2016

erwinmombay Nov 2, 2016

dvoytenko Nov 3, 2016

erwinmombay Nov 3, 2016

dvoytenko Nov 3, 2016

zhouyx Nov 2, 2016

erwinmombay Nov 3, 2016

zhouyx Nov 2, 2016

zhouyx Nov 3, 2016

erwinmombay Nov 3, 2016

zhouyx commented Nov 3, 2016

dvoytenko Nov 3, 2016

erwinmombay Nov 3, 2016

dvoytenko Nov 3, 2016

erwinmombay Nov 3, 2016

dvoytenko Nov 3, 2016

dvoytenko Nov 3, 2016

erwinmombay Nov 3, 2016

dvoytenko Nov 3, 2016

erwinmombay Nov 3, 2016

dvoytenko Nov 3, 2016

dvoytenko commented Nov 3, 2016

erwinmombay commented Nov 3, 2016

erwinmombay commented Nov 3, 2016

dvoytenko commented Nov 3, 2016

		@@ -179,10 +179,11 @@ function adoptShared(global, opts, callback) {
		if (!global.AMP.extension) {

add version parameter to AMP.extension signature #5989

add version parameter to AMP.extension signature #5989

Conversation

erwinmombay commented Nov 2, 2016 • edited by dvoytenko

erwinmombay commented Nov 2, 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

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

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

zhouyx commented Nov 3, 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

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

dvoytenko commented Nov 3, 2016

erwinmombay commented Nov 3, 2016

erwinmombay commented Nov 3, 2016

dvoytenko commented Nov 3, 2016

erwinmombay commented Nov 2, 2016 •

edited by dvoytenko