Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tune heuristic when to load ads. #819

Merged
merged 1 commit into from
Nov 3, 2015
Merged

Conversation

cramforce
Copy link
Member

Instead of only loading them when they are in viewport, we also load
them if they are outside the viewport:

  • if the user has ever scrolled (So we don't interfere with e.g. swiping through articles).
  • we haven't started loading another ad in the last second.

In practice this should load a large percentage of ads before they enter the viewport
while interference with scrolling is kept to a similar value as with the old heuristic.

@ampsauce
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 363e144f4449eacca67e4173e6b9084ad0a38bef
Build details: https://travis-ci.org/ampsauce/amphtml/builds/88571641

(Please note that this is a fully automated comment.)

@@ -205,6 +229,14 @@ export function installAd(win) {

/** @override */
layoutCallback() {
console.error('ADLAYOUT')
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dvoytenko
Copy link
Contributor

LGTM with few comments.

@dvoytenko dvoytenko added the LGTM label Nov 2, 2015
@cramforce cramforce force-pushed the prefetch branch 2 times, most recently from ae7dbe2 to cb24f92 Compare November 2, 2015 23:10
@cramforce cramforce changed the title [RFC] Tune heuristic when to load ads. Tune heuristic when to load ads. Nov 2, 2015
Instead of only loading them when they are in viewport, we also load
them if they are outside the viewport:

- if the user has ever scrolled (So we don't interfere with e.g. swiping through articles).
- we haven't started loading another ad in the last second.

In practice this should load a large percentage of ads before they enter the viewport
while interference with scrolling is kept to a similar value as with the old heuristic.
cramforce added a commit that referenced this pull request Nov 3, 2015
Tune heuristic when to load ads.
@cramforce cramforce merged commit 59d7967 into ampproject:master Nov 3, 2015
@cramforce cramforce deleted the prefetch branch November 3, 2015 01:35
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.

3 participants