https://searchfox.org/mozilla-central/rev/5fb48bf50516ed2529d533e5dfe49b4752efb8b8/browser/base/content/browser.js#5103-5215 is just over 100 lines of JS that would be better off being its own self-contained module.

The code should live in a new sys.mjs module, probably under browser/themes, by using MOZ_SRC_FILES to package it and then moz-src:///browser/themes/ToolbarIconColor.sys.mjs to load it when needed.

As a follow-up (probably separate bug + commit), we should see about caching the computations form this module based on the window state in some way.

I need guidance on a couple things:

• Is the way I added the module in moz.build correct?

• If I run ./mach test browser/base/content/test/performance/browser_toolbariconcolor_restyles.js --headless, the tests seems to pass, but I see error output logging when the test is running:

 0:11.34 INFO Console message: [JavaScript Warning: "Loading failed for the module with source “chrome://global/content/elements/moz-fieldset.mjs”." {file: "about:newtab" line: 0}]
 0:11.34 INFO Console message: [JavaScript Warning: "Loading failed for the module with source “chrome://global/content/elements/moz-support-link.mjs”." {file: "about:newtab" line: 0}]

0:10.87 GECKO(37210) JavaScript error: resource:///modules/StartupRecorder.sys.mjs, line 217: TypeError: can't access property "removeEventListener", win is undefined

(In reply to Shane Ziegler from comment #2)

I need guidance on a couple things:

• Is the way I added the module in moz.build correct?

Yep, looks good!

• If I run ./mach test browser/base/content/test/performance/browser_toolbariconcolor_restyles.js --headless, the tests seems to pass, but I see error output logging when the test is running:

 0:11.34 INFO Console message: [JavaScript Warning: "Loading failed for the module with source “chrome://global/content/elements/moz-fieldset.mjs”." {file: "about:newtab" line: 0}]
 0:11.34 INFO Console message: [JavaScript Warning: "Loading failed for the module with source “chrome://global/content/elements/moz-support-link.mjs”." {file: "about:newtab" line: 0}]

0:10.87 GECKO(37210) JavaScript error: resource:///modules/StartupRecorder.sys.mjs, line 217: TypeError: can't access property "removeEventListener", win is undefined

Hm, not sure what those are about but I don't think they're related to your patch so I wouldn't worry about them for now. I'll do the rest of the review and comms on phab :-)

I changed the code to move the _toolbarLuminanceCache to a global in the module file and created a WeakMap for keeping track of state and eventhandlers across different windows.

I am not sure if creating a boundHandler and saving it in the state for each window in the WeakMap is overkill or not. I needed window in inferFromText, so I grabbed window in the init() as part of _boundHandler and saved it in the state WeakMap, so I could pass it through handleEvent for when it calls inferFromText, and later for removing the event handler, but maybe the window is accessible in handleEvent(event) just using event.target.ownerGlobal?

Do you know of any other test besides browser/base/content/test/performance/browser_toolbariconcolor_restyles.js that I should be running? I found that one in SearchFox, when I was looking for everything using ToolbarIconColor.

(In reply to Shane Ziegler from comment #4)

I changed the code to move the _toolbarLuminanceCache to a global in the module file and created a WeakMap for keeping track of state and eventhandlers across different windows.

Nice!

I am not sure if creating a boundHandler and saving it in the state for each window in the WeakMap is overkill or not. I needed window in inferFromText, so I grabbed window in the init() as part of _boundHandler and saved it in the state WeakMap, so I could pass it through handleEvent for when it calls inferFromText, and later for removing the event handler, but maybe the window is accessible in handleEvent(event) just using event.target.ownerGlobal?

Yeah, in this situation I'd just use event.target.ownerGlobal. That reduces a little bit the chance that anything leaks by keeping a handle to the window after it closes.

Do you know of any other test besides browser/base/content/test/performance/browser_toolbariconcolor_restyles.js that I should be running? I found that one in SearchFox, when I was looking for everything using ToolbarIconColor.

Hm, well, I'm no expert in this particular bit of code, and I couldn't quickly see any other tests for the behaviour. That's a little unfortunate. It's possible we're just missing them though - maybe Hanna or Dão can help? Though it's also possible there aren't a great deal of automated tests for this code in which case we'll have to do some manual testing when writing/reviewing. :-)

I have made the change to just grab window from event.target.ownerGlobal in handleEvent and pass it the inferFromText method. I did some testing and could see the brighttext attribute being set to true when using a dark theme and removed when on a light theme, along with the toolbar icons switching from dark outline to light. I also had some console logs in there during testing, to allow me to see that cache hits and misses were actually happening with the _toolbarLuminanceCache hash map.

Looks like Emilio reviewed your patch before I got to it. Sorry for sending you in the wrong direction on the luminance cache being shareable. I still think that would be valuable but we'll need to do a bit more digging to work out how we could determine cache keys/invalidation, and that's probably best done as a follow-up to this bug (as the joke goes, there are only 2 hard problems in computer science: naming things, cache invalidation, and off-by-one errors...).

Do you happen to have two more refactor bugs that me and another contributor could each work on?

(In reply to Shane Ziegler from comment #8)

Do you happen to have two more refactor bugs that me and another contributor could each work on?

Sorry for the slow responses, a bit busy this week - not off-hand but I can create some. Will try to find time for that tomorrow, so keeping needinfo for that.

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

Do you know of any other test besides browser/base/content/test/performance/browser_toolbariconcolor_restyles.js that I should be running? I found that one in SearchFox, when I was looking for everything using ToolbarIconColor.

Hm, well, I'm no expert in this particular bit of code, and I couldn't quickly see any other tests for the behaviour. That's a little unfortunate. It's possible we're just missing them though - maybe Hanna or Dão can help? Though it's also possible there aren't a great deal of automated tests for this code in which case we'll have to do some manual testing when writing/reviewing. :-)

Whoops sorry for the late reply - I'm not aware of any additional tests, but I would be happy to kick off a try push when the patch is approved to double check. Dão may have more specific advice.

(In reply to Shane Ziegler from comment #8)

Do you happen to have two more refactor bugs that me and another contributor could each work on?

OK, I filed bug 1959618 and bug 1959616 :-)

Gijs, can you land this or should I make these changes?

FWIW I think it wouldn't hurt to do the rest of uninit unconditionally - removeEventListener no-ops if the listener is unknown and the delete will also no-op if window isn't in the map, and we otherwise don't use state (if you wanted to keep it, could do _windowStateMap.has(window) instead).

I can land it. :-)

https://hg.mozilla.org/mozilla-central/rev/b66e3b6527bf