Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure fetch/ajax image requests accept webp when supported #8262

Merged
merged 5 commits into from
Oct 17, 2019

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented May 19, 2019

Launch Checklist

fixes #8261

  • briefly describe the changes in this PR
    See further detail at ensure image requests send an accept header #8261 , but in essence this PR ensures image requests from GL JS to servers, include accepts image/webp where supported, so the server knows if it can respond with a webp image or not
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • check this webp support test is only run once, instead of at each tile request, and what impact it has on performance tested, only run once
  • manually test the debug page
    tested

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewharvey Are you still working on this PR? The change itself looks fine to accept, but I want to understand how this affects the serving of non-tile resources if webp is added to the accept header, before moving forward with it.

@samanpwbb @jseppi Would the sprite images endpoint be affected by the addition of an accepts header with image/webp?

@andrewharvey
Copy link
Collaborator Author

@andrewharvey Are you still working on this PR? The change itself looks fine to accept, but I want to understand how this affects the serving of non-tile resources if webp is added to the accept header, before moving forward with it.

Yes, I can pick this up again.

@samanpwbb @jseppi Would the sprite images endpoint be affected by the addition of an accepts header with image/webp?

For more context, GL JS will always request a sprite image with the extension .png:

let imageRequest = getImage(requestManager.transformRequest(requestManager.normalizeSpriteURL(baseURL, format, '.png'), ResourceType.SpriteImage), (err, img) => {

However as explained at #8261, when making a Fetch or XMLHttpRequest, the browser will pass / as the accept header and hence the server won't be able to detect if the client supports webp or not.

I tested Mapbox's API requesting the sprite.png with accepts: image/webp and there was no difference, the response was still the PNG sprite, which is fine. So it doesn't look like this breaks anything for sprites. Even if the server was smart and detected the accepts header and returned a webp sprite in spite of the .png extension that should also be fine, it should work.

@andrewharvey
Copy link
Collaborator Author

andrewharvey commented Sep 14, 2019

(edited)

I did further testing,

It does only test for webp once at the start (checking of one of the things I noted to test).

  • On Chrome, it works fine.
  • On Firefox, there was a lag for the webp support check coming back positive, so the first few requests wouldn't pass the accepts webp header but later requests would. It looks like this issue stems from the webp support check.

@andrewharvey andrewharvey changed the title [WIP] ensure fetch and ajax requests for images pass accept webp when supported ensure fetch and ajax requests for images pass accept webp when supported Sep 14, 2019
@andrewharvey andrewharvey marked this pull request as ready for review September 14, 2019 09:31
@asheemmamoowala
Copy link
Contributor

On Firefox, there was a lag for the webp support check coming back positive, so the first few requests wouldn't pass the accepts webp header but later requests would. It looks like this issue stems from the webp support check.

@andrewharvey how much of an issue is this? Does it matter if a few requests don't use webp images, or should the requests be blocked until the check is completed?

@andrewharvey
Copy link
Collaborator Author

@andrewharvey how much of an issue is this? Does it matter if a few requests don't use webp images, or should the requests be blocked until the check is completed?

Nothing more than a minor annoyance. I wouldn't hold up the requests because of this.

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are ✅. Can be merged with a unit test.

@ryanhamley
Copy link
Contributor

@andrewharvey Are you still working on this? I think we just need a unit test to merge it.

@andrewharvey
Copy link
Collaborator Author

@andrewharvey Are you still working on this? I think we just need a unit test to merge it.

@ryanhamley yes! unit test committed ✔️

@ryanhamley ryanhamley changed the title ensure fetch and ajax requests for images pass accept webp when supported ensure fetch/ajax image requests accept webp when supported Oct 17, 2019
@ryanhamley ryanhamley merged commit 19f776d into master Oct 17, 2019
@ryanhamley ryanhamley deleted the raster-accepts-webp branch October 17, 2019 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ensure image requests send an accept header
3 participants