From d4d90a0b8492639ae1555cdb3bc28fbb0efd5b0e Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Sat, 27 Aug 2016 12:38:15 +0200 Subject: [PATCH 1/2] Increase theming performance 1. Set proper caching headers (`Pragma: cache`) 2. Resize image proportionally to a max size of 1920px 3. Store images with progressive mode This resizes a previous 2.8 MB picture to 300kb and makes it rendering going down from 11 seconds to less than 1 here. And future requests won't have to download the file newly. --- apps/theming/js/settings-admin.js | 8 +-- .../lib/Controller/ThemingController.php | 34 ++++++++++- .../Controller/ThemingControllerTest.php | 57 +++++++++++++++++++ 3 files changed, 92 insertions(+), 7 deletions(-) diff --git a/apps/theming/js/settings-admin.js b/apps/theming/js/settings-admin.js index 01ff912384..86f97dc6dc 100644 --- a/apps/theming/js/settings-admin.js +++ b/apps/theming/js/settings-admin.js @@ -116,8 +116,8 @@ $(document).ready(function () { submit: function(e, response) { OC.msg.startSaving('#theming_settings_msg'); }, - fail: function (e, data){ - OC.msg.finishedSaving('#theming_settings_msg', response); + fail: function (e, response){ + OC.msg.finishedError('#theming_settings_msg', response._response.jqXHR.responseJSON.data.message); } }; var uploadParamsLogin = { @@ -130,8 +130,8 @@ $(document).ready(function () { submit: function(e, response) { OC.msg.startSaving('#theming_settings_msg'); }, - fail: function (e, data){ - OC.msg.finishedSaving('#theming_settings_msg', response); + fail: function (e, response){ + OC.msg.finishedError('#theming_settings_msg', response._response.jqXHR.responseJSON.data.message); } }; diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index 8d3e2a5f2e..faf59e4bb1 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -171,7 +171,8 @@ class ThemingController extends Controller { 'message' => $this->l->t('No file uploaded') ] ], - Http::STATUS_UNPROCESSABLE_ENTITY); + Http::STATUS_UNPROCESSABLE_ENTITY + ); } $name = ''; if(!empty($newLogo)) { @@ -182,7 +183,30 @@ class ThemingController extends Controller { } if(!empty($newBackgroundLogo)) { $target = $this->rootFolder->newFile('themedbackgroundlogo'); - stream_copy_to_stream(fopen($newBackgroundLogo['tmp_name'], 'r'), $target->fopen('w')); + + $image = @imagecreatefromstring(file_get_contents($newBackgroundLogo['tmp_name'], 'r')); + if($image === false) { + return new DataResponse( + [ + 'data' => [ + 'message' => $this->l->t('Unsupported image type'), + ], + 'status' => 'failure', + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + } + + // Optimize the image since some people may upload images that will be + // either to big or are not progressive rendering. + if(function_exists('imagescale')) { + // FIXME: Once PHP 5.5.0 is a requirement the above check can be removed + $image = imagescale($image, 1920); + } + imageinterlace($image, 1); + imagejpeg($image, $target->fopen('w'), 75); + imagedestroy($image); + $this->template->set('backgroundMime', $newBackgroundLogo['type']); $name = $newBackgroundLogo['name']; } @@ -236,6 +260,7 @@ class ThemingController extends Controller { $response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime())); $response->addHeader('Content-Disposition', 'attachment'); $response->addHeader('Content-Type', $this->config->getAppValue($this->appName, 'logoMime', '')); + $response->addHeader('Pragma', 'cache'); return $response; } @@ -256,6 +281,7 @@ class ThemingController extends Controller { $response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime())); $response->addHeader('Content-Disposition', 'attachment'); $response->addHeader('Content-Type', $this->config->getAppValue($this->appName, 'backgroundMime', '')); + $response->addHeader('Pragma', 'cache'); return $response; } @@ -334,6 +360,7 @@ class ThemingController extends Controller { $response = new DataDownloadResponse($responseCss, 'style', 'text/css'); $response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime())); + $response->addHeader('Pragma', 'cache'); $response->cacheFor(3600); return $response; } @@ -354,8 +381,9 @@ class ThemingController extends Controller { }; })();'; $response = new Http\DataDisplayResponse($responseJS); - $response->addHeader("Content-type","text/javascript"); + $response->addHeader('Content-type', 'text/javascript'); $response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime())); + $response->addHeader('Pragma', 'cache'); $response->cacheFor(3600); return $response; } diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 688e3d62bf..3e49fa9206 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -214,6 +214,7 @@ class ThemingControllerTest extends TestCase { $destination = \OC::$server->getTempManager()->getTemporaryFolder(); touch($tmpLogo); + file_put_contents($tmpLogo, file_get_contents(__DIR__ . '/../../../../tests/data/desktopapp.png')); $this->request ->expects($this->at(0)) ->method('getUploadedFile') @@ -261,6 +262,52 @@ class ThemingControllerTest extends TestCase { $this->assertEquals($expected, $this->themingController->updateLogo()); } + public function testUpdateLogoLoginScreenUploadWithInvalidImage() { + $tmpLogo = \OC::$server->getTempManager()->getTemporaryFolder() . '/logo.svg'; + $destination = \OC::$server->getTempManager()->getTemporaryFolder(); + + touch($tmpLogo); + file_put_contents($tmpLogo, file_get_contents(__DIR__ . '/../../../../tests/data/data.zip')); + $this->request + ->expects($this->at(0)) + ->method('getUploadedFile') + ->with('uploadlogo') + ->willReturn(null); + $this->request + ->expects($this->at(1)) + ->method('getUploadedFile') + ->with('upload-login-background') + ->willReturn([ + 'tmp_name' => $tmpLogo, + 'type' => 'text/svg', + 'name' => 'logo.svg', + ]); + $this->l10n + ->expects($this->once()) + ->method('t') + ->with('Unsupported image type') + ->willReturn('Unsupported image type'); + $file = $this->getMockBuilder('\\OCP\\Files\\File') + ->disableOriginalConstructor() + ->getMock(); + $this->rootFolder + ->expects($this->once()) + ->method('newFile') + ->with('themedbackgroundlogo') + ->willReturn($file); + $expected = new DataResponse( + [ + 'data' => + [ + 'message' => 'Unsupported image type', + ], + 'status' => 'failure' + ], + Http::STATUS_UNPROCESSABLE_ENTITY + ); + $this->assertEquals($expected, $this->themingController->updateLogo()); + } + public function testUndo() { $this->l10n ->expects($this->once()) @@ -311,6 +358,7 @@ class ThemingControllerTest extends TestCase { $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); $expected->addHeader('Content-Disposition', 'attachment'); $expected->addHeader('Content-Type', 'text/svg'); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getLogo()); } @@ -340,6 +388,7 @@ class ThemingControllerTest extends TestCase { $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); $expected->addHeader('Content-Disposition', 'attachment'); $expected->addHeader('Content-Type', 'image/png'); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getLoginBackground()); } @@ -400,6 +449,7 @@ class ThemingControllerTest extends TestCase { $expected->cacheFor(3600); $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getStylesheet()); } @@ -464,6 +514,7 @@ class ThemingControllerTest extends TestCase { $expected->cacheFor(3600); $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getStylesheet()); } @@ -507,6 +558,7 @@ class ThemingControllerTest extends TestCase { $expected->cacheFor(3600); $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getStylesheet()); } @@ -542,6 +594,7 @@ class ThemingControllerTest extends TestCase { $expected->cacheFor(3600); $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getStylesheet()); } @@ -618,6 +671,7 @@ class ThemingControllerTest extends TestCase { $expected->cacheFor(3600); $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getStylesheet()); } @@ -698,6 +752,7 @@ class ThemingControllerTest extends TestCase { $expected->cacheFor(3600); $expected->addHeader('Expires', date(\DateTime::RFC2822, 123)); + $expected->addHeader('Pragma', 'cache'); @$this->assertEquals($expected, $this->themingController->getStylesheet()); } @@ -732,6 +787,7 @@ class ThemingControllerTest extends TestCase { $expected = new Http\DataDisplayResponse($expectedResponse); $expected->addHeader("Content-type","text/javascript"); $expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime())); + $expected->addHeader('Pragma', 'cache'); $expected->cacheFor(3600); @$this->assertEquals($expected, $this->themingController->getJavascript()); } @@ -765,6 +821,7 @@ class ThemingControllerTest extends TestCase { $expected = new Http\DataDisplayResponse($expectedResponse); $expected->addHeader("Content-type","text/javascript"); $expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime())); + $expected->addHeader('Pragma', 'cache'); $expected->cacheFor(3600); @$this->assertEquals($expected, $this->themingController->getJavascript()); } From 49da5267a908cec8fe6af937c758bbb261336d39 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Sat, 27 Aug 2016 21:38:41 +0200 Subject: [PATCH 2/2] Use temporary file as cache --- apps/theming/lib/Controller/ThemingController.php | 12 ++++++++++-- .../tests/Controller/ThemingControllerTest.php | 8 ++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index faf59e4bb1..24fa10e89a 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -39,6 +39,7 @@ use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; use OCA\Theming\Util; +use OCP\ITempManager; /** * Class ThemingController @@ -60,6 +61,8 @@ class ThemingController extends Controller { private $config; /** @var IRootFolder */ private $rootFolder; + /** @var ITempManager */ + private $tempManager; /** * ThemingController constructor. @@ -72,6 +75,7 @@ class ThemingController extends Controller { * @param ITimeFactory $timeFactory * @param IL10N $l * @param IRootFolder $rootFolder + * @param ITempManager $tempManager */ public function __construct( $appName, @@ -81,7 +85,8 @@ class ThemingController extends Controller { Util $util, ITimeFactory $timeFactory, IL10N $l, - IRootFolder $rootFolder + IRootFolder $rootFolder, + ITempManager $tempManager ) { parent::__construct($appName, $request); @@ -91,6 +96,7 @@ class ThemingController extends Controller { $this->l = $l; $this->config = $config; $this->rootFolder = $rootFolder; + $this->tempManager = $tempManager; } /** @@ -199,14 +205,16 @@ class ThemingController extends Controller { // Optimize the image since some people may upload images that will be // either to big or are not progressive rendering. + $tmpFile = $this->tempManager->getTemporaryFile(); if(function_exists('imagescale')) { // FIXME: Once PHP 5.5.0 is a requirement the above check can be removed $image = imagescale($image, 1920); } imageinterlace($image, 1); - imagejpeg($image, $target->fopen('w'), 75); + imagejpeg($image, $tmpFile, 75); imagedestroy($image); + stream_copy_to_stream(fopen($tmpFile, 'r'), $target->fopen('w')); $this->template->set('backgroundMime', $newBackgroundLogo['type']); $name = $newBackgroundLogo['name']; } diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 3e49fa9206..60f1ecbf6f 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -32,6 +32,7 @@ use OCP\Files\IRootFolder; use OCP\IConfig; use OCP\IL10N; use OCP\IRequest; +use OCP\ITempManager; use Test\TestCase; use OCA\Theming\ThemingDefaults; @@ -52,6 +53,8 @@ class ThemingControllerTest extends TestCase { private $themingController; /** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */ private $rootFolder; + /** @var ITempManager */ + private $tempManager; public function setUp() { $this->request = $this->getMockBuilder('OCP\IRequest')->getMock(); @@ -64,10 +67,10 @@ class ThemingControllerTest extends TestCase { ->getMock(); $this->l10n = $this->getMockBuilder('OCP\IL10N')->getMock(); $this->rootFolder = $this->getMockBuilder('OCP\Files\IRootFolder')->getMock(); - $this->timeFactory->expects($this->any()) ->method('getTime') ->willReturn(123); + $this->tempManager = \OC::$server->getTempManager(); $this->themingController = new ThemingController( 'theming', @@ -77,7 +80,8 @@ class ThemingControllerTest extends TestCase { $this->util, $this->timeFactory, $this->l10n, - $this->rootFolder + $this->rootFolder, + $this->tempManager ); return parent::setUp();