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

AdSense/Doubleclick Fast Fetch update AMP creative metadata format change #10738

Merged
merged 2 commits into from Aug 1, 2017
Merged

AdSense/Doubleclick Fast Fetch update AMP creative metadata format change #10738

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab296a1
handle metadata format change
keithwrightbos Aug 1, 2017
df32d07
set initial value for metadataStart var
keithwrightbos Aug 1, 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
26 changes: 14 additions & 12 deletions extensions/amp-a4a/0.1/amp-a4a.js
View file
Open in desktop
Expand Up @@ -59,12 +59,11 @@ import {getContextMetadata} from '../../../src/iframe-attributes';
// import '../../amp-ad/0.1/amp-ad-ui';
// import '../../amp-ad/0.1/amp-ad-xorigin-iframe-handler';

/** @type {string} */
const METADATA_STRING = '<script type="application/json" amp-ad-metadata>';

/** @type {string} */
const METADATA_STRING_NO_QUOTES =
'<script type=application/json amp-ad-metadata>';
/** @type {Array<string>} */
const METADATA_STRINGS = [
'<script amp-ad-metadata type=application/json>',
'<script type="application/json" amp-ad-metadata>',
'<script type=application/json amp-ad-metadata>'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering whether we just want to look for the first string, i.e., <script amp-metadata type=...>. According to an internal discussion in May, attribute names should always be sorted, and quotes should be omitted. I'm not sure that's become officially specified yet though. Most likely the best thing to do for now is to leave a TODO for later cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to be backwards compatible for now but as mentioned in the description: "Add support for multiple versions (to be cleaned up in the future once stable)."

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of a comment in the code itself. Are TODOs not a customary practice in this project?


// TODO(tdrl): Temporary, while we're verifying whether SafeFrame is an
// acceptable solution to the 'Safari on iOS doesn't fetch iframe src from
Expand Down Expand Up @@ -1360,11 +1359,14 @@ export class AmpA4A extends AMP.BaseElement {
* TODO(keithwrightbos@): report error cases
*/
getAmpAdMetadata_(creative) {
let metadataString = METADATA_STRING;
let metadataStart = creative.lastIndexOf(METADATA_STRING);
if (metadataStart < 0) {
metadataString = METADATA_STRING_NO_QUOTES;
metadataStart = creative.lastIndexOf(METADATA_STRING_NO_QUOTES);
let metadataStart = -1;
let metadataString;
for (let i = 0; i < METADATA_STRINGS.length; i++) {
metadataString = METADATA_STRINGS[i];
metadataStart = creative.lastIndexOf(metadataString);
if (metadataStart >= 0) {
break;
}
}
if (metadataStart < 0) {
// Couldn't find a metadata blob.
Expand Down Expand Up @@ -1428,7 +1430,7 @@ export class AmpA4A extends AMP.BaseElement {
} catch (err) {
dev().warn(
TAG, this.element.getAttribute('type'), 'Invalid amp metadata: %s',
creative.slice(metadataStart + METADATA_STRING.length, metadataEnd));
creative.slice(metadataStart + metadataString.length, metadataEnd));
return null;
}
}
Expand Down