1927755 - Medium rectangle images fetched from the UAPI are being proxied through the Pocket CDN

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task)

Description

[Mike Conley (:mconley) (:⚙️)]

efixler noticed while debugging some UAPI stuff that images returned from that endpoint are being fetched by way of the Pocket CDN - example:

GET https://img-getpocket.cdn.mozilla.net/296x148/filters:format(jpeg):quality(60):no_upscale():strip_exif()/https%3A%2F%2Fads-img.mozilla.org%2Fv1%2Fimages%3Fimage_data%3XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX-Q

Since we own both endpoints, this seems like a waste, and probably not great for latency / cloud spend.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

There's more to it than just latency and unnecessary work - the Pocket proxy is also cropping the images, so we're going to want this fixed for the medium rectangle experiment.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Medium Rectangle image in Nightly vs correct display

Comment 4

[Mike Conley (:mconley) (:⚙️)]

This is because of these lines in DSImage, which try to use srcset to request the right image resolution based on the viewport size: https://searchfox.org/mozilla-central/rev/53e8dfd81c32f1ab275516406ec06a68136aaef0/browser/components/newtab/content-src/components/DiscoveryStreamComponents/DSImage/DSImage.jsx#161-185

Those sizes are hard-coded here: https://searchfox.org/mozilla-central/rev/53e8dfd81c32f1ab275516406ec06a68136aaef0/browser/components/newtab/content-src/components/DiscoveryStreamComponents/DSCard/DSCard.jsx#198-229

I suspect we should not supply any sizes for this.props.format == "rectangle" when we pass them here: https://searchfox.org/mozilla-central/rev/53e8dfd81c32f1ab275516406ec06a68136aaef0/browser/components/newtab/content-src/components/DiscoveryStreamComponents/DSCard/DSCard.jsx#686

For readability sake, we might consider computing the right sizes to pass along outside of this return so we don't have to use ternary or something.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hm... I just tried to hack the above solution together to prove out the approach, and noticed that I'm getting CORS errors attempting to directly access the image from https://ads-img.mozilla.org.

dmueller, maybe you know - is that endpoint missing the Access-Control-Allow-Origin: * rule that'd let us fetch the image directly for newtab?

Comment 6

[daniel mueller]

Can saw your comment here and you're correct that we didn't have that header set yet. He put up a PR to add that, so it'll need to get merged & deployed

Comment 7

[cbguder]

This is now fixed, and images return Access-Control-Allow-Origin: * in staging and production. It might take up to an hour for caches to update.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Yep, confirmed - thank you!

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1927755 - Don't proxy medium rectangle images returned from UAPI through the Pocket CDN. r?#home-newtab-reviewers!

Passing an empty sizes array to DSImage will cause it to just use the raw source
URL for the image.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

The patch in this bug only fixed this for medium rectangle images from UAPI, but we should probably do it for all images returned by UAPI.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/826df8cac715
Don't proxy medium rectangle images returned from UAPI through the Pocket CDN. r=home-newtab-reviewers,thecount

Comment 12

[amarc]

https://hg.mozilla.org/mozilla-central/rev/826df8cac715

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1927755 - Don't proxy medium rectangle images returned from UAPI through the Pocket CDN.

Passing an empty sizes array to DSImage will cause it to just use the raw source
URL for the image.

Original Revision: https://phabricator.services.mozilla.com/D227676

Comment 14

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Users in the medium rectangle IAB standard ad sizing experiment will see a weirdly cropped and scaled ad.
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: Have QE follow the plan in https://mozilla-hub.atlassian.net/browse/QA-2668 and then observe the medium rectangle ad. It should not be cropped or scaled up.
• Risk associated with taking this patch: Very little.
• Explanation of risk level: This is fairly well-contained, and should only affect clients within the experiment treatment branch.
• String changes made/needed: None.
• Is Android affected?: no

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/327e15fdb8f4