AMP Packager: Extract <link rel=preload as=image> into Link header

Also updates the `<link>` generation to use `imagesrcset` and `imagesizes`, which are both supported in Chrome 73+. We don't need to do anything fancy!

Re: #461
PiperOrigin-RevId: 337974534

Author: Googler

transformer/request/request.pb.go

@@ -119,8 +119,18 @@ message Metadata {
     string as = 2;
 
     // The media attribute for image preload link. This attribute is useful
-    // only for image links.
-    string media = 3;
+    // only for image links. Deprecated, instead use the `attributes` field.
+    string media = 3 [deprecated = true];
+
+    message Attribute {
+      string key = 1;
+
+      string val = 2;
+    }
+
+    // Optional remaining attributes. This does not include the `href` or `as`
+    // attributes.
+    repeated Attribute attributes = 4;
   }
   // Absolute URLs of resources that should be preloaded when the AMP is
   // prefetched. In a signed exchange (SXG) context, these would be included as

transformer/request/request.proto

@@ -112,6 +112,9 @@ var ampAttrRE = func() *regexp.Regexp {
 	return r
 }()
 
+// firstSrcsetSourceRE captures the first source URL from a srcset.
+var firstSrcsetSourceRE = regexp.MustCompile(`[^\s,]+`)
+
 // The allowed AMP formats, and their serialization as an html "amp4" attribute.
 var ampFormatSuffixes = map[rpb.Request_HtmlFormat]string{
 	rpb.Request_AMP:       "",
@@ -198,20 +201,54 @@ func extractPreloads(dom *amphtml.DOM) []*rpb.Metadata_Preload {
 	// If you add additional preloads here, verify that they can not be
 	// unintentionally author supplied.
 	preloads := []*rpb.Metadata_Preload{}
-	for child := dom.HeadNode.FirstChild; child != nil; child = child.NextSibling {
-		switch child.DataAtom {
+	next := dom.HeadNode.FirstChild
+	for next != nil {
+		// Set the next node to visit
+		current := next
+		next = current.NextSibling
+
+		switch current.DataAtom {
 		case atom.Script:
-			if src, ok := htmlnode.GetAttributeVal(child, "", "src"); ok {
+			if src, ok := htmlnode.GetAttributeVal(current, "", "src"); ok {
 				preloads = append(preloads, &rpb.Metadata_Preload{Url: src, As: "script"})
 			}
 		case atom.Link:
-			if rel, ok := htmlnode.GetAttributeVal(child, "", "rel"); ok {
+			if rel, ok := htmlnode.GetAttributeVal(current, "", "rel"); ok {
 				if strings.EqualFold(rel, "stylesheet") {
-					if href, ok := htmlnode.GetAttributeVal(child, "", "href"); ok {
+					if href, ok := htmlnode.GetAttributeVal(current, "", "href"); ok {
 						preloads = append(preloads, &rpb.Metadata_Preload{Url: href, As: "style"})
 					}
+				} else if strings.EqualFold(rel, "preload") {
+					if as, ok := htmlnode.GetAttributeVal(current, "", "as"); ok && strings.EqualFold(as, "image") {
+						href, _ := htmlnode.GetAttributeVal(current, "", "href")
+						// It's valid for a <link> to not have a href, but when we generate a Link header it must have one.
+						if href == "" {
+							imagesrcset, ok := htmlnode.GetAttributeVal(current, "", "imagesrcset")
+							if !ok {
+								continue
+							}
+							// The href doesn't really matter here. Browsers will ignore it and instead prioritize whichever source is selected from imagesrcset. However, the Link header *must* have it.
+							// Stub this by just finding the first source URL possible.
+							firstSource := firstSrcsetSourceRE.FindString(imagesrcset)
+							if firstSource == "" {
+								continue
+							}
+							href = firstSource
+						}
+						preload := &rpb.Metadata_Preload{Url: href, As: "image"}
+						for _, attr := range current.Attr {
+							if strings.EqualFold(attr.Key, "rel") || strings.EqualFold(attr.Key, "as") || strings.EqualFold(attr.Key, "href") {
+								continue
+							}
+							preload.Attributes = append(preload.Attributes, &rpb.Metadata_Preload_Attribute{
+								Key: attr.Key,
+								Val: attr.Val,
+							})
+						}
+						preloads = append(preloads, preload)
+						htmlnode.RemoveNode(&current)
+					}
 				}
-				// TODO: This should extract <link rel=preload>s with arbitrary attributes, if the href is is on-cache.
 			}
 		}
 		if len(preloads) == maxPreloads {
@@ -313,12 +350,14 @@ func Process(r *rpb.Request) (string, *rpb.Metadata, error) {
 	if err := runTransformers(context, fns); err != nil {
 		return "", nil, err
 	}
+	// extractPreloads is an implicit transformer, and must run before printer.
+	preloads := extractPreloads(context.DOM)
 	var o strings.Builder
 	if err := printer.Print(&o, context.DOM.RootNode); err != nil {
 		return "", nil, err
 	}
 	metadata := rpb.Metadata{
-		Preloads:   extractPreloads(context.DOM),
+		Preloads:   preloads,
 		MaxAgeSecs: computeMaxAgeSeconds(context.DOM),
 	}
 	return o.String(), &metadata, nil

transformer/transformer.go

@@ -9,6 +9,7 @@ import (
 	rpb "github.com/ampproject/amppackager/transformer/request"
 	"github.com/ampproject/amppackager/transformer/transformers"
 	"github.com/google/go-cmp/cmp"
+	"github.com/kylelemons/godebug/diff"
 	"github.com/golang/protobuf/proto"
 )
 
@@ -61,55 +62,91 @@ func TestPreloads(t *testing.T) {
 	// Programmatically prepare the `> maxPreloads` test case.
 	var manyScriptsHTML strings.Builder
 	manyScriptsPreloads := []*rpb.Metadata_Preload{}
-	manyScriptsHTML.WriteString("<html ⚡>")
+	manyScriptsHTML.WriteString("<html ⚡><head>")
 	for i := 0; i <= maxPreloads; i++ {
-		fmt.Fprintf(&manyScriptsHTML, `<script src="foo%d"></script>`, i)
+		fmt.Fprintf(&manyScriptsHTML, `<script src=foo%d></script>`, i)
 		if i < maxPreloads {
 			manyScriptsPreloads = append(manyScriptsPreloads, &rpb.Metadata_Preload{Url: fmt.Sprintf("foo%d", i), As: "script"})
 		}
 	}
 
 	tcs := []struct {
 		html             string
+		expectedHTML     string
 		expectedPreloads []*rpb.Metadata_Preload
 	}{
 		{
 			"<html ⚡><script>",
+			"<html ⚡><head><script></script></head><body></body></html>",
 			[]*rpb.Metadata_Preload{},
 		},
 		{
 			"<html ⚡><script src=foo>",
+			"<html ⚡><head><script src=foo></script></head><body></body></html>",
 			[]*rpb.Metadata_Preload{{Url: "foo", As: "script"}},
 		},
 		{
 			"<html ⚡><link rel=foaf href=foo>",
+			"<html ⚡><head><link href=foo rel=foaf></head><body></body></html>",
 			[]*rpb.Metadata_Preload{},
 		},
 		{
 			"<html ⚡><link rel=stylesheet href=foo>",
+			"<html ⚡><head><link href=foo rel=stylesheet></head><body></body></html>",
 			[]*rpb.Metadata_Preload{{Url: "foo", As: "style"}},
 		},
 		{ // case-insensitive
 			"<html ⚡><link rel=STYLEsheet href=foo>",
+			"<html ⚡><head><link href=foo rel=STYLEsheet></head><body></body></html>",
 			[]*rpb.Metadata_Preload{{Url: "foo", As: "style"}},
 		},
 		{
 			"<html ⚡><link rel=stylesheet href=foo><script src=bar>",
+			"<html ⚡><head><link href=foo rel=stylesheet><script src=bar></script></head><body></body></html>",
 			[]*rpb.Metadata_Preload{{Url: "foo", As: "style"}, {Url: "bar", As: "script"}},
 		},
 		{
 			manyScriptsHTML.String(),
+			manyScriptsHTML.String() + "</head><body></body></html>",
 			manyScriptsPreloads,
 		},
+		{
+			"<html ⚡><link rel=preload href=foo>",
+			"<html ⚡><head><link href=foo rel=preload></head><body></body></html>",
+			[]*rpb.Metadata_Preload{},
+		},
+		{
+			"<html ⚡><link rel=preload as=image href=foo>",
+			"<html ⚡><head></head><body></body></html>",
+			[]*rpb.Metadata_Preload{{Url: "foo", As: "image"}},
+		},
+		{
+			"<html ⚡><link rel=preload as=image href=foo imagesrcset=bar>",
+			"<html ⚡><head></head><body></body></html>",
+			[]*rpb.Metadata_Preload{{Url: "foo", As: "image", Attributes: []*rpb.Metadata_Preload_Attribute{{Key: "imagesrcset", Val: "bar"}}}},
+		},
+		{
+			"<html ⚡><link rel=preload as=image href=foo imagesrcset=bar imagesizes=100vw>",
+			"<html ⚡><head></head><body></body></html>",
+			[]*rpb.Metadata_Preload{{Url: "foo", As: "image", Attributes: []*rpb.Metadata_Preload_Attribute{{Key: "imagesrcset", Val: "bar"}, {Key: "imagesizes", Val: "100vw"}}}},
+		},
+		{
+			"<html ⚡><link rel=preload as=image href=foo media=print>",
+			"<html ⚡><head></head><body></body></html>",
+			[]*rpb.Metadata_Preload{{Url: "foo", As: "image", Attributes: []*rpb.Metadata_Preload_Attribute{{Key: "media", Val: "print"}}}},
+		},
 	}
 
 	for _, tc := range tcs {
 		t.Run(tc.html, func(t *testing.T) {
-			_, metadata, err := Process(&rpb.Request{Html: tc.html, Config: rpb.Request_NONE})
+			output, metadata, err := Process(&rpb.Request{Html: tc.html, Config: rpb.Request_NONE})
 			if err != nil {
 				t.Fatalf("unexpected failure: %v", err)
 			}
 
+			if diff := diff.Diff(output, tc.expectedHTML); diff != "" {
+				t.Errorf("html output differs (-want +got):\n%s", diff)
+			}
 			if diff := cmp.Diff(tc.expectedPreloads, metadata.Preloads, cmp.Comparer(proto.Equal)); diff != "" {
 				t.Errorf("preloads differ (-want +got):\n%s", diff)
 			}

transformer/transformer_test.go

@@ -15,9 +15,6 @@
 package transformers
 
 import (
-	"fmt"
-	"sort"
-	"strconv"
 	"strings"
 
 	"github.com/ampproject/amppackager/transformer/internal/amphtml"
@@ -31,15 +28,10 @@ const maxHeroImages int = 2
 type HeroImage struct {
 	src    string
 	srcset string
+	sizes  string
 	ampImg *html.Node
 }
 
-// mediaQuerySource represents an individual source from a srcset with a unique media query that will only load the href when appropriate.
-type mediaQuerySource struct {
-	href  string
-	media string
-}
-
 // PreloadImage adds link rel="preload" to head element to preload the most revalent image in the AMP document,
 // and inserts an img tag if the image is an amp-img.
 func PreloadImage(e *Context) error {
@@ -73,7 +65,20 @@ func PreloadImage(e *Context) error {
 }
 
 func prioritizeHeroImage(e *Context, heroImage HeroImage) {
-	for _, link := range buildLinkPreloads(heroImage) {
+	if heroImage.src != "" || heroImage.srcset != "" {
+		link := htmlnode.Element("link",
+			html.Attribute{Key: "rel", Val: "preload"},
+			html.Attribute{Key: "as", Val: "image"},
+		)
+		if heroImage.src != "" {
+			htmlnode.SetAttribute(link, "", "href", heroImage.src)
+		}
+		if heroImage.srcset != "" {
+			htmlnode.SetAttribute(link, "", "imagesrcset", heroImage.srcset)
+		}
+		if heroImage.sizes != "" {
+			htmlnode.SetAttribute(link, "", "imagesizes", heroImage.sizes)
+		}
 		e.DOM.HeadNode.AppendChild(link)
 	}
 
@@ -83,37 +88,6 @@ func prioritizeHeroImage(e *Context, heroImage HeroImage) {
 	}
 }
 
-func buildLinkPreloads(heroImage HeroImage) []*html.Node {
-	// One of these is guaranteed.
-	if heroImage.srcset != "" {
-		if medias, ok := srcsetToMediaQueries(heroImage.srcset); ok {
-			links := make([]*html.Node, len(medias))
-			for i, mediaSrc := range medias {
-				links[i] = htmlnode.Element("link",
-					html.Attribute{Key: "rel", Val: "preload"},
-					html.Attribute{Key: "as", Val: "image"},
-					html.Attribute{Key: "href", Val: mediaSrc.href},
-					html.Attribute{Key: "media", Val: mediaSrc.media},
-				)
-			}
-
-			return links
-		}
-	}
-
-	if heroImage.src == "" {
-		return []*html.Node{}
-	}
-
-	return []*html.Node{
-		htmlnode.Element("link",
-			html.Attribute{Key: "rel", Val: "preload"},
-			html.Attribute{Key: "as", Val: "image"},
-			html.Attribute{Key: "href", Val: heroImage.src},
-		),
-	}
-}
-
 func buildImg(ampImg *html.Node) *html.Node {
 	img := htmlnode.Element("img",
 		html.Attribute{Key: "class", Val: "i-amphtml-fill-content i-amphtml-replaced-content"},
@@ -172,65 +146,6 @@ func ValidateSrc(in string, has bool) (string, bool) {
 	return in, true
 }
 
-// Converts the raw srcset value into multiple sources with an appropriate media query to select that source.
-// TODO(jridgewell, amaltas): Support pixel density with -webkit-max-device-pixel-ratio and -webkit-min-device-pixel-ratio
-func srcsetToMediaQueries(srcset string) ([]mediaQuerySource, bool) {
-	type Source struct {
-		href string
-		size int
-	}
-
-	srcSets := strings.Split(strings.TrimSpace(srcset), ",")
-	length := len(srcSets)
-	sources := make([]Source, length)
-	medias := make([]mediaQuerySource, length)
-
-	if length == 0 {
-		return medias, false
-	}
-
-	for i, src := range srcSets {
-		imgComponents := strings.Fields(src)
-		if len(imgComponents) != 2 {
-			return medias, false
-		}
-
-		source := Source{imgComponents[0], 0}
-
-		if strings.HasSuffix(imgComponents[1], "w") {
-			size, err := strconv.Atoi(strings.TrimSuffix(imgComponents[1], "w"))
-			if err != nil {
-				return medias, false
-			}
-
-			source.size = size
-		} else {
-			return medias, false
-		}
-
-		sources[i] = source
-	}
-
-	// Sort the images based on their target sizes in asc order.
-	sort.Slice(sources, func(i, j int) bool {
-		return sources[i].size < sources[j].size
-	})
-
-	for i, ci := range sources {
-		var mediaQuery string
-		if i == 0 {
-			mediaQuery = fmt.Sprintf("(max-width: %dpx)", ci.size)
-		} else if i == length-1 {
-			mediaQuery = fmt.Sprintf("(min-width: %dpx)", sources[i-1].size+1)
-		} else {
-			mediaQuery = fmt.Sprintf("(min-width: %dpx) and (max-width: %dpx)", sources[i-1].size+1, ci.size)
-		}
-
-		medias[i] = mediaQuerySource{ci.href, "screen and " + mediaQuery}
-	}
-	return medias, true
-}
-
 func lazyLoadRemainingAmpImgs(n *html.Node) {
 	for n != nil {
 		if n.Data == "amp-img" {