1880913 - Move BrowserSearch out of browser.js

Status: RESOLVED FIXED

(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.

Comment 2

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #1)

• 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.

Gijs, any thoughts about this?

Comment 3

[Moritz Beier [:mbeier]]

Attachment: Bug 1880913 - Part 1: Move urlbar placeholder handling from BrowserSearch to UrlbarInput. r?#urlbar-reviewers

Comment 4

[Moritz Beier [:mbeier]]

Attachment: Bug 1880913 - Part 2: Move search request handling from browser.js to SearchUIUtils. r?#search-reviewers

Comment 5

[Moritz Beier [:mbeier]]

Attachment: Bug 1880913 - Part 3: Move open search handling from BrowserSearch to dedicated module. r?standard8

Comment 6

[Moritz Beier [:mbeier]]

Attachment: Bug 1880913 - Part 4: Remove BrowserSearch.searchBar getter. r?#search-reviewers

Comment 7

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #2)

(In reply to Mark Banner (:standard8) from comment #1)

• 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.

Gijs, any thoughts about this?

I mean, basically it sounds like you need a dependency injection mechanism. Typically we use the observer service for that.

At the risk of this being "I made this really nice hammer, looks like you've got some nails" - if you need more data than fits in the subject/data parts of the observer service APIs, and/or you care about having to eagerly initialize to registerObserver the observer, you could maybe use the category manager indirection mechanism I recently introduced to do the toolkit->browser dependency? See https://searchfox.org/mozilla-central/source/toolkit/modules/BrowserUtils.sys.mjs#479 . I tried to write good documentation at the time and that got scuppered by having to move the method to a different module (so now https://firefox-source-docs.mozilla.org/xpcom/xpcomutils.html exists but doesn't have the documentation). The TL;DR is that you'd add a category manager entry in a .manifest like this:

https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/browser/components/BrowserComponents.manifest#13

category browser-window-delayed-startup resource:///modules/ContentAnalysis.sys.mjs ContentAnalysis.initialize

where you'd replace browser-window-delayed-startup with some topic that makes more sense (like an observer service topic), you'd replace the module URL with the one for your module that needs to listen to the thing, and the last bit should be a Singleton.method() thing where Singleton is exported by the module and method can be called on the singleton. The callsite uses:

    BrowserUtils.callModulesFromCategory(
      "my-fancy-new-topic",
      whateverArgs,
      theCallSiteMightLike
    );

and it Just Works. If multiple things need invoking, each adds their own category manager entry, and the single callModulesFromCategory call will invoke each in turn (and report errors but continue to call all the other ones, etc.)

Does that help?

Comment 8

[Mark Banner (:standard8)]

Yes, thank you. I think we can fine a way to make it work from there.

Comment 9

[Moritz Beier [:mbeier]]

Attachment: Bug 1880913 - Part 5: Display search engine removal notification via category manager. r?standard8

Comment 10

[Moritz Beier [:mbeier]]

Attachment: Bug 1880913 - Part 6: Remove BrowserSearch and BrowserSearchTelemetry from browser.js. r?standard8

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61c9028c010c
Part 1: Move urlbar placeholder handling from BrowserSearch to UrlbarInput. r=urlbar-reviewers,Standard8
https://hg.mozilla.org/integration/autoland/rev/10bcc3d37876
Part 2: Move search request handling from browser.js to SearchUIUtils. r=search-reviewers,firefox-desktop-core-reviewers ,mossop,scunnane
https://hg.mozilla.org/integration/autoland/rev/bef665565986
Part 3: Move open search handling from BrowserSearch to dedicated module. r=Standard8,urlbar-reviewers
https://hg.mozilla.org/integration/autoland/rev/cd2d032982ae
Part 4: Remove BrowserSearch.searchBar getter. r=search-reviewers,scunnane
https://hg.mozilla.org/integration/autoland/rev/97d8e3ec2480
Part 5: Display search engine removal notification via category manager. r=Standard8,urlbar-reviewers
https://hg.mozilla.org/integration/autoland/rev/b76ccb2dff6e
Part 6: Remove BrowserSearch and BrowserSearchTelemetry from browser.js. r=Standard8

Comment 12

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/61c9028c010c
https://hg.mozilla.org/mozilla-central/rev/10bcc3d37876
https://hg.mozilla.org/mozilla-central/rev/bef665565986
https://hg.mozilla.org/mozilla-central/rev/cd2d032982ae
https://hg.mozilla.org/mozilla-central/rev/97d8e3ec2480
https://hg.mozilla.org/mozilla-central/rev/b76ccb2dff6e