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.

Sign up for GitHub

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

Jump to bottom

Load one ad a time. #9808

Merged
merged 2 commits into from Jun 10, 2017
Merged

Load one ad a time. #9808

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55aca54
Load one ad a time.
lannka Jun 8, 2017
feffaca
address comments
lannka Jun 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions extensions/amp-a4a/0.1/amp-a4a.js
View file
Open in desktop
Expand Up @@ -1224,28 +1224,28 @@ export class AmpA4A extends AMP.BaseElement {
this.promiseErrorHandler_(
new Error('fallback to 3p'),
/* ignoreStack */ true);
incrementLoadingAds(this.win);
// Haven't rendered yet, so try rendering via one of our
// cross-domain iframe solutions.
const method = this.experimentalNonAmpCreativeRenderMethod_;
let renderPromise = Promise.resolve(false);
if ((method == XORIGIN_MODE.SAFEFRAME ||
method == XORIGIN_MODE.NAMEFRAME) &&
this.creativeBody_) {
const renderPromise = this.renderViaNameAttrOfXOriginIframe_(
renderPromise = this.renderViaNameAttrOfXOriginIframe_(
this.creativeBody_);
this.creativeBody_ = null; // Free resources.
return renderPromise;
} else if (this.adUrl_) {
assertHttpsUrl(this.adUrl_, this.element);
return this.renderViaCachedContentIframe_(this.adUrl_);
renderPromise = this.renderViaCachedContentIframe_(this.adUrl_);
} else {
// Ad URL may not exist if buildAdUrl throws error or returns empty.
// If error occurred, it would have already been reported but let's
// report to user in case of empty.
user().warn(TAG, this.element.getAttribute('type'),
'No creative or URL available -- A4A can\'t render any ad');
return Promise.resolve(false);
}
incrementLoadingAds(this.win, renderPromise);
return renderPromise;
}

/**
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-ad/0.1/amp-ad-3p-impl.js
View file
Open in desktop
Expand Up @@ -219,8 +219,7 @@ export class AmpAd3PImpl extends AMP.BaseElement {
user().assert(!this.isInFixedContainer_,
'<amp-ad> is not allowed to be placed in elements with ' +
'position:fixed: %s', this.element);
incrementLoadingAds(this.win);
return this.layoutPromise_ = getAdCid(this).then(cid => {
this.layoutPromise_ = getAdCid(this).then(cid => {
const opt_context = {
clientId: cid || null,
container: this.container_,
Expand All @@ -237,6 +236,8 @@ export class AmpAd3PImpl extends AMP.BaseElement {
this);
return this.xOriginIframeHandler_.init(iframe);
});
incrementLoadingAds(this.win, this.layoutPromise_);
return this.layoutPromise_;
}

/** @override */
Expand Down
15 changes: 8 additions & 7 deletions extensions/amp-ad/0.1/concurrent-load.js
View file
Open in desktop
Expand Up @@ -58,16 +58,17 @@ export function getAmpAdRenderOutsideViewport(element) {
/**
* Increments loading ads count for throttling.
* @param {!Window} win
* @param {!Promise=} opt_loadingPromise
*/
export function incrementLoadingAds(win) {
export function incrementLoadingAds(win, opt_loadingPromise) {
if (win[LOADING_ADS_WIN_ID_] === undefined) {
win[LOADING_ADS_WIN_ID_] = 0;
}
win[LOADING_ADS_WIN_ID_]++;
timerFor(win).delay(() => {
// Unfortunately we don't really have a good way to measure how long it
// takes to load an ad, so we'll just pretend it takes 1 second for
// now.
win[LOADING_ADS_WIN_ID_]--;
}, 1000);
timerFor(win)
.timeoutPromise(1000, opt_loadingPromise || new Promise(() => {}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a never resolving promise, can't you just pass null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

timeoutPromise() only require the timeout value to be defined. would timeoutPromise(1000, opt_loadingPromise) be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.catch(() => {})
.then(() => {
win[LOADING_ADS_WIN_ID_]--;
});
}
17 changes: 16 additions & 1 deletion extensions/amp-ad/0.1/test/test-concurrent-load.js
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@ import {
} from '../concurrent-load';
import {createElementWithAttributes} from '../../../../src/dom';
import {installTimerService} from '../../../../src/service/timer-impl';
import {macroTask} from '../../../../testing/yield';
import * as lolex from 'lolex';

describes.realWin('concurrent-load', {}, env => {
Expand Down Expand Up @@ -75,13 +76,27 @@ describes.realWin('concurrent-load', {}, env => {
installTimerService(win);
});

it('should throttle ad loading one per second', () => {
it('should throttle ad loading one per second', function* () {
expect(is3pThrottled(win)).to.be.false;
incrementLoadingAds(win);
expect(is3pThrottled(win)).to.be.true;
clock.tick(999);
yield macroTask();
Copy link
Contributor

Choose a reason for hiding this comment

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

You could return the promise from incrementLoadingAds.

Copy link
Contributor Author

@lannka lannka Jun 8, 2017 •
edited

Choose a reason for hiding this comment

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

The test then becomes:

clock.tick(999);
expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield promise;
expect(is3pThrottled(win)).to.be.false;

the first check is meaningless here.

Copy link
Contributor

Choose a reason for hiding this comment

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

How?

Copy link
Contributor Author

@lannka lannka Jun 8, 2017 •
edited

Choose a reason for hiding this comment

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

it's unfair to only yield on the 2nd tick. (and you can't yield on the first, which will run into a deadlock :-))

this is not testing the right thing:

clock.tick(1000);
expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield promise;
expect(is3pThrottled(win)).to.be.false;

Copy link
Contributor

@jridgewell jridgewell Jun 8, 2017 •
edited

Choose a reason for hiding this comment

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

Right now, the macroTask isn't testing anything. You've frozen time in test's window.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should yield before doing each check.

Here we try to test that loading ads count will decrease by 1 after 1 sec, and only after 1 sec.
With yield promise, we can test that the ads count decrease by 1 after 1 sec.
However with

clock.tick(999);
expect(is3pThrottled(win)).to.be.true;

we can't tell if the promise is resolved only after 1sec. It is possible that the promise has already resolved after 999ms, but macroTasks not executed, loading ad count hasn't decresed by 1 when test code ran.

This is probably right to do:

clock.tick(999);
yield macroTask();
expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield promise;
expect(is3pThrottled(win)).to.be.false;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, the macroTask isn't testing anything. You've frozen time in test's window.

@jridgewell here we want to yield an event loop so that if the timer has triggered the promise resolver, we wait till the resolution finish.

@zhouyx your suggestion is also treating the 2 ticks differently, which I don't like

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd agree with @zhouyx here, if you want to test specifically that it only resolves after 1s. I'm still against relying on macrotasks for promise changes to complete, though.

Copy link
Contributor Author

@lannka lannka Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

why do we want to treat the 2 ticks differently? isn't it cheating? are we doing a test to just get it passed?

we have to do 2 checks with all other conditions the same, if we really want to test the 1000ms triggering logic.

the test is translated to:

  1. clock tick 999ms
  2. ad should still be throttled in the next event loop (1ms to go!)
  3. clock tick 1ms
  4. ad should NOT be throttled starting from the next event loop

expect(is3pThrottled(win)).to.be.true;
clock.tick(1);
yield macroTask();
expect(is3pThrottled(win)).to.be.false;
});

it('should throttle ad one a time', function* () {
expect(is3pThrottled(win)).to.be.false;
let resolver;
incrementLoadingAds(win, new Promise(res => {
resolver = res;
}));
expect(is3pThrottled(win)).to.be.true;
resolver();
yield macroTask();
expect(is3pThrottled(win)).to.be.false;
});
});
Expand Down