Adds AmpAdTemplate #16593

glevitzky · 2018-07-06T18:56:45Z

Adds feature that allows templatized ads to be fetched and rendered via Fast Fetch.

Sample implementation:

On the page:

<amp-ad type="test" width=320 height=50
  data-request-var-param1="val1"
  data-request-var-param2="val2"
  . . .
  data-request-var-paramn="valn"
  template>
</amp-ad>

Note that all attributes of the form data-request-var-<param_name>=<value> will be added to the request URL by appending&<param_name>=<value> to the registered URL (see below).

In amp-a4a/0.1/template-config.js:

export const NetworkRegistry = {
  // . . .
  myAdNetwork: {
    requestUrl: 'https://www.generic-ad-network.com?optionalStaticParam=foo',
  },
};

Some fields will automatically be substituted into the URL. Currently these are:

height
width

E.g., something like ...&sz=widthxheight will be expanded to ...&sz=320x50, if the size of the slot at request time is 320x50.

Addresses #15464.

jridgewell · 2018-07-09T20:46:25Z

extensions/amp-a4a/0.1/template-renderer.js

+                .querySelector('template');
+            templateElement.parentNode
+                .replaceChild(renderedElement, templateElement);
+          });


👍, my nits are fixed. Thanks!

codecov-io · 2018-07-16T21:10:28Z

Codecov Report

Merging #16593 into master will increase coverage by 0.02%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master   #16593      +/-   ##
==========================================
+ Coverage    77.9%   77.93%   +0.02%     
==========================================
  Files         562      564       +2     
  Lines       41131    41210      +79     
==========================================
+ Hits        32043    32115      +72     
- Misses       9088     9095       +7

Flag	Coverage Δ
#integration_tests	`36.12% <ø> (-0.06%)`	⬇️
#unit_tests	`76.99% <91.66%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e1828c...627c3b7. Read the comment docs.

lannka · 2018-07-23T18:15:30Z

High level:

I suggest to not have the template attribute. For real ad network integration, we should let the ad server decide what rendering method it wants.
How about type="custom-template" instead of type="test"
Nit: rename the attributes to data-request-param-abc-dce. Also convert the dash separated var name to camel case, as a convention in amp-analytis data-vars-*. (strings.js#dashToCamelCase)

glevitzky · 2018-07-23T19:59:30Z

@lannka Thanks for the feedback.

If we do not use template attribute, how do we divert in amp-ad to use the correct constructor?
Easy enough.
Renamed to data-request-param.... Conversion to camel case happens automatically when reading element.dataset, but I've cleaned up the code around this a bit. Not sure how we should handle a case where pub wants a dash-separated param on request, but we can deal with that down the road.

lannka · 2018-07-23T20:47:06Z

extensions/amp-a4a/0.1/template-renderer.js

@@ -22,27 +22,29 @@ import {getAmpAdTemplateHelper} from './template-validator';
 */
 export class TemplateRenderer extends FriendlyFrameRenderer {


in general, inheritance like this is discouraged. Let's extract shared logic to a util method instead, and have TemplateRenderer extends Renderer . See here for more background around why.

Instead of extending FriendlyFrameRenderer, can I keep an instance of it in TemplateRenderer? Since TemplateRenderer eventually renders into a friendly frame, it's unavoidable that I'll need the entirety of FriendlyFrameRenderer's logic.

totally. i suggest convert FriendlyFrameRenderer into a util class without extending Renderer.

For now I'd prefer to keep it as its own Renderer subclass, so its consistent and uniform with the other renderers. This will especially be useful once I finally get around to tackling the amp-a4a refactor.

how about a structure like:

TemplateRenderer -> Renderer
AmpRenderer (naming TBD) -> Renderer

Both renderer import the friendlyframeutil to share code.

My concern is that the util would just be the renderer itself, as there's little to nothing that would be left after extracting the common code. Can we circle back to this in a future PR?

Having one renderer impl extend another renderer impl is the code pattern i want to avoid. Let's avoid at the beginning.

Given it's introduced in a previous PR, I'm OK to have this PR in first. But please do refactor in the next PR.

Sure, I completely agree with you on avoiding a TemplateRenderer -> FriendlyFrameRenderer -> Renderer hierarchy -- that's why I changed TemplateRenderer to use an instance of FriendlyFrameRenderer rather than extend it.

But, yes, we can do a refactor/cleanup after this PR. Meanwhile, feel free to start reviewing this.

lannka · 2018-07-23T20:52:19Z

extensions/amp-a4a/0.1/template-config.js

+export let NetworkRegistryDef;
+
+/** @const {!NetworkRegistryDef} */
+export const NetworkRegistry = {


I think the default config for ad networks can be a feature added later. As I'm not yet sure that it will be that helpful.

This amp-ad-custom extension is mainly for demo or trial purpose. Any real ad network should consider have its own extension amp-ad-network-a.

lannka · 2018-07-27T20:01:59Z

extensions/amp-a4a/0.1/amp-ad-template.js

+    let url = this.baseRequestUrl_;
+
+    // We collect all fields in the dataset of the form
+    // 'data-request-var-<field_name>=<val>`, and append &<field_name>=<val> to


update doc here.

lannka · 2018-07-27T20:04:30Z

extensions/amp-a4a/0.1/amp-ad-template.js

+        }
+      }
+    });
+    this.getContext().adUrl = url;


is it needed right now?

I want to make sure we only add necessary stuff into the context object. And I wish we can later document clearly what the context object contains.

Can you check all the other context data, and remove the ones are not used.

Yes, this is used by friendly-frame-renderer.js on line 97, though I'm not certain how necessary that is.

lannka · 2018-07-27T20:07:47Z

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

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import {AmpAdTemplate} from '../../amp-a4a/0.1/amp-ad-template';


should we just move AmpAdTemplate here?

Do you mean move the whole module into this file, or just move the module into this extension?

whole module to this file. basically merge the 2 files.

lannka · 2018-07-27T20:09:30Z

build-system/app.js

@@ -933,6 +933,7 @@ app.get('/adzerk/*', (req, res) => {
  fs.readFileAsync(filePath).then(file => {
    res.setHeader('Content-Type', 'application/json');
    res.setHeader('AMP-template-amp-creative', 'amp-mustache');


I thought we already changed to
AMP-Ad-Template-Extension: amp-mustache

We did, but I must've forgotten to change it here.

and you can remove this line now

ghost · 2018-08-17T19:35:19Z

This pull request introduces 1 alert when merging 4c0365c into cb7e267 - view on LGTM.com

new alerts:

1 for Syntax error

Comment posted by LGTM.com

lannka · 2018-08-17T20:26:32Z

build-system/app.js

@@ -933,6 +933,7 @@ app.get('/adzerk/*', (req, res) => {
  fs.readFileAsync(filePath).then(file => {
    res.setHeader('Content-Type', 'application/json');
    res.setHeader('AMP-template-amp-creative', 'amp-mustache');


and you can remove this line now

lannka · 2018-08-17T20:39:55Z

extensions/amp-a4a/0.1/template-validator.js

        (headers.get(AMP_TEMPLATED_CREATIVE_HEADER_NAME) !== 'amp-mustache' &&
         headers.get(DEPRECATED_AMP_TEMPLATED_CREATIVE_HEADER_NAME) !==
           'amp-mustache')) {
      return Promise.resolve(
          /** @type {!./amp-ad-type-defs.ValidatorOutput} */ ({
            creativeData: {
-              creative: body,
+              crossDomainData: {


I'm trying to understand why this extra layer is needed?

Can't recall if I had a reason or not. So far seems to work fine without it, so I've removed it.

lannka · 2018-08-17T20:43:35Z

extensions/amp-a4a/0.1/template-renderer.js

 import {getAmpAdTemplateHelper} from './template-validator';

 /**
 * Render AMP creative into FriendlyFrame via templatization.
 */
-export class TemplateRenderer extends FriendlyFrameRenderer {
+export class TemplateRenderer extends Renderer {


this is good. now we also need to change FriendlyFrameRender to a helper util class.

lannka · 2018-08-17T20:45:41Z

extensions/amp-a4a/0.1/template-renderer.js

+            return Promise.resolve();
+          }
+          const templateHelper = getAmpAdTemplateHelper(context.win);
+          const {iframe} = this.friendlyFrameRenderer_;


once you made friendlyFrameRenderer an util class, then its render() can return this iframe.

* init * Tests + more. * Lint + types * First test now passing. * More tests. * Removing erronesouly added file. * Inserting element rather than overriding innerHTML. * Another erroneously added file. * Added amp-template-ad * Basic functionality in place. * Reseting yarn.lock * Lint. * Fixed tests + types. * Lint * Added test-amp-ad-template.js + dep-check fix. * Merges. * Using replaceChild + parent of templateElement rather than body. * Added NameFrameRenderer as non-AMP rendering mode + additional test. * Added URL expansion. * More tests. * Added amp-ad test + fixed template-validator test. * Cleaning up added element in test-tempate-renderer. * Restructring how query params are added to request. * Minor feedback changes. * Added amp-ad-custom extension. * Restoring to previous commit. * Fixing template-renderer. * Removing amp-ad changes. * Things back in functional state. * Moving amp-ad-template to amp-ad-custom. * Fixed accidentally reverted header. * Factored out friendly-frame-util + minor fixes. * Adding test to ff util. * XHR type decl fix. * One more XHR type decl fix. * One more XHR type decl fix. * Minor type + dep fixes. * Test fix. * Test fix.

googlebot added the cla: yes label Jul 6, 2018

glevitzky force-pushed the templateads branch from 23f9c76 to 5f92459 Compare July 9, 2018 20:12

glevitzky requested a review from lannka July 9, 2018 20:15

glevitzky mentioned this pull request Jul 9, 2018

Adds TemplateRenderer #16122

Merged

jridgewell reviewed Jul 9, 2018

View reviewed changes

glevitzky force-pushed the templateads branch from d01f393 to 181e264 Compare July 16, 2018 14:31

lannka reviewed Jul 23, 2018

View reviewed changes

glevitzky force-pushed the templateads branch from 34cc4ce to 627c3b7 Compare July 24, 2018 14:48

lannka reviewed Jul 27, 2018

View reviewed changes

glevitzky added 18 commits August 17, 2018 14:29

init

0a7170f

Tests + more.

ed0f68e

Lint + types

6d707c4

First test now passing.

6103088

More tests.

b3e53e4

Removing erronesouly added file.

8eacbe1

Inserting element rather than overriding innerHTML.

088f916

Another erroneously added file.

6e64ce9

Added amp-template-ad

1da9ee1

Basic functionality in place.

ba74ebe

Reseting yarn.lock

0b92d7f

Lint.

2443152

Fixed tests + types.

bb2eceb

Lint

b25dcc5

Added test-amp-ad-template.js + dep-check fix.

76fedaf

Merges.

b530106

Using replaceChild + parent of templateElement rather than body.

f20f042

Added NameFrameRenderer as non-AMP rendering mode + additional test.

9334686

glevitzky added 7 commits August 17, 2018 14:29

Added URL expansion.

6d9bc8b

More tests.

39fb400

Added amp-ad test + fixed template-validator test.

2ee5c1a

Cleaning up added element in test-tempate-renderer.

e549835

Restructring how query params are added to request.

3c4581c

Minor feedback changes.

c5afc8e

Added amp-ad-custom extension.

e568c63

glevitzky force-pushed the templateads branch 2 times, most recently from 1f4b820 to 9df7385 Compare August 17, 2018 18:48

Restoring to previous commit.

b81cbd2

glevitzky force-pushed the templateads branch from 0d6b13d to b81cbd2 Compare August 17, 2018 19:03

glevitzky added 2 commits August 17, 2018 15:04

Fixing template-renderer.

841b343

Removing amp-ad changes.

4c0365c

glevitzky added 3 commits August 17, 2018 15:51

Things back in functional state.

a7de768

Moving amp-ad-template to amp-ad-custom.

6c7f51c

Fixed accidentally reverted header.

d189f3e

lannka reviewed Aug 17, 2018

View reviewed changes

glevitzky added 2 commits August 20, 2018 15:44

Factored out friendly-frame-util + minor fixes.

dc8ff38

Adding test to ff util.

d5da03e

lannka approved these changes Aug 23, 2018

View reviewed changes

glevitzky added 6 commits August 27, 2018 11:30

XHR type decl fix.

183d76e

One more XHR type decl fix.

666aaa0

One more XHR type decl fix.

85b27b8

Minor type + dep fixes.

516e39e

Test fix.

fe4133c

Test fix.

a093c7a

lannka merged commit c76afea into ampproject:master Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds AmpAdTemplate #16593

Adds AmpAdTemplate #16593

glevitzky commented Jul 6, 2018 •

edited

jridgewell Jul 9, 2018

codecov-io commented Jul 16, 2018 •

edited

lannka commented Jul 23, 2018

glevitzky commented Jul 23, 2018

lannka Jul 23, 2018

glevitzky Jul 24, 2018

lannka Jul 24, 2018

glevitzky Jul 24, 2018

lannka Jul 24, 2018

glevitzky Jul 25, 2018

lannka Jul 25, 2018

glevitzky Jul 25, 2018

lannka Jul 23, 2018 •

edited

lannka Jul 27, 2018

glevitzky Aug 10, 2018

lannka Jul 27, 2018

glevitzky Aug 10, 2018

lannka Jul 27, 2018

glevitzky Aug 10, 2018

lannka Aug 17, 2018

glevitzky Aug 17, 2018

lannka Jul 27, 2018

glevitzky Aug 10, 2018

lannka Aug 17, 2018

glevitzky Aug 20, 2018

ghost commented Aug 17, 2018

lannka Aug 17, 2018

lannka Aug 17, 2018

glevitzky Aug 20, 2018

lannka Aug 17, 2018

glevitzky Aug 20, 2018

lannka Aug 17, 2018

glevitzky Aug 20, 2018

		@@ -22,27 +22,29 @@ import {getAmpAdTemplateHelper} from './template-validator';
		*/
		export class TemplateRenderer extends FriendlyFrameRenderer {

Adds AmpAdTemplate #16593

Adds AmpAdTemplate #16593

Conversation

glevitzky commented Jul 6, 2018 • edited

Choose a reason for hiding this comment

codecov-io commented Jul 16, 2018 • edited

Codecov Report

lannka commented Jul 23, 2018

glevitzky commented Jul 23, 2018

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 Jul 23, 2018 • 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

ghost commented Aug 17, 2018

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

glevitzky commented Jul 6, 2018 •

edited

codecov-io commented Jul 16, 2018 •

edited

lannka Jul 23, 2018 •

edited