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

amp-auto-ads: Ad placement configuration errors are user errors #9001

Merged
merged 2 commits into from
Apr 28, 2017
Merged

amp-auto-ads: Ad placement configuration errors are user errors #9001

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
04ddf71
Ad placement configuration errors are user errors
jridgewell Apr 27, 2017
4229768
Fix type checks
jridgewell Apr 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions extensions/amp-auto-ads/0.1/ad-strategy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import {DataAttributeDef, PlacementState} from './placement';
import {dev} from '../../../src/log';
import {user} from '../../../src/log';

/** @const */
const TAG = 'amp-auto-ads';
Expand Down Expand Up @@ -85,7 +85,7 @@ export class AdStrategy {
placeNextAd_() {
const nextPlacement = this.availablePlacements_.shift();
if (!nextPlacement) {
dev().warn(TAG, 'unable to fulfill ad strategy');
user().warn(TAG, 'unable to fulfill ad strategy');
return Promise.resolve(false);
}
return nextPlacement.placeAd(this.baseAttributes_, this.adTracker_)
Expand Down
6 changes: 3 additions & 3 deletions extensions/amp-auto-ads/0.1/amp-auto-ads.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import {AdTracker, getExistingAds} from './ad-tracker';
import {AdStrategy} from './ad-strategy';
import {dev, user} from '../../../src/log';
import {user} from '../../../src/log';
import {xhrFor} from '../../../src/services';
import {getAdNetworkConfig} from './ad-network-config';
import {isExperimentOn} from '../../../src/experiments';
Expand Down Expand Up @@ -72,8 +72,8 @@ export class AmpAutoAds extends AMP.BaseElement {
return xhrFor(this.win)
.fetchJson(configUrl, xhrInit)
.catch(reason => {
dev().error(TAG, 'amp-auto-ads config xhr failed: ' + reason);
return null;
user().error(TAG, 'amp-auto-ads config xhr failed: ' + reason);
throw reason;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we keep the return 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.

This also cancels the entire promise queue if fetching/parsing of the configuration fails.

This is specifically to stop further processing, since we can't do anything without a config.

});
}
}
Expand Down
16 changes: 8 additions & 8 deletions extensions/amp-auto-ads/0.1/placement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {dev} from '../../../src/log';
import {dev, user} from '../../../src/log';
import {getAttributesFromConfigObj} from './attributes';
import {resourcesForDoc} from '../../../src/services';
import {
Expand Down Expand Up @@ -217,7 +217,7 @@ export class Placement {
export function getPlacementsFromConfigObj(win, configObj) {
const placementObjs = configObj['placements'];
if (!placementObjs) {
dev().warn(TAG, 'No placements in config');
user().warn(TAG, 'No placements in config');
return [];
}
const placements = [];
Expand All @@ -237,18 +237,18 @@ export function getPlacementsFromConfigObj(win, configObj) {
function getPlacementsFromObject(win, placementObj, placements) {
const injector = INJECTORS[placementObj['pos']];
if (!injector) {
dev().warn(TAG, 'No injector for position');
user().warn(TAG, 'No injector for position');
return;
}
const anchor = placementObj['anchor'];
if (!anchor) {
dev().warn(TAG, 'No anchor in placement');
user().warn(TAG, 'No anchor in placement');
return;
}
const anchorElements =
getAnchorElements(win.document.documentElement, anchor);
if (!anchorElements.length) {
dev().warn(TAG, 'No anchor element found');
user().warn(TAG, 'No anchor element found');
return;
}
let margins = undefined;
Expand Down Expand Up @@ -282,7 +282,7 @@ function getPlacementsFromObject(win, placementObj, placements) {
function getAnchorElements(rootElement, anchorObj) {
const selector = anchorObj['selector'];
if (!selector) {
dev().warn(TAG, 'No selector in anchor');
user().warn(TAG, 'No selector in anchor');
return [];
}
let elements = [].slice.call(scopedQuerySelectorAll(rootElement, selector));
Expand Down Expand Up @@ -323,13 +323,13 @@ function isPositionValid(anchorElement, position) {
position == Position.BEFORE || position == Position.AFTER ?
anchorElement.parentElement : anchorElement;
if (!elementToCheckOrNull) {
dev().warn(TAG, 'Parentless anchor with BEFORE/AFTER position.');
user().warn(TAG, 'Parentless anchor with BEFORE/AFTER position.');
return false;
}
const elementToCheck = dev().assertElement(elementToCheckOrNull);
return !BLACKLISTED_ANCESTOR_TAGS.some(tagName => {
if (closestByTag(elementToCheck, tagName)) {
dev().warn(TAG, 'Placement inside blacklisted ancestor: ' + tagName);
user().warn(TAG, 'Placement inside blacklisted ancestor: ' + tagName);
return true;
}
return false;
Expand Down