Skip to content

Creative API: page visibility#4021

Merged
lannka merged 6 commits intoampproject:masterfrom
nekodo:creative-api-page-visibility
Jul 22, 2016
Merged

Creative API: page visibility#4021
lannka merged 6 commits intoampproject:masterfrom
nekodo:creative-api-page-visibility

Conversation

@nekodo
Copy link
Copy Markdown
Contributor

@nekodo nekodo commented Jul 12, 2016

Adds support for subscribing to embed-state messages (which contain page visibility information) from nested ad iframes (as done previosuly for IntersectionObserver). A new message is added, send-embed-state, used to subscribe to the updates.

The code for handling subscription is refactored out of IntersectionObserver and into a reusable class in iframe-helper.js . I also added a test for subscription to embed-state updates as there did not seem to be any at the moment.

This is the next change in the implementation of #3019, apologies for the delay.

@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.

this.embedStateApi_ = new SubscriptionApi(
this.iframe_, 'send-embed-state', is3p,
() => this.sendEmbedInfo_(this.baseInstance_.isInViewport()));
this.embedStateApi_.init();
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.

Looks like we always create than immediately call #init. Is there a reason to separate the two steps?

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.

I suppose not at the moment, switched it to private and made the constructor call it.

return;
}
// Note that we multicast the update to all interested windows.
postMessageToWindows(
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.

if this method is no more used externally, can be private?

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.

It will be used though, see #4023 for an example.

* invoked whenever a new window subscribes.
*/
constructor(iframe, type, is3p, requestCallback) {
this.iframe_ = iframe;
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.

Please add annotations to those private members.

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.

Done.

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Jul 14, 2016

Some more comments. Looks good in general.

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

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.

6 participants