Data srcset

[Lofesa]

A new version for rewrite srcset and data-srcset. Maybe doc must be updated to add pagespeed AddUrlValuedAttribute amp-img (data)-srcset srcsetimage Related to PR #1899

[jmarantz]

I'm concerned about amp-img. I feel like we might not want to rewrite URLs of such images, but only rewrite their content as part of IPRO. The reason is that the rewrites are user-agent-specific, and that amp HTML will be cached across user-agents.

[Lofesa]

I'm concerned about amp-img. I feel like we might not want to rewrite URLs of such images, but >only rewrite their content as part of IPRO. The reason is that the rewrites are user-agent-specific, >and that amp HTML will be cached across user-agents. What do you mean with "..that amp HTML will be cached across user-agents" ? If the page is served from the origin server, the images are served acordly to the browser user agent, if served from cdn.ampproject.org, the images are served w/ the optimizations for their UA. Maybe we exclude it from pagespeed if the referal is cdn.ampproject.org. Some like this in nginx:

map $http_referer $bad_referer {
    default                  0;
    "~* .*cdn.ampproject.org.*"       1;
}
if ($bad_referer) {
    pagespeed off;
}

EDIT: More on this. I think that amp pages for cdn.ampproject.org must be served with PageSpeed=off, this cdn uses a custom version of pagespeed, I can see this header i it:

x-page-speed | 0.9.10.99-9999

EDIT2: Seem that pagespeed off; is not allowed in a if in nginx EDIT3: Found this:

map $http_referer $bad_referer {
    default                  0;
    "~* .*cdn.ampproject.org.*"       1;
}
if ($bad_referer) {
    set $args &args&PageSpeed=off;
}

[Lofesa]

@jmarantz

Thanks for digging into this further!

What do you think about adding some unit tests?

About the amp-img: cdn.ampproject.org fecht resources with the Google bot for mobil UA.

Unit test for srcset, data-srcset and amp-img src and srcset are added in a previous PR #1899 so I think in this PR are not needed or are you talking about others unit test?

[Lofesa]

In the file net/instaweb/rewriter/cache_extender.cc I have duplicated the structure that start at line 233

if (element->keyword() == HtmlName::kImg &&
      driver()->MayCacheExtendImages()) {
    HtmlElement::Attribute* srcset = element->FindAttribute(HtmlName::kSrcset);
    if (srcset != nullptr) {

to catch the case attribute is data-srcset.

This maybe simplyfied by:

if (attribute(i).category == semantic_type::kSrcSetImage &&
       driver()->MayCacheExtendImages()) {
     HtmlElement::Attribute* data_srcset = attribute[i].url;
     SrcSetSlotCollectionPtr slot_collection(
          driver()->GetSrcSetSlotCollection(this, element, data_srcset));

and don´t have these duplicated for srcset and data-srcset, but I´m unsure. Can some one take a look?

[Lofesa]

@oschaaf Some unit test where set in the PR #1899 . Amp-img and data-srcset, if I remember, so maybe only a unit test for UrlValuedAttribute is needed.

For the doc... some like this?

PR-Data-srcset.pdf

[oschaaf]

Closing this in an effort to clean up stale pull requests (but feel free to re-open if you want to pick this up again).