1416163 - Implement a helper (EveryWindow.jsm) to run arbitrary per-window code

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P5)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Currently this is possible by using tabbrowser's addProgressListener, but this requires keeping track of windows and adding a listener to each one. If tabbrowser sent an observer notification, consumers could get the xul:browser (and hence the tab and the window) "immediately".

Example use case: showing a PopupNotification in a tab when it navigates to an arbitrary URL (very useful for upcoming breach alerts feature).

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Example patch

Wanted quick feedback on any perf implications this might have, before attempting to get this reviewed.

I can think of at least one thing, though I don't think it's too significant (but I could be wrong): notifyObservers is a sync API, and sending out this notification without a guaranteed consumer might be wasteful. What do you think?

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Some thinking out loud:

I guess this patch is not strictly necessary: a workaround would be to use a per-window script that uses tabbrowser's addTabsProgressListener API. Also, supplying the URL spec as aData is unnecessary, and is probably not a good idea since the string length is unpredictable (could be a data URI).

This being said, I still would like this API, it would mean being able to write a single JSM that manages everything for features that need to keep track of some state that involves multiple windows.

I'll give this some more thought and may close the bug if I convince myself it's unnecessary.

Comment 3

[Dão Gottwald [:dao]]

Adding a global equivalent for onLocationChange specifically seems a bit arbitrary, but I don't think we want to do this for all tabbrowser notifications either.

To reduce the boilerplate code in your module, can you just add a helper module that keeps track of windows?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I agree that this sounds like something that a JSM could / should manage, by way of the addTabsProgressListener.

This would likely be an improvement over the (arguably simpler) nsIObserver notification, since we _could_ throttle the notifications to listeners to only fire after we've hit an idle callback. That would probably be preferable than notifying everybody synchronously once the location change occurs, and hoping they do things quickly.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8927249 [details] [diff] [review]
Example patch

See above.

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8927249 [details] [diff] [review]
Example patch

Clearing the flag as others have already replied.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: EveryWindow.jsm

This patch adds a JSM that supplies window-management boilerplate code. The idea is that any consumer that wants to perform some action for every current and future browser window can register itself via EveryWindow.registerCallback, which accepts two callbacks: init, which is run for all windows at the time of registration and also for all future windows, and uninit, which is run when a window is closed or the consumer is unregistered.

Dão, could you specifically comment on the general approach I'm taking with this, before I request code review? Is this kind of what you were thinking of in comment 3? Also, do we need a consumer in m-c before we can land something like this?

FWIW: In the next version of the patch I want to switch from using Services.wm.addListener to simply observing browser-delayed-startup-finished, but first I need to find out the best way to listen for windows getting closed. I'll also work on tests if I get positive feedback about this patch (i.e. confidence that some version of it can land in m-c).

Comment 8

[Dão Gottwald [:dao]]

Sounds good. You'll probably also want a way to get all current windows in case there were already windows open before a consumer registers itself.

(In reply to Nihanth Subramanya [:nhnt11] from comment #7)
> Dão, could you specifically comment on the general approach I'm taking with
> this, before I request code review? Is this kind of what you were thinking
> of in comment 3? Also, do we need a consumer in m-c before we can land
> something like this?

Not necessarily, but it would be nice if we only shipped the release version with a consumer.

> FWIW: In the next version of the patch I want to switch from using
> Services.wm.addListener to simply observing
> browser-delayed-startup-finished, but first I need to find out the best way
> to listen for windows getting closed.

The unload event.

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1416163 - Implement EveryWindow.jsm to run arbitrary per-window code. r=johannh

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Found some time to make progress on this.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab42e340222aeb5b6a1d211b4cb0dc3fae123399

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c640010582e2
Implement EveryWindow.jsm to run arbitrary per-window code. r=johannh

Comment 12

[Noemi Erli[:noemi_erli]]

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

Comment 13

[Dão Gottwald [:dao]]

Are you aware of BrowserWindowTracker.jsm that has been added after this bug had been dropped over a year ago? Seems to me that we should extend that module if needed rather than adding another similar one.

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Dão Gottwald [::dao] from comment #13)

Are you aware of BrowserWindowTracker.jsm that has been added after this bug had been dropped over a year ago? Seems to me that we should extend that module if needed rather than adding another similar one.

Yeah, I'm aware of BrowserWindowTracker. I thought about extending that module but it didn't feel like I could do that without some refactoring. I'll file a bug to investigate merging the two modules. Thanks!