Skip to content

make use of native IntersectionObserver in 3p iframe#6503

Merged
zhouyx merged 7 commits intoampproject:masterfrom
zhouyx:3p-io
Dec 7, 2016
Merged

make use of native IntersectionObserver in 3p iframe#6503
zhouyx merged 7 commits intoampproject:masterfrom
zhouyx:3p-io

Conversation

@zhouyx
Copy link
Copy Markdown
Contributor

@zhouyx zhouyx commented Dec 6, 2016

close #5603

@zhouyx zhouyx added the 3P label Dec 6, 2016
}, {
threshold: DEFAULT_THRESHOLD,
});
io.observe(window.document.documentElement);
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.

window.document.documentElement is the best I can do here. If it does not fill up our 3p iframe, we get different intersection changeEntry between native IO and polyfill IO.

});
io.observe(window.document.documentElement);
const unlistener = () => io.unobserve(window.document.documentElement);
return unlistener;
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.

return in the above line?

ads/_ping_.js Outdated
export function _ping_(global, data) {
global.document.getElementById('c').textContent = data.ping;

if (!data.nativeIo) {
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.

to be explicit, data.nativeIntersectionObserver

data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
data-valid='true'
data-native-io='true'
data-unlisten-io='true'
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.

nit, maybe pass in a number as seconds?

@zhouyx zhouyx merged commit 7ff2657 into ampproject:master Dec 7, 2016
@zhouyx zhouyx deleted the 3p-io branch December 7, 2016 18:54

if (!data.nativeIntersectionObserver) {
const nullIO = () => {};
nullIO.prototype = null;
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 is an error, arrow functions do not have prototypes. Also, setting a prototype to null doesn't prevent instances from getting a prototype chain, only Object.create(null) does that.

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.

hmmm, but it did work though. What should I do to assign the window.IntersectionObserver to null in this case. type-check require it to be a function, and this is my hack to fake native IntersectionObserver 😢

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.

Use a function declaration instead.

function nullIO() {};
nullIO.prototype = Object.create(null); // Note you'll have to update your prototype check

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.

mkhatib added a commit to mkhatib/amphtml that referenced this pull request Dec 15, 2016
mkhatib added a commit to mkhatib/amphtml that referenced this pull request Dec 15, 2016
mkhatib added a commit that referenced this pull request Dec 15, 2016
mkhatib added a commit that referenced this pull request Dec 15, 2016
mkhatib added a commit that referenced this pull request Dec 15, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* use native io and disable io for test

* return unlisten func

* update comment

* s

* fix type check

* add comment

* fix type check
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016
* use native io and disable io for test

* return unlisten func

* update comment

* s

* fix type check

* add comment

* fix type check
Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 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.

Stop using IntersectionObserver polyfill in ads if native support is present

3 participants