1880913 - Move BrowserSearch out of browser.js

Status: NEW

(Firefox :: Search, task, P3)

Description

[:Gijs (he/him)]

BrowserSearch is a large (about 600 lines of code, out of 10k) object in browser.js that is responsible for a number of different things relating to web search:

• handling search requests for specific text with the default engine from various places:
  • the context menu
  • the command line
  • webextensions
• handling browser search engine changes. This comprises:
  • dealing with observer notifications for browser-search-engine-modified. This handling is unfortunate, as the topic is fired whenever any engine changes, and at that point every browser window will scan all its tabs to work out if the engine in question was on offer somewhere, and potentially update UI for it - even for background tabs etc.
  • updating URL bar search icons if the page offers a search engine / that engine gets installed etc.
  • updating URL bar state when the default (private/non-private) search engine changes
  • updating URL bar state when the window first opens
• opening new browser windows and/or focusing the right UI if/when the user uses a "search" command (also from the macOS global menubar, not just browser.js)
• showing some kind of notification when a user's default search engine has been removed

Although all of this is search-related, it's not clear that having this spread of responsibilities on the one object is really helpful at this stage.

Off-hand, my proposal would be:

• handling search requests could happen in more search-service specific code, which could take a browser reference and keyword (ie query) as an argument. It can then use the browser reference to actually load the search (probably via URILoadingHelper).
• the code tracking what pages offer what search engines should be managed more directly in one place. A plausible option for this would be inside a dedicated sys.mjs module that is loaded the first time the user loads a page that offers a search engine (which triggers code here). It should have a single observer, and keep track of which browsers offer engines. This would also offer the opportunity to tighten up this somewhat byzantine location where the list is cleared (but not the hiddenEngines list, or the list for background pages when they navigate! As far as I can tell those are never cleared, which seems like a bug...). I expect instead of expando properties, the code could use a weakmap from browsing contexts to engine lists, or something like that, and update UI when the relevant browsing context is destroyed or its engines are added/removed.
• browser/components/urlbar/UrlbarInput.sys.mjs or similar should be responsible for updating the URL bar placeholder. It doesn't make sense that this responsibility lives with BrowserSearch. The search bar should similarly be responsible for its own placeholders.
• the notification bits should move into the search service, the only caller. Updating the URL bar placeholder should happen automatically, in different code, based on the same observer topic / event as this code.

Mark, does this seem reasonable? I guess maybe we should split off some of these items into sub-bugs?

Comment 1

[Mark Banner (:standard8)]

(In reply to :Gijs (he/him) from comment #0)

Off-hand, my proposal would be:

• handling search requests could happen in more search-service specific code, which could take a browser reference and keyword (ie query) as an argument. It can then use the browser reference to actually load the search (probably via URILoadingHelper).

SearchUIUtils.sys.mjs would probably be the best place for that.

• the code tracking what pages offer what search engines should be managed more directly in one place. A plausible option for this would be inside a dedicated sys.mjs module that is loaded the first time the user loads a page that offers a search engine (which triggers code here). It should have a single observer, and keep track of which browsers offer engines. This would also offer the opportunity to tighten up this somewhat byzantine location where the list is cleared (but not the hiddenEngines list, or the list for background pages when they navigate! As far as I can tell those are never cleared, which seems like a bug...). I expect instead of expando properties, the code could use a weakmap from browsing contexts to engine lists, or something like that, and update UI when the relevant browsing context is destroyed or its engines are added/removed.

A dedicated system module would probably be the best thing here, I don't think it will fit anywhere else, and it needs to be accessible from multiple places (urlbar, search bar).

• browser/components/urlbar/UrlbarInput.sys.mjs or similar should be responsible for updating the URL bar placeholder. It doesn't make sense that this responsibility lives with BrowserSearch. The search bar should similarly be responsible for its own placeholders.

Agreed. I think the search bar is already responsible for its own placeholders, unless I missed something.

• the notification bits should move into the search service, the only caller. Updating the URL bar placeholder should happen automatically, in different code, based on the same observer topic / event as this code.

It shouldn't be in the search service as that's in toolkit and we were explicitly trying to keep UI out of it. tbh, I'm not sure the best way of doing this. We could put it in SearchUIUtils, though I would be more concerned about the toolkit -> browser dependency with that case. One nice thing of the current way, is that any app can define this on the window and have it shown.