Permalink
Browse files

url-valued-attributes: support css

Fixes #1324

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

Fixes #1356

rm range based loops for c++98 compatibility in branch 33
  • Loading branch information...
jeffkaufman authored and crowell committed Jun 24, 2016
1 parent 0654743 commit 59a198284e82bccb303d7e27705bcd3dbd22eda3
@@ -647,6 +647,11 @@ NameVirtualHost localhost:@@APACHE_SECONDARY_PORT@@
ModPagespeedUrlValuedAttribute video alt-b Image
ModPagespeedUrlValuedAttribute video alt-b Image

ModPagespeedUrlValuedAttribute link data-stylesheet Stylesheet
ModPagespeedUrlValuedAttribute span data-stylesheet-a Stylesheet
ModPagespeedUrlValuedAttribute span data-stylesheet-b Stylesheet
ModPagespeedUrlValuedAttribute span data-stylesheet-c Stylesheet

# Also test that we can redefine spec-defined attributes.
ModPagespeedUrlValuedAttribute blockquote cite Image
</VirtualHost>
@@ -0,0 +1,26 @@
<!-- this should get inlined -->
<link rel=stylesheet href="../mod_pagespeed_example/styles/bold.css">

<!-- these should get rewritten in-place -->
<link data-stylesheet="../mod_pagespeed_example/styles/bold.css">
<span data-stylesheet-a="../mod_pagespeed_example/styles/bold.css"
data-stylesheet-b="../mod_pagespeed_example/styles/bold.css"
data-stylesheet-c="../mod_pagespeed_example/styles/bold.css"></span>

<!-- this should still get rewritten in-place -->
<link rel=invalid data-stylesheet="../mod_pagespeed_example/styles/bold.css">

<!-- this should get inlined, and data-stylesheet rewritten in-place -->
<link rel=stylesheet
href="../mod_pagespeed_example/styles/bold.css"
data-stylesheet="../mod_pagespeed_example/styles/bold.css">

<!-- javascript to give a combining barrier -->
<script src="this-doesnt-exist.js"></script>

<!-- combining should treat span/data-stylesheet-a as a barrier -->
<link rel=stylesheet href="../mod_pagespeed_example/styles/blue.css">
<span data-stylesheet-a="../mod_pagespeed_example/styles/bold.css"></span>
<link rel=stylesheet href="../mod_pagespeed_example/styles/yellow.css">


@@ -34,6 +34,7 @@
#include "net/instaweb/rewriter/public/resource.h"
#include "net/instaweb/rewriter/public/resource_combiner.h"
#include "net/instaweb/rewriter/public/resource_slot.h"
#include "net/instaweb/rewriter/public/resource_tag_scanner.h"
#include "net/instaweb/rewriter/public/rewrite_context.h"
#include "net/instaweb/rewriter/public/rewrite_driver.h"
#include "net/instaweb/rewriter/public/rewrite_filter.h"
@@ -460,6 +461,20 @@ void CssCombineFilter::StartElementImpl(HtmlElement* element) {
if (!context_->AddElement(element, href)) {
NextCombination("resource not rewritable");
}
} else {
// Treat custom UrlValuedAttributes as combining barriers.
resource_tag_scanner::UrlCategoryVector attributes;
// This includes checking for spec-defined ones, but any elements that would
// match spec-defined ones would have hit the ParseCssElement case above.
resource_tag_scanner::ScanElement(
element, driver()->options(), &attributes);
resource_tag_scanner::UrlCategoryVector::iterator uc;
for (uc = attributes.begin(); uc != attributes.end(); uc++) {
if (uc->category == semantic_type::kStylesheet) {
NextCombination("custom or alternate stylesheet attribute");
return;
}
}
}
}

@@ -451,15 +451,15 @@ class CssCombineFilterTest : public RewriteTestBase {
StringVector *css_urls) {
// Put original CSS files into our fetcher.
GoogleString html_url = StrCat(kDomain, "base_url.html");
const char a_css_url[] = "http://other_domain.test/foo/a.css";
const char b_css_url[] = "http://other_domain.test/foo/b.css";
const char c_css_url[] = "http://other_domain.test/foo/c.css";
static const char kACssUrl[] = "http://other_domain.test/foo/a.css";
static const char kBCssUrl[] = "http://other_domain.test/foo/b.css";
static const char kCCssUrl[] = "http://other_domain.test/foo/c.css";

ResponseHeaders default_css_header;
SetDefaultLongCacheHeaders(&kContentTypeCss, &default_css_header);
SetFetchResponse(a_css_url, default_css_header, kACssBody);
SetFetchResponse(b_css_url, default_css_header, kBCssBody);
SetFetchResponse(c_css_url, default_css_header, kCCssBody);
SetFetchResponse(kACssUrl, default_css_header, kACssBody);
SetFetchResponse(kBCssUrl, default_css_header, kBCssBody);
SetFetchResponse(kCCssUrl, default_css_header, kCCssBody);

// Rewrite
ParseUrl(html_url, html_input);
@@ -499,8 +499,8 @@ class CssCombineFilterTest : public RewriteTestBase {
AddDomain("a.com");
AddDomain("b.com");
bool supply_mock = false;
const char kUrl1[] = "http://a.com/1.css";
const char kUrl2[] = "http://b.com/2.css";
static const char kUrl1[] = "http://a.com/1.css";
static const char kUrl2[] = "http://b.com/2.css";
css_in.Add(kUrl1, kYellow, "", supply_mock);
css_in.Add(kUrl2, kBlue, "", supply_mock);
ResponseHeaders default_css_header;
@@ -769,62 +769,70 @@ TEST_F(CssCombineFilterWithDebugTest, IEDirectiveBarrier) {

TEST_F(CssCombineFilterTest, StyleBarrier) {
SetHtmlMimetype();
const char style_barrier[] = "<style>a { color: red }</style>\n";
static const char kStyleBarrier[] = "<style>a { color: red }</style>\n";
UseMd5Hasher();
CombineCss("combine_css_style", style_barrier, "", true);
CombineCss("combine_css_style", kStyleBarrier, "", true);
}

TEST_F(CssCombineFilterWithDebugTest, StyleBarrier) {
SetHtmlMimetype();
const char style_barrier[] = "<style>a { color: red }</style>\n";
static const char kStyleBarrier[] = "<style>a { color: red }</style>\n";
UseMd5Hasher();
CombineCss("combine_css_style", style_barrier,
CombineCss("combine_css_style", kStyleBarrier,
"<!--combine_css: Could not combine over barrier: inline style-->",
true);
}

TEST_F(CssCombineFilterTest, BogusLinkBarrier) {
SetHtmlMimetype();
const char bogus_barrier[] = "<link rel='stylesheet' "
static const char kBogusBarrier[] = "<link rel='stylesheet' "
"href='crazee://big/blue/fake' type='text/css'>\n";
UseMd5Hasher();
CombineCss("combine_css_bogus_link", bogus_barrier, "", true);
CombineCss("combine_css_bogus_link", kBogusBarrier, "", true);
}

TEST_F(CssCombineFilterWithDebugTest, BogusLinkBarrier) {
SetHtmlMimetype();
const char bogus_barrier[] = "<link rel='stylesheet' "
static const char kBogusBarrier[] = "<link rel='stylesheet' "
"href='crazee://big/blue/fake' type='text/css'>\n";
UseMd5Hasher();
CombineCss("combine_css_bogus_link", bogus_barrier,
CombineCss("combine_css_bogus_link", kBogusBarrier,
"<!--combine_css: Could not combine over barrier: "
"resource not rewritable-->", true);
}

TEST_F(CssCombineFilterTest, AlternateStylesheetBarrier) {
SetHtmlMimetype();
const char barrier[] =
static const char kBarrier[] =
"<link rel='alternate stylesheet' type='text/css' href='a.css'>";
UseMd5Hasher();
CombineCss("alternate_stylesheet_barrier", kBarrier, "", true);
}

TEST_F(CssCombineFilterWithDebugTest, AlternateStylesheetBarrier) {
SetHtmlMimetype();
static const char kBarrier[] =
"<link rel='alternate stylesheet' type='text/css' href='a.css'>";
UseMd5Hasher();
// TODO(sligocki): This should actually be a barrier: s/false/true/
// Add CssCombineFilterWithDebugTest version as well when it is.
CombineCss("alternate_stylesheet_barrier", barrier, "", false);
CombineCss("alternate_stylesheet_barrier", kBarrier,
"<!--combine_css: Could not combine over barrier: "
"custom or alternate stylesheet attribute-->", true);
}

TEST_F(CssCombineFilterTest, NonStandardAttributesBarrier) {
SetHtmlMimetype();
const char barrier[] =
static const char kBarrier[] =
"<link rel='stylesheet' type='text/css' href='a.css' foo='bar'>";
UseMd5Hasher();
CombineCss("non_standard_attributes_barrier", barrier, "", true);
CombineCss("non_standard_attributes_barrier", kBarrier, "", true);
}

TEST_F(CssCombineFilterWithDebugTest, NonStandardAttributesBarrier) {
SetHtmlMimetype();
const char barrier[] =
static const char kBarrier[] =
"<link rel='stylesheet' type='text/css' href='a.css' foo='bar'>";
UseMd5Hasher();
CombineCss("non_standard_attributes_barrier", barrier,
CombineCss("non_standard_attributes_barrier", kBarrier,
"<!--combine_css: Could not combine over barrier: "
"potentially non-combinable attribute: &#39;foo&#39;-->", true);
}
@@ -899,7 +907,7 @@ TEST_F(CssCombineFilterTest, StripBom) {

TEST_F(CssCombineFilterTest, StripBomReconstruct) {
// Make sure we strip the BOM properly when reconstructing, too.
const char kCssText[] = "div {background-image:url(fancy.png);}";
static const char kCssText[] = "div {background-image:url(fancy.png);}";
SetResponseWithDefaultHeaders(kCssA, kContentTypeCss,
StrCat(kUtf8Bom, kCssText),
300);
@@ -916,45 +924,45 @@ TEST_F(CssCombineFilterTest, StripBomReconstruct) {

TEST_F(CssCombineFilterTest, CombineCssWithNoscriptBarrier) {
SetHtmlMimetype();
const char noscript_barrier[] =
static const char kNoscriptBarrier[] =
"<noscript>\n"
" <link rel='stylesheet' href='d.css' type='text/css'>\n"
"</noscript>\n";

// Put this in the Test class to remove repetition here and below.
GoogleString d_css_url = StrCat(kDomain, "d.css");
const char d_css_body[] = ".c4 {\n color: green;\n}\n";
static const char kDCssBody[] = ".c4 {\n color: green;\n}\n";
ResponseHeaders default_css_header;
SetDefaultLongCacheHeaders(&kContentTypeCss, &default_css_header);
SetFetchResponse(d_css_url, default_css_header, d_css_body);
SetFetchResponse(d_css_url, default_css_header, kDCssBody);

UseMd5Hasher();
CombineCss("combine_css_noscript", noscript_barrier, "", true);
CombineCss("combine_css_noscript", kNoscriptBarrier, "", true);
}

TEST_F(CssCombineFilterTest, CombineCssWithFakeNoscriptBarrier) {
SetHtmlMimetype();
const char non_barrier[] =
static const char kNonBarrier[] =
"<noscript>\n"
" <p>You have no scripts installed</p>\n"
"</noscript>\n";
UseMd5Hasher();
CombineCss("combine_css_fake_noscript", non_barrier, "", false);
CombineCss("combine_css_fake_noscript", kNonBarrier, "", false);
}

TEST_F(CssCombineFilterTest, CombineCssWithMediaBarrier) {
SetHtmlMimetype();
const char media_barrier[] =
static const char kMediaBarrier[] =
"<link rel='stylesheet' href='d.css' type='text/css' media='print'>\n";

GoogleString d_css_url = StrCat(kDomain, "d.css");
const char d_css_body[] = ".c4 {\n color: green;\n}\n";
static const char kDCssBody[] = ".c4 {\n color: green;\n}\n";
ResponseHeaders default_css_header;
SetDefaultLongCacheHeaders(&kContentTypeCss, &default_css_header);
SetFetchResponse(d_css_url, default_css_header, d_css_body);
SetFetchResponse(d_css_url, default_css_header, kDCssBody);

UseMd5Hasher();
CombineCss("combine_css_media", media_barrier, "", true);
CombineCss("combine_css_media", kMediaBarrier, "", true);
}

TEST_F(CssCombineFilterTest, CombineCssWithNonMediaBarrier) {
@@ -967,14 +975,14 @@ TEST_F(CssCombineFilterTest, CombineCssWithNonMediaBarrier) {
GoogleString c_css_url = StrCat(kDomain, "c.css");
GoogleString d_css_url = StrCat(kDomain, "d.css");

const char d_css_body[] = ".c4 {\n color: green;\n}\n";
static const char kDCssBody[] = ".c4 {\n color: green;\n}\n";

ResponseHeaders default_css_header;
SetDefaultLongCacheHeaders(&kContentTypeCss, &default_css_header);
SetFetchResponse(a_css_url, default_css_header, kACssBody);
SetFetchResponse(b_css_url, default_css_header, kBCssBody);
SetFetchResponse(c_css_url, default_css_header, kCCssBody);
SetFetchResponse(d_css_url, default_css_header, d_css_body);
SetFetchResponse(d_css_url, default_css_header, kDCssBody);

// Only the first two CSS files should be combined.
GoogleString html_input(StrCat(
@@ -1127,14 +1135,14 @@ TEST_F(CssCombineFilterTest, CombineCssNoInput) {
SetDefaultLongCacheHeaders(&kContentTypeCss, &default_css_header);
SetFetchResponse(StrCat(kTestDomain, kCssB),
default_css_header, ".a {}");
static const char html_input[] =
static const char kHtmlInput[] =
"<head>\n"
" <link rel='stylesheet' href='a_broken.css' type='text/css'>\n"
" <link rel='stylesheet' href='b.css' type='text/css'>\n"
"</head>\n"
"<body><div class='c1'><div class='c2'><p>\n"
" Yellow on Blue</p></div></div></body>";
ValidateNoChanges("combine_css_missing_input", html_input);
ValidateNoChanges("combine_css_missing_input", kHtmlInput);
}

TEST_F(CssCombineFilterTest, CombineCssXhtml) {
@@ -1748,7 +1756,7 @@ class CssFilterWithCombineTest : public CssCombineFilterTest {
GoogleString OptimizedContent() { return "div{}div{}"; }

void SetupResources() {
const char kCssText[] = " div { } ";
static const char kCssText[] = " div { } ";
SetResponseWithDefaultHeaders(kCssA, kContentTypeCss, kCssText, 300);
SetResponseWithDefaultHeaders(kCssB, kContentTypeCss, kCssText, 300);
}
@@ -1832,8 +1840,8 @@ TEST_F(CssFilterWithCombineTestUrlNamer, TestFollowCombine) {
// A verbatim copy of the test above but using TestUrlNamer.
const GoogleString kCssOut =
EncodeCssCombineAndOptimize(kTestDomain, MultiUrl(kCssA, kCssB));
const char kCssText[] = " div { } ";
const char kCssTextOptimized[] = "div{}";
static const char kCssText[] = " div { } ";
static const char kCssTextOptimized[] = "div{}";

SetResponseWithDefaultHeaders(kCssA, kContentTypeCss, kCssText, 300);
SetResponseWithDefaultHeaders(kCssB, kContentTypeCss, kCssText, 300);
@@ -2031,24 +2039,24 @@ TEST_F(CollapseWhitespaceGeneralTest, CollapseAfterCombine) {
default_css_header, ".c { color: blue; }");

// Before and expected after text.
const char before[] =
static const char kBefore[] =
"<html>\n"
" <head>\n"
" <link rel=stylesheet type=text/css href=a.css>\n"
" <link rel=stylesheet type=text/css href=b.css>\n"
" <link rel=stylesheet type=text/css href=c.css>\n"
" </head>\n"
"</html>\n";
const char after_template[] =
static const char kAfterTemplate[] =
"<html>\n"
"<head>\n"
"<link rel=stylesheet type=text/css href=%s />\n"
"</head>\n"
"</html>\n";
GoogleString after = StringPrintf(after_template, Encode(
GoogleString after = StringPrintf(kAfterTemplate, Encode(
"", "cc", "0", MultiUrl(kCssA, kCssB, "c.css"), "css").c_str());

ValidateExpected("collapse_after_combine", before, after);
ValidateExpected("collapse_after_combine", kBefore, after);
}

/*
@@ -46,6 +46,7 @@
#include "net/instaweb/rewriter/public/request_properties.h"
#include "net/instaweb/rewriter/public/resource.h"
#include "net/instaweb/rewriter/public/resource_slot.h"
#include "net/instaweb/rewriter/public/resource_tag_scanner.h"
#include "net/instaweb/rewriter/public/rewrite_context.h"
#include "net/instaweb/rewriter/public/rewrite_driver.h"
#include "net/instaweb/rewriter/public/rewrite_options.h"
@@ -997,22 +998,20 @@ void CssFilter::EndElementImpl(HtmlElement* element) {
if (in_style_element_) {
CHECK(style_element_ == element); // HtmlParse should not pass unmatching.
in_style_element_ = false;

// Rewrite an external style.
} else if (element->keyword() == HtmlName::kLink &&
driver()->IsRewritable(element)) {
if (CssTagScanner::IsStylesheetOrAlternate(
element->AttributeValue(HtmlName::kRel))) {
HtmlElement::Attribute* element_href = element->FindAttribute(
HtmlName::kHref);
if (element_href != NULL) {
// If it has a href= attribute
StartExternalRewrite(element, element_href);
}
if (driver()->IsRewritable(element)) {
resource_tag_scanner::UrlCategoryVector attributes;
resource_tag_scanner::ScanElement(
element, driver()->options(), &attributes);
resource_tag_scanner::UrlCategoryVector::iterator uc;
for (uc = attributes.begin(); uc != attributes.end(); uc++) {
if (uc->category == semantic_type::kStylesheet) {
StartExternalRewrite(element, uc->url);
}
}
// Note any meta tag charset specifier.
} else if (meta_tag_charset_.empty() &&
element->keyword() == HtmlName::kMeta) {
}
if (meta_tag_charset_.empty() && element->keyword() == HtmlName::kMeta) {
// Note any meta tag charset specifier.
GoogleString content, mime_type, charset;
if (ExtractMetaTagDetails(*element, NULL, &content, &mime_type, &charset)) {
meta_tag_charset_ = charset;
@@ -200,6 +200,15 @@ semantic_type::Category CategorizeAttribute(
StringCaseEqual(attribute->name_str(), attribute_i)) {
return category_i;
}
if (element->keyword() == HtmlName::kStyle) {
// When inlining we turn <link> into <style> while preserving attributes.
// This means that any custom attributes defined for <link> should also be
// interpreted on <style>.
if (StringCaseEqual("link", element_i) &&
StringCaseEqual(attribute->name_str(), attribute_i)) {
return category_i;
}
}
}

// Handle spec-defined attributes.
Oops, something went wrong.

0 comments on commit 59a1982

Please sign in to comment.