1953323 - Remove #tippytop telemetry in favicon URL

Status: UNCONFIRMED

(Firefox :: New Tab Page, defect)

Description

[Jason Go]

Steps to reproduce:

If you look at https://searchfox.org/mozilla-central/source/browser/components/topsites/TopSites.sys.mjs#1164, the favicon is being fetched with an added hash #tippytop in the URL. This has been the case since the beginning of the original file 8 years ago from https://github.com/mozilla/gecko-dev/blob/cf71bbf47e827e4dab0a3fec428b48fbc9e71ed7/browser/extensions/activity-stream/lib/FaviconFeed.jsm

Expected results:

There are no documented best practice nor any known real world benefit in tracking #tippytop in favicon URLs. The code can be safely removed.

Comment 1

[Jason Go]

Here's the direct quote of the code mentioned above:

// The #tippytop is to be able to identify them for telemetry.
iconUri = iconUri.mutate().setRef("tippytop").finalize();

Comment 2

[Jason Go]

Also in the test suite why do we need to have the #tippytop url ref? What is its significance in testing and even in production code? It does not do anything other than being there.

https://searchfox.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_NewTabUtils.js#434-459

  let faviconData = new Map();
  faviconData.set("https://mozilla.com", `${base64URL}#tippytop`);
  await PlacesTestUtils.addFavicons(faviconData);

  await provider._addFavicons(links);
  Assert.equal(
    links[0].mimeType,
    "image/png",
    "Got the right mime type before deleting it"
  );
  Assert.equal(
    links[0].faviconLength,
    links[0].favicon.length,
    "Got the right length for the byte array"
  );
  Assert.equal(
    provider._faviconBytesToDataURI(links)[0].favicon,
    base64URL,
    "Got the right favicon"
  );
  Assert.equal(
    links[0].faviconSize,
    1,
    "Got the right favicon size (width and height of favicon)"
  );
  Assert.equal(links[0].faviconRef, "tippytop", "Got the favicon url ref");

And this one too: https://searchfox.org/mozilla-central/source/browser/components/topsites/test/unit/test_FaviconProvider.js#144-147

    const result = await PlacesUtils.promiseFaviconData(TEST_PAGE_URL);
    // eslint-disable-next-line no-use-before-define
    const expectedFaviconData = readFileData(TEST_FAVICON_FILE);
    Assert.equal(result.uri.spec, `${TEST_FAVICON_URL.spec}#tippytop`);

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:thecount, could you have a look please?

For more information, please visit BugBot documentation.

Comment 4

[Ina Popescu, Desktop QA]

I don't think this issue can be reproduced manually through typical user interaction or via DevTools. Developer confirmation is needed to clarify whether this is intended. Marking this as qa-not-actionable for now.