Implementation of amp-ad-exit #9390

clawr · 2017-05-17T01:23:46Z

Implementation of the amp-ad-exit component for managing ad navigation (#8515).

Changes from the I2I:

Only RANDOM, CLICK_X, and CLICK_Y variables can be used, along with custom vars.
It doesn't work well with tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements.
ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change.

The component does some lightweight validation of the JSON config when it's
built. This may be overkill.

Tested:
gulp test --files 'extensions/amp-ad-exit/0.1/test/*'
gulp check-types

Closes #8515

lannka · 2017-05-17T22:22:15Z

@clawr are you still working on it? ping the thread when it's ready for review.

googlebot · 2017-05-18T20:36:18Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-05-18T21:08:29Z

CLAs look good, thanks!

clawr · 2017-05-18T22:09:35Z

@lannka I think it's ready for review. A test is failing but it seems unrelated (gulp dist --fortesting passes locally): https://travis-ci.org/ampproject/amphtml/jobs/233798694

lannka · 2017-05-23T17:24:28Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+export class AmpAdExit extends AMP.BaseElement {
+  /** @param {!AmpElement} element */
+  constructor(element) {
+    super(element);


please introduce an experiment flag for the new feature. check experiments.js

Done. I'm just guarding the exit action, do you think that's sufficient?

lannka · 2017-05-23T18:17:57Z

extensions/amp-ad-exit/amp-ad-exit.md

+<script type="application/json">
+{
+  "targets": {
+    "landing-page": {


camelCase is prefered

lannka · 2017-05-23T18:18:03Z

extensions/amp-ad-exit/amp-ad-exit.md

+{
+  "targets": {
+    "landing-page": {
+      "final_url": "https://example.com/artisan-baking/?from=_CLICKAREA",


lannka · 2017-05-23T18:23:48Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+    this.config_ = {targets: {}, filters: {}};
+
+    this.defaultFilterSpecs_ = {
+      'mindelay': makeClickDelaySpec(1000),


lannka · 2017-05-23T18:25:58Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+            Object.keys(this.defaultFilterSpecs_), this.defaultFilterSpecs_,
+            event) ||
+        !this.filter_(target.filters, this.config_.filters, event)) {
+      user().info(TAG, 'Click deemed unintenful');


Removed this log. It's redundant with the ones in filter_

clawr

addressed comments, PTAL

jridgewell · 2017-05-17T18:25:08Z

extensions/amp-ad-exit/0.1/filters/click-delay.js

+}
+
+/**
+ * @return {!../config.ClickDelayConfig}


Need JSDoc for delayMs

Removed this function.

jridgewell · 2017-05-17T18:25:38Z

extensions/amp-ad-exit/0.1/filters/click-delay.js

+ * @param {!../config.FilterConfig} spec
+ * @return {boolean} Whether the config defines a ClickDelay filter.
+ */
+export function assertClickDelaySpec(spec) {


Asserts throw if they fail, this is just a check.

jridgewell · 2017-05-23T19:03:49Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+    super(element);
+
+    /** @private {!./config.AmpAdExitConfig} */
+    this.config_ = {targets: {}, filters: {}};


Why nest this in an object?

It was convenient to use the parsed JSON. With @lannka's suggested refactoring this isn't useful anymore.

jridgewell · 2017-05-23T19:07:03Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+      return;
+    }
+    if (!this.filter_(
+            Object.keys(this.defaultFilterSpecs_), this.defaultFilterSpecs_,


This can be done once and cached.

The call to Object.keys()? Removed, obsolete.

jridgewell · 2017-05-23T19:08:56Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+      }
+    }
+    catch (e) {
+      user().error(TAG, 'Invalid JSON config', e);


This try catch is wrapping too many throws, it should just be wrapping the parse.

jridgewell · 2017-05-23T19:10:25Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+  /** @override */
+  viewportCallback(inViewport) {
+    if (inViewport) {
+      this.clickDelayFilter_.resetClock();


What kind of layout does this element have? Non-displayed elements won't ever have #viewportCallback called.

Oh, thanks. Moved this to buildCallback(). Was hoping to activate the timer when the ad became viewable (roughly), but I guess this way won't work. I'll stick with starting the clock at build time for now.

jridgewell · 2017-05-23T19:11:38Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+    };
+    if (target.vars) {
+      for (const customVar in target.vars) {
+        if (customVar.startsWith('_')) {


Use the startsWith helper, the method isn't available everywhere.

Changed to just look at the first char. I would have expected this to be compiled to ES5, is that not the case? Are there other ES6 features I should avoid?

jridgewell · 2017-05-23T19:14:52Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+   */
+  pingTrackingUrl_(url) {
+    user().fine(TAG, `pinging ${url}`);
+    const useBeacon = this.config_.transport[TransportMode.BEACON] == true ||


config_.transport may not have been defined.

jridgewell · 2017-05-23T19:15:43Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+  pingTrackingUrl_(url) {
+    user().fine(TAG, `pinging ${url}`);
+    const useBeacon = this.config_.transport[TransportMode.BEACON] == true ||
+        !this.config_.transport.hasOwnProperty(TransportMode.BEACON);


This can be simplified to this.config_.transport[TransportMode.BEACON] !== false, once you fix the above error.

jridgewell · 2017-05-23T19:20:42Z

extensions/amp-ad-exit/0.1/filters/click-delay.js

+export class ClickDelayFilter {
+  constructor() {
+    /** @private {number} */
+    this.inViewportTime_ = Date.now();


Initialize to -Infinity

lannka · 2017-05-23T20:32:53Z

extensions/amp-ad-exit/amp-ad-exit.md

+amp-ad-exit needs an `id` to be referenced by tappable elements.
+
+## Validation
+See [amp-ad-exit rules](https://github.com/ampproject/amphtml/blob/master/extensions/amp-ad-exit/0.1/validator-amp-ad-exit.protoascii) in the AMP validator specification.


please also mention that this extension is only available in amp4ads
@Gregable FYI, we need validator support for this.

lannka · 2017-05-23T20:33:56Z

examples/amp-ad-exit.amp.html

@@ -0,0 +1,86 @@
+<!doctype html>
+<html ⚡>


use amp4ads

lannka · 2017-05-23T20:34:20Z

examples/amp-ad-exit.amp.html

+}
+    </style>
+  <script async custom-element="amp-ad-exit" src="https://cdn.ampproject.org/v0/amp-ad-exit-0.1.js"></script>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>


.../amp4ads-v0.js

lannka · 2017-05-23T20:39:54Z

examples/amp-ad-exit.amp.html

+    "target1": {
+      "finalUrl": "http://localhost:8000/?product1&x=CLICK_X&y=CLICK_Y&e=_ELEM&shouldNotBeReplaced=AMP_VERSION",
+      "vars": {
+        "_ELEM": {"defaultValue": "headline"}


use camelCase for user vars. capital case is reserved for pre-defined macros

Done. Should we try to enforce this in users' configs? I already require an underscore to start.

lannka · 2017-05-23T20:43:46Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+   */
+  exit({args, event}) {
+    if (!isExperimentOn(this.win, 'amp-ad-exit')) {
+      user().warn('amp-ad-exit experiment is off.');


make it an error: user().assert()

lannka · 2017-05-23T20:50:26Z

extensions/amp-ad-exit/0.1/filters/filter.js

+
+/** @enum {string} */
+export const FilterType = {
+  CLICK_DELAY: 'click_delay',


camelCase for the value

lannka · 2017-05-23T20:53:41Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+      'minDelay': makeClickDelaySpec(1000),
+    };
+
+    this.clickDelayFilter_ = new ClickDelayFilter();


can we instantiate filters only when they're used?

we can have one object for each named filter (with the its spec). and this AmpAdExist class does not need to understand the types of filters.

lannka · 2017-05-23T20:58:19Z

extensions/amp-ad-exit/0.1/filters/click-delay.js

+    return (Date.now() - this.inViewportTime_) >= spec.delay;
+  }
+
+  resetClock() {


make it generic and move to interface?

jridgewell · 2017-05-25T19:54:16Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+    };
+    if (target.vars) {
+      for (const customVar in target.vars) {
+        if (customVar.charAt(0) == '_') {


You can just use array access notation.

jridgewell · 2017-05-25T20:00:12Z

extensions/amp-ad-exit/0.1/filters/click-delay.js

+    this.delay_ = delay;
+
+    /** @private {number} */
+    this.inViewportTime_ = -Infinity;


Sorry, this should be Infinity. 😳

@lannka

/to @lannka

lannka

Looks good in general. Some minor issues.

lannka · 2017-05-25T20:57:03Z

examples/amp-ad-exit.amp.html

+    }
+  },
+  "filters": {
+    "long_delay": {


lannka · 2017-05-25T20:57:15Z

examples/amp-ad-exit.amp.html

+      "vars": {
+        "_elem": {"defaultValue": "headline"}
+      },
+      "filters": ["long_delay"]


lannka · 2017-05-30T20:55:49Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+  }
+
+  /**
+   * Attemts to issue a request to `url` to report the click. The request method


lannka · 2017-05-30T23:07:05Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+    };
+    if (target.vars) {
+      for (const customVar in target.vars) {
+        if (customVar[0] == '_') {


if the _ constraint for custom var is only to prevent system vars being overridden, we can do custom var first then let system var to override them.

so that we keep the naming convention across AMP.

We'd still need to maintain a blacklist of system variables that aren't used here, like DOCUMENT_REFERRER. I think a whitelist of {RANDOM, CLICK_X, CLICK_Y} is more reliable.

lannka · 2017-05-30T23:08:11Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+    this.defaultFilters_.forEach(f => f.buildCallback());
+
+    const children = this.element.children;
+    if (children.length != 1) {


user().assert ?

lannka · 2017-05-30T23:08:37Z

extensions/amp-ad-exit/0.1/amp-ad-exit.js

+      throw new Error('The tag should contain exactly one <script> child.');
+    }
+    const child = children[0];
+    if (!isJsonScriptTag(child)) {


user().assert?

lannka · 2017-05-30T23:13:40Z

extensions/amp-ad-exit/0.1/filters/click-delay.js

+   * Resets the clock to measure clicks from the current time.
+   * @override
+   */
+  buildCallback() {


shall we just call reset? though it's normally called in a buildCallback, but not necessary

Removed this function and changed AmpAdExit to construct all filters in its buildCallback.

lannka · 2017-05-30T23:17:12Z

extensions/amp-ad-exit/0.1/config.js

+    switch (type) {
+      case FilterType.CLICK_DELAY:
+        user().assert(
+            isValidClickDelaySpec(filters[name]),


instead of checking here, can we do it in filter's constructor. so we keep relevant logic in the same place, no need to jump around for new changes.

ampproject#8515). Changes from the I2I: - Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars. - It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements. - ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change. The component does some lightweight validation of the JSON config when it's built. This may be overkill. Tested: gulp test --files 'extensions/amp-ad-exit/0.1/test/*' gulp check-types

…ters in an interface method).

…x validation rules. Address PR: add experiment flag; use camelCase for config properties address some comments Instantiate a new Filter for each spec.

…k().

* Implementation of the amp-ad-exit component for managing ad navigation (ampproject#8515). Changes from the I2I: - Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars. - It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements. - ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change. The component does some lightweight validation of the JSON config when it's built. This may be overkill. Tested: gulp test --files 'extensions/amp-ad-exit/0.1/test/*' gulp check-types * Update JSDoc for some functions. Fix lint issue (ignore unused parameters in an interface method). * sinon.spy -> sandbox.spy * Add amp-ad-exit to the list of allowed extensions in A4A pages and fix validation rules. Address PR: add experiment flag; use camelCase for config properties address some comments Instantiate a new Filter for each spec. * Instantiate a new Filter for each spec. * remove duplicate doctype tag in example * Make ActionEventDef public for type annotations. * Add filter factory. * fix event var * update type annotation for Filter.filter * address PR comments * Move filter config validation to the ctor. Remove Filter.buildCallback(). * camelCase in documentation examples

* Bidtellect amp implementation… (ampproject#9518) * Bidtellect amp implementation… * removing renderStartImplemented… * spaces? * adding example * Fix test-amp-ad-network-doubleclick-impl (ampproject#9645) * Optimizations to speed up Travis PR builds (ampproject#9626) Travis builds have become slow during busy periods of the day due to various reasons. This PR does the following: Reorganizes build tasks across fewer build shards Reduces the number of VMs used by PR and master builds to 2 No longer does a full gulp dist for unit tests (Possible to do after ampproject#9404) Reduces the total CPU time used for PR builds to < 20 mins Fixes ampproject#9500 Fixes ampproject#9387 * Animations: full on-action API (ampproject#9641) * Animations: full on-action API * review fixes * Fix Validator tests on Travis, which now uses Node v4 (ampproject#9654) * Amp-imgur : Implement imgur embed (ampproject#9405) (ampproject#9434) * Amp-imgur : Implement imgur embed (ampproject#9405) * amp-imgur test code modified * amp-imgur validation code changed * fix amp-imgur lint check * Remove unused Layour variables * Test code modified * Ingegration amp-imgur extension * [amp-imgur] add resize logic * Remove 3p iframes and add resize message listener * Fix some code with reviews * change some syntax * change validation rules * add find event source * [amp-imgur] remove unnecessary code and add object check * remove spec_name from validation code * Remove unnecessary code and add owners file * Change required tag name to in validation file * data-imgurid -> data-imgur-id * Add whitelist for amp-imgur * remove unused AmpImgur import in test file * remove unused preconnect callback * Update amp-cors-requests.md (ampproject#9636) Added updates and corrections per Dima's feedback. * Expose login url building to amp access service (ampproject#9300) * Animations: support style keyframes (ampproject#9647) * Animations: support style keyframes * docs * review fixes * lints * Animations: subtargets format (ampproject#9655) * Animations: subtargets format * fix docs * Fix extern for integration test (ampproject#9657) * Remove whitelisted link for PR 9434 (ampproject#9656) * Add callouts for CORS + cleanup (ampproject#9591) * Add callouts for CORS + cleanup * Update amp-access.md * Doubleclick Fast Fetch: Send default safeframe version on ad request (ampproject#9613) * Send default safeframe version on ad request * fix test failure * amp-ima-video: Fixes some undefined variables in compiled extension (ampproject#9617) * Add experimental input-debounced to the actions and event doc (ampproject#9650) * Copy and rename compiled script directly for deprecated version (ampproject#9587) * Remove A4A Dependency on ads/_config.js (ampproject#9462) * Added doubleclick specific config * Modified other networks * Update MockA4AImpl * Remove unneeded config lines * Add cid, remove renderStartImplemented * Fix lint complaint * Changed from mandatory config to overrrideable method * Addressed feedback, modified getAdCid * Addressed additional feedback * Responded to feedback * Removed unnneeded guard against null value * Changed signature of function * Fixed lint issues * addressed comments * Addressed comments, fixed broken test * Change signature / update tests * Fix wrong contents and white spaces in amp-imgur.md (ampproject#9658) * Performance: separate ini-load from first visible ini-load (ampproject#9665) * amp-bind: Fix more integration tests (ampproject#9598) * more bind test fixes * include object.assign polyfill * move polyfills to separate file * replace use of map() from bind-validator with ownProperty() * fix types and test * Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) (ampproject#9668) * removing overflow touch on no-scroll for ios * Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) * make sure css is compiled before entry points (ampproject#9643) * update npm package version (ampproject#9671) * A4A: expose visibilityCsi (ampproject#9667) * A4A: expose visibilityCsi * tests * amp-bind: Fix embedding in FIF (ampproject#9541) * move element service changes from ampproject#9447 * store element service ids in extension struct * add unit test, fix other tests * Fix null value binding (ampproject#9674) * Add amp-form's submit action to the docs (ampproject#9675) * Write cookie for ad-cid. (ampproject#9594) * Write cookie for ad-cid. * fix tests * amp-access-laterpay fixes (ampproject#9633) * Add support for LaterPay subscriptions * Fix canonical link on laterpay example * Tweak default styling * Add an example locale message for the header * Fix indentation of JsDoc (ampproject#9677) It should be indented at the same level as the method it's documenting. * Add example to amp iframe docs (ampproject#9660) * Change image to static link hosted on ampproject.org * Update amp-iframe doc w example + gen cleanup * Adds SFG experiment branches to doubleclick-a4a-config.js. (ampproject#9662) * Added experiment branches corresponding to exp=a4a:(5|6). * Removed test file. * Removed changes to yarn.lock + minor fixes. * Fixed adsense-a4a-config.js. * Fixed return statement. * Removed reference to style tag for opacity (ampproject#9634) As style tag for `opacity` was replaced with `amp-boilerplate (back in this [commit](ampproject@0a056ca), updated the text to reflect those changes. * De-flake amp-bind integration tests (ampproject#9683) * split bind fixtures into one file per extension * fix width/height binding bug, avoid race condition w/ dynamic containers * actually, just skip the tests affected by race condition for now * Remove timer calls to prevent future flakiness (ampproject#9478) * Remove timer calls to prevent future flakiness * Revert the line order change * Poll instead of using stub callback constructor * Add jsdoc and rename variables * amp-animation: polyfill partial keyframes from CSS (ampproject#9689) * Animations: whitelist offset distance and regroup condition types (ampproject#9688) * Use Docker containers in Travis (ampproject#9666) * clean up web-worker experiment (ampproject#9706) * Load examiner.js when #development=2 (ampproject#9680) * Load examiner.js when #development=1 * address comments * Use development=2 * Fix presubmit * Implementation of amp-ad-exit (ampproject#9390) * Implementation of the amp-ad-exit component for managing ad navigation (ampproject#8515). Changes from the I2I: - Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars. - It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements. - ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change. The component does some lightweight validation of the JSON config when it's built. This may be overkill. Tested: gulp test --files 'extensions/amp-ad-exit/0.1/test/*' gulp check-types * Update JSDoc for some functions. Fix lint issue (ignore unused parameters in an interface method). * sinon.spy -> sandbox.spy * Add amp-ad-exit to the list of allowed extensions in A4A pages and fix validation rules. Address PR: add experiment flag; use camelCase for config properties address some comments Instantiate a new Filter for each spec. * Instantiate a new Filter for each spec. * remove duplicate doctype tag in example * Make ActionEventDef public for type annotations. * Add filter factory. * fix event var * update type annotation for Filter.filter * address PR comments * Move filter config validation to the ctor. Remove Filter.buildCallback(). * camelCase in documentation examples * Introducing "it.yield", a convenient way to test promise code (ampproject#9601) * Introduce yield. * Address comments. * fix done state error handling * Document amp-access as a special target (ampproject#9568) `amp-access` is a special target that's not mentioned in this document. It's special because you can't give an arbitrary ID to it, i.e., you can't just change the id of the `amp-access` script tag to `amp-access2` and expect `on="tap:amp-access2.login"` to work. Given that the "actions" for `amp-access` is a bit dynamic depending on the structure of the `amp-access` JSON, instead of listing out the actions in a tabular form, a reference to the `amp-access` documentation is added. * Run presubmit tests soon after building (ampproject#9716) * Revert "Use Docker containers in Travis (ampproject#9666)" (ampproject#9717) This reverts commit 98ecb9b. It turns out that while using Docker containers instead of VMs on Travis reduces VM startup time, it has significantly increased build and test time, increasing the overall PR check round trip from < 20 mins to > 30 mins. Compare https://travis-ci.org/ampproject/amphtml/jobs/238464404 (Docker) against https://travis-ci.org/ampproject/amphtml/jobs/238462066 (VM). Fixes ampproject#9651 * Validator Rollup (ampproject#9719) * Add validator tests for amp-imgur. * Disallow amp-embed as child of amp-app-banner. * Minor cleanup, Make sure our set numbers are consistent in javascript. * amp-state: Allow both `src` and script child (ampproject#9721) * adapt code from ampproject#8897 * also_requires_attr should be in trigger * Remove experiment for input-debounced. Update docs. (ampproject#9724) * Validator Rollup (ampproject#9727) * Minor cleanup of amp-ad-exit * Disallow amp-ad/amp-embed as children of amp ad containers when data-multi-size is present. * Revision bump * Enabling dynamic queryparam addition to anchour links (ampproject#9684) * Enabling queryparam addition to anchour links * Code review changes * Additional cache urls (ampproject#9733) * Significantly speed up gulp build --css-only (ampproject#9726) Prior to this PR, gulp build --css-only would take ~ 1 minute to run, and end up building a bunch of js files in addition to compiling css. After this PR, gulp build --css-only takes ~ 0.5 seconds to run (a 100x speed up), and the only output files in build/ after running it are .css, and .css.js files. Resulting speeds on my workstation: gulp build --css-only: ~500ms gulp build: ~1m 45s gulp dist --fortesting: ~3m 30s Fixes ampproject#9640 * add amp-analytics Facebook Pixel support (ampproject#9449) * add amp-analytics Facebook Pixel support * add amp-analytics Facebook Pixel support * fix test on extensions/amp-analytics/0.1/test/vendor-requests.json * add Facebook Pixel standard events https://developers.facebook.com/docs/ads-for-websites/pixel-events/v2.9#events * Update vendors.js fix conflicts * Update vendors.js fix conflicts * Update vendor-requests.json fix conflicts * Update vendor-requests.json fix conflicts * Update vendor-requests.json * Update analytics.amp.html * Update analytics-vendors.amp.html * Update vendors.js * Update analytics-vendors.amp.html * fix lint error * update * facebook pixel * Update vendor-requests.json * Update analytics.amp.html * fix lint error * fix lint error * fix alphabetical order * Update yarn.lock * AMP-ad supports Seznam Imedia ad network (ampproject#8893) * AMP-ad supports Seznam Imedia ad network * Fixed requested changes * Fixed requested changes * Render API implemented * Improved placing ads * Fixed requested changes * Render API fixed context * Renamed IM.cz to Imedia * Described JSON parameters

Gregable · 2017-06-16T22:50:46Z

This change has been released to the validator and will be live within an hour.

googlebot added the cla: yes label May 17, 2017

lannka self-requested a review May 17, 2017 05:00

googlebot added cla: no and removed cla: yes labels May 18, 2017

clawr force-pushed the amp-ad-exit-pr branch from c133cb0 to fc68369 Compare May 18, 2017 21:08

googlebot added cla: yes and removed cla: no labels May 18, 2017

clawr force-pushed the amp-ad-exit-pr branch from fc68369 to 3d78d9a Compare May 18, 2017 21:15

lannka requested changes May 23, 2017

View reviewed changes

clawr commented May 23, 2017

View reviewed changes

jridgewell previously requested changes May 23, 2017

View reviewed changes

lannka reviewed May 23, 2017

View reviewed changes

clawr force-pushed the amp-ad-exit-pr branch 3 times, most recently from 8711bb4 to 35534a4 Compare May 24, 2017 21:45

jridgewell reviewed May 25, 2017

View reviewed changes

lannka reviewed May 30, 2017

View reviewed changes

clawr added 8 commits May 31, 2017 12:04

Update JSDoc for some functions. Fix lint issue (ignore unused parame…

ef50b41

…ters in an interface method).

sinon.spy -> sandbox.spy

9c84f14

Add amp-ad-exit to the list of allowed extensions in A4A pages and fi…

196e3ae

…x validation rules. Address PR: add experiment flag; use camelCase for config properties address some comments Instantiate a new Filter for each spec.

Instantiate a new Filter for each spec.

3adbbf0

remove duplicate doctype tag in example

976c431

Make ActionEventDef public for type annotations.

07a340c

Add filter factory.

c717543

clawr added 5 commits May 31, 2017 12:04

fix event var

9b8f694

update type annotation for Filter.filter

41c656c

address PR comments

2060e33

Move filter config validation to the ctor. Remove Filter.buildCallbac…

4d40b2b

…k().

camelCase in documentation examples

b317545

clawr force-pushed the amp-ad-exit-pr branch from 0887c0b to b317545 Compare May 31, 2017 19:04

lannka approved these changes Jun 2, 2017

View reviewed changes

lannka merged commit dc5763a into ampproject:master Jun 3, 2017

ampprojectbot added the PR use: In Beta / Experimental label Jun 12, 2017

Gregable mentioned this pull request Jun 26, 2017

Update changelog to reflect release notes. #10144

Merged

Implementation of amp-ad-exit #9390

Implementation of amp-ad-exit #9390

Conversation

clawr commented May 17, 2017 • edited by jridgewell

lannka commented May 17, 2017

googlebot commented May 18, 2017

googlebot commented May 18, 2017

clawr commented May 18, 2017

Choose a reason for hiding this comment

clawr May 23, 2017 • edited

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

clawr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clawr May 24, 2017 • edited

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

clawr May 24, 2017 • edited

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

clawr May 24, 2017 • edited

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

lannka left a 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

Choose a reason for hiding this comment

clawr commented May 17, 2017 •

edited by jridgewell

clawr May 23, 2017 •

edited

clawr May 24, 2017 •

edited

clawr May 24, 2017 •

edited

clawr May 24, 2017 •

edited