Do you really need all of this style and extensions?

This should be within getPageLevelParameters_. Also, correct me if I'm wrong but this would cause the RTC request to be sent for every slot instead of once per page. We should refactor the code so that getPageLevelParameters_ returns a cached value if already calculated which will include ensuring RTC is only sent once.

I'm confused, getPageLevelParameters_ calls googlePageParameters so why do you need to do it?

Type here can be more precise (e.g. ?Promise<!Object<string,string>>)

Do not worry about other networks, assume only doubleclick

I think this can just be sendRtcRequest_ (no need to include promise)

RTC modifies block level parameters so you don't actually have to wait on page level before merging. You can move the merge directly into sendRTCRequest. This has the added benefit of allowing you to add other parameters (e.g. time to get RTC response) without putting that logic in getAdUrl.

Also I don't think how you have this will work as this.jsonTargeting_ is unique per block but you only have one promise that merges. So the flow actually needs to be that the RTC promise just returns back the RTC response and then you call merge on every block. Does that make sense?

There are 3 failure conditions of which any failure should prevent ad request if indicated by pub:

• network failure
• timeout
• malformed response

This is addressing the last item but I would argue that only malformation we care about is failure to JSON parse which we can handle as part of sendRTCRequest flow

You don't actually need blockParameters for RTC. You only need populateAdUrlState to have set this.jsonTargeting. I suggest we structure as follows:

• getPageParams
• populateAdUrlState
• sendRtcRequest which modifies this.jsonTargeting_ and return an object mapping to be merged into the end url. sendRtcRequest contains ALL RTC logic (perhaps rename to executeRTC?) from merge to timeout to reject if pub indicates no ad request on failure
• getBlockParams (AFTER RTC completion)
• Object.assign block, RTC, page (note that order indicates order of url parameters!)

done

Consider looking for other fields in the object to point out typos to pubs.

Can make this @enum {string}

done

Add MS to indicate its milliseconds

done

nit: remove extra, empty lines

done

You need to call populateAdUrlState before sendRtcRequest to ensure that this.jsonTargeting_ has been populated

done

if (!ampRtcPageElement) return Promise.resolve();
let rtcConfig;
let endPoint;
// NOTE usage of textContent instead of innerHTML
if (!isObject(rtcConfig = tryParseJson(ampRtcPageElement.textContent)) || !(endPoint = rtcConfig['endPoint']) || typeof endPoint != 'string' || !isSecureUrl(endPoint)) {
user().warn(TAG, 'invalid RTC config...');
return Promise.resolve();
}

done

Whether or not the response was cached should be controlled by the end point via cache headers. I think it makes more sense to have an option that disables state while revalidate behavior (e.g. staleRevalidateDisabled: true). The behavior is that the first request has no special cache headers and if SWR is not disabled, we send a second request with no-cache option

Note my previous comment does not cancel the ad request if invalid config. I think this makes more sense as the pub clearly misconfigured and we should default to ensuring they get ad revenue

ack

I suggest we rename this function to executeRTC and have it return Promise<!Object<string,string>> allowing for custom parameters to be merged into the ad url (e.g. time delay). Note that this will be page level by definition.

Note that given the current flow, we will possibly send the SWR request before the first request has returned or possibly errored due to non-200 response. Let's change this flow so that sending the cache request is actually dependent on the first rtc request promise. Something like:

const rtcRequest = this.executeRTC_();
if (!swrDisabled) {
rtcRequest.then(() => {
// send rtc request with no-cache option
});
}
return rtcPromise = timerFor(window).timeoutPromise(
RTC_TIMEOUT, rtcRequest).then(res => {
// merge into this.jsonTargeting_ and generate ad url here
return customParams;
});

Not a fan of the fact that RTC (which is doubleclick specific) is cluttering up amp-ad. We should rethink how to do this later...

note: still filling these cases in as I go. just pushed out to get the other changes out there.

What about 204 no content? Other 200 codes?

Add a comment indicating that empty is assumed to indicate ok response with nothing to merge. Can also just do if (!text)

done

I prefer a consistent object type. How about instead you just check if !r.rtcResponse here?

done

No need to call deepMerge if this.jsonTargeting_['targeting'] exists, right? Can just assign it. Not sure how much that saves in terms of performance

I assume you meant that we don't need to call deepMerge if this.jsonTargeting['targeting'] does not exist. I'll change it

I'm thinking a cleaner interface is to just have executeRtc return parameters to be added.

done, switched to using Object.assign too. even cleaner

Consider creating helper function that takes keyName (e.g. targeting) to reduce duplicate code

done

Does this comment still apply?

yes.

Need to update return type here and mergeRtc

done

Anyway you could not require looking for 'artc'?

yea don't actually need it. Removed.

you don't actually need this check as assigning null/undefined has no effect. You can change this to:

const parameters = Object.assign(this.getBlockParameters_(), values[1], pageLevelParameters);

done

Let's add that we're sending the ad request anyway: 'invalid RTC config, sending ad request w/o callout: ${...}'

done

Did you consider edge case values such as 'true', 1, and '1'? Ideally there would be a helper function to ensure we are consistent across the codebase (perhaps one already exists?). See sendAdRequestOnFailure you have below.

Also let's move this directly into the if condition below, no need to create variable.

I'll add 'true' as valid here too, but I'm not sure adding 1 and '1' is necessarily correct.

Add a comment that SWR should be disabled if the RTC is not cachable (no cache header). Can we read the cache header from the response? Also something about Cache-Control not being supported in IE?

This indentation looks off...