firefox: fix search engine icons

[kira-bruneau]

Description

Closes #6450

See also the reference I used to implement the migrations for our Nix config: https://searchfox.org/mozilla-central/rev/e3f42ec9320748b2aab3d474d1e47075def9000c/toolkit/components/search/SearchSettings.sys.mjs#760-876.

Checklist

• Change is backwards compatible.

• Code formatted with ./format.

• Code tested through nix-shell --pure tests -A run.all
  or nix build --reference-lock-file flake.lock ./tests#test-all using Flakes.

• Test cases updated/added. See example.

• Commit messages are formatted like

  {component}: {description}
  
  {long description}
  

  See CONTRIBUTING for more information and recent commit messages for examples.

• If this PR adds a new module

  • Added myself as module maintainer. See example.

Maintainer CC

@brckd @rycee

[5225225]

this was inherited from the previous code, but while it's being updated anyways:
this url is actually a 404, it's favicon.ico

[kira-bruneau]

oh yep thanks for catching that!

[5225225]

Hiding existing engines doesn't seem to be working here

I have

"Bing".metaData.hidden = true;
"Google".metaData.hidden = true;
"eBay".metaData.hidden = true;
"Wikipedia (en)".metaData.hidden = true;

in my config, and the HM generated file looks like

{
  "_isAppProvided": true,
  "_metaData": {
    "hidden": true
  },
  "_name": "Bing"
},

(using nix run nixpkgs#dejsonlz4 -- search.json.mozlz4 | jq .)

once i open firefox, it gets converted into

{
  "id": "bing",
  "_name": "Bing",
  "_isAppProvided": true,
  "_metaData": {}
},

... maybe firefox is expecting id to be populated, and bailing out when it isn't, resetting the metadata? unsure. (on that note, it would be nice to make the names in the config actually id, but that'd be a breaking change and i'm not sure how you'd migrate... in any case, that doesn't need to be done here)

adding new search engines with favicons works fine, both by url and using

iconMapObj."32" = pkgs.fetchurl {
  url = "https://github.githubassets.com/favicons/favicon-dark.png";
  hash = "sha256-vwpwhqqIKCid8OSMGppX4U/ltDf7qEx+/00wXR0HT/E=";
};

[khaneliman]

Suggested change

	"JSON object names for iconMapObj are deprecated, use ${size} instead of ${name}"
	"JSON object names for `iconMapObj` are deprecated, use `${size}` instead of `${name}`"

[khaneliman]

Suggested change

	"16" = warn "iconURL is deprecated, use icon = ${
	strings.escapeNixString engine.iconURL
	} instead" engine.iconURL;
	"16" = warn "`iconURL` is deprecated, use `icon = ${
	strings.escapeNixString engine.iconURL
	}` instead" engine.iconURL;

[khaneliman]

Suggested change

	"16" = warn "iconUpdateURL is deprecated, use icon = ${
	strings.escapeNixString engine.iconUpdateURL
	} instead" engine.iconUpdateURL;
	} // (engine.iconMapObj or { });
	"16" = warn "`iconUpdateURL` is deprecated, use `icon = ${
	strings.escapeNixString engine.iconUpdateURL
	}` instead" engine.iconUpdateURL;
	} // (engine.iconMapObj or { });

[khaneliman]

Suggested change

	iconMapObj."16" = "https://wiki.nixos.org/favicon.png";
	iconMapObj."16" = "https://wiki.nixos.org/favicon.ico";

[khaneliman]

Hiding existing engines doesn't seem to be working here

I have

"Bing".metaData.hidden = true;
"Google".metaData.hidden = true;
"eBay".metaData.hidden = true;
"Wikipedia (en)".metaData.hidden = true;

in my config, and the HM generated file looks like

{
  "_isAppProvided": true,
  "_metaData": {
    "hidden": true
  },
  "_name": "Bing"
},

(using nix run nixpkgs#dejsonlz4 -- search.json.mozlz4 | jq .)

once i open firefox, it gets converted into

{
  "id": "bing",
  "_name": "Bing",
  "_isAppProvided": true,
  "_metaData": {}
},

... maybe firefox is expecting id to be populated, and bailing out when it isn't, resetting the metadata? unsure. (on that note, it would be nice to make the names in the config actually id, but that'd be a breaking change and i'm not sure how you'd migrate... in any case, that doesn't need to be done here)

Is this a regression from this change or just another thing that needs to be migrated/fixed ? Didn't see where this would have affected that. Unless, it's just the version that we pass. Looking at the other migrations on searchfox, I didn't see any obvious migrations either that would help.

EDIT: Also looks like changing the default search engine regressed, so must just be that version string being passed affecting this stuff.

search = {
      default = "DuckDuckGo";
      privateDefault = "DuckDuckGo";
}

Doesn't work with this change.

[kira-bruneau]

Thank you both for catching these regressions! I assumed that the icon changes were the only breaking changes based of migrations that exist in firefox, I'll look into them.

EDIT: It looks like the associated migrations were embedded directly into the implementation rather than in migrateSettings 🫠. For v7, search engine ids were introduced to replace the old way of referring to engines by name: https://searchfox.org/mozilla-central/rev/e3f42ec9320748b2aab3d474d1e47075def9000c/toolkit/components/search/SearchSettings.sys.mjs#543-600.

I'll look into porting this migration for our module.