Merge pull request #1103 from nextcloud/improve-theming-performance

Increase theming performance

3 files changed, 107 insertions, 10 deletions

diff --git a/apps/theming/js/settings-admin.js b/apps/theming/js/settings-admin.js
index 0de3b28bab0..c896da321c8 100644
--- a/apps/theming/js/settings-admin.js
+++ b/apps/theming/js/settings-admin.js
@@ -157,8 +157,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 = {
@@ -171,8 +171,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 8a7aaec6b5e..fbb4c904773 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;
 	}
 
 	/**
@@ -171,7 +177,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 +189,32 @@ 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.
+			$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, $tmpFile, 75);
+			imagedestroy($image);
+
+			stream_copy_to_stream(fopen($tmpFile, 'r'), $target->fopen('w'));
 			$this->template->set('backgroundMime', $newBackgroundLogo['type']);
 			$name = $newBackgroundLogo['name'];
 		}
@@ -236,6 +268,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 +289,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;
 	}
 
@@ -358,6 +392,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;
 	}
@@ -378,8 +413,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 da2137e9da0..d053d8c1a1c 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();
@@ -214,6 +218,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 +266,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 +362,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 +392,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());
 	}
 
@@ -422,6 +475,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());
 	}
 
@@ -510,6 +564,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());
 	}
 
@@ -553,6 +608,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());
 	}
 
@@ -588,6 +644,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());
 	}
 
@@ -687,6 +744,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());
 	}
 
@@ -791,6 +849,7 @@ class ThemingControllerTest extends TestCase {
 		$expected = new Http\DataDownloadResponse($expectedData, 'style', 'text/css');
 		$expected->cacheFor(3600);
 		$expected->addHeader('Expires', date(\DateTime::RFC2822, 123));
+		$expected->addHeader('Pragma', 'cache');
 		@$this->assertEquals($expected, $this->themingController->getStylesheet());
 	}
 
@@ -825,6 +884,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());
 	}
@@ -858,6 +918,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());
 	}