Chrome Version: 68 (affects any since 2015)
OS: All

I found this code in firefox_importer.cc:

      if (item->url.is_valid())
        search_engine_info.url = base::UTF8ToUTF16(item->url.spec());
      else if (bookmark_html_reader::CanImportURLAsSearchEngine(
                   item->url,
                   &search_engine_url))
        search_engine_info.url = base::UTF8ToUTF16(search_engine_url);

This is designed to take a search engine URL, like "https://www.google.com.au/search?q=%s" and store it in the search engines table. What this code says is:

1. If the string represents a valid URL, just dump it in the table without processing. The comment above suggests this is designed for keyword bookmarks without a "%s", just so you can use the keyword to access the bookmark without a query.
2. Else, if the string is a search engine (i.e., it contains "%s"), process the "%s", converting it into a "{searchTerms}" which is used internally to substitute the search query.

HOWEVER, it doesn't work at all like the comment suggests. Case 1 is almost always taken, because "%s" is *VALID* in any part of the URL other than the domain (which, curiously, all the test cases in bookmark_html_reader_unittest test for). So in practice, we never convert "%s" into "{searchTerms}", we just dump the string with "%s" straight into the search engine list.

Clearly, Cases 1 and 2 should be swapped around, right???

What's really strange is that IT WORKS ANYWAY. Without ever calling CanImportURLAsSearchEngine, search engines imported this way substitute correctly. This suggests that there is absolutely no reason to use CanImportURLAsSearchEngine, or convert "%s" to "{searchTerms}". So why does CanImportURLAsSearchEngine exist at all, and why is it used by normal bookmark importing?

This is all very smelly. Ilya, you reviewed r311834 which added all this logic. Do you have any insight surrounding the above questions?