Don't store webp-animated and non-webp capable image opts under same …

…cache key. Change the non-webp cache key to avoid picking up any that we previously messed up. Also restructure the code a bit to lower the risk of this occuring again. Part of issue #1216
apache · Dec 15, 2015 · e398794668f487f55db5c74baba286c708f7e0ee · e398794
1 parent 0f77262
commit e398794668f487f55db5c74baba286c708f7e0ee
Unified Split

Showing with 58 additions and 8 deletions.

+22 −0 net/instaweb/rewriter/image_rewrite_filter_test.cc

+17 −7 net/instaweb/rewriter/image_url_encoder.cc

+19 −1 net/instaweb/rewriter/image_url_encoder_test.cc
diff --git a/net/instaweb/rewriter/image_rewrite_filter_test.cc b/net/instaweb/rewriter/image_rewrite_filter_test.cc
@@ -3740,4 +3740,26 @@ TEST_F(ImageRewriteTest, GifToWebpLosslessWithWebpAnimatedUa) {
                          true);
 }
+  
+TEST_F(ImageRewriteTest, AnimatedNoCacheReuse) {
+  
+  // Make sure we don't reuse results for animated webp-capable UAs for
+  
+  // non-webp targets.
+  
+  AddFileToMockFetcher(StrCat(kTestDomain, "a.jpeg"), kPuzzleJpgFile,
+  
+                       kContentTypeJpeg, 100);
+  
+
+  
+  options()->EnableFilter(RewriteOptions::kConvertJpegToWebp);
+  
+  options()->EnableFilter(RewriteOptions::kConvertToWebpAnimated);
+  
+  options()->set_image_recompress_quality(85);
+  
+  rewrite_driver()->AddFilters();
+  
+
+  
+  // WebP capable browser --- made a WebP image.
+  
+  SetCurrentUserAgent("webp-animated");
+  
+  ValidateExpected("webp broswer", "<img src=a.jpeg>",
+  
+                   "<img src=xa.jpeg.pagespeed.ic.0.webp>");
+  
+  ClearRewriteDriver();
+  
+
+  
+  // Not a WebP browser -- don't!
+  
+  SetCurrentUserAgent("curl");
+  
+  ValidateNoChanges("non-webp broswer", "<img src=a.jpeg>");
+  
+}
+  
+
 }  // namespace net_instaweb
diff --git a/net/instaweb/rewriter/image_url_encoder.cc b/net/instaweb/rewriter/image_url_encoder.cc
@@ -42,6 +42,10 @@ const char kMissingDimension = 'N';
 // Constants for UserAgent cache key enteries.
 const char kWebpLossyUserAgentKey[] = "w";
 const char kWebpLossyLossLessAlphaUserAgentKey[] = "v";
+  
+const char kWebpAnimatedUserAgentKey[] = "a";
+  
+// This used to not have a separate key, but we mixed up animated and it
+  
+// at one point, so this is now here to force a flush.
+  
+const char kWebpNoneUserAgentKey[] = ".";
 const char kMobileUserAgentKey[] = "m";
 const char kSmallScreenKey[] = "ss";
@@ -305,13 +309,19 @@ void ImageUrlEncoder::SetSmallScreen(const RewriteDriver& driver,
 GoogleString ImageUrlEncoder::CacheKeyFromResourceContext(
    const ResourceContext& resource_context) {
  GoogleString user_agent_cache_key = "";
-  
-  if (resource_context.libwebp_level() ==
-  
-      ResourceContext::LIBWEBP_LOSSY_LOSSLESS_ALPHA) {
-  
-    StrAppend(&user_agent_cache_key, kWebpLossyLossLessAlphaUserAgentKey);
-  
-  }
-  
-  if (resource_context.libwebp_level() ==
-  
-      ResourceContext::LIBWEBP_LOSSY_ONLY) {
-  
-    StrAppend(&user_agent_cache_key, kWebpLossyUserAgentKey);
+  
+  switch (resource_context.libwebp_level()) {
+  
+    case ResourceContext::LIBWEBP_NONE:
+  
+      StrAppend(&user_agent_cache_key, kWebpNoneUserAgentKey);
+  
+      break;
+  
+    case ResourceContext::LIBWEBP_LOSSY_LOSSLESS_ALPHA:
+  
+      StrAppend(&user_agent_cache_key, kWebpLossyLossLessAlphaUserAgentKey);
+  
+      break;
+  
+    case ResourceContext::LIBWEBP_LOSSY_ONLY:
+  
+      StrAppend(&user_agent_cache_key, kWebpLossyUserAgentKey);
+  
+      break;
+  
+    case ResourceContext::LIBWEBP_ANIMATED:
+  
+      StrAppend(&user_agent_cache_key, kWebpAnimatedUserAgentKey);
+  
+      break;
  }
  if (resource_context.mobile_user_agent()) {
    StrAppend(&user_agent_cache_key, kMobileUserAgentKey);
diff --git a/net/instaweb/rewriter/image_url_encoder_test.cc b/net/instaweb/rewriter/image_url_encoder_test.cc
@@ -363,7 +363,25 @@ TEST_F(ImageUrlEncoderTest, SmallScreen) {
  context.set_use_small_screen_quality(true);
  GoogleString cache_key =
      ImageUrlEncoder::CacheKeyFromResourceContext(context);
-  
-  EXPECT_EQ("ss", cache_key);
+  
+  EXPECT_EQ(".ss", cache_key);
+  
+}
+  
+
+  
+TEST_F(ImageUrlEncoderTest, DifferentWebpLevels) {
+  
+  // Make sure different levels of WebP support get different cache keys.
+  
+  std::set<GoogleString> seen;
+  
+  for (int webp_level = ResourceContext::LibWebpLevel_MIN;
+  
+       webp_level <= ResourceContext::LibWebpLevel_MAX;
+  
+       ++webp_level) {
+  
+    if (ResourceContext::LibWebpLevel_IsValid(webp_level)) {
+  
+      ResourceContext ctx;
+  
+      ctx.set_libwebp_level(
+  
+          static_cast<ResourceContext::LibWebpLevel>(webp_level));
+  
+      GoogleString cache_key =
+  
+          ImageUrlEncoder::CacheKeyFromResourceContext(ctx);
+  
+      EXPECT_EQ(seen.find(cache_key), seen.end());
+  
+      seen.insert(cache_key);
+  
+    }
+  
+  }
 }
 TEST_F(ImageUrlEncoderTest, BadFirst) {