New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't strip rel=preload hints for resource types we don't optimize #1392

Closed
jeffkaufman opened this Issue Sep 12, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@jeffkaufman
Copy link
Contributor

jeffkaufman commented Sep 12, 2016

Right now if we see:

<link rel=preload as=font href=...>

or:

<link rel=preload as=document href=...>

we will strip the preload hint. We should instead only be stripping hints for resources we optimize:

<link rel=preload as=script href=...>
<link rel=preload as=style href=...>
<link rel=preload as=image href=...>

Specifically, if as is something other than script, style, or image we should leave the hint alone.

@jmarantz

This comment has been minimized.

Copy link
Contributor

jmarantz commented Sep 12, 2016

And we should only strip preload hints for images when image-rewriting is on, and options()->image_preserve_urls() is off.

Similar for css_preserve_urls() and the variety of css-rewriting options (mostly combine_css and rewrite_css and cache_extend_styles).

Similar for js_preserve_urls() and js-rewriting options including combine_javascript and rewrite_external_javascript and cache_extend_javascript but not rewrite_inline_javascript.

We'll have to be a little careful about curating the exact list of filters that trigger the stripping.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Sep 12, 2016

Right now strip_subresource_hints_filter.cc enables hint removal if:

driver_->can_modify_urls() &&
(!(options->css_preserve_urls() &&
   options->image_preserve_urls() &&
   options->js_preserve_urls()));

Each HtmlFilter has bool CanModifyUrls(), and driver_->can_modify_urls() just returns whether any enabled filter has CanModifyUrls() returning true``(SeeHtmlParse::CheckFilterBehavior`.)

This was written this way because at the time hints didn't specify types.

We can extend this to add CanModifyImageUrls(), CanModifySyleUrls(), CanModifyScriptUrls() but that's a much bigger chunk of work.

@jmarantz

This comment has been minimized.

Copy link
Contributor

jmarantz commented Sep 12, 2016

I might be kind of frustrated if I was using MPS just for aggressive image optimization, and it stripped all my CSS/JS hints. How much work is this really?

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Sep 12, 2016

How much work is this really?

Kind of a pain. Right now RewriteFilter::CanModifyUrls just returns true, so we'd need to go through all our filter classes and specify what kinds of urls they modify.

Alternatively, if X_preserve_urls() is set we could not strip X hints, and so your example person could set preserve for css+js but not images. That one is much easier to code, since strip_subresource_hints_filter can see (and already uses) the preserve settings.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Sep 12, 2016

Planning to go the preserve route.

We can come back and do the full thing later if we want to.

jeffkaufman added a commit that referenced this issue Sep 13, 2016

strip-subresource-hints: respect preserve with rel=preload, use href …
…and not src

* With rel=preload the hint tells us what type of resource it is, and if
  urls have been preserved for that type we should not strip it.
* If the rel=preload type isn't image, script, or style we shouldn't
  strip it, because those are the only urls we change.
* The filter was originally written to use src= when it should have used
  href=, which meant it removed hints it shouldn't have.

This is a minimal change for backporting for branch 33.  For the master branch
I have a draft of a more complex change that does additional cleanups.

Fixes: #1392
Fixes: #1393

jeffkaufman added a commit that referenced this issue Sep 13, 2016

strip-subresource-hints: respect preserve with rel=preload, use href …
…and not src

* With rel=preload the hint tells us what type of resource it is, and if
  urls have been preserved for that type we should not strip it.
* If the rel=preload type isn't image, script, or style we shouldn't
  strip it, because those are the only urls we change.
* The filter was originally written to use src= when it should have used
  href=, which meant it removed hints it shouldn't have.

This is a minimal change for backporting for branch 33.  For the master branch
I have a draft of a more complex change that does additional cleanups.

Fixes: #1392
Fixes: #1393

jeffkaufman added a commit that referenced this issue Sep 14, 2016

strip-subresource-hints: respect preserve with rel=preload, use href …
…and not src

* With rel=preload the hint tells us what type of resource it is, and if
  urls have been preserved for that type we should not strip it.
* If the rel=preload type isn't image, script, or style we shouldn't
  strip it, because those are the only urls we change.
* The filter was originally written to use src= when it should have used
  href=, which meant it removed hints it shouldn't have.

This is a minimal change for backporting for branch 33.  For the master branch
I have a draft of a more complex change that does additional cleanups.

Fixes: #1392
Fixes: #1393

jeffkaufman added a commit that referenced this issue Sep 14, 2016

strip-subresource-hints: respect preserve with rel=preload, use href …
…and not src (#1394)

* With rel=preload the hint tells us what type of resource it is, and if
  urls have been preserved for that type we should not strip it.
* If the rel=preload type isn't image, script, or style we shouldn't
  strip it, because those are the only urls we change.
* The filter was originally written to use src= when it should have used
  href=, which meant it removed hints it shouldn't have.

This is a minimal change for backporting for branch 33.  For the master branch
I have a draft of a more complex change that does additional cleanups.

Fixes: #1392
Fixes: #1393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment