Bug 1359456 - Page-icon protocol is wrongly removing any ref from the page url. r=adw
MozReview-Commit-ID: FejFQj1Px7n
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -402,16 +402,18 @@ FetchIconPerSpec(const RefPtr<Database>&
// for a specific size when an associated icon for that size doesn't exist.
nsCOMPtr<mozIStorageStatement> stmt = aDB->GetStatement(
"/* do not warn (bug no: not worth having a compound index) */ "
"SELECT width, icon_url, root "
"FROM moz_icons i "
"JOIN moz_icons_to_pages ON i.id = icon_id "
"JOIN moz_pages_w_icons p ON p.id = page_id "
"WHERE page_url_hash = hash(:url) AND page_url = :url "
+ "OR (:hash_idx AND page_url_hash = hash(substr(:url, 0, :hash_idx)) "
+ "AND page_url = substr(:url, 0, :hash_idx)) "
"UNION ALL "
"SELECT width, icon_url, root "
"FROM moz_icons i "
"WHERE fixed_icon_url_hash = hash(fixup_url(:root_icon_url)) "
"ORDER BY width DESC, root ASC "
);
NS_ENSURE_STATE(stmt);
mozStorageStatementScoper scoper(stmt);
@@ -420,16 +422,19 @@ FetchIconPerSpec(const RefPtr<Database>&
NS_ENSURE_SUCCESS(rv, rv);
nsAutoCString rootIconFixedUrl(aPageHost);
if (!rootIconFixedUrl.IsEmpty()) {
rootIconFixedUrl.AppendLiteral("/favicon.ico");
}
rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("root_icon_url"),
rootIconFixedUrl);
NS_ENSURE_SUCCESS(rv, rv);
+ int32_t hashIdx = PromiseFlatCString(aPageSpec).RFind("#");
+ rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hash_idx"), hashIdx + 1);
+ NS_ENSURE_SUCCESS(rv, rv);
// Return the biggest icon close to the preferred width. It may be bigger
// or smaller if the preferred width isn't found.
bool hasResult;
while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
int32_t width;
rv = stmt->GetInt32(0, &width);
if (!aIconData.spec.IsEmpty() && width < aPreferredWidth) {
--- a/toolkit/components/places/PageIconProtocolHandler.js
+++ b/toolkit/components/places/PageIconProtocolHandler.js
@@ -87,17 +87,17 @@ PageIconProtocolHandler.prototype = {
// Create our channel.
let channel = Cc["@mozilla.org/network/input-stream-channel;1"]
.createInstance(Ci.nsIInputStreamChannel);
channel.QueryInterface(Ci.nsIChannel);
channel.setURI(uri);
channel.contentStream = pipe.inputStream;
channel.loadInfo = loadInfo;
- let pageURI = NetUtil.newURI(uri.cloneIgnoringRef().path);
+ let pageURI = NetUtil.newURI(uri.path.replace(/[&#]size=[^&]+$/, ""));
let preferredSize = PlacesUtils.favicons.preferredSizeFromURI(uri);
PlacesUtils.favicons.getFaviconDataForPage(pageURI, (iconURI, len, data, mimeType) => {
channel.contentType = mimeType;
if (len == 0) {
streamDefaultFavicon(uri, loadInfo, pipe.outputStream);
} else {
try {
serveIcon(pipe, data, len);
--- a/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js
+++ b/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js
@@ -42,19 +42,33 @@ add_task(function* () {
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");
+
+ // Ass the icon also for the page with ref.
+ yield PlacesTestUtils.addVisits(PAGE_URL + "#other§=12");
+ yield setFaviconForPage(PAGE_URL + "#other§=12", ICON16_URL, false);
+ yield setFaviconForPage(PAGE_URL + "#other§=12", ICON32_URL, false);
+ yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#other§=12", 16),
+ PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON16_URL)),
+ "Pre-existing refs should be retained");
yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#other§=12", 32),
PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
- "Pre-existing refs should be ignored");
+ "Pre-existing refs should be retained");
+
+ // If the ref-ed url is unknown, should still try to fetch icon for the unref-ed url.
+ yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#randomstuff", 32),
+ PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
+ "Non-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");
});
--- a/toolkit/components/places/tests/favicons/test_page-icon_protocol.js
+++ b/toolkit/components/places/tests/favicons/test_page-icon_protocol.js
@@ -77,8 +77,21 @@ add_task(function* svg_icon() {
faviconURI, SMALLSVG_DATA_URI.spec, 0, Services.scriptSecurityManager.getSystemPrincipal());
yield setFaviconForPage(TEST_URI, faviconURI);
let svgIcon = yield fetchIconForSpec(SMALLSVG_DATA_URI.spec);
do_print(svgIcon.contentType)
let pageIcon = yield fetchIconForSpec("page-icon:" + TEST_URI.spec);
Assert.equal(svgIcon.contentType, pageIcon.contentType);
Assert.deepEqual(svgIcon.data, pageIcon.data, "Got the root favicon data");
});
+
+add_task(function* page_with_ref() {
+ for (let url of ["http://places.test.ref/#myref",
+ "http://places.test.ref/#!&b=16",
+ "http://places.test.ref/#"]) {
+ yield PlacesTestUtils.addVisits(url);
+ yield setFaviconForPage(url, ICON_URI, false);
+ let {data, contentType} = yield fetchIconForSpec("page-icon:" + url);
+ Assert.equal(contentType, gFavicon.contentType);
+ Assert.deepEqual(data, gFavicon.data, "Got the favicon data");
+ yield PlacesUtils.history.remove(url);
+ }
+});