Make render-start respond to resize info by zhouyx · Pull Request #4842 · ampproject/amphtml

zhouyx · 2016-09-06T22:46:20Z

This is to make render-start API support multi-size ad. #3176
It's also an improvement to ad load behavior #4022

zhouyx · 2016-09-07T18:43:46Z

3p/integration.js


-function triggerRenderStart() {
-  nonSensitiveDataPostMessage('render-start');
+function triggerRenderStart(width, height) {


I copy the format from other function that postMessages. triggerRenderStart() and triggerRenderStart(width, height) both work, how can we represent this in our code and let 3p ad know?

Let's make the param a opt_data object, for future extensibility.
A JS doc for this optional param would help.

We should also update ads/README.md.

fixed. will add JS doc

zhouyx · 2016-09-07T21:11:21Z

extensions/amp-ad/0.1/amp-ad-api-handler.js

   * @private
   */
  sendEmbedInfo_(inViewport) {
+    if (!this.embedStateApi_) {


viewportCallback() can be called anytime after element build. this.embedStateApi_ may not be initiated when this function is called. Add the check here.

lannka · 2016-09-07T21:51:57Z

ads/README.md

 - `window.context.noContentAvailable` is a function that the ad system can call if the ad slot was not filled. The container page will then react by showing fallback content or collapsing the ad if allowed by AMP resizing rules.
 - `window.context.reportRenderedEntityIdentifier` MUST be called by ads, when they know information about which creative was rendered into a particular ad frame and should contain information to allow identifying the creative. Consider including a small string identifying the ad network. This is used by AMP for reporting purposes. The value MUST NOT contain user data or personal identifiable information.
- `window.context.renderStart` is a function that the ad system can call if the ad start rendering. The ad would then set the visibility of the iframe to visible. The ad type needs to be included in `waitForRenderStart` list in [integration.js](../3p/integration.js) for this function to be available.
+- `window.context.renderStart(opt_data)` is a function that the ad system can call if the ad start rendering. The ad would then set the visibility of the iframe to visible. The ad type needs to be included in `waitForRenderStart` list in [integration.js](../3p/integration.js) for this function to be available. Ad system can use opt_data object {width, height} to send the rendered ad size which AMP will use for resize. Please check Ad resizing part for more information.


... the returned ad size to request a resize. ...

Also, pls format opt_data into code style, same for {width, height}.

Please also move this bullet point to the top (before noContentAvailable). And consider to change the wording from can to should, as we plan to push more ad networks to implement the new APIs.

should we also do this for noContentAvailabe?

consider to change the wording from can to should

Yes, please

lannka · 2016-09-07T22:16:11Z

extensions/amp-ad/0.1/test/test-amp-ad-api-handler.js

+      it('should resolve and resize on message "render-start" w/ size if '
+          + 'render-start is implemented by 3P', () => {
+        adImpl.adType = 'doubleclick';
+        sandbox.stub(adImpl, 'attemptChangeSize', () => {


please verify the size is correctly passed to attemptChangeSize

lannka · 2016-09-07T22:44:31Z

ads/README.md

 - `window.context.noContentAvailable` is a function that the ad system can call if the ad slot was not filled. The container page will then react by showing fallback content or collapsing the ad if allowed by AMP resizing rules.
 - `window.context.reportRenderedEntityIdentifier` MUST be called by ads, when they know information about which creative was rendered into a particular ad frame and should contain information to allow identifying the creative. Consider including a small string identifying the ad network. This is used by AMP for reporting purposes. The value MUST NOT contain user data or personal identifiable information.
- `window.context.renderStart` is a function that the ad system can call if the ad start rendering. The ad would then set the visibility of the iframe to visible. The ad type needs to be included in `waitForRenderStart` list in [integration.js](../3p/integration.js) for this function to be available.
+- `window.context.renderStart(opt_data)` is a function that the ad system can call if the ad start rendering. The ad would then set the visibility of the iframe to visible. The ad type needs to be included in `waitForRenderStart` list in [integration.js](../3p/integration.js) for this function to be available. Ad system can use `opt_data` object of form `{width, height}` to send the returned ad size to request a resize. Please check Ad resizing part for more information.


Also add a link to "Ad resizing"

lannka · 2016-09-07T22:46:58Z

LGTM with some cosmetic nitpicks.

lannka · 2016-09-08T00:05:52Z

ads/README.md

 ### Methods available to the ad.

- `window.context.noContentAvailable` is a function that the ad system can call if the ad slot was not filled. The container page will then react by showing fallback content or collapsing the ad if allowed by AMP resizing rules.
+- `window.context.renderStart(opt_data)` is a function that the ad system should call if the ad start rendering. The ad would then set the visibility of the iframe to visible. The ad type needs to be included in `waitForRenderStart` list in [integration.js](../3p/integration.js) for this function to be available. Ad system can use `opt_data` object of form `{width, height}` to send the returned ad size to request a resize. Please check [Ad resizing](https://github.com/ampproject/amphtml/blob/master/ads/README.md#ad-resizing) for more information.


About the link, I think #ad-resizing should be enough?

you're right

zhouyx mentioned this pull request Sep 7, 2016

Match resizing behavior of iframe inside amp-ad and amp-iframe #4840

Merged

zhouyx added 3 commits September 7, 2016 11:13

make render-start receive resize info

30900de

fix check-types

b134a15

rebase

4eec2d7

zhouyx force-pushed the render-start-resize branch from 12343bd to 4eec2d7 Compare September 7, 2016 18:41

zhouyx added the NEEDS REVIEW label Sep 7, 2016

zhouyx assigned lannka Sep 7, 2016

zhouyx reviewed Sep 7, 2016
View reviewed changes

update doc

c624efd

zhouyx reviewed Sep 7, 2016
View reviewed changes

add JS Doc

ae66bd3

lannka reviewed Sep 7, 2016
View reviewed changes

fix

cb2975d

lannka reviewed Sep 7, 2016
View reviewed changes

zhouyx added 2 commits September 7, 2016 15:36

test fix

3c5082a

update doc

115dda5

lannka reviewed Sep 7, 2016
View reviewed changes

lannka added LGTM and removed NEEDS REVIEW labels Sep 7, 2016

fix

215a64b

lannka reviewed Sep 8, 2016
View reviewed changes

Conversation

zhouyx commented Sep 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhouyx commented Sep 6, 2016 •

edited

Loading

zhouyx Sep 7, 2016 •

edited

Loading