Page MenuHomePhabricator

ImageGateway should utilize browser cache when calling MediaWIki API
Closed, DuplicatePublic3 Estimated Story Points

Description

ImageGateway is currently setup to cache all responses:

ImageGateway.prototype.getThumb = function ( title ) {
	var cachedThumb = this._cache[title],
		$window = util.getWindow(),
		imageSizeMultiplier = ( window.devicePixelRatio && window.devicePixelRatio > 1 ) ?
			window.devicePixelRatio : 1;

	if ( !cachedThumb ) {
		this._cache[title] = this.api.get( actionParams( {
			prop: 'imageinfo',
			titles: title,
			iiprop: [ 'url', 'extmetadata' ],
			// request an image devicePixelRatio times bigger than the reported screen size
			// for retina displays and zooming
			iiurlwidth: findSizeBucket( $window.width() * imageSizeMultiplier ),
			iiurlheight: findSizeBucket( $window.height() * imageSizeMultiplier )
		} ) ).then( function ( resp ) {
			// imageinfo is undefined for missing pages.
			if ( resp.query && resp.query.pages &&
				resp.query.pages[0] && resp.query.pages[0].imageinfo ) {
				return resp.query.pages[0].imageinfo[0];
			}
			throw new Error( 'The API failed to return any pages matching the titles.' );
		} );
	}

	return this._cache[title];
};

However, the cache from ImageGateway is not being used and hasn't been used in a long time. On each onSlide event, ImageCarousel makes a new instance of itself and makes a new instance of ImageGateway which bypasses any benefits of the caching code.

This ticket is about utilizing cache and making it production ready. In its current state, the code will cache all responses including failed responses! We should only cache successful responses though. Instead of building our own cache, we should utilize the API:Caching data feature of MediaWiki API and expect browsers to cache the API response. This will not only fix the caching, but also keep the cache on different pageviews.

Acceptance criteria

Event Timeline

Change 502621 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove cache from ImageGateway

https://gerrit.wikimedia.org/r/502621

Jdlrobson triaged this task as Medium priority.May 2 2019, 8:39 PM
Jdlrobson moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.
Jdlrobson moved this task from Triaged but Future to Upcoming on the Web-Team-Backlog board.
Jdlrobson subscribed.

An estimation would be good since there is a patchset for this issue.

Jdlrobson raised the priority of this task from Medium to High.May 9 2019, 5:50 PM

This was discussed in grooming today and there was a valid concern about removing the caching behavior since it was implemented for a reason and each advancement of the image overlay fires an api request right now (also when you go back to slides that have already been visited). Perhaps we need to change this card into utilizing the caching behavior instead.

@nray thanks for the comment. Yes, my biggest concern was that someone added that cache for some reason. I tracked down the code and found rEMFR70978e57d40a: Add media viewer [alpha], the caching was implemented in the initial version 6 yr ago.
I'm not sure if this was a requirement or a nice to have, anyway, if it's already implemented - we should keep it. It's always a nice performance improvement.

Instead of removing the caching I would suggest to keep it, just fix it so it works properly. I can see couple valid solutions:

  • instead of using this._cache maybe we can use sessionStorage? The cache would be freed when user closes the browser. But the problem happens if someone uploads new pictures, we should invalidate cache on that device (or maybe cache the revisionId?)
  • make the ImageGateway persistent across different overlays
  • worst case scenario, make the ImageGateway a singleton

@pmiazga My preference would be the second option - right now ImageCarousel instantiates a new ImageGateway in its constructor because nothing is injecting the gateway, but I think we should be able to inject one instance of ImageGateway into imageCarousel and have ImageCarousel reuse that one instance on every onSlide. I think that's the simplest solution and would use the cache

Change 502621 abandoned by Nray:
Remove cache from ImageGateway

Reason:
group discussed this today and utilizing the cache is preferred

https://gerrit.wikimedia.org/r/502621

nray renamed this task from Remove cache from ImageGateway to Use cache from ImageGateway.May 22 2019, 11:26 PM
nray updated the task description. (Show Details)

@pmiazga I revised this ticket to use the caching behavior instead. Feel free to also revise the ticket or let me know if you have any questions/concerns

@nray thanks. This looks much better now.

Can we use MediaWiki API caching instead and drop the cache? Caching responses shouldn't be a responsibility of frontend code and increases maintenance burden.

pmiazga renamed this task from Use cache from ImageGateway to ImageGateway should utilize browser cache when calling MediaWIki API.Jun 3 2019, 8:27 PM
pmiazga updated the task description. (Show Details)
ovasileva set the point value for this task to 3.Jun 19 2019, 4:40 PM

Can we use MediaWiki API caching instead and drop the cache?

Set the Cache-Control header to private; max-age=$maxAge where $maxAge is the length of time in seconds that you want the end user's browser to consider the response fresh.

… I think.

Jdlrobson lowered the priority of this task from High to Medium.Jul 23 2019, 6:36 PM
Jdlrobson moved this task from Upcoming to Triaged but Future on the Web-Team-Backlog board.