Skip to content

Commit

Permalink
Replace pass by *string with pass by string. Apparently strings alrea…
Browse files Browse the repository at this point in the history 
…dy have

one level of indirection via their struct form, making it very low value to
pass by *string, and potentially more dangerous idiom.

PiperOrigin-RevId: 228567535
  • Loading branch information
@twifkak
Greg Grothaus authored and twifkak committed Jan 10, 2019
1 parent 94bccbf commit 35d938b
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 45 deletions.
36 changes: 18 additions & 18 deletions transformer/internal/amphtml/urls.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,48 +30,48 @@ import (
// sameURLIgnoringFragment is a helper for AbsoluteUrlValue below.
// Returns true if |base| is the same as |u| with the exception that |u| may
// also have an additional fragment component.
func sameURLIgnoringFragment(base *string, u *url.URL) bool {
func sameURLIgnoringFragment(base string, u *url.URL) bool {
// Due to https://github.com/golang/go/issues/29603 we have an extra check
// for the empty fragment case.
if u.Fragment == "" {
return *base == u.String()
return base == u.String()
}

return *base+"#"+u.Fragment == u.String()
return base+"#"+u.Fragment == u.String()
}

// isProtocolRelative is a mostly correct parse for protocol relative inputs
// by looking for a "//" prefix after stripping any leading whitespace and
// control characters.
func isProtocolRelative(urlParam *string) bool {
for *urlParam != "" && (*urlParam)[0] <= 0x20 {
*urlParam = (*urlParam)[1 : len(*urlParam)-1]
func isProtocolRelative(urlParam string) bool {
for urlParam != "" && (urlParam)[0] <= 0x20 {
urlParam = (urlParam)[1 : len(urlParam)-1]
}
return strings.HasPrefix(*urlParam, "//")
return strings.HasPrefix(urlParam, "//")
}

// ToAbsoluteURL absolute-ifies |urlParam|, using |baseURL| as the base if
// |urlParam| is relative. If |urlParam| contains a fragment, this method
// will return only a fragment if it's absolute URL matches |documentURL|,
// which prevents changing an in-document navigation to a out-of-document
// navigation.
func ToAbsoluteURL(documentURL *string, baseURL *url.URL,
urlParam *string) string {
if *urlParam == "" {
func ToAbsoluteURL(documentURL string, baseURL *url.URL,
urlParam string) string {
if urlParam == "" {
return ""
}

absoluteURL, err := baseURL.Parse(*urlParam)
absoluteURL, err := baseURL.Parse(urlParam)
// TODO(gregable): Should we strip this URL instead (ie: return "").
if err != nil {
return *urlParam
return urlParam
}

// TODO(gregable): We should probably assemble data: / mailto: / etc URLs,
// which will force them to be URL encoded, but this was left to maintain
// the old behavior for now.
if absoluteURL.Scheme != "http" && absoluteURL.Scheme != "https" {
return *urlParam
return urlParam
}

// Check for a specific case of protocol relative URL (ex: "//foo.com/")
Expand All @@ -88,7 +88,7 @@ func ToAbsoluteURL(documentURL *string, baseURL *url.URL,
// Note that we also try to identify empty fragments (ex: href="#").
// net/url doesn't support these (https://github.com/golang/go/issues/29603)
// so we try to detect them heuristically.
if (absoluteURL.Fragment != "" || strings.HasPrefix(*urlParam, "#")) &&
if (absoluteURL.Fragment != "" || strings.HasPrefix(urlParam, "#")) &&
sameURLIgnoringFragment(documentURL, absoluteURL) {
return "#" + absoluteURL.Fragment
}
Expand Down Expand Up @@ -143,10 +143,10 @@ func (c *CacheURL) String() string {
// GetCacheURL returns an AMP Cache URL structure for the URL identified by
// the given offset (relative to 'input') or an error if the URL could not be
// parsed.
func (so *SubresourceOffset) GetCacheURL(documentURL *string, base *url.URL,
input *string) (*CacheURL, error) {
urlStr := (*input)[so.Start:so.End]
absolute := ToAbsoluteURL(documentURL, base, &urlStr)
func (so *SubresourceOffset) GetCacheURL(documentURL string, base *url.URL,
input string) (*CacheURL, error) {
urlStr := (input)[so.Start:so.End]
absolute := ToAbsoluteURL(documentURL, base, urlStr)
if len(absolute) == 0 {
return nil, errors.New("unable to convert empty URL string")
}
Expand Down
4 changes: 2 additions & 2 deletions transformer/internal/amphtml/urls_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func TestToAbsoluteURL(t *testing.T) {
}
for _, tc := range tcs {
baseURL, _ := url.Parse(tc.baseURL)
actual := ToAbsoluteURL(&tc.documentURL, baseURL, &tc.input)
actual := ToAbsoluteURL(tc.documentURL, baseURL, tc.input)
if actual != tc.expectedAbsolute {
t.Errorf("%s: ToAbsoluteURL=%s want=%s", tc.desc, actual, tc.expectedAbsolute)
}
Expand Down Expand Up @@ -208,7 +208,7 @@ func TestGetCacheURL(t *testing.T) {
}
so := SubresourceOffset{SubType: subtype, Start: 0, End: len(tc.input), DesiredImageWidth: tc.width}
rootURL := "https://example.com/"
cu, err := so.GetCacheURL(&rootURL, base, &tc.input)
cu, err := so.GetCacheURL(rootURL, base, tc.input)
if tc.expectError {
if err == nil {
t.Errorf("%s: ToCacheImageURL(%s, %d) expected error. Got none", tc.desc, tc.input, tc.width)
Expand Down
23 changes: 11 additions & 12 deletions transformer/transformers/absoluteurl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,26 +79,26 @@ func AbsoluteURL(e *Context) error {
}

// Make attributes with URLs portable on any tag
rewriteAbsoluteURLs(n, &documentURL, e.BaseURL, anyTagAttrs)
rewriteAbsoluteURLs(n, documentURL, e.BaseURL, anyTagAttrs)

switch n.DataAtom {
case atom.Form:
// Make attributes with URLs absolute on <form> tag.
rewriteAbsoluteURLs(n, &documentURL, e.BaseURL, formTagAttrs)
rewriteAbsoluteURLs(n, documentURL, e.BaseURL, formTagAttrs)
case atom.Img:
// Make attributes with URLs portable on <img> tag.
rewriteAbsoluteURLs(n, &documentURL, e.BaseURL, imgTagAttrs)
rewriteAbsoluteURLs(n, documentURL, e.BaseURL, imgTagAttrs)
default:
switch n.Data {
case "amp-install-serviceworker":
// Make attributes with URLs portable on <amp-install-serviceworker> tag.
rewriteAbsoluteURLs(n, &documentURL, e.BaseURL, ampInstallServiceWorkerTagAttrs)
rewriteAbsoluteURLs(n, documentURL, e.BaseURL, ampInstallServiceWorkerTagAttrs)
case amphtml.AMPStory:
// Make attributes with URLs portable on <amp-story> tag.
rewriteAbsoluteURLs(n, &documentURL, e.BaseURL, ampStoryTagAttrs)
rewriteAbsoluteURLs(n, documentURL, e.BaseURL, ampStoryTagAttrs)
case "amp-story-page":
// Make attributes with URLs portable on <amp-story-page> tag.
rewriteAbsoluteURLs(n, &documentURL, e.BaseURL, ampStoryPageTagAttrs)
rewriteAbsoluteURLs(n, documentURL, e.BaseURL, ampStoryPageTagAttrs)
}
}

Expand Down Expand Up @@ -138,11 +138,10 @@ func AbsoluteURL(e *Context) error {
}
} else {
htmlnode.SetAttribute(n, "", "href",
amphtml.ToAbsoluteURL(&documentURL, e.BaseURL, &href.Val))
amphtml.ToAbsoluteURL(documentURL, e.BaseURL, href.Val))
}
case atom.A:
newValue := amphtml.ToAbsoluteURL(
&documentURL, e.BaseURL, &href.Val)
newValue := amphtml.ToAbsoluteURL(documentURL, e.BaseURL, href.Val)
// Set a default target
// 1. If the href is not a fragment AND
// 2. If there is no target OR
Expand All @@ -156,7 +155,7 @@ func AbsoluteURL(e *Context) error {
default:
// Absoluteify any remaining tags with an href attribute.
htmlnode.SetAttribute(n, "", "href",
amphtml.ToAbsoluteURL(&documentURL, e.BaseURL, &href.Val))
amphtml.ToAbsoluteURL(documentURL, e.BaseURL, href.Val))
}
}
}
Expand All @@ -181,12 +180,12 @@ func isAllowedTarget(t string) bool {

// rewriteAbsoluteURLs rewrites URLs in the given slice of attributes
// to be absolute for the base URL provided.
func rewriteAbsoluteURLs(n *html.Node, documentURL *string, baseURL *url.URL,
func rewriteAbsoluteURLs(n *html.Node, documentURL string, baseURL *url.URL,
tagAttrs []string) {
for _, attr := range tagAttrs {
if v, ok := htmlnode.GetAttributeVal(n, "", attr); ok {
htmlnode.SetAttribute(n, "", attr,
amphtml.ToAbsoluteURL(documentURL, baseURL, &v))
amphtml.ToAbsoluteURL(documentURL, baseURL, v))
}
}
}
24 changes: 11 additions & 13 deletions transformer/transformers/urlrewrite.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (

type rewritable interface {
// Rewrite the URLs within
rewrite(*string, *url.URL, *string, map[string]struct{})
rewrite(string, *url.URL, string, map[string]struct{})
}

type urlRewriteContext []rewritable
Expand Down Expand Up @@ -138,7 +138,7 @@ func URLRewrite(e *Context) error {
// After the contextual information has been gathered, use it to rewrite the appropriate URLs
// and adding any preconnects if necessary.
documentURL := e.DocumentURL.String()
preconnects := convertToAMPCacheURLs(ctx, &documentURL, e.BaseURL)
preconnects := convertToAMPCacheURLs(ctx, documentURL, e.BaseURL)
for _, k := range preconnects {
n := htmlnode.Element("link", html.Attribute{Key: "href", Val: k}, html.Attribute{Key: "rel", Val: "dns-prefetch preconnect"})
e.DOM.HeadNode.AppendChild(n)
Expand All @@ -149,11 +149,11 @@ func URLRewrite(e *Context) error {

// convertToAMPCacheURLs examines the generated context and rewrites all the necessary
// URLs to point to the AMP Cache, returning a list of preconnects that need to be added.
func convertToAMPCacheURLs(ctx urlRewriteContext, documentURL *string, base *url.URL) []string {
func convertToAMPCacheURLs(ctx urlRewriteContext, documentURL string, base *url.URL) []string {
preconnects := make(map[string]struct{})
mainSubdomain := amphtml.ToCacheURLSubdomain(base.Hostname())
for _, rw := range ctx {
rw.rewrite(documentURL, base, &mainSubdomain, preconnects)
rw.rewrite(documentURL, base, mainSubdomain, preconnects)
}
sortedPreconnects := make([]string, 0, len(preconnects))
for k := range preconnects {
Expand All @@ -164,9 +164,8 @@ func convertToAMPCacheURLs(ctx urlRewriteContext, documentURL *string, base *url
}

// rewrite the URLs described by the elementNodeContext. rewriteable implementation.
func (nc *elementNodeContext) rewrite(documentURL *string, baseURL *url.URL,
mainSubdomain *string,
preconnects map[string]struct{}) {
func (nc *elementNodeContext) rewrite(documentURL string, baseURL *url.URL,
mainSubdomain string, preconnects map[string]struct{}) {
if len(nc.attrName) == 0 || len(nc.offsets) == 0 {
return
}
Expand All @@ -180,17 +179,16 @@ func (nc *elementNodeContext) rewrite(documentURL *string, baseURL *url.URL,
}

// rewrite the URLs described by the textNodeContext. rewriteable implementation.
func (nc *textNodeContext) rewrite(documentURL *string, baseURL *url.URL,
mainSubdomain *string,
preconnects map[string]struct{}) {
func (nc *textNodeContext) rewrite(documentURL string, baseURL *url.URL,
mainSubdomain string, preconnects map[string]struct{}) {
nc.node.Data = replaceURLs(nc.node.Data, nc.offsets, documentURL, baseURL,
mainSubdomain, preconnects)
}

// replaceURLs replaces all the URLs in the data string found at the various
// offsets with their AMP Cache equivalent, returning a new data string.
func replaceURLs(data string, offsets []amphtml.SubresourceOffset,
documentURL *string, baseURL *url.URL, mainSubdomain *string,
documentURL string, baseURL *url.URL, mainSubdomain string,
preconnects map[string]struct{}) string {
if len(offsets) == 0 {
// noop
Expand All @@ -204,14 +202,14 @@ func replaceURLs(data string, offsets []amphtml.SubresourceOffset,
sb.WriteString(data[pos:so.Start])
pos = so.Start
}
cu, err := so.GetCacheURL(documentURL, baseURL, &data)
cu, err := so.GetCacheURL(documentURL, baseURL, data)
if err != nil {
// noop
continue
}
sb.WriteString(cu.String())
pos = so.End
if len(*mainSubdomain) > 0 && *mainSubdomain != cu.Subdomain {
if len(mainSubdomain) > 0 && mainSubdomain != cu.Subdomain {
preconnects[cu.OriginDomain()] = struct{}{}
}
}
Expand Down

0 comments on commit 35d938b

Please sign in to comment.