Move from EventSource loading to individual requests

[juliushaertl]

Fixes: #245

Licence: AGPL

Description

Move from EventSources to individual requests for loading thumbnails.

Features

Screenshots or screencasts

Caveats

Tests

Test plan

Tested on

• [ ] Windows/Firefox
• [ ] Android 6.1/Chrome

TODO

• [x] Implement proper thumbnail route with ETag support for gallery (maybe we can use the servers preview endpoint, but I need to check that one handles public shared links)
  • This would also allow us to fix https://github.com/nextcloud/gallery/issues/14
• [x] Test more image formats
• [ ] Remove EventSource code

Check list

• [ ] Code is properly documented
• [ ] Code is properly formatted
• [ ] Commits have been squashed
• [ ] Tests (unit, integration, api and/or acceptance) are included
• [ ] Documentation (manuals or wiki) has been updated or is not required

Reviewers

[juliushaertl]

cc @rullzer

[rullzer]

Cool stuff I'll check it out

[Janhouse]

I have been using Nextcloud for a couple days and I am in no way familiar with code base but I will share some findings and thoughts.

I played around with this and added some ETag headers and checks in PreviewController.php.

For any caching to work "requesttoken" parameter has to be removed from preview links in js and additionally "@NoCSRFRequired" is required for the method. But once that is done it works quite well.

I was a bit concerned about the security implications because preview links have sequential file numbers. I am not sure about old browsers but modern browsers shouldn't let you access image data from external site through JavaScript (for example using canvas) unless CORS header explicitly allows it. Old browsers don't have canvas so I'm not sure why "requesttoken" was used here in the first place.

ETag hash value is conveniently available in PreviewController, getPreview(), $file->getEtag(). At this point file has already been loaded in memory (correct me if I am wrong and some sort of lazy loading exists for php) but at least it doesn't have to be sent to the client so it is still a win. Obviously it would be better if hashes and last-modified times would be stored in some cache for fast access but initially that might be overkill.

Ideally Cache-Control should be used with "private,max-age=xxx" instead of "no-cache", further improving browser caching. In my opinion having them fully cached without asking server each time would be more logical, because image files don't usually change that frequently and when opening full preview, etag hash is already passed as a URL parameter so it should always serve latest file version (unless it takes it from ETag header somehow).

If all of these caching options and times could be configurable from config.php that would be awesome.

Additional performance improvement could be made by removal of fade-in animation that is used when preview loads.

While testing this I could not figure out how Pragma and Expires headers kept showing up without specifically setting them even when using the basic OCP\AppFramework\Http\Response, so that was a bit annoying, but as I said, I have been using Nextcloud only for a couple days.

In any case, can't wait to see this change being implemented and merged. :smiley:

[rullzer]

@Janhouse lot of valid points. However the main preview endpoint in the server not this app. Already does a lot better job at caching. This PR is mainly there to get rid of the eventsource stuff.

Now we can slowly migrate it over. So that the browsers can actually do proper caching etc.

[oparoz]

Test fails, but they're all related to connecting to the public profile, so maybe it's unrelated.

[juliushaertl]

Let me do some more polishing if we want to merge this as a first step.

[juliushaertl]

Test fails, but they're all related to connecting to the public profile, so maybe it's unrelated.

They seem to be the same that fail at master as well.

@rullzer This should be good to go as a first step, please have a look. I'll also remove the event source code, but it is a bit easier to quickly review without those changes for now.

I kept using the gallery preview endpoint, since we need it for public previews anyway and it properly returns svg files.

We could use the preview endpoint from files_sharing, but that would introduce a cross app dependency. I still think that would be fine, since the files_sharing app needs to be enabled for public gallery pages anyway.

[rullzer]

Ok lets merge this for now and then we can slowly improve. :rocket: