Skip to content
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

Support UrlValuedAttribute with CSS #1324

Closed
jeffkaufman opened this issue Jun 23, 2016 · 2 comments
Closed

Support UrlValuedAttribute with CSS #1324

jeffkaufman opened this issue Jun 23, 2016 · 2 comments
Assignees

Comments

@jeffkaufman
Copy link
Contributor

@jeffkaufman jeffkaufman commented Jun 23, 2016

Using UrlValuedAttribute with Stylesheet doesn't work:

~ $ curl http://www.jefftk.com/css-prefetch?PageSpeedFilters=
<link rel=stylesheet href="example.css">
<link rel=stylesheet css-prefetch="example.css">
<link css-prefetch="example.css">
<link rel=stylesheet css-prefetch="example.css" href="example.css">
<link css-prefetch="example.css" href="example.css">

~ $ curl http://www.jefftk.com/css-prefetch?PageSpeedFilters=rewrite_css
<link rel=stylesheet href="A.example.css.pagespeed.cf.ibS6ETNCIK.css">
<link rel=stylesheet css-prefetch="example.css">
<link css-prefetch="example.css">
<link rel=stylesheet css-prefetch="example.css" href="A.example.css.pagespeed.cf.ibS6ETNCIK.css">
<link css-prefetch="example.css" href="example.css">

~ $ curl http://www.jefftk.com/css-prefetch?PageSpeedFilters=extend_cache_css
<link rel=stylesheet href="example.css.pagespeed.ce.uEwpUZgfsK.css">
<link rel=stylesheet css-prefetch="example.css.pagespeed.ce.uEwpUZgfsK.css">
<link css-prefetch="example.css.pagespeed.ce.uEwpUZgfsK.css">
<link rel=stylesheet css-prefetch="example.css.pagespeed.ce.uEwpUZgfsK.css" href="example.css.pagespeed.ce.uEwpUZgfsK.css">
<link css-prefetch="example.css.pagespeed.ce.uEwpUZgfsK.css" href="example.css">

So adding that config does make us recognize it as css for extend_cache_css purposes but not for rewrite_css purposes.

It looks like for css we don't use resource_tag_scanner::ScanElement but instead implement a css-specific tag scanner in CssTagScanner::ParseCssElement.

It doesn't look like there's any config tweak that would make this work, or even a simple code tweak. Instead we would need to:

  1. Modify CssTagScanner to call resource_tag_scanner to find out about any overrides it defines.

  2. Modify all callers of CssTagScanner::ParseCssElement to accept getting multiple stylesheet urls in response.

@jeffkaufman
Copy link
Contributor Author

@jeffkaufman jeffkaufman commented Jun 23, 2016

Loading

@jeffkaufman jeffkaufman self-assigned this Jun 23, 2016
@jeffkaufman
Copy link
Contributor Author

@jeffkaufman jeffkaufman commented Jun 23, 2016

Actually, I don't think CssTagScanner::ParseCssElement is the right place for this: that's for inlining, combining, and other fancy stuff. Someone telling us about a new kind of stylesheet attribute probably just wants us to optimize it in a stand-alone way, which means CssFilter::EndElementImpl is the place.

Loading

crowell added a commit that referenced this issue Jul 21, 2016
crowell added a commit that referenced this issue Jul 21, 2016
Fixes #1324

system-test: fix 'url-valued stylesheet attributes' flake

Fixes #1356
crowell added a commit that referenced this issue Jul 21, 2016
Fixes #1324

system-test: fix 'url-valued stylesheet attributes' flake

Fixes #1356

rm range based loops for c++98 compatibility in branch 33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant