Fix Bug 1482479 - Update SEARCH_SHORTCUTS and top_sites.json to support the 9 desired search urls #4320
Conversation
lib/SearchShortcuts.jsm
Outdated
{keyword: "@yandex", shortURL: "yandex", url: "https://yandex.com", searchIdentifier: /^yandex/}, | ||
{keyword: "@amazon", shortURL: "amazon", url: "https://amazon.com", searchIdentifier: /^amazon/} | ||
{keyword: "@bing", shortURL: "bing", url: "https://bing.com", searchIdentifier: /^bing/}, | ||
{keyword: "@duckduckgo", shortURL: "duckduckgo", url: "https://duckduckgo.com", searchIdentifier: /^duckduckgo/}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the searchIdentifier
here should be ddg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/SearchShortcuts.jsm
Outdated
{keyword: "@google", shortURL: "google", url: "https://google.com", searchIdentifier: /^google/}, | ||
{keyword: "@twitter", shortURL: "twitter", url: "https://twitter.com", searchIdentifier: /^twitter/}, | ||
{keyword: "@wikipedia", shortURL: "wikipedia", url: "https://wikipedia.com", searchIdentifier: /^wikipedia/}, | ||
{keyword: "@yandex", shortURL: "yandex", url: "https://yandex.com", searchIdentifier: /^yandex/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like the ordering is important. mkaply says to get the ordering from Services.search.. I guess we can put them in order when we do the .reduce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Services.search.getDefaultEngines()
gives only defaults and in the expected order. Maybe we should iterate on that and then test each regex..... ?
lib/SearchShortcuts.jsm
Outdated
{keyword: "@ebay", shortURL: "ebay", url: "https://ebay.com", searchIdentifier: /^ebay/}, | ||
{keyword: "@google", shortURL: "google", url: "https://google.com", searchIdentifier: /^google/}, | ||
{keyword: "@twitter", shortURL: "twitter", url: "https://twitter.com", searchIdentifier: /^twitter/}, | ||
{keyword: "@wikipedia", shortURL: "wikipedia", url: "https://wikipedia.com", searchIdentifier: /^wikipedia/}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be .org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments to address 👍
69977fe
to
0ff98e0
Compare
lib/TopSitesFeed.jsm
Outdated
if (!Services.search.getEngines().find(e => e.identifier && e.identifier.match(shortcut.searchIdentifier))) { | ||
return result; | ||
const searchShortcuts = Services.search.getDefaultEngines().reduce((result, engine) => { | ||
if (!engine.identifier) { return result; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test failure. also, you can maybe just nest to avoid the early single line return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✖ should dispatch UPDATE_SEARCH_SHORTCUTS on updateSearchShortcuts
Firefox 54.0.0 (Linux 0.0.0)
Services.search.getDefaultEngines is not a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good with fixed test!
lib/TopSitesFeed.jsm
Outdated
if (!Services.search.getEngines().find(e => e.identifier && e.identifier.match(shortcut.searchIdentifier))) { | ||
return result; | ||
const searchShortcuts = Services.search.getDefaultEngines().reduce((result, engine) => { | ||
if (!engine.identifier) { return result; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✖ should dispatch UPDATE_SEARCH_SHORTCUTS on updateSearchShortcuts
Firefox 54.0.0 (Linux 0.0.0)
Services.search.getDefaultEngines is not a function
…rt the 9 desired search urls
b52e4a7
to
b117299
Compare
No description provided.