diff --git a/toolkit/components/places/FaviconHelpers.cpp b/toolkit/components/places/FaviconHelpers.cpp index 752acf1d6668..8e0ab0dbbf4c 100644 --- a/toolkit/components/places/FaviconHelpers.cpp +++ b/toolkit/components/places/FaviconHelpers.cpp @@ -310,7 +310,7 @@ FetchIconInfo(const RefPtr& aDB, "FROM moz_icons " "WHERE fixed_icon_url_hash = hash(fixup_url(:url)) " "AND icon_url = :url " - "ORDER BY width ASC " + "ORDER BY width DESC " ); NS_ENSURE_STATE(stmt); mozStorageStatementScoper scoper(stmt); @@ -880,25 +880,21 @@ AsyncAssociateIconToPage::Run() ":icon_id) " ); NS_ENSURE_STATE(stmt); - mozStorageStatementScoper scoper(stmt); - nsCOMPtr paramsArray; - rv = stmt->NewBindingParamsArray(getter_AddRefs(paramsArray)); - NS_ENSURE_SUCCESS(rv, rv); + + // For some reason using BindingParamsArray here fails execution, so we must + // execute the statements one by one. + // In the future we may want to investigate the reasons, sounds like related + // to contraints. for (const auto& payload : mIcon.payloads) { + mozStorageStatementScoper scoper(stmt); nsCOMPtr params; - rv = paramsArray->NewBindingParams(getter_AddRefs(params)); + rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPage.spec); NS_ENSURE_SUCCESS(rv, rv); - rv = URIBinder::Bind(params, NS_LITERAL_CSTRING("page_url"), mPage.spec); + rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("icon_id"), payload.id); NS_ENSURE_SUCCESS(rv, rv); - rv = params->BindInt64ByName(NS_LITERAL_CSTRING("icon_id"), payload.id); - NS_ENSURE_SUCCESS(rv, rv); - rv = paramsArray->AddParams(params); + rv = stmt->Execute(); NS_ENSURE_SUCCESS(rv, rv); } - rv = stmt->BindParameters(paramsArray); - NS_ENSURE_SUCCESS(rv, rv); - rv = stmt->Execute(); - NS_ENSURE_SUCCESS(rv, rv); mIcon.status |= ICON_STATUS_ASSOCIATED; diff --git a/toolkit/components/places/FaviconHelpers.h b/toolkit/components/places/FaviconHelpers.h index 95815340b010..b63c896cef71 100644 --- a/toolkit/components/places/FaviconHelpers.h +++ b/toolkit/components/places/FaviconHelpers.h @@ -119,6 +119,21 @@ struct PageData nsCString guid; }; +/** + * Info for a frame. + */ +struct FrameData +{ + FrameData(uint16_t aIndex, uint16_t aWidth) + : index(aIndex) + , width(aWidth) + { + } + + uint16_t index; + uint16_t width; +}; + /** * Async fetches icon from database or network, associates it with the required * page and finally notifies the change. diff --git a/toolkit/components/places/nsFaviconService.cpp b/toolkit/components/places/nsFaviconService.cpp index 6e8d393075d4..3b8443d8dca9 100644 --- a/toolkit/components/places/nsFaviconService.cpp +++ b/toolkit/components/places/nsFaviconService.cpp @@ -57,6 +57,69 @@ public: NS_IMETHOD HandleCompletion(uint16_t aReason); }; +namespace { + +/** + * Extracts and filters native sizes from the given container, based on the + * list of sizes we are supposed to retain. + * All calculation is done considering square sizes and the largest side. + * In case of multiple frames of the same size, only the first one is retained. + */ +nsresult +GetFramesInfoForContainer(imgIContainer* aContainer, + nsTArray& aFramesInfo) { + // Don't extract frames from animated images. + bool animated; + nsresult rv = aContainer->GetAnimated(&animated); + if (NS_FAILED(rv) || !animated) { + nsTArray nativeSizes; + rv = aContainer->GetNativeSizes(nativeSizes); + if (NS_SUCCEEDED(rv) && nativeSizes.Length() > 1) { + for (uint32_t i = 0; i < nativeSizes.Length(); ++i) { + nsIntSize nativeSize = nativeSizes[i]; + // Only retain square frames. + if (nativeSize.width != nativeSize.height) { + continue; + } + // Check if it's one of the sizes we care about. + auto end = std::end(sFaviconSizes); + uint16_t* matchingSize = std::find(std::begin(sFaviconSizes), end, + nativeSize.width); + if (matchingSize != end) { + // We must avoid duped sizes, an image could contain multiple frames of + // the same size, but we can only store one. We could use an hashtable, + // but considered the average low number of frames, we'll just do a + // linear search. + bool dupe = false; + for (const auto& frameInfo : aFramesInfo) { + if (frameInfo.width == *matchingSize) { + dupe = true; + break; + } + } + if (!dupe) { + aFramesInfo.AppendElement(FrameData(i, *matchingSize)); + } + } + } + } + } + + if (aFramesInfo.Length() == 0) { + // Always have at least the default size. + int32_t width; + rv = aContainer->GetWidth(&width); + NS_ENSURE_SUCCESS(rv, rv); + int32_t height; + rv = aContainer->GetHeight(&height); + NS_ENSURE_SUCCESS(rv, rv); + // For non-square images, pick the largest side. + aFramesInfo.AppendElement(FrameData(0, std::max(width, height))); + } + return NS_OK; +} + +} // namespace PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsFaviconService, gFaviconService) @@ -677,47 +740,46 @@ nsFaviconService::OptimizeIconSizes(IconData& aIcon) getter_AddRefs(container)); NS_ENSURE_SUCCESS(rv, rv); - IconPayload newPayload; - newPayload.mimeType = NS_LITERAL_CSTRING(PNG_MIME_TYPE); - // TODO: for ico files we should extract every single payload. - int32_t width; - rv = container->GetWidth(&width); + // For ICO files, we must evaluate each of the frames we care about. + nsTArray framesInfo; + rv = GetFramesInfoForContainer(container, framesInfo); NS_ENSURE_SUCCESS(rv, rv); - int32_t height; - rv = container->GetHeight(&height); - NS_ENSURE_SUCCESS(rv, rv); - // For non-square images, pick the largest side. - int32_t originalSize = std::max(width, height); - newPayload.width = originalSize; - for (uint16_t size : sFaviconSizes) { - if (size <= originalSize) { - newPayload.width = size; - break; + + for (const auto& frameInfo : framesInfo) { + IconPayload newPayload; + newPayload.mimeType = NS_LITERAL_CSTRING(PNG_MIME_TYPE); + newPayload.width = frameInfo.width; + for (uint16_t size : sFaviconSizes) { + if (size <= frameInfo.width) { + newPayload.width = size; + break; + } } - } - // If the original payload is png and the size is the same, no reason to - // rescale the image. - if (newPayload.mimeType.Equals(payload.mimeType) && - newPayload.width == originalSize) { - newPayload.data = payload.data; - } else { - // scale and recompress - nsCOMPtr iconStream; - rv = GetImgTools()->EncodeScaledImage(container, - newPayload.mimeType, - newPayload.width, - newPayload.width, - EmptyString(), - getter_AddRefs(iconStream)); - NS_ENSURE_SUCCESS(rv, rv); - // Read the stream into the new buffer. - rv = NS_ConsumeStream(iconStream, UINT32_MAX, newPayload.data); - NS_ENSURE_SUCCESS(rv, rv); - } + // If the original payload is png and the size is the same, no reason to + // rescale the image. + if (newPayload.mimeType.Equals(payload.mimeType) && + newPayload.width == frameInfo.width) { + newPayload.data = payload.data; + } else { + // Scale and recompress. + // Since EncodeScaledImage use SYNC_DECODE, it will pick the best frame. + nsCOMPtr iconStream; + rv = GetImgTools()->EncodeScaledImage(container, + newPayload.mimeType, + newPayload.width, + newPayload.width, + EmptyString(), + getter_AddRefs(iconStream)); + NS_ENSURE_SUCCESS(rv, rv); + // Read the stream into the new buffer. + rv = NS_ConsumeStream(iconStream, UINT32_MAX, newPayload.data); + NS_ENSURE_SUCCESS(rv, rv); + } - if (newPayload.data.Length() < nsIFaviconService::MAX_FAVICON_BUFFER_SIZE) { - aIcon.payloads.AppendElement(newPayload); + if (newPayload.data.Length() < nsIFaviconService::MAX_FAVICON_BUFFER_SIZE) { + aIcon.payloads.AppendElement(newPayload); + } } return NS_OK; diff --git a/toolkit/components/places/tests/favicons/expected-favicon-big48.ico.png b/toolkit/components/places/tests/favicons/expected-favicon-big48.ico.png index 0d6c1b14a030..5f2db533000e 100644 Binary files a/toolkit/components/places/tests/favicons/expected-favicon-big48.ico.png and b/toolkit/components/places/tests/favicons/expected-favicon-big48.ico.png differ diff --git a/toolkit/components/places/tests/favicons/favicon-multi-frame16.png b/toolkit/components/places/tests/favicons/favicon-multi-frame16.png new file mode 100644 index 000000000000..9f64ce06b0cf Binary files /dev/null and b/toolkit/components/places/tests/favicons/favicon-multi-frame16.png differ diff --git a/toolkit/components/places/tests/favicons/favicon-multi-frame32.png b/toolkit/components/places/tests/favicons/favicon-multi-frame32.png new file mode 100644 index 000000000000..f9406345b100 Binary files /dev/null and b/toolkit/components/places/tests/favicons/favicon-multi-frame32.png differ diff --git a/toolkit/components/places/tests/favicons/favicon-multi-frame64.png b/toolkit/components/places/tests/favicons/favicon-multi-frame64.png new file mode 100644 index 000000000000..9db56436e70b Binary files /dev/null and b/toolkit/components/places/tests/favicons/favicon-multi-frame64.png differ diff --git a/toolkit/components/places/tests/favicons/favicon-multi.ico b/toolkit/components/places/tests/favicons/favicon-multi.ico new file mode 100644 index 000000000000..e98adcafebd6 Binary files /dev/null and b/toolkit/components/places/tests/favicons/favicon-multi.ico differ diff --git a/toolkit/components/places/tests/favicons/test_favicons_conversions.js b/toolkit/components/places/tests/favicons/test_favicons_conversions.js index da69444afa71..5411d7eda878 100644 --- a/toolkit/components/places/tests/favicons/test_favicons_conversions.js +++ b/toolkit/components/places/tests/favicons/test_favicons_conversions.js @@ -29,103 +29,96 @@ var isWindows = ("@mozilla.org/windows-registry-key;1" in Cc); * @param aCallback * This function is called after the check finished. */ -function checkFaviconDataConversion(aFileName, aFileMimeType, aFileLength, - aExpectConversion, aVaryOnWindows, - aCallback) { +function* checkFaviconDataConversion(aFileName, aFileMimeType, aFileLength, + aExpectConversion, aVaryOnWindows) { let pageURI = NetUtil.newURI("http://places.test/page/" + aFileName); - PlacesTestUtils.addVisits({ uri: pageURI, transition: TRANSITION_TYPED }).then( - function() { - let faviconURI = NetUtil.newURI("http://places.test/icon/" + aFileName); - let fileData = readFileOfLength(aFileName, aFileLength); + yield PlacesTestUtils.addVisits({ uri: pageURI, transition: TRANSITION_TYPED }); + let faviconURI = NetUtil.newURI("http://places.test/icon/" + aFileName); + let fileData = readFileOfLength(aFileName, aFileLength); - PlacesUtils.favicons.replaceFaviconData(faviconURI, fileData, fileData.length, - aFileMimeType); - PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, faviconURI, true, - PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, - function CFDC_verify(aURI, aDataLen, aData, aMimeType) { - if (!aExpectConversion) { - do_check_true(compareArrays(aData, fileData)); - do_check_eq(aMimeType, aFileMimeType); - } else { - if (!aVaryOnWindows || !isWindows) { - let expectedFile = do_get_file("expected-" + aFileName + ".png"); - do_check_true(compareArrays(aData, readFileData(expectedFile))); - } - do_check_eq(aMimeType, "image/png"); + PlacesUtils.favicons.replaceFaviconData(faviconURI, fileData, fileData.length, + aFileMimeType); + yield new Promise(resolve => { + PlacesUtils.favicons.setAndFetchFaviconForPage(pageURI, faviconURI, true, + PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE, + (aURI, aDataLen, aData, aMimeType) => { + if (!aExpectConversion) { + do_check_true(compareArrays(aData, fileData)); + do_check_eq(aMimeType, aFileMimeType); + } else { + if (!aVaryOnWindows || !isWindows) { + let expectedFile = do_get_file("expected-" + aFileName + ".png"); + do_check_true(compareArrays(aData, readFileData(expectedFile))); } - - aCallback(); - }, Services.scriptSecurityManager.getSystemPrincipal()); - }); + do_check_eq(aMimeType, "image/png"); + } + resolve(); + }, Services.scriptSecurityManager.getSystemPrincipal()); + }); } -// Tests -function run_test() { - run_next_test(); -} - -add_test(function test_storing_a_normal_16x16_icon() { +add_task(function* test_storing_a_normal_16x16_icon() { // 16x16 png, 286 bytes. // optimized: no - checkFaviconDataConversion("favicon-normal16.png", "image/png", 286, - false, false, run_next_test); + yield checkFaviconDataConversion("favicon-normal16.png", "image/png", 286, + false, false); }); -add_test(function test_storing_a_normal_32x32_icon() { +add_task(function* test_storing_a_normal_32x32_icon() { // 32x32 png, 344 bytes. // optimized: no - checkFaviconDataConversion("favicon-normal32.png", "image/png", 344, - false, false, run_next_test); + yield checkFaviconDataConversion("favicon-normal32.png", "image/png", 344, + false, false); }); -add_test(function test_storing_a_big_16x16_icon() { +add_task(function* test_storing_a_big_16x16_icon() { // in: 16x16 ico, 1406 bytes. // optimized: yes - checkFaviconDataConversion("favicon-big16.ico", "image/x-icon", 1406, - true, false, run_next_test); + yield checkFaviconDataConversion("favicon-big16.ico", "image/x-icon", 1406, + true, false); }); -add_test(function test_storing_an_oversize_4x4_icon() { +add_task(function* test_storing_an_oversize_4x4_icon() { // in: 4x4 jpg, 4751 bytes. // optimized: yes - checkFaviconDataConversion("favicon-big4.jpg", "image/jpeg", 4751, - true, false, run_next_test); + yield checkFaviconDataConversion("favicon-big4.jpg", "image/jpeg", 4751, + true, false); }); -add_test(function test_storing_an_oversize_32x32_icon() { +add_task(function* test_storing_an_oversize_32x32_icon() { // in: 32x32 jpg, 3494 bytes. // optimized: yes - checkFaviconDataConversion("favicon-big32.jpg", "image/jpeg", 3494, - true, true, run_next_test); + yield checkFaviconDataConversion("favicon-big32.jpg", "image/jpeg", 3494, + true, true); }); -add_test(function test_storing_an_oversize_48x48_icon() { +add_task(function* test_storing_an_oversize_48x48_icon() { // in: 48x48 ico, 56646 bytes. // (howstuffworks.com icon, contains 13 icons with sizes from 16x16 to // 48x48 in varying depths) // optimized: yes - checkFaviconDataConversion("favicon-big48.ico", "image/x-icon", 56646, - true, false, run_next_test); + yield checkFaviconDataConversion("favicon-big48.ico", "image/x-icon", 56646, + true, false); }); -add_test(function test_storing_an_oversize_64x64_icon() { +add_task(function* test_storing_an_oversize_64x64_icon() { // in: 64x64 png, 10698 bytes. // optimized: yes - checkFaviconDataConversion("favicon-big64.png", "image/png", 10698, - true, false, run_next_test); + yield checkFaviconDataConversion("favicon-big64.png", "image/png", 10698, + true, false); }); -add_test(function test_scaling_an_oversize_160x3_icon() { +add_task(function* test_scaling_an_oversize_160x3_icon() { // in: 160x3 jpg, 5095 bytes. // optimized: yes - checkFaviconDataConversion("favicon-scale160x3.jpg", "image/jpeg", 5095, - true, false, run_next_test); + yield checkFaviconDataConversion("favicon-scale160x3.jpg", "image/jpeg", 5095, + true, false); }); -add_test(function test_scaling_an_oversize_3x160_icon() { +add_task(function* test_scaling_an_oversize_3x160_icon() { // in: 3x160 jpg, 5059 bytes. // optimized: yes - checkFaviconDataConversion("favicon-scale3x160.jpg", "image/jpeg", 5059, - true, false, run_next_test); + yield checkFaviconDataConversion("favicon-scale3x160.jpg", "image/jpeg", 5059, + true, false); }); diff --git a/toolkit/components/places/tests/favicons/test_multiple_frames.js b/toolkit/components/places/tests/favicons/test_multiple_frames.js new file mode 100644 index 000000000000..833bc3f4b0a2 --- /dev/null +++ b/toolkit/components/places/tests/favicons/test_multiple_frames.js @@ -0,0 +1,43 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +/** + * This file tests support for icons with multiple frames (like .ico files). + */ + +add_task(function* () { + // in: 48x48 ico, 56646 bytes. + // (howstuffworks.com icon, contains 13 icons with sizes from 16x16 to + // 48x48 in varying depths) + let pageURI = NetUtil.newURI("http://places.test/page/"); + yield PlacesTestUtils.addVisits(pageURI); + let faviconURI = NetUtil.newURI("http://places.test/icon/favicon-multi.ico"); + // Fake window. + let win = { devicePixelRatio: 1.0 }; + let icoData = readFileData(do_get_file("favicon-multi.ico")); + PlacesUtils.favicons.replaceFaviconData(faviconURI, icoData, icoData.length, + "image/x-icon"); + yield setFaviconForPage(pageURI, faviconURI); + + for (let size of [16, 32, 64]) { + let file = do_get_file(`favicon-multi-frame${size}.png`); + let data = readFileData(file); + + do_print("Check getFaviconDataForPage"); + let icon = yield getFaviconDataForPage(pageURI, size); + Assert.equal(icon.mimeType, "image/png"); + Assert.deepEqual(icon.data, data); + + do_print("Check moz-anno:favicon protocol"); + yield compareFavicons( + Services.io.newFileURI(file), + PlacesUtils.urlWithSizeRef(win, PlacesUtils.favicons.getFaviconLinkForIcon(faviconURI).spec, size) + ); + + do_print("Check page-icon protocol"); + yield compareFavicons( + Services.io.newFileURI(file), + PlacesUtils.urlWithSizeRef(win, "page-icon:" + pageURI.spec, size) + ); + } +}); diff --git a/toolkit/components/places/tests/favicons/xpcshell.ini b/toolkit/components/places/tests/favicons/xpcshell.ini index 33138249276c..050dcba5dd82 100644 --- a/toolkit/components/places/tests/favicons/xpcshell.ini +++ b/toolkit/components/places/tests/favicons/xpcshell.ini @@ -14,6 +14,10 @@ support-files = favicon-big4.jpg favicon-big48.ico favicon-big64.png + favicon-multi.ico + favicon-multi-frame16.png + favicon-multi-frame32.png + favicon-multi-frame64.png favicon-normal16.png favicon-normal32.png favicon-scale160x3.jpg @@ -26,6 +30,7 @@ support-files = [test_getFaviconDataForPage.js] [test_getFaviconURLForPage.js] [test_moz-anno_favicon_mime_type.js] +[test_multiple_frames.js] [test_page-icon_protocol.js] [test_query_result_favicon_changed_on_child.js] [test_replaceFaviconData.js] diff --git a/toolkit/components/places/tests/head_common.js b/toolkit/components/places/tests/head_common.js index 2b36c07b76b9..c6ac8988cb79 100644 --- a/toolkit/components/places/tests/head_common.js +++ b/toolkit/components/places/tests/head_common.js @@ -889,6 +889,16 @@ function getFaviconUrlForPage(page, width = 0) { }); } +function getFaviconDataForPage(page, width = 0) { + let pageURI = page instanceof Ci.nsIURI ? page + : NetUtil.newURI(new URL(page).href); + return new Promise(resolve => { + PlacesUtils.favicons.getFaviconDataForPage(pageURI, (iconUri, len, data, mimeType) => { + resolve({ data, mimeType }); + }, width); + }); +} + /** * Asynchronously compares contents from 2 favicon urls. */