-
-
Notifications
You must be signed in to change notification settings - Fork 590
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
Relying on ajax headers from tiled image #1833
base: master
Are you sure you want to change the base?
Conversation
…jax headers as part of cache key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for moving this forward! Apologies for the small change of direction, but I suppose it's natural that things come to light as we dive into the task. :-)
BTW, it looks like the code from #1832 is in this PR. If you merge the latest OpenSeadragon master in, it should sort that out. For future reference, it's good to make a separate branch for each pull request.
Looks like this is almost ready!
@@ -1515,7 +1515,7 @@ function loadTile( tiledImage, tile, time ) { | |||
tiledImage._imageLoader.addJob({ | |||
src: tile.url, | |||
loadWithAjax: tile.loadWithAjax, | |||
ajaxHeaders: tile.ajaxHeaders, | |||
ajaxHeaders: tiledImage.ajaxHeaders, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm taking a closer look at the ajaxHeaders functionality, it looks like the tile source has a getTileAjaxHeaders
which allows for different headers at the tile level. I'm not sure who, if anyone, is using that functionality, but I don't think we should break it with this pull request.
Currently, getTileAjaxHeaders is called by the tiledImage when the tile is created, and the tiledImage's headers are combined with the tile's at that time. I propose that instead we load just the tile's headers into the tile at creation, and combine them with the tiledImage's headers here. That way we can support both scenarios (per-tile headers, and tiledImage headers that change dynamically).
} else { | ||
this.cacheKey = this.url; | ||
} | ||
this.cacheKey = this.url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the reason to include the headers here is in case the system uses the same URL for all tiles and it's the header that specifies which tile. Continuing to include the headers won't interfere with the dynamic tiledImage headers, since with this PR the only headers here will be the tile-specific ones.
Fixes #1807 by relying on the ajax headers that are provided by the tiled image. So external overrides of ajax headers will get considered within every request.