Move lazyloader script load to end of head (or just before the first …

…non-html tag) rather than before the first image on the page. Also inserts the script pretty-much unconditionally (if we see the jquery slider script in head we don't bother, but merely having no images doesn't prevent script insertion).
apache · Jun 5, 2015 · f01252e8ebea981610f3bbe969eec2b9a11235ed · f01252e
1 parent d6d597e
commit f01252e8ebea981610f3bbe969eec2b9a11235ed
Unified Split

Showing with 112 additions and 49 deletions.

+41 −3 net/instaweb/rewriter/lazyload_images_filter.cc

+66 −43 net/instaweb/rewriter/lazyload_images_filter_test.cc

+5 −3 net/instaweb/rewriter/public/lazyload_images_filter.h
diff --git a/net/instaweb/rewriter/lazyload_images_filter.cc b/net/instaweb/rewriter/lazyload_images_filter.cc
@@ -18,6 +18,7 @@
 
 #include "net/instaweb/rewriter/public/lazyload_images_filter.h"
 
+#include "base/logging.h"
 #include "net/instaweb/htmlparse/public/html_element.h"
 #include "net/instaweb/htmlparse/public/html_name.h"
 #include "net/instaweb/htmlparse/public/html_node.h"
@@ -82,6 +83,8 @@ void LazyloadImagesFilter::StartDocumentImpl() {
 }
 
 void LazyloadImagesFilter::EndDocument() {
+  // TODO(jmaessen): Fix filter to insert this script
+  // conditionally.
   driver()->UpdatePropertyValueInDomCohort(
       driver()->fallback_property_page(),
       kIsLazyloadScriptInsertedPropertyName,
@@ -90,6 +93,7 @@ void LazyloadImagesFilter::EndDocument() {
 
 void LazyloadImagesFilter::Clear() {
   skip_rewrite_ = NULL;
+  head_element_ = NULL;
   main_script_inserted_ = false;
   abort_rewrite_ = false;
   abort_script_inserted_ = false;
@@ -125,6 +129,22 @@ void LazyloadImagesFilter::StartElementImpl(HtmlElement* element) {
   if (noscript_element() != NULL) {
     return;
   }
+  if (!main_script_inserted_ && head_element_ == NULL) {
+    switch (element->keyword()) {
+      case HtmlName::kHtml:
+      case HtmlName::kLink:
+      case HtmlName::kMeta:
+      case HtmlName::kScript:
+      case HtmlName::kStyle:
+        break;
+      case HtmlName::kHead:
+        head_element_ = element;
+        break;
+      default:
+        InsertLazyloadJsCode(element);
+        break;
+    }
+  }
   if (skip_rewrite_ == NULL) {
     if (element->keyword() == HtmlName::kNoembed ||
         element->keyword() == HtmlName::kMarquee) {
@@ -171,8 +191,13 @@ void LazyloadImagesFilter::EndElementImpl(HtmlElement* element) {
     }
     return;
   }
+  if (head_element_ == element) {
+    InsertLazyloadJsCode(NULL);
+    head_element_ = NULL;
+  }
   if (abort_rewrite_) {
-    if (!abort_script_inserted_ && main_script_inserted_) {
+    if (!abort_script_inserted_ && main_script_inserted_ &&
+        num_images_lazily_loaded_ > 0) {
       // If we have already rewritten some elements on the page, insert a
       // script to load all previously rewritten images.
       HtmlElement* script = driver()->NewElement(element, HtmlName::kScript);
@@ -281,9 +306,22 @@ void LazyloadImagesFilter::EndElementImpl(HtmlElement* element) {
 }
 
 void LazyloadImagesFilter::InsertLazyloadJsCode(HtmlElement* element) {
-  if (!driver()->is_lazyload_script_flushed()) {
+  if (!driver()->is_lazyload_script_flushed() &&
+      (!abort_rewrite_ || num_images_lazily_loaded_ > 0)) {
     HtmlElement* script = driver()->NewElement(element, HtmlName::kScript);
-    driver()->InsertNodeBeforeNode(element, script);
+    if (element != NULL) {
+      driver()->InsertNodeBeforeNode(element, script);
+    } else if (driver()->CanAppendChild(head_element_)) {
+      // insert at end of head.
+      driver()->AppendChild(head_element_, script);
+    } else {
+      // Could not insert at end of head even though we just saw the end of head
+      // event!  Should not happen, but this will ensure that we insert the
+      // script before the next tag we see.
+      LOG(DFATAL) << "Can't append child to <head> at the </head> event!";
+      main_script_inserted_ = false;
+      return;
+    }
     StaticAssetManager* static_asset_manager =
         driver()->server_context()->static_asset_manager();
     GoogleString lazyload_js = GetLazyloadJsSnippet(

diff --git a/net/instaweb/rewriter/lazyload_images_filter_test.cc b/net/instaweb/rewriter/lazyload_images_filter_test.cc
@@ -90,7 +90,8 @@ class LazyloadImagesFilterTest : public RewriteTestBase {
 TEST_F(LazyloadImagesFilterTest, SingleHead) {
   InitLazyloadImagesFilter(false);
 
-  ValidateExpected("lazyload_images",
+  ValidateExpected(
+      "lazyload_images",
       "<head></head>"
       "<body>"
       "<img />"
@@ -114,7 +115,9 @@ TEST_F(LazyloadImagesFilterTest, SingleHead) {
       "<img src=\"1.jpg\" onload=\"blah();\" />"
       "<img src=\"1.jpg\" class=\"123 dfcg-metabox\" />"
       "</body>",
-      StrCat("<head></head><body><img/>"
+      StrCat("<head>",
+             GetLazyloadScriptHtml(),
+             "</head><body><img/>"
              "<img src=\"\"/>"
              "<noscript>"
              "<img src=\"noscript.jpg\"/>"
@@ -125,7 +128,6 @@ TEST_F(LazyloadImagesFilterTest, SingleHead) {
              "<marquee>"
              "<img src=\"marquee.jpg\"/>"
              "</marquee>",
-             GetLazyloadScriptHtml(),
              GenerateRewrittenImageTag("img", "1.jpg", ""),
              "<img src=\"1.jpg\" pagespeed_no_defer />"
              "<img src=\"1.jpg\" data-src=\"2.jpg\"/>",
@@ -167,16 +169,16 @@ TEST_F(LazyloadImagesFilterTest, Blacklist) {
   ValidateExpected(
       "lazyload_images",
       input_html,
-      StrCat("<head></head><body>"
-             "<img src=\"http://www.1.com/blacklist.jpg\"/>",
+      StrCat("<head>",
              GetLazyloadScriptHtml(),
-             StrCat(
-                 GenerateRewrittenImageTag(
-                     "img", "http://www.1.com/img1", ""),
-                 GenerateRewrittenImageTag(
-                     "img", "img2", ""),
-                 GetLazyloadPostscriptHtml(),
-                 "</body>")));
+             "</head><body>"
+             "<img src=\"http://www.1.com/blacklist.jpg\"/>",
+             GenerateRewrittenImageTag(
+                 "img", "http://www.1.com/img1", ""),
+             GenerateRewrittenImageTag(
+                 "img", "img2", ""),
+             GetLazyloadPostscriptHtml(),
+             "</body>"));
   EXPECT_EQ(3, logging_info()->rewriter_info().size());
   ExpectLogRecord(0, RewriterApplication::NOT_APPLIED, true, false);
   ExpectLogRecord(1, RewriterApplication::APPLIED_OK, false, false);
@@ -210,16 +212,16 @@ TEST_F(LazyloadImagesFilterTest, CriticalImages) {
   ValidateExpected(
       "lazyload_images",
       input_html,
-      StrCat("<head></head><body>"
-             "<img src=\"http://www.1.com/critical\"/>",
+      StrCat("<head>",
              GetLazyloadScriptHtml(),
-             StrCat(
-                 GenerateRewrittenImageTag(
-                     "img", "http://www.1.com/critical2", ""),
-                 "<img src=\"critical3\"/>"
-                 "<img src=\"", rewritten_url, "\"/>",
-                 GetLazyloadPostscriptHtml(),
-                 "</body>")));
+             "</head><body>"
+             "<img src=\"http://www.1.com/critical\"/>",
+             GenerateRewrittenImageTag(
+                 "img", "http://www.1.com/critical2", ""),
+             "<img src=\"critical3\"/>"
+             "<img src=\"", rewritten_url, "\"/>",
+             GetLazyloadPostscriptHtml(),
+             "</body>"));
   EXPECT_EQ(4, logging_info()->rewriter_info().size());
   ExpectLogRecord(0, RewriterApplication::NOT_APPLIED, false, true);
   ExpectLogRecord(1, RewriterApplication::APPLIED_OK, false, false);
@@ -252,14 +254,16 @@ TEST_F(LazyloadImagesFilterTest, CriticalImages) {
 TEST_F(LazyloadImagesFilterTest, SingleHeadLoadOnOnload) {
   options()->set_lazyload_images_after_onload(true);
   InitLazyloadImagesFilter(false);
-  ValidateExpected("lazyload_images",
+  ValidateExpected(
+      "lazyload_images",
       "<head></head>"
       "<body>"
       "<img src=\"1.jpg\" />"
       "</body>",
-      StrCat("<head></head>"
-             "<body>",
+      StrCat("<head>",
              GetLazyloadScriptHtml(),
+             "</head>"
+             "<body>",
              GenerateRewrittenImageTag("img", "1.jpg", ""),
              GetLazyloadPostscriptHtml(),
              "</body>"));
@@ -271,31 +275,36 @@ TEST_F(LazyloadImagesFilterTest, SingleHeadLoadOnOnload) {
 // not an onload attribute added by PageSpeed.
 TEST_F(LazyloadImagesFilterTest, NoLazyloadImagesWithOnloadAttribute) {
   InitLazyloadImagesFilter(false);
-  ValidateExpected("lazyload_images",
+  ValidateExpected(
+      "lazyload_images",
       "<head></head>"
       "<body>"
       "<img src=\"1.jpg\" onload=\"do_something();\"/>"
       "</body>",
-      "<head></head>"
-      "<body>"
-      "<img src=\"1.jpg\" onload=\"do_something();\"/>"
-      "</body>");
+      StrCat("<head>",
+             GetLazyloadScriptHtml(),
+             "</head>"
+             "<body>"
+             "<img src=\"1.jpg\" onload=\"do_something();\"/>"
+             "</body>"));
 }
 
 // Verify that lazyload_images gets applied on image elements that have an
 // onload handler whose value is CriticalImagesBeaconFilter::kImageOnloadCode.
 TEST_F(LazyloadImagesFilterTest, LazyloadWithPagespeedAddedOnloadAttribute) {
   InitLazyloadImagesFilter(false);
-  ValidateExpected("lazyload_images",
+  ValidateExpected(
+      "lazyload_images",
       StrCat("<head></head>"
              "<body>"
              "<img src=\"1.jpg\" onload=\"",
              CriticalImagesBeaconFilter::kImageOnloadCode,
              "\"/>"
              "</body>"),
-      StrCat("<head></head>"
-             "<body>",
+      StrCat("<head>",
              GetLazyloadScriptHtml(),
+             "</head>"
+             "<body>",
              GenerateRewrittenImageTag("img", "1.jpg", ""),
              GetLazyloadPostscriptHtml(),
              "</body>"));
@@ -314,8 +323,8 @@ TEST_F(LazyloadImagesFilterTest, MultipleBodies) {
       "<script></script>"
       "</body>",
       StrCat(
-          "<body>",
           GetLazyloadScriptHtml(),
+          "<body>",
           GenerateRewrittenImageTag("img", "1.jpg", ""),
           GetLazyloadPostscriptHtml(),
           StrCat(
@@ -337,8 +346,8 @@ TEST_F(LazyloadImagesFilterTest, NoHeadTag) {
       "<body>"
       "<img src=\"1.jpg\" />"
       "</body>",
-      StrCat("<body>",
-             GetLazyloadScriptHtml(),
+      StrCat(GetLazyloadScriptHtml(),
+             "<body>",
              GenerateRewrittenImageTag("img", "1.jpg", ""),
              GetLazyloadPostscriptHtml(),
              "</body>"));
@@ -367,8 +376,8 @@ TEST_F(LazyloadImagesFilterTest, CustomImageUrl) {
       "<body>"
       "<img src=\"1.jpg\" />"
       "</body>",
-      StrCat("<body>",
-             GetLazyloadScriptHtml(),
+      StrCat(GetLazyloadScriptHtml(),
+             "<body>",
              GenerateRewrittenImageTag("img", "1.jpg", ""),
              GetLazyloadPostscriptHtml(),
              "</body>"));
@@ -382,7 +391,8 @@ TEST_F(LazyloadImagesFilterTest, DfcgClass) {
       "<img src=\"1.jpg\"/>"
       "</div>"
       "</body>";
-  ValidateNoChanges("lazyload_images", input_html);
+  ValidateExpected("DfcgClass",
+                   input_html, StrCat(GetLazyloadScriptHtml(), input_html));
 }
 
 TEST_F(LazyloadImagesFilterTest, NivoClass) {
@@ -393,7 +403,8 @@ TEST_F(LazyloadImagesFilterTest, NivoClass) {
       "</div>"
       "<img class=\"nivo\" src=\"1.jpg\"/>"
       "</body>";
-  ValidateNoChanges("lazyload_images", input_html);
+  ValidateExpected("NivoClass",
+                   input_html, StrCat(GetLazyloadScriptHtml(), input_html));
 }
 
 TEST_F(LazyloadImagesFilterTest, ClassContainsSlider) {
@@ -404,13 +415,16 @@ TEST_F(LazyloadImagesFilterTest, ClassContainsSlider) {
       "</div>"
       "<img class=\"my_sLiDer\" src=\"1.jpg\"/>"
       "</body>";
-  ValidateNoChanges("lazyload_images", input_html);
+  ValidateExpected("SliderClass",
+                   input_html, StrCat(GetLazyloadScriptHtml(), input_html));
 }
 
 TEST_F(LazyloadImagesFilterTest, NoImages) {
   InitLazyloadImagesFilter(false);
   GoogleString input_html = "<head></head><body></body>";
-  ValidateNoChanges("lazyload_images", input_html);
+  ValidateExpected("NoImages", input_html,
+                   StrCat("<head>", GetLazyloadScriptHtml(),
+                          "</head><body></body>"));
   EXPECT_EQ(0, logging_info()->rewriter_info().size());
 }
 
@@ -440,7 +454,8 @@ TEST_F(LazyloadImagesFilterTest, LazyloadDisabledWithJquerySlider) {
       "<img src=\"1.jpg\"/>"
       "</body>";
   // No change in the html.
-  ValidateNoChanges("lazyload_images", input_html);
+  ValidateExpected("JQuerySlider", input_html,
+                   StrCat(GetLazyloadScriptHtml(), input_html));
 }
 
 TEST_F(LazyloadImagesFilterTest, LazyloadDisabledWithJquerySliderAfterHead) {
@@ -451,7 +466,15 @@ TEST_F(LazyloadImagesFilterTest, LazyloadDisabledWithJquerySliderAfterHead) {
       "<script src=\"jquery.sexyslider.js\"/>"
       "<img src=\"1.jpg\"/>"
       "</body>";
-  ValidateNoChanges("abort_script_inserted", input_html);
+  GoogleString expected_html = StrCat(
+      "<head>",
+      GetLazyloadScriptHtml(),
+      "</head>"
+      "<body>"
+      "<script src=\"jquery.sexyslider.js\"/>"
+      "<img src=\"1.jpg\"/>"
+      "</body>");
+  ValidateExpected("abort_script_inserted", input_html, expected_html);
 }
 
 TEST_F(LazyloadImagesFilterTest, LazyloadDisabledForOldBlackberry) {

diff --git a/net/instaweb/rewriter/public/lazyload_images_filter.h b/net/instaweb/rewriter/public/lazyload_images_filter.h
@@ -19,15 +19,15 @@
 #ifndef NET_INSTAWEB_REWRITER_PUBLIC_LAZYLOAD_IMAGES_FILTER_H_
 #define NET_INSTAWEB_REWRITER_PUBLIC_LAZYLOAD_IMAGES_FILTER_H_
 
+#include "net/instaweb/htmlparse/public/html_element.h"
 #include "net/instaweb/rewriter/public/common_filter.h"
+#include "net/instaweb/rewriter/public/rewrite_driver.h"
+#include "net/instaweb/rewriter/public/rewrite_options.h"
 #include "net/instaweb/util/enums.pb.h"
 #include "net/instaweb/util/public/string.h"
 
 namespace net_instaweb {
 
-class HtmlElement;
-class RewriteDriver;
-class RewriteOptions;
 class StaticAssetManager;
 class Statistics;
 
@@ -121,6 +121,8 @@ class LazyloadImagesFilter : public CommonFilter {
   // If non-NULL, we skip rewriting till we reach
   // LazyloadImagesFilter::EndElement(skip_rewrite_).
   HtmlElement* skip_rewrite_;
+  // Head element - preferred insertion point for scripts.
+  HtmlElement* head_element_;
   // Indicates if the main javascript has been inserted into the page.
   bool main_script_inserted_;
   // Indicates whether we should abort rewriting the page.