Make hint_preload_subresources work correctly when turned on by query params, too ---

by always registering cohort, and deciding whether we need it at read time. Also add the corresponding example page and integration test. Fix it on nginx by adding a separate hook for post-property-cache init, as its actually not ready in StartParse w/ProxyFetch (while it is with Apache) Also remove some needless quoting that was pointed out in review.
2016-10-04 13:15:21 -04:00
parent f0ce01f740
commit 0fb90651b2
17 changed files with 122 additions and 43 deletions
@@ -0,0 +1,12 @@
+<html>
+  <head>
+    <title>hint_preload_subresources example</title>
+    <link rel="stylesheet" type="text/css" href="styles/all_using_imports.css">
+  </head>
+  <body>
+    <img src="./images/Puzzle.jpg">
+    <p><script src="inline_javascript.js"></script>
+    if hint_preload_subresources is on, this page should get Link: headers
+    describing stylesheets and JavaScript it uses (but not the image).
+  </body>
+</html>
@@ -366,6 +366,27 @@
  </td>
 </tr>

+<tr class="filter_row">
+  <td class="code">
+    <a href="https://developers.google.com/speed/pagespeed/module/filter-hint-preload-subresources">
+      hint_preload_subresources
+    </a>
+  </td>
+  <td class="desc">
+    Inserts link: headers to preload CSS and JavaScript resources.
+  </td>
+  <td class="before">
+    <a href="hint_preload_subresources.html?PageSpeed=off">
+      before
+    </a>
+  </td>
+  <td class="after">
+    <a href="hint_preload_subresources.html?PageSpeed=on&amp;PageSpeedFilters=hint_preload_subresources">
+      after
+    </a>
+  </td>
+</tr>
+
 <tr class="filter_row">
  <td class="code">
    <a href="https://developers.google.com/speed/pagespeed/module/filter-css-inline">
@@ -79,6 +79,7 @@ class CollectDependenciesFilterTest : public RewriteTestBase {
    page_ = NewMockPage(kRequestUrl);
    rewrite_driver()->set_property_page(page_);
    pcache_->Read(page_);
+    rewrite_driver()->PropertyCacheSetupDone();
    SetHtmlMimetype();  // Don't wrap scripts in <![CDATA[ ]]>
  }

@@ -58,6 +58,7 @@ class DependencyTrackerTest : public RewriteTestBase {
    page_ = NewMockPage(kRequestUrl);
    rewrite_driver()->set_property_page(page_);
    pcache_->Read(page_);
+    rewrite_driver()->PropertyCacheSetupDone();
  }

  void TestInRewriteDriver(bool actually_enabled) {
@@ -227,6 +227,16 @@ class RewriteDriver : public HtmlParse {
    write_property_cache_dom_cohort_ = x;
  }

+  // Returns the list of cohorts that should be read in based on
+  // our options.
+  static PropertyCache::CohortVector GetCohortList(
+      const PropertyCache* pcache, const RewriteOptions* options,
+      const ServerContext* server_context);
+
+  // Should be called once everything in the property cache has been read,
+  // and the pages set on the object.
+  void PropertyCacheSetupDone();
+
  RequestContextPtr request_context() { return request_context_; }
  void set_request_context(const RequestContextPtr& x);

@@ -100,7 +100,7 @@ void PushPreloadFilter::StartDocumentImpl() {
        dep_url.Relativize(kAbsolutePath, driver()->google_url());

    GoogleString link_val =
-        StrCat("<", GoogleUrl::Sanitize(rel_url), ">; rel=\"preload\"");
+        StrCat("<", GoogleUrl::Sanitize(rel_url), ">; rel=preload");

    switch (dep.content_type()) {
      case DEP_JAVASCRIPT:
@@ -64,6 +64,7 @@ class PushPreloadFilterTest : public RewriteTestBase {
    page_ = NewMockPage(kRequestUrl);
    rewrite_driver()->set_property_page(page_);
    pcache_->Read(page_);
+    rewrite_driver()->PropertyCacheSetupDone();
    SetHtmlMimetype();  // Don't wrap scripts in <![CDATA[ ]]>
  }

@@ -109,10 +110,10 @@ TEST_F(PushPreloadFilterTest, BasicOperation) {

  ASSERT_EQ(2, links.size());
  EXPECT_STREQ(
-      "</A.a.css.pagespeed.cf.0.css>; rel=\"preload\"; as=style; nopush",
+      "</A.a.css.pagespeed.cf.0.css>; rel=preload; as=style; nopush",
      *links[0]);
  EXPECT_STREQ(
-      "</b.js.pagespeed.jm.0.js>; rel=\"preload\"; as=script; nopush",
+      "</b.js.pagespeed.jm.0.js>; rel=preload; as=script; nopush",
      *links[1]);
 }

@@ -142,7 +143,7 @@ TEST_F(PushPreloadFilterTest, Invalidation) {

  // Only b.js should be pushed --- or rather the .pagespeed version.
  ASSERT_EQ(1, links.size());
-  EXPECT_STREQ("</b.js.pagespeed.jm.0.js>; rel=\"preload\"; as=script; nopush",
+  EXPECT_STREQ("</b.js.pagespeed.jm.0.js>; rel=preload; as=script; nopush",
               *links[0]);
 }

@@ -231,22 +232,22 @@ TEST_F(PushPreloadFilterTest, IndirectCollected) {
  // These should be in preorder wrt to the dependencies between
  // CSS and things in it
  EXPECT_STREQ(
-        "</A.c.css.pagespeed.cf.0.css>; rel=\"preload\"; as=style; nopush",
+        "</A.c.css.pagespeed.cf.0.css>; rel=preload; as=style; nopush",
        *links[0]);
  EXPECT_STREQ(
-        "</i1.css>; rel=\"preload\"; as=style; nopush",
+        "</i1.css>; rel=preload; as=style; nopush",
        *links[1]);
  EXPECT_STREQ(
-        "</i2.css>; rel=\"preload\"; as=style; nopush",
+        "</i2.css>; rel=preload; as=style; nopush",
        *links[2]);
  EXPECT_STREQ(
-        "</A.d.css.pagespeed.cf.0.css>; rel=\"preload\"; as=style; nopush",
+        "</A.d.css.pagespeed.cf.0.css>; rel=preload; as=style; nopush",
        *links[3]);
  // not i3, since it's print only.

  // i1 already hinted.
  // i4 isn't, though.
-  EXPECT_STREQ("</i4.css>; rel=\"preload\"; as=style; nopush", *links[4]);
+  EXPECT_STREQ("</i4.css>; rel=preload; as=style; nopush", *links[4]);
 }

 }  // namespace
@@ -940,6 +940,23 @@ void RewriteDriver::SetServerContext(ServerContext* server_context)
  url_trim_filter_.reset(new UrlLeftTrimFilter(this, statistics()));
 }

+PropertyCache::CohortVector RewriteDriver::GetCohortList(
+    const PropertyCache* pcache, const RewriteOptions* options,
+    const ServerContext* server_context) {
+  bool need_deps = options->NeedsDependenciesCohort();
+  PropertyCache::CohortVector filtered_cohorts;
+  for (const PropertyCache::Cohort* cohort : pcache->GetAllCohorts()) {
+    if (need_deps || cohort != server_context->dependencies_cohort()) {
+      filtered_cohorts.push_back(cohort);
+    }
+  }
+  return filtered_cohorts;
+}
+
+void RewriteDriver::PropertyCacheSetupDone() {
+  dependency_tracker_->Start();
+}
+
 RequestTrace* RewriteDriver::trace_context() {
  return request_context_.get() == NULL ? NULL :
      request_context_->root_trace_context();
@@ -2316,8 +2333,6 @@ bool RewriteDriver::StartParseId(const StringPiece& url, const StringPiece& id,
  start_time_ms_ = server_context_->timer()->NowMs();
  set_log_rewrite_timing(options()->log_rewrite_timing());

-  dependency_tracker_->Start();
-
  if (debug_filter_ != NULL) {
    debug_filter_->InitParse();
  }
@@ -295,7 +295,8 @@ void InstawebContext::ComputeContentEncoding(request_rec* request) {

 void InstawebContext::BlockingPropertyCacheLookup() {
  PropertyCallback* property_callback = NULL;
-  if (server_context_->page_property_cache()->enabled()) {
+  PropertyCache* pcache = server_context_->page_property_cache();
+  if (pcache->enabled()) {
    const UserAgentMatcher* user_agent_matcher =
        server_context_->user_agent_matcher();
    UserAgentMatcher::DeviceType device_type =
@@ -303,13 +304,18 @@ void InstawebContext::BlockingPropertyCacheLookup() {
    GoogleString options_signature_hash =
        server_context_->GetRewriteOptionsSignatureHash(
            rewrite_driver_->options());
+
    property_callback = new PropertyCallback(
        absolute_url_,
        options_signature_hash,
        device_type,
        rewrite_driver_,
        server_context_->thread_system());
-    server_context_->page_property_cache()->Read(property_callback);
+    pcache->ReadWithCohorts(
+        RewriteDriver::GetCohortList(pcache, rewrite_driver_->options(),
+                                     server_context_),
+        property_callback);
+    rewrite_driver_->PropertyCacheSetupDone();
    DCHECK(property_callback->done());
  }
 }
@@ -855,6 +855,7 @@ void ProxyFetch::PropertyCacheComplete(
    driver_->set_origin_property_page(
        callback_collector->ReleaseOriginPropertyPage());
    driver_->set_device_type(callback_collector->device_type());
+    driver_->PropertyCacheSetupDone();
  }
  // We have to set the callback to NULL to let ScheduleQueueExecutionIfNeeded
  // proceed (it waits until it's NULL). And we have to delete it because then
@@ -1236,15 +1237,6 @@ void ProxyFetch::HandleIdleAlarm() {

 namespace {

-PropertyCache::CohortVector GetCohortList(
-    const ServerContext* server_context) {
-  PropertyCache* page_property_cache = server_context->page_property_cache();
-  // TODO(morlovich): Filter out dependencies cohort here if it's not needed.
-  const PropertyCache::CohortVector cohort_list =
-      page_property_cache->GetAllCohorts();
-  return cohort_list;
-}
-
 bool UrlMightHavePropertyCacheEntry(const GoogleUrl& url) {
  const ContentType* type = NameExtensionToContentType(url.LeafSansQuery());
  if (type == NULL) {
@@ -1399,7 +1391,8 @@ ProxyFetchPropertyCallbackCollector*
  }

  // All callbacks need to be registered before Reads to avoid race.
-  PropertyCache::CohortVector cohort_list = GetCohortList(server_context);
+  PropertyCache::CohortVector cohort_list = RewriteDriver::GetCohortList(
+      page_property_cache, options, server_context);
  if (property_callback != NULL) {
    page_property_cache->ReadWithCohorts(cohort_list, property_callback);
  }
@@ -60,6 +60,7 @@ run_test broken_images
 run_test make_show_ads_async
 run_test responsive_images
 run_test shortcut_icons
+run_test hint_preload_subresources

 # These have to run after image_rewrite tests. Otherwise it causes some images
 # to be loaded into memory before they should be.
@@ -0,0 +1,16 @@
+test_filter hint_preload_subresources works, and finds indirects
+
+# Expect 5 resources to be hinted
+fetch_until -save $URL 'grep -c ^Link:' 5 --save-headers
+OUT=$(cat $FETCH_UNTIL_OUTFILE)
+
+check_from "$OUT" fgrep \
+  'Link: </mod_pagespeed_example/styles/all_using_imports.css>; rel=preload; as=style; nopush'
+check_from "$OUT" fgrep \
+  'Link: </mod_pagespeed_example/styles/yellow.css>; rel=preload; as=style; nopush'
+check_from "$OUT" fgrep \
+  'Link: </mod_pagespeed_example/styles/blue.css>; rel=preload; as=style; nopush'
+check_from "$OUT" fgrep \
+  'Link: </mod_pagespeed_example/styles/bold.css>; rel=preload; as=style; nopush'
+check_from "$OUT" fgrep \
+  'Link: </mod_pagespeed_example/inline_javascript.js>; rel=preload; as=script; nopush'
@@ -290,13 +290,17 @@ GoogleString CachePropertyStore::Name() const {
  return out;
 }

-GoogleString CachePropertyStore::FormatName2(StringPiece cohort_name1,
+GoogleString CachePropertyStore::FormatName3(StringPiece cohort_name1,
                                             StringPiece cohort_cache1,
                                             StringPiece cohort_name2,
-                                             StringPiece cohort_cache2) {
+                                             StringPiece cohort_cache2,
+                                             StringPiece cohort_name3,
+                                             StringPiece cohort_cache3) {
  return StrCat(cohort_name1, ":", cohort_cache1,
                "\n",
-                cohort_name2, ":", cohort_cache2);
+                cohort_name2, ":", cohort_cache2,
+                "\n",
+                cohort_name3, ":", cohort_cache3);
 }

 }  // namespace net_instaweb
@@ -97,10 +97,12 @@ class CachePropertyStore : public PropertyStore {

  virtual GoogleString Name() const;

-  static GoogleString FormatName2(StringPiece cohort_name1,
+  static GoogleString FormatName3(StringPiece cohort_name1,
                                  StringPiece cohort_cache1,
                                  StringPiece cohort_name2,
-                                  StringPiece cohort_cache2);
+                                  StringPiece cohort_cache2,
+                                  StringPiece cohort_name3,
+                                  StringPiece cohort_cache3);

 private:
  GoogleString cache_key_prefix_;
@@ -217,7 +217,7 @@ class PropertyCache {
                       PropertyPage* property_page) const;

  // Returns all the cohorts from cache.
-  const CohortVector GetAllCohorts() const { return cohort_list_; }
+  const CohortVector& GetAllCohorts() const { return cohort_list_; }

  // Determines whether a value that was read is reasonably stable.
  bool IsStable(const PropertyValue* property) const {
@@ -440,19 +440,12 @@ void SystemCaches::SetupPcacheCohorts(ServerContext* server_context,
                                      bool enable_property_cache) {
  server_context->set_enable_property_cache(enable_property_cache);
  PropertyCache* pcache = server_context->page_property_cache();
-
-  const PropertyCache::Cohort* cohort =
-      server_context->AddCohort(RewriteDriver::kBeaconCohort, pcache);
-  server_context->set_beacon_cohort(cohort);
-
-  cohort = server_context->AddCohort(RewriteDriver::kDomCohort, pcache);
-  server_context->set_dom_cohort(cohort);
-
-  if (server_context->global_options()->NeedsDependenciesCohort()) {
-    cohort = server_context->AddCohort(RewriteDriver::kDependenciesCohort,
-                                      pcache);
-    server_context->set_dependencies_cohort(cohort);
-  }
+  server_context->set_beacon_cohort(
+      server_context->AddCohort(RewriteDriver::kBeaconCohort, pcache));
+  server_context->set_dom_cohort(
+      server_context->AddCohort(RewriteDriver::kDomCohort, pcache));
+  server_context->set_dependencies_cohort(
+      server_context->AddCohort(RewriteDriver::kDependenciesCohort, pcache));
 }

 void SystemCaches::SetupCaches(ServerContext* server_context,
@@ -322,10 +322,13 @@ class SystemCachesTest : public CustomRewriteTestBase<SystemRewriteOptions> {
  }

  GoogleString Pcache(StringPiece cache) {
-    return CachePropertyStore::FormatName2(
+    return CachePropertyStore::FormatName3(
        RewriteDriver::kBeaconCohort,
        Stats(PropertyCache::GetStatsPrefix(RewriteDriver::kBeaconCohort),
              cache),
+        RewriteDriver::kDependenciesCohort,
+        Stats(PropertyCache::GetStatsPrefix(RewriteDriver::kDependenciesCohort),
+              cache),
        RewriteDriver::kDomCohort,
        Stats(PropertyCache::GetStatsPrefix(RewriteDriver::kDomCohort),
              cache));