jenkins-bot has submitted this change and it was merged. Change subject: Use cross-origin img attribute instead of data URI ......................................................................
Use cross-origin img attribute instead of data URI After lots of experimenting with Wireshark and current Chrome + Firefox on Ubuntu 13.10, this is my current understanding of the caching when preloading images with AJAX requests: * on Chrome, the image request always comes from browser cache * Firefox makes two separate requests by default * Firefox with img.crossOrigin = 'anonymous' makes two separate requests, but the second one is a 304 (does not load the image twice) * when the image has already been cached by the browser (but not in this session), Chrome skips both requests; Firefox skips the AJAX request, but sends the normal one, and it returns with 304. "wish I knew this when I started" things: * the Chrome DevTools has an option to disable cache. When this is enabled, requests in the same document context still come from cache (so if I load the page, fire an AJAX request, then without reloading the page, fire an AJAX request to the same URL, then the second request will be cached), but an AJAX request - image request pair is an exception from this. * when using Ctrl-F5 in Firefox, requests on that page will never hit the cache (even AJAX request fired after user activity; even if two identical requests follow each other). When using clear cache + normal reload, this is not the case. * if the image does not have an Allow-Origin header and is loaded with crossOrigin=true, Firefox will refuse to load it. Chrome will log an error in the console saying it refused to load it, but will actually load it. * Wireshark rocks. Pushed some tech debt (browser + domain whitelist) into other tickets: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/232 https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/233 Reverted commits: 8a8d74f01d3dbd6d0c43b7fadc5284d204091761. 63021d0b0e95442cce101f9f92de8f0ff97d5f49. Change-Id: I84ab2f3ac0a9706926adf7fe8726ecd9e9f843e0 Bug: 61542 Mingle: https://wikimedia.mingle.thoughtworks.com/projects/multimedia/cards/207 --- M resources/mmv/mmv.performance.js M resources/mmv/provider/mmv.provider.Image.js M tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb M tests/qunit/mmv.performance.test.js M tests/qunit/provider/mmv.provider.Image.test.js 5 files changed, 206 insertions(+), 128 deletions(-) Approvals: Gilles: Looks good to me, approved jenkins-bot: Verified diff --git a/resources/mmv/mmv.performance.js b/resources/mmv/mmv.performance.js index 5c98f3e..a6b6403 100755 --- a/resources/mmv/mmv.performance.js +++ b/resources/mmv/mmv.performance.js @@ -29,12 +29,6 @@ P = Performance.prototype; /** - * How long to wait to ensure window.performance is populated - * @property {number} - */ - P.delay = 1000; - - /** * Global setup that should be done while the page loads */ P.init = function () { @@ -53,39 +47,30 @@ * cached by the browser, as it will consume unnecessary bandwidth for the user. * @param {string} type the type of request to be measured * @param {string} url URL to be measured - * @param {string} responseType responseType for the XHR options * @returns {jQuery.Promise} A promise that resolves when the contents of the URL have been fetched */ - P.record = function ( type, url, responseType ) { + P.record = function ( type, url ) { var deferred = $.Deferred(), request, perf = this, start; - request = this.newXHR(); - - request.onreadystatechange = function () { - var total = $.now() - start; - - if ( request.readyState === 4 ) { - deferred.resolve( request.response ); - - perf.recordEntryDelayed( type, total, url, request ); - } - }; - - start = $.now(); - try { + request = this.newXHR(); + request.onreadystatechange = function () { + var total = $.now() - start; + + if ( request.readyState === 4 ) { + deferred.resolve( request.response ); + perf.recordEntryDelayed( type, total, url, request ); + } + }; + + start = $.now(); request.open( 'GET', url, true ); - - if ( responseType !== undefined ) { - request.responseType = responseType; - } - request.send(); } catch ( e ) { - // This happens on old browsers that don't support CORS + // old browser not supporting XMLHttpRequest or CORS, or CORS is not permitted return deferred.reject(); } @@ -123,7 +108,7 @@ return; } - // Don't record entries that hit the browser cache on undetailed requests + // Don't record entries that hit the browser cache if ( total !== undefined && total < 1 ) { return; } @@ -322,7 +307,7 @@ // it hasn't been added yet at this point setTimeout( function() { perf.recordEntry( type, total, url, request ); - }, this.delay ); + }, 0 ); }; /** @@ -427,4 +412,5 @@ new Performance().init(); mw.mmv.Performance = Performance; + }( mediaWiki, jQuery ) ); diff --git a/resources/mmv/provider/mmv.provider.Image.js b/resources/mmv/provider/mmv.provider.Image.js index 3abb4af..ee9a2f9 100644 --- a/resources/mmv/provider/mmv.provider.Image.js +++ b/resources/mmv/provider/mmv.provider.Image.js @@ -38,43 +38,26 @@ /** * Loads an image and returns it. + * Includes performance metrics. * @param {string} url * @return {jQuery.Promise.<HTMLImageElement>} a promise which resolves to the image object */ Image.prototype.get = function ( url ) { - var deferred, start, - img = new window.Image(), + var provider = this, cacheKey = url, - provider = this; + start, + rawGet; if ( !this.cache[cacheKey] ) { - deferred = $.Deferred(); - this.cache[cacheKey] = deferred.promise(); - - img.onload = function() { - // Start is only defined for old browsers - if ( start !== undefined ) { - provider.performance.recordEntry( 'image', $.now() - start ); - } - deferred.resolve( img ); - }; - - img.onerror = function() { - deferred.reject( 'could not load image from ' + url ); - }; - - // We can only measure detailed image performance on browsers that are capable of - // reading the XHR binary results as data URI. Otherwise loading the image as XHR - // and then assigning the same URL to an image's src attribute - // would cause a double request (won't hit browser cache). - if ( this.browserSupportsBinaryOperations() ) { - this.performance.record( 'image', url, 'arraybuffer' ).then( function( response ) { - img.src = provider.binaryToDataURI( response ); - } ); + if ( this.imagePreloadingSupported() ) { + rawGet = $.proxy( provider.rawGet, provider, url, true ); + this.cache[cacheKey] = this.performance.record( 'image', url ).then( rawGet, rawGet ); } else { - // On old browsers we just do oldschool timing without details start = $.now(); - img.src = url; + this.cache[cacheKey] = this.rawGet( url ); + this.cache[cacheKey].always( function () { + provider.performance.recordEntry( 'image', $.now() - start, url ); + } ); } } @@ -82,34 +65,46 @@ }; /** - * Converts a binary image into a data URI - * @param {string} binary Binary image - * @returns {string} base64-encoded data URI representing the image + * Internal version of get(): no caching, no performance metrics. + * @param {string} url + * @param {boolean} [cors] if true, use CORS for preloading + * @return {jQuery.Promise.<HTMLImageElement>} a promise which resolves to the image object */ - Image.prototype.binaryToDataURI = function ( binary ) { - var i, - bytes = new Uint8Array( binary ), - raw = ''; + Image.prototype.rawGet = function ( url, cors ) { + var img = new window.Image(), + deferred = $.Deferred(); - for ( i = 0; i < bytes.length; i++ ) { - raw += String.fromCharCode( bytes[ i ] ); + // On Firefox this will trigger a CORS request for the image, instead of a normal one, + // and allows using the image in a canvas etc. + // We don't really care about that, but it seems the request will share cache with the + // AJAX requests this way, so we can do AJAX preloading. + // On other browsers hopefully it will have the same effect or at least won't make + // things worse. + // FIXME the image won't load of there is no Allowed-Origin header! we will want a + // whitelist for this. + if ( cors ) { + img.crossOrigin = 'anonymous'; } - // I've tested this on Firefox, Chrome, IE10, IE11, Safari, Opera - // and for all of them we can get away with not giving the proper mime type - // If we run into a browser where that's a problem, we'll need - // to read the binary contents to determine the mime type - return 'data:image;base64,' + btoa( raw ); + img.onload = function() { + deferred.resolve( img ); + }; + img.onerror = function() { + deferred.reject( 'could not load image from ' + url ); + }; + + img.src = url; + + return deferred; }; /** - * Checks if the browser is capable of converting binary content to a data URI - * @returns {boolean} + * Checks whether the current browser supports AJAX preloading of images. + * @return {boolean} */ - Image.prototype.browserSupportsBinaryOperations = function () { - return window.btoa !== undefined && - window.Uint8Array !== undefined && - String.fromCharCode !== undefined; + Image.prototype.imagePreloadingSupported = function () { + // FIXME this is a *very* rough guess, but it'll work as the first estimation. + return 'crossOrigin' in new Image(); }; mw.mmv.provider.Image = Image; diff --git a/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb b/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb index fd831b0..664f7b0 100644 --- a/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb +++ b/tests/browser/features/step_definitions/basic_mmv_navigation_steps.rb @@ -69,7 +69,7 @@ page.mmv_image_div_element.should be_visible # Check image content - page.mmv_image_div_element.image_element.attribute('src').should match /Kerala|^data:image;base64,.+gBeRmlsZSB/ + page.mmv_image_div_element.image_element.attribute('src').should match /Kerala/ # Check basic metadata is present @@ -112,7 +112,7 @@ page.mmv_image_div_element.should be_visible # Check image content - page.mmv_image_div_element.image_element.attribute('src').should match /Offsite|^data:image;base64,.+gB4Rmls/ + page.mmv_image_div_element.image_element.attribute('src').should match /Offsite/ # Check basic metadata is present diff --git a/tests/qunit/mmv.performance.test.js b/tests/qunit/mmv.performance.test.js index 6a938d5..5f5e27d 100644 --- a/tests/qunit/mmv.performance.test.js +++ b/tests/qunit/mmv.performance.test.js @@ -30,6 +30,24 @@ }; } + function createFakeXHR( response ) { + return { + readyState: 0, + open: $.noop, + send: function () { + var xhr = this; + + setTimeout( function () { + xhr.readyState = 4; + xhr.response = response; + if ( $.isFunction( xhr.onreadystatechange ) ) { + xhr.onreadystatechange(); + } + }, 0 ); + } + }; + } + QUnit.test( 'recordEntry: basic', 5, function ( assert ) { var performance = new mw.mmv.Performance(), oldEventLog = mw.eventLog, @@ -235,6 +253,42 @@ assert.ok( $.isEmptyObject( varnishXCache ), 'Varnish cache results are empty' ); } ); + QUnit.test( 'record()', 4, function ( assert ) { + var type = 'foo', + url = 'http://example.com/', + response = {}, + performance = new mw.mmv.Performance(); + + performance.newXHR = function () { return createFakeXHR( response ); }; + + QUnit.stop(); + performance.recordEntryDelayed = function ( recordType, _, recordUrl, recordRequest ) { + assert.strictEqual( recordType, type, 'type is recorded correctly' ); + assert.strictEqual( recordUrl, url, 'url is recorded correctly' ); + assert.strictEqual( recordRequest.response, response, 'response is recorded correctly' ); + QUnit.start(); + }; + + QUnit.stop(); + performance.record( type, url ).done( function ( recordResponse ) { + assert.strictEqual( recordResponse, response, 'response is passed to callback' ); + QUnit.start(); + } ); + } ); + + QUnit.asyncTest( 'record() with old browser', 1, function ( assert ) { + var type = 'foo', + url = 'http://example.com/', + performance = new mw.mmv.Performance(); + + performance.newXHR = function () { throw 'XMLHttpRequest? What\'s that?'; }; + + performance.record( type, url ).fail( function () { + assert.ok( true, 'the promise is rejected when XMLHttpRequest is not supported' ); + QUnit.start(); + } ); + } ); + QUnit.test( 'mw.mmv.Api', 3, function ( assert ) { var api, oldRecord = mw.mmv.Performance.prototype.recordJQueryEntryDelayed, diff --git a/tests/qunit/provider/mmv.provider.Image.test.js b/tests/qunit/provider/mmv.provider.Image.test.js index 16dbfe7..eae5754 100644 --- a/tests/qunit/provider/mmv.provider.Image.test.js +++ b/tests/qunit/provider/mmv.provider.Image.test.js @@ -16,22 +16,6 @@ */ ( function ( mw, $ ) { - var i, - binary, - dataURI = 'data:image;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQAQMAAAAlPW0' - + 'iAAAABlBMVEUAAAD///+l2Z/dAAAAM0lEQVR4nGP4/5/h/1+G/58ZDrAz3D/McH' - + '8yw83NDDeNGe4Ug9C9zwz3gVLMDA/A6P9/AFGGFyjOXZtQAAAAAElFTkSuQmCC', - hex = '89504e470d0a1a0a0000000d4948445200000010000000100103000000253d6d' - + '2200000006504c5445000000ffffffa5d99fdd0000003349444154789c63f8ff' - + '9fe1ff5f86ff9f190eb033dc3fcc707f32c3cdcd0c378d19ee1483d0bdcf0cf7' - + '8152cc0c0fc0e8ff7f0051861728ce5d9b500000000049454e44ae426082'; - - binary = new Uint8Array( hex.length / 2 ); - - for ( i = 0; i < hex.length; i += 2 ) { - binary[ i / 2 ] = parseInt( hex.substr( i, 2 ), 16 ); - } - QUnit.module( 'mmv.provider.Image', QUnit.newMwEnvironment() ); QUnit.test( 'Image constructor sanity check', 1, function ( assert ) { @@ -40,54 +24,113 @@ assert.ok( imageProvider ); } ); - QUnit.test( 'Image load success test', 5, function ( assert ) { - var imageProvider = new mw.mmv.provider.Image(), - oldPerformance = imageProvider.performance, - fakeURL = 'fakeURL'; + QUnit.test( 'Image load success test', 2, function ( assert ) { + var url = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQAQMAAAAlPW0' + + 'iAAAABlBMVEUAAAD///+l2Z/dAAAAM0lEQVR4nGP4/5/h/1+G/58ZDrAz3D/McH' + + '8yw83NDDeNGe4Ug9C9zwz3gVLMDA/A6P9/AFGGFyjOXZtQAAAAAElFTkSuQmCC', + imageProvider = new mw.mmv.provider.Image(); - imageProvider.performance.delay = 0; - - imageProvider.performance.newXHR = function () { - return { readyState: 4, - response: binary, - send: function () { this.onreadystatechange(); }, - open: $.noop }; + imageProvider.performance = { + imagePreloadingSupported: function () { return false; }, + recordEntry: $.noop }; QUnit.stop(); - imageProvider.performance.recordEntry = function ( type, total, url ) { - assert.strictEqual( type, 'image', 'Type matches' ); - assert.ok( total < 10, 'Total is less than 10ms' ); - assert.strictEqual( url, fakeURL, 'URL matches' ); - - QUnit.start(); - - imageProvider.performance = oldPerformance; - - return $.Deferred().resolve(); - }; - - QUnit.stop(); - imageProvider.get( fakeURL ).then( function( image ) { + imageProvider.get( url ).then( function( image ) { assert.ok( image instanceof HTMLImageElement, 'success handler was called with the image element'); - assert.strictEqual( image.src, dataURI ); + assert.strictEqual( image.src, url, 'image src is correct'); QUnit.start(); } ); + } ); + + QUnit.test( 'Image caching test', 6, function ( assert ) { + var url = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQAQMAAAAlPW0' + + 'iAAAABlBMVEUAAAD///+l2Z/dAAAAM0lEQVR4nGP4/5/h/1+G/58ZDrAz3D/McH' + + '8yw83NDDeNGe4Ug9C9zwz3gVLMDA/A6P9/AFGGFyjOXZtQAAAAAElFTkSuQmCC', + url2 = 'data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==', + result, + imageProvider = new mw.mmv.provider.Image(); + + imageProvider.performance = { + imagePreloadingSupported: function () { return false; }, + recordEntry: $.noop + }; + + QUnit.stop(); + imageProvider.get( url ).then( function( image ) { + result = image; + assert.ok( image instanceof HTMLImageElement, + 'success handler was called with the image element'); + assert.strictEqual( image.src, url, 'image src is correct'); + QUnit.start(); + } ); + + QUnit.stop(); + imageProvider.get( url ).then( function( image ) { + assert.strictEqual( image, result, 'image element is cached and not regenerated' ); + assert.strictEqual( image.src, url, 'image src is correct'); + QUnit.start(); + } ); + + QUnit.stop(); + imageProvider.get( url2 ).then( function( image ) { + assert.notStrictEqual( image, result, 'image element for different url is not cached' ); + assert.strictEqual( image.src, url2, 'image src is correct'); + QUnit.start(); + } ); + } ); QUnit.asyncTest( 'Image load fail test', 1, function ( assert ) { var imageProvider = new mw.mmv.provider.Image(); + imageProvider.performance = { + imagePreloadingSupported: function () { return false; }, + recordEntry: $.noop + }; + imageProvider.get( 'doesntexist.png' ).fail( function() { - assert.ok( true, 'fail handler was called' ); - QUnit.start(); - } ); + assert.ok( true, 'fail handler was called' ); + QUnit.start(); + } ); } ); - QUnit.test( 'binaryToDataURI', 1, function ( assert ) { - var imageProvider = new mw.mmv.provider.Image(); + QUnit.test( 'Image load test with preloading supported', 1, function ( assert ) { + var url = mw.config.get( 'wgScriptPath' ) + '/skins/vector/images/search-ltr.png', + imageProvider = new mw.mmv.provider.Image(), + endsWith = function ( a, b ) { return a.indexOf( b ) === a.length - b.length; }; - assert.strictEqual( imageProvider.binaryToDataURI( binary ), dataURI, 'Binary is correctly converted to data URI' ); + imageProvider.imagePreloadingSupported = function () { return true; }; + imageProvider.performance = { + record: function() { return $.Deferred().resolve(); } + }; + + QUnit.stop(); + imageProvider.get( url ).done( function( image ) { + // can't test equality as browsers transform this to a full URL + assert.ok( endsWith( image.src, url ), 'local image loaded with correct source'); + QUnit.start(); + } ).fail( function () { + // do not hold up the tests if the image failed to load + assert.ok( false, 'uh-oh, couldnt load - might be a problem with the test installation' ); + QUnit.start(); + } ); + } ); + + QUnit.test( 'Failed image load test with preloading supported', 1, function ( assert ) { + var url = 'nosuchimage.png', + imageProvider = new mw.mmv.provider.Image(); + + imageProvider.imagePreloadingSupported = function () { return true; }; + imageProvider.performance = { + record: function() { return $.Deferred().resolve(); } + }; + + QUnit.stop(); + imageProvider.get( url ).fail( function () { + assert.ok( true, 'Fail callback called for non-existing image' ); + QUnit.start(); + } ); } ); }( mediaWiki, jQuery ) ); -- To view, visit https://gerrit.wikimedia.org/r/114403 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I84ab2f3ac0a9706926adf7fe8726ecd9e9f843e0 Gerrit-PatchSet: 11 Gerrit-Project: mediawiki/extensions/MultimediaViewer Gerrit-Branch: master Gerrit-Owner: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Aarcos <aarcos.w...@gmail.com> Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Gilles <gdu...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
