What happens if data-amp-slot-index isn't coercible to a number? I think you'll get NaN back? Is that okay? Also, what happens if it's something that does coerce to a number, but really shouldn't? (Like null or [''], both of which coerce to 0.)

This set by code in amp-ad, to a number. So I would prefer to simply remove the coerce.

Aside: Most other AMP code seems to use win for the owner window. global looks like a convention inherited from inside Google? Any chance we could change this to win everywhere in here, just for greater consistency with AMP code?

Done

This is a required parameter, yes? Is there some user-level warning or error somewhere if it's omitted? I don't recall if we use it in the A4A network impls, but in the ads/google/doubleclick.js code, there's a call to 3p/3p.validateData that might be relevant.

I'll add a TODO. Yes, validateData is the right thing to use. So it ought to be moved somewhere to be used by both 3p and A4A. And someone needs to figure out which are required. (I used to know....)

Is this going to be different than you'd get from a4a.getViewport().getSize()?

Also make private?

Used getSize(), so function is now gone.

There is a test-utils.js for this file. Seems like a lot of these methods should have tests there (and/or revise/extend the tests that already exist.)

I added a test for this function, and a TODO for others that I am not touching in this PR.

Why do you need to disable this lint check here, and below?

No longer needed. Removed.

New hotness! You can now make this:

describes.sandboxed('amp-ad-network-doubleclick-impl', {}, () => {
  // Omit let sandbox and sinon.sandbox.create() and sandbox.restore().
  // Rest of test stuff
});

Done

If you're going to do document.createElement, worth doing it inside an isolated iframe, I think. Can also use src/dom#createElementWithAttributes as shorthand. Try nesting the following inside the outer describes.sandboxed:

  describes.fakeWin('some descriptive name', {}, env => {
    let win;
    let doc;
    beforeEach(() => {
      win = env.win;
      doc = win.document;
      doubleclickImplElem = createElementWithAttributes(doc, 'amp-ad', {
        type: 'doubleclick',
        data-ad-client: 'adsense',
        width: /* ??? */,
        height:  /* ??? */,
      });
      // Do rest of beforeEach initialization.
    });
    // Test cases and so on.
  });

Not sure you need the return here?

Ditto comments from AdSense impl.

Can we have some documentation on the difference between queryParams and unboundedQueryParams?

I'm not changing this, and didn't write it it originally, but sure.
I added doc.

Would it be possible/possible to recover from either of these promises failing? Does it occur in the wild? I'm trying to be much more explicit about possible error conditions and how we would handle/recover

I'll add a TODO.

Is there any real value in googleAdUrl and buildAdUrl being separate functions? I believe buildAdUrl can be private

Will merge googleAdUrl and buildAdUrl. Style guide says not to use private:

Exported functions may be defined directly on the exports object, or else declared locally and exported separately. Non-exported functions are encouraged and should not be declared @Private.

Also make private?

Can this be explicit what is returned for various error conditions?

Make private?

Style guide says no. (See above.)

I'm curious how this or references below could ever throw?

Don't know. This was copied from code elsewhere. If you think it can't throw, I'm happy to remove the try/catch.

Original comment:
// On some UAs (particularly UCWEB), these can throw NS_ERROR_FAILUREs.

Setting dtd here means that it does not include the time required to execute buildUrl. Instead, I suggest:
const url = buildUrl(baseUrl, allQueryParams, MAX_URL_LENGTH - 10, {name: 'trunc', value: '1'});
return url += '&dtd=' + elapsedTimeWithCeiling(Date.now(), startTime);

Done

win.gaGlobal = win.gaGlobal || {vid: clientId, hid: documentInfo. pageViewId};

Done

You reference a4a.element above in a4a.element.getAttribute('data-amp-slot-index'), might as well move this up and use it there

Done

Let's add a loop iteration count to ensure this will always exit

Same here

nit: noving -> moving

Done

Let's add a comment here that data-amp-slot-index is populated within amp-ad (had to do some splunking to ensure it was always set) as well as a dev().assert(slotId != undefined);

Done

Can use createElementWithAttributes in src/dom

Done