✨Introduce rendering for unsigned creatives #29332
Conversation
| this.creativeSize_ = size || this.creativeSize_; | ||
|
|
||
| // Update priority. | ||
| this.updateLayoutPriority(LayoutPriority.CONTENT); |
There was a problem hiding this comment.
isn't this too early? should update priority when we know it's (claimed to be) AMP.
There was a problem hiding this comment.
Yeah you are probably right, but I haven't implemented the check for ⚡️ or fallback yet. Added a todo to make sure I update this value where appropriate.
| streamResponseToWriter(this.win, response, detachedStream); | ||
|
|
||
| this.sanitizedHeadPromise_ = transformStream | ||
| transformStream |
There was a problem hiding this comment.
this is a unreferenced promise chain. might cause issues. do we want to return or catch?
There was a problem hiding this comment.
or shall we assign it to this.sanitizedHeadPromise_ ? i saw it is used in renderFriendlyTrustless_.
There was a problem hiding this comment.
Sorry I'm not quite sure what you mean here
There was a problem hiding this comment.
i don't quite understand the reason for the change in L909 to L912
There was a problem hiding this comment.
Ah I see, yes the change was so that we initialize as a promise instead of null, otherwise if layoutCallback was called before the fetch response returned it would throw that null has no .then in renderFriendlyTrustless.
I know it says 3000 lines, but it's really not so bad if you use the
?w=1whitespace trick :)I will start working on enabling the integration test tomorrow, but I didn't want to lose the momentum on this. Happy to wait to merge until we get integration test going. I haven't looked at integration yet, but 3p fallback is not yet fully supported so may have to wait for that.
Part of #27189