Move extractSize to a separate hook #10127
Conversation
| .then(adResponse => { | ||
| expect(adResponse).to.deep.equal( | ||
| {creative, signature: base64UrlDecodeToBytes('AQAB')}); | ||
| expect(impl.extractSize(headers)).to.deep.equal(size); |
There was a problem hiding this comment.
Nit: indent is off here.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| @@ -649,20 +654,23 @@ export class AmpA4A extends AMP.BaseElement { | |||
| // This block returns the ad creative and signature, if available; null | |||
There was a problem hiding this comment.
Let's update these comments to reflect that size will also be passed down through here.
| expect(loadExtensionSpy.withArgs('amp-analytics')).to.be.called; | ||
| signature: base64UrlDecodeToBytes('AQAB'), | ||
| }); | ||
| expect(impl.extractSize(headers)).to.deep.equal(size); |
There was a problem hiding this comment.
Nit: indent here as well.
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| if (!headerValue) { | ||
| return null; | ||
| } | ||
| if (!(/[0-9]+x[0-9]+/.test(headerValue))) { |
There was a problem hiding this comment.
If you're going to do a RegExp, why not just capture the values out of the regexp and save the split?
| if (size) { | ||
| this.size_ = size; | ||
| } else { | ||
| size = this.size_; |
There was a problem hiding this comment.
This always felt odd to me. Why not have this return null if its not in the headers and move the logic to when we apply sizing?
There was a problem hiding this comment.
I'll be honest, I don't really understand how this code works. Do you mean the call to setStyles in onCreativeRender? Where would that code get the information about what size to use, if it wasn't set here?
There was a problem hiding this comment.
OK no need to address in this PR.
| if (size) { | ||
| this.size_ = size; | ||
| } else { | ||
| size = this.size_; |
There was a problem hiding this comment.
OK no need to address in this PR.
Determining the size of a Fast Fetch creative is now done through its own overridable method in
AmpA4A. This will facilitate the deprecation ofextractCreativeAndSignature, which in turn will facilitate having two different signature schemes controlled by a client-side experiment.One of the PRs that #9040 is being split into. Related to #7618.