Remove requirement for both mobile and desktop URL metrics for preconnect links in Embed Optimizer #1764
Conversation
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
…bed type Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
| private function get_preconnect_urls( OD_HTML_Tag_Processor $processor ): array { | ||
| /* | ||
| * The following embeds have been chosen for optimization due to their relative popularity among all embed types. | ||
| * See <https://colab.sandbox.google.com/drive/1nSpg3qoCLY-cBTV2zOUkgUCU7R7X2f_R?resourcekey=0-MgT7Ur0pT__vw-5_AHjgWQ#scrollTo=utZv59sXzXvS>. |
There was a problem hiding this comment.
FYI, this link doesn't seem to be publicly accessible.
There was a problem hiding this comment.
Oh, thanks for that.
@adamsilverstein Can this be exported to a Gist?
| $embed_wrapper_xpath = self::get_embed_wrapper_xpath( $processor->get_xpath() ); | ||
|
|
||
| $max_intersection_ratio = $context->url_metric_group_collection->get_element_max_intersection_ratio( $embed_wrapper_xpath ); | ||
| if ( ! ( $max_intersection_ratio > 0.0 ) && embed_optimizer_update_markup( $processor, false ) && ! $this->added_lazy_script ) { |
There was a problem hiding this comment.
@westonruter Do we need to use PHP_FLOAT_EPSILON here? Wouldn't ! ( $max_intersection_ratio > 0.0 ) suffice, as that's what is used when adding preconnect links?
There was a problem hiding this comment.
I think ! ( $max_intersection_ratio > 0.0 ) is equivalent to 0.0 === $max_intersection_ratio (given the $max_intersection_ratio will never be negative), and both seem to work fine, but it seems in some cases even when a finite decimal is provided, it can result in a very unexpected value, for example #1765. So in this way, it seems safer to do:
| if ( ! ( $max_intersection_ratio > 0.0 ) && embed_optimizer_update_markup( $processor, false ) && ! $this->added_lazy_script ) { | |
| if ( $max_intersection_ratio < PHP_FLOAT_EPSILON && embed_optimizer_update_markup( $processor, false ) && ! $this->added_lazy_script ) { |
There was a problem hiding this comment.
Makes sense, thanks for clarifying. Done in 0b4150c.
| */ | ||
| private function get_preconnect_urls( OD_HTML_Tag_Processor $processor ): array { | ||
| /* | ||
| * The following embeds have been chosen for optimization due to their relative popularity among all embed types. |
There was a problem hiding this comment.
It seems the comment lines here are missing an space before the *
There was a problem hiding this comment.
Oops. Fixed it in 0b4150c.
westonruter
added
[Type] Bug
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Summary
Fixes #1757
Relevant technical choices
Extracted the preconnect and lazy-loading logic into separate methods.