Skip to content

Creative API: resize#4023

Merged
lannka merged 7 commits intoampproject:masterfrom
nekodo:context-api-resize
Jul 25, 2016
Merged

Creative API: resize#4023
lannka merged 7 commits intoampproject:masterfrom
nekodo:context-api-resize

Conversation

@nekodo
Copy link
Copy Markdown
Contributor

@nekodo nekodo commented Jul 12, 2016

Modifies to allow nested frames to request resize using embed-size messages. Adds a test that verifies messages from nested frames are handled correctly, also tested manually.

This is the another change in the implementation of #3019.

…-size messages. Added a test using a nested frame to verify that these messages are received correctly.
@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

if (newHeight !== undefined || newWidth !== undefined) {
this.updateSize_(newHeight, newWidth, source, origin);
}
}, this.is3p_, this.is3p_));
Copy link
Copy Markdown
Contributor

@lannka lannka Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it intended to use is3p_ for opt_includingNestedWindows?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. The option was created with 3P use cases in mind. TBH, I'm not sure amp-ad ever has this set to false, but I'm keeping things consistent.

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jul 23, 2016

LGTM
please resolve merge conflict

@lannka lannka added LGTM and removed NEEDS REVIEW labels Jul 23, 2016
@nekodo
Copy link
Copy Markdown
Contributor Author

nekodo commented Jul 25, 2016

Resolved conflicts.

@lannka lannka merged commit 6bccb65 into ampproject:master Jul 25, 2016
if (this.pendingResizeRequest_) {
// There is an already pending resize request, fail it.
this.sendEmbedSizeResponse_(false /* success */);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor is breaking the existing resize functionality.
I am going to revert this for now before we cut the next canary.

cc @lannka @zhouyx

Please reach out to @lannka/myself for more details

camelburrito added a commit to camelburrito/amphtml that referenced this pull request Jul 25, 2016
camelburrito pushed a commit that referenced this pull request Jul 25, 2016
ariangibson pushed a commit to Mixpo/amphtml that referenced this pull request Sep 7, 2016
* Changed <amp-ad> to allow nested frames to request resize using embed-size messages. Added a test using a nested frame to verify that these messages are received correctly.
ariangibson pushed a commit to Mixpo/amphtml that referenced this pull request Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants