Refactor all buildImagePlaceholder_ to use createPlaceholderCallback #5344
Conversation
|
|
||
| // TODO(#5321): Investigate if this is actually the correct URL format for generated | ||
| // dynamic image. | ||
| placeholder.setAttribute('src', |
There was a problem hiding this comment.
do we need to move over the alt attribute to the place holder?
There was a problem hiding this comment.
Yes if alt is supported on the element we should propagate it. Will review to add it where supported
There was a problem hiding this comment.
We don't support alt attribute on most of these players.
| 'visibility': '', | ||
| }); | ||
| }); | ||
| placeholder.setAttribute('noprerender', ''); |
There was a problem hiding this comment.
was this part of the eval that this shouldn't be prerendered?
There was a problem hiding this comment.
I am actually not 100%. Sure was it only if the request is sending or setting cookies during prerender? I'll review the document.
In this PR u added noprerender to items that uses the same domain for loading the main content and prerender content.
There was a problem hiding this comment.
Checked the doc, doesn't seem we have problems with these, so dropping noprerender attribute from these.
|
PTAL 👀 |
|
LGTM. @mkhatib thanks for double checking |
(except YouTube for #3216)
Fixes #3212