1925985 - Consolidate "windows" getter from TabManager and WindowManager classes

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Noticed while reviewing https://phabricator.services.mozilla.com/D225065.

We have a windows getter in both the TabManager and WindowManager classes. Hereby the one in TabManager returns windows only that are not closed so it is an enhancement to the one in the WindowManager. Nevertheless given that this getter is about windows we should have it in the WindowManager class only.

Comment 1

[Spencer]

May I give this a try if it's still available for assignment?

Comment 2

[Julian Descottes [:jdescottes]]

(In reply to Spencer from comment #1)

May I give this a try if it's still available for assignment?

Sure! Feel free to submit a patch, the bug will automatically be assigned to you then.

Based on the description from :whimboo above, I think the idea is basically to update the windows getter from WindowManager:

https://searchfox.org/mozilla-central/rev/55837bbe3e47f9b4fa91ef83a44b53823626f01d/remote/shared/WindowManager.sys.mjs#42-44

get windows() {
  return Services.wm.getEnumerator(null);
}

with the code from TabManager:
https://searchfox.org/mozilla-central/rev/55837bbe3e47f9b4fa91ef83a44b53823626f01d/remote/shared/TabManager.sys.mjs#86-103

/**
 * Retrieve all the open windows.
 *
 * @returns {Array<Window>}
 *     All the open windows. Will return an empty list if no window is open.
 */
get windows() {
  const windows = [];

  for (const win of Services.wm.getEnumerator(null)) {
    if (win.closed) {
      continue;
    }
    windows.push(win);
  }

  return windows;
}

Then update all call sites using TabManager.windows to use WindowManager.windows instead, and finally remove the duplicated helper from TabManager.
The only difficulty might be if one of the callers actually needs to handle closed windows, which are no longer returned by the TabManager version of the helper, but hopefully that shouldn't be the case.

Comment 3

[Spencer]

Attachment: Bug 1925985 - [WindowManager] Folded TabManager.windows functionality into WindowManager.windows, rerouted reference. r=jdescottes

Comment 4

[Spencer]

I submitted a refactor that accomplishes the folding of the TabManager windows getter code into WindowManager's getter and rerouted a reference to TabManager as well as cleaned up the redundant windows getter from TabManager.

I also submitted the subsequent commit to the try servers and it passed successfully, but I did notice a couple more references to TabManager's windows getter inside of the browser_TabManager.js test file on lines 340 and 345.

My first thought was to change both TabManager references to WindowManager.windows and that would functionally resolve the discrepancy in the test file, but then that test would be a test performed on WindowManager from within a TabManager test file. I wasn't sure how that would be received in the interest of code consistency, so I thought that I would need to make a separate browser_WindowManager.js test file to house that minor test refactor in the interest of that consistency, though making a new test file from scratch like that would be new for me as a new contributor.

What do you think I should do? Leave it as is, change the references on 340 and 345 to WindowManager only or make a new test file with the changes?

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Spencer from comment #4)

What do you think I should do? Leave it as is, change the references on 340 and 345 to WindowManager only or make a new test file with the changes?

Replied on phabricator, a new test sounds good here, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0b569aec617
[WindowManager] Folded TabManager.windows functionality into WindowManager.windows, rerouted reference. r=jdescottes,webdriver-reviewers

Comment 7

[Atila Butkovits]

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

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Spencer, if you are interested to work on something else please let us know. We have a good number of other stuff to work on. Just let us know about the expected complexity you would like to have. Thanks!

Comment 9

[Spencer]

Hi Henrik, thank you for the outreach. Other than the current task I have assigned to me, I am looking to continue working on issues of this nature. Since I'm still rather a novice on the Firefox code base for now, I want to just for now slowly and gradually increase my task complexity from matters like this one. Bug fixes limited to singular components or refactoring and creation of consolidating helper functions are things I'm comfortable with trying to tackle for right now. As such, if you know of any open tasks like or related to these that are available for assignment please let me know! Thanks!

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That is understable. Thank you for the information! I'll keep it in mind. Also I saw that you are working on bug 1927829 as well. So lets follow-up over there.