1461130 - windows.getAll converts all windows even if they don't match windowTypes

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement)

Description

[Oriol Brufau [:Oriol]]

https://searchfox.org/mozilla-central/rev/babf96cf0c37b608e9663e2af73a4d21819c4af1/browser/components/extensions/parent/ext-windows.js#118-119

  let windows = Array.from(windowManager.getAll(), win => win.convert(getInfo))
                .filter(typeFilter);

Say that I have 1000 open normal windows, and I run this code:

  browser.windows.getAll({windowTypes: ["popup"]})

Then the code above will convert all these windows, even if they will be filtered out. This is useless expensive work. Only windows that match should be converted.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1461130 - Don't convert filtered out windows in windows.getAll

Review commit: https://reviewboard.mozilla.org/r/243612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243612/

Comment 2

[Oriol Brufau [:Oriol]]

It seems that "normal" and "popup" are the only possible window types on Firefox, so I guess that another optimization would be not iterating windows at all if windowTypes doesn't contain any of these. But not sure if it's worth.

Comment 3

[Oriol Brufau [:Oriol]]

Comment on attachment 8975286 [details]
Bug 1461130 - Don't convert filtered out windows in windows.getAll

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243612/diff/1-2/

Comment 4

[Oriol Brufau [:Oriol]]

I also added that, if windowTypes includes both "normal" and "popup", all windows will match, so there is no need to actually check. This avoids an extra call of the type getter.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8975286 [details]
Bug 1461130 - Don't convert filtered out windows in windows.getAll

https://reviewboard.mozilla.org/r/243612/#review249744

ISTM this would be simpler to just fix the array building to only convert matched windows.

return Array.from(windowManager.getAll()).filter(typeFilter).map(win => win.convert(getInfo));


Are there adequate tests covering this change?

Comment 6

[Oriol Brufau [:Oriol]]

This seems a proper test: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/browser/components/extensions/test/browser/browser_ext_windows.js#25,28,33,40

I was trying to optimize other things, but OK, I will revert to my first patch. It behaves like your code but without creating intermediate arrays.

Comment 7

[Oriol Brufau [:Oriol]]

Comment on attachment 8975286 [details]
Bug 1461130 - Don't convert filtered out windows in windows.getAll

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243612/diff/2-3/

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8975286 [details]
Bug 1461130 - Don't convert filtered out windows in windows.getAll

https://reviewboard.mozilla.org/r/243612/#review249940

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cb2a199d50d1
Don't convert filtered out windows in windows.getAll r=mixedpuppy

Comment 10

[Stefan Hindli [:stefan_hindli]]

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

Comment 11

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 12

[Oriol Brufau [:Oriol]]

No change in behavior, just avoiding useless work.