Fix for ad-network-fake-impl to work in Safari #9331
Conversation
| return { | ||
| creative: encoder.encode(creative).buffer, | ||
| creative: utf8EncodeSync(creative), | ||
| signature: 'FAKESIG', |
There was a problem hiding this comment.
Looking at https://github.com/ampproject/amphtml/blob/master/src/utils/bytes.js#L84 in the case where TextEncoder is defined, utf8EncodeSync(creative) is equivalent to encoder.encode(creative), not encoder.encode(creative).buffer.
If it's easy, e.g. not too many other uses, I'd suggest changing utf8EncodeSync to return .buffer in this case (line 86). But if that would cause problems to unrelated code, then it looks like this code here would have to change to something like foo=utf8EncodeSync(creative); foo = foo.buffer || foo; return { creative:foo, ... }
There was a problem hiding this comment.
Thanks @jonkeller utg8EncodeSync always returns a Uint8Array so just added .buffer back here.
|
@aghassemi are you still on this? |
|
oh totally fell through the cracks. Will do the review comment and submit |
|
@lannka @jonkeller PTAL |
| @@ -61,9 +59,8 @@ export class AmpAdNetworkFakeImpl extends AmpA4A { | |||
| // `localDev` and primarily used for A4A Envelope for testing. | |||
| // See DEVELOPING.md for more info. | |||
| const creative = this.transformCreativeLocalDev_(deserialized); | |||
| const encoder = new TextEncoder('utf-8'); | |||
There was a problem hiding this comment.
Can we go ahead and forbid TextEncoder/TextDecoder?
/to @jonkeller