From 96836550256fd0ed8cc5b1afc443b7b13e053936 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Wed, 15 Mar 2017 16:08:28 +0100 Subject: [PATCH] Bug 977177 - Add size ref fragment to icon protocols. r=adw Both page-icon: and moz-anno:favicon: should support a simple #size=NNN ref fragment to allow consumers to request a specific sized icon (if available). The service will do its best to satisfy the request, but it's not guaranteed. If no size hint is found, it will default to the biggest icon payload available. It's currently not possible to test the fragment on moz-anno:favicon: since that requires imagelib support to extract payloads from ico files. Thus a test will be added at a later time. MozReview-Commit-ID: G221MKY7rfs --HG-- extra : rebase_source : 029a6fcaa9dadda24f8d3ebc9b36c1a0ec6314c6 --- .../places/PageIconProtocolHandler.js | 5 +- toolkit/components/places/PlacesUtils.jsm | 17 ++++++ .../places/nsAnnoProtocolHandler.cpp | 37 ++++++++---- .../components/places/nsFaviconService.cpp | 42 +++++++++++-- toolkit/components/places/nsFaviconService.h | 6 +- .../components/places/nsIFaviconService.idl | 9 +++ .../favicons/test_favicons_protocols_ref.js | 60 +++++++++++++++++++ .../places/tests/favicons/xpcshell.ini | 1 + .../components/places/tests/head_common.js | 4 +- 9 files changed, 157 insertions(+), 24 deletions(-) create mode 100644 toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js diff --git a/toolkit/components/places/PageIconProtocolHandler.js b/toolkit/components/places/PageIconProtocolHandler.js index 07bf90b984d8..56cc20bdbf11 100644 --- a/toolkit/components/places/PageIconProtocolHandler.js +++ b/toolkit/components/places/PageIconProtocolHandler.js @@ -92,7 +92,8 @@ PageIconProtocolHandler.prototype = { channel.contentStream = pipe.inputStream; channel.loadInfo = loadInfo; - let pageURI = NetUtil.newURI(uri.path); + let pageURI = NetUtil.newURI(uri.cloneIgnoringRef().path); + let preferredSize = PlacesUtils.favicons.preferredSizeFromURI(uri); PlacesUtils.favicons.getFaviconDataForPage(pageURI, (iconURI, len, data, mimeType) => { channel.contentType = mimeType; if (len == 0) { @@ -104,7 +105,7 @@ PageIconProtocolHandler.prototype = { streamDefaultFavicon(uri, loadInfo, pipe.outputStream); } } - }); + }, preferredSize); return channel; } catch (ex) { diff --git a/toolkit/components/places/PlacesUtils.jsm b/toolkit/components/places/PlacesUtils.jsm index d85f9026176e..952c9b679450 100644 --- a/toolkit/components/places/PlacesUtils.jsm +++ b/toolkit/components/places/PlacesUtils.jsm @@ -1671,6 +1671,23 @@ this.PlacesUtils = { return deferred.promise; }, + /** + * Returns the passed URL with a #size ref for the specified size and + * devicePixelRatio. + * + * @param window + * The window where the icon will appear. + * @param href + * The string href we should add the ref to. + * @param size + * The target image size + * @return The URL with the fragment at the end, in the same formar as input. + */ + urlWithSizeRef(window, href, size) { + return href + (href.includes("#") ? "&" : "#") + + "size=" + (Math.round(size) * window.devicePixelRatio); + }, + /** * Get the unique id for an item (a bookmark, a folder or a separator) given * its item id. diff --git a/toolkit/components/places/nsAnnoProtocolHandler.cpp b/toolkit/components/places/nsAnnoProtocolHandler.cpp index 99ccb7a7f965..84aecc749544 100644 --- a/toolkit/components/places/nsAnnoProtocolHandler.cpp +++ b/toolkit/components/places/nsAnnoProtocolHandler.cpp @@ -73,12 +73,16 @@ namespace { class faviconAsyncLoader : public AsyncStatementCallback { public: - faviconAsyncLoader(nsIChannel *aChannel, nsIStreamListener *aListener) + faviconAsyncLoader(nsIChannel *aChannel, nsIStreamListener *aListener, + uint16_t aPreferredSize) : mChannel(aChannel) , mListener(aListener) + , mPreferredSize(aPreferredSize) { MOZ_ASSERT(aChannel, "Not providing a channel will result in crashes!"); MOZ_ASSERT(aListener, "Not providing a stream listener will result in crashes!"); + MOZ_ASSERT(aChannel, "Not providing a channel!"); + // Set the default content type. Unused << mChannel->SetContentType(NS_LITERAL_CSTRING(PNG_MIME_TYPE)); } @@ -90,16 +94,16 @@ public: { nsCOMPtr row; while (NS_SUCCEEDED(aResultSet->GetNextRow(getter_AddRefs(row))) && row) { - // TODO: For now just return the biggest icon, that is the first one. - // Later this should allow to return a specific size. - if (!mData.IsEmpty()) { - return NS_OK; - } - int32_t width; nsresult rv = row->GetInt32(1, &width); NS_ENSURE_SUCCESS(rv, rv); + // Check if we already found an image >= than the preferred size, + // otherwise keep examining the next results. + if (width < mPreferredSize && !mData.IsEmpty()) { + return NS_OK; + } + // Eventually override the default mimeType for svg. if (width == UINT16_MAX) { rv = mChannel->SetContentType(NS_LITERAL_CSTRING(SVG_MIME_TYPE)); @@ -165,6 +169,7 @@ private: nsCOMPtr mChannel; nsCOMPtr mListener; nsCString mData; + uint16_t mPreferredSize; }; } // namespace @@ -315,15 +320,23 @@ nsAnnoProtocolHandler::NewFaviconChannel(nsIURI *aURI, nsIURI *aAnnotationURI, }; // Now we go ahead and get our data asynchronously for the favicon. - nsCOMPtr callback = - new faviconAsyncLoader(channel, listener); - + // Ignore the ref part of the URI before querying the database because + // we may have added a size fragment for rendering purposes. nsFaviconService* faviconService = nsFaviconService::GetFaviconService(); + nsAutoCString faviconSpec; + nsresult rv = annotationURI->GetSpecIgnoringRef(faviconSpec); // Any failures fallback to the default icon channel. - if (!callback || !faviconService) + if (NS_FAILED(rv) || !faviconService) return fallback(); - nsresult rv = faviconService->GetFaviconDataAsync(annotationURI, callback); + uint16_t preferredSize = UINT16_MAX; + MOZ_ALWAYS_SUCCEEDS(faviconService->PreferredSizeFromURI(annotationURI, &preferredSize)); + nsCOMPtr callback = + new faviconAsyncLoader(channel, listener, preferredSize); + if (!callback) + return fallback(); + + rv = faviconService->GetFaviconDataAsync(faviconSpec, callback); if (NS_FAILED(rv)) return fallback(); diff --git a/toolkit/components/places/nsFaviconService.cpp b/toolkit/components/places/nsFaviconService.cpp index ab807e87d946..c5bad4b34512 100644 --- a/toolkit/components/places/nsFaviconService.cpp +++ b/toolkit/components/places/nsFaviconService.cpp @@ -718,23 +718,20 @@ nsFaviconService::OptimizeIconSizes(IconData& aIcon) } nsresult -nsFaviconService::GetFaviconDataAsync(nsIURI* aFaviconURI, +nsFaviconService::GetFaviconDataAsync(const nsCString& aFaviconURI, mozIStorageStatementCallback *aCallback) { MOZ_ASSERT(aCallback, "Doesn't make sense to call this without a callback"); nsCOMPtr stmt = mDB->GetAsyncStatement( + "/*Do not warn (bug no: not worth adding an index */ " "SELECT data, width FROM moz_icons " "WHERE fixed_icon_url_hash = hash(fixup_url(:url)) AND icon_url = :url " "ORDER BY width DESC" ); NS_ENSURE_STATE(stmt); - // Ignore the ref part of the URI before querying the database because - // we may have added a media fragment for rendering purposes. - nsAutoCString faviconURI; - aFaviconURI->GetSpecIgnoringRef(faviconURI); - nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("url"), faviconURI); + nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("url"), aFaviconURI); NS_ENSURE_SUCCESS(rv, rv); nsCOMPtr pendingStatement; @@ -762,6 +759,39 @@ nsFaviconService::ConvertUnsupportedPayloads(mozIStorageConnection* aDBConn) } } +NS_IMETHODIMP +nsFaviconService::PreferredSizeFromURI(nsIURI* aURI, uint16_t* _size) +{ + *_size = UINT16_MAX; + nsAutoCString ref; + // Check for a ref first. + if (NS_FAILED(aURI->GetRef(ref)) || ref.Length() == 0) + return NS_OK; + + // Look for a "size=" fragment. + int32_t start = ref.RFind("size="); + if (start >= 0 && ref.Length() > static_cast(start) + 5) { + nsDependentCSubstring size; + // This is safe regardless, since Rebind checks start is not over Length(). + size.Rebind(ref, start + 5); + // Check if the string contains any non-digit. + auto begin = size.BeginReading(), end = size.EndReading(); + for (auto ch = begin; ch < end; ++ch) { + if (*ch < '0' || *ch > '9') { + // Not a digit. + return NS_OK; + } + } + // Convert the string to an integer value. + nsresult rv; + uint16_t val = PromiseFlatCString(size).ToInteger(&rv); + if (NS_SUCCEEDED(rv)) { + *_size = val; + } + } + return NS_OK; +} + //////////////////////////////////////////////////////////////////////////////// //// ExpireFaviconsStatementCallbackNotifier diff --git a/toolkit/components/places/nsFaviconService.h b/toolkit/components/places/nsFaviconService.h index f506e7024a2a..d233e9bfab47 100644 --- a/toolkit/components/places/nsFaviconService.h +++ b/toolkit/components/places/nsFaviconService.h @@ -94,14 +94,14 @@ public: /** * Obtains the favicon data asynchronously. * - * @param aFaviconURI - * The URI representing the favicon we are looking for. + * @param aFaviconSpec + * The spec of the URI representing the favicon we are looking for. * @param aCallback * The callback where results or errors will be dispatch to. In the * returned result, the favicon binary data will be at index 0, and the * mime type will be at index 1. */ - nsresult GetFaviconDataAsync(nsIURI* aFaviconURI, + nsresult GetFaviconDataAsync(const nsCString& aFaviconSpec, mozIStorageStatementCallback* aCallback); /** diff --git a/toolkit/components/places/nsIFaviconService.idl b/toolkit/components/places/nsIFaviconService.idl index 12d187f32ae1..842220cdf473 100644 --- a/toolkit/components/places/nsIFaviconService.idl +++ b/toolkit/components/places/nsIFaviconService.idl @@ -54,6 +54,15 @@ interface nsIFaviconService : nsISupports */ void expireAllFavicons(); + /** + * Tries to extract the preferred size from an icon uri ref fragment. + * + * @param aURI + * The URI to parse. + * @return The preferred size, or UINT16_MAX if not present. + */ + unsigned short preferredSizeFromURI(in nsIURI aURI); + /** * Adds a given favicon's URI to the failed favicon cache. * diff --git a/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js b/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js new file mode 100644 index 000000000000..15f86c34de22 --- /dev/null +++ b/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js @@ -0,0 +1,60 @@ +/** + * This file tests the size ref on the icons protocols. + */ + +const PAGE_URL = "http://icon.mozilla.org/"; +const ICON16_URL = "http://mochi.test:8888/tests/toolkit/components/places/tests/browser/favicon-normal16.png"; +const ICON32_URL = "http://mochi.test:8888/tests/toolkit/components/places/tests/browser/favicon-normal32.png"; + +add_task(function* () { + yield PlacesTestUtils.addVisits(PAGE_URL); + // Add 2 differently sized favicons for this page. + + let data = readFileData(do_get_file("favicon-normal16.png")); + PlacesUtils.favicons.replaceFaviconData(NetUtil.newURI(ICON16_URL), + data, data.length, "image/png"); + yield setFaviconForPage(PAGE_URL, ICON16_URL); + data = readFileData(do_get_file("favicon-normal32.png")); + PlacesUtils.favicons.replaceFaviconData(NetUtil.newURI(ICON32_URL), + data, data.length, "image/png"); + yield setFaviconForPage(PAGE_URL, ICON32_URL); + + const PAGE_ICON_URL = "page-icon:" + PAGE_URL; + + yield compareFavicons(PAGE_ICON_URL, + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Not specifying a ref should return the bigger icon"); + // Fake window object. + let win = { devicePixelRatio: 1.0 }; + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 16), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON16_URL)), + "Size=16 should return the 16px icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 32), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Size=32 should return the 32px icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 33), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Size=33 should return the 32px icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 17), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Size=17 should return the 32px icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 1), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON16_URL)), + "Size=1 should return the 16px icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 0), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Size=0 should return the bigger icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, -1), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Invalid size should return the bigger icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#other§=12", 32), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Pre-existing refs should be ignored"); + win = { devicePixelRatio: 1.1 }; + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 16), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Size=16 with HIDPI should return the 32px icon"); + yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 32), + PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)), + "Size=32 with HIDPI should return the 32px icon"); +}); diff --git a/toolkit/components/places/tests/favicons/xpcshell.ini b/toolkit/components/places/tests/favicons/xpcshell.ini index 26b89cf482ab..b8f594cdb136 100644 --- a/toolkit/components/places/tests/favicons/xpcshell.ini +++ b/toolkit/components/places/tests/favicons/xpcshell.ini @@ -21,6 +21,7 @@ support-files = [test_expireAllFavicons.js] [test_favicons_conversions.js] +[test_favicons_protocols_ref.js] [test_getFaviconDataForPage.js] [test_getFaviconURLForPage.js] [test_moz-anno_favicon_mime_type.js] diff --git a/toolkit/components/places/tests/head_common.js b/toolkit/components/places/tests/head_common.js index 2b4ce01ff96c..cf62cae0d9ea 100644 --- a/toolkit/components/places/tests/head_common.js +++ b/toolkit/components/places/tests/head_common.js @@ -887,7 +887,7 @@ function* compareFavicons(icon1, icon2, msg) { icon2 = new URL(icon2 instanceof Ci.nsIURI ? icon2.spec : icon2); function getIconData(icon) { - new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { NetUtil.asyncFetch({ uri: icon.href, loadUsingSystemPrincipal: true, contentPolicyType: Ci.nsIContentPolicy.TYPE_INTERNAL_IMAGE_FAVICON @@ -901,6 +901,8 @@ function* compareFavicons(icon1, icon2, msg) { } let data1 = yield getIconData(icon1); + Assert.ok(data1.length > 0, "Should fetch icon data"); let data2 = yield getIconData(icon2); + Assert.ok(data2.length > 0, "Should fetch icon data"); Assert.deepEqual(data1, data2, msg); }