I see cases like https://searchfox.org/mozilla-central/search?q=reportPhishingBroadcaster&path= and https://searchfox.org/mozilla-central/search?q=devtoolsMenuBroadcaster_PageSource&path= where there is only one consumer of a broadcaster. Unless if I'm missing something, those cases could be simplified to directly set attributes on the one consumer and avoid broadcasting/observing altogether. Here's the list of broadcasters: https://searchfox.org/mozilla-central/search?q=%3Cbroadcaster&path=

Hm, I thought it was necessary to use a single-consumer broadcaster for altering the state of the global TP toggle in bug 1462468, although I struggle to remember why exactly. I can take a look at that... Considering that this is a P5 it might be good open up sub-bugs as good first or second bugs for contributors.

(In reply to Johann Hofmann [:johannh] from comment #1) > Hm, I thought it was necessary to use a single-consumer broadcaster for > altering the state of the global TP toggle in bug 1462468, although I > struggle to remember why exactly. I can take a look at that... Thanks for the heads up. Looks like that bug added a test so hopefully if there is an issue it will be caught. If you remember any more details please let me know. > Considering that this is a P5 it might be good open up sub-bugs as good > first or second bugs for contributors. I filed it as P5, although now I realize that it's blocking progress on XHTML conversion for top-level windows so I'm going to bump up priority and take a look at it myself. It's definitely possible there will be spin-offs though that might be good candidates.

(In reply to Brian Grinstead [:bgrins] from comment #2) > (In reply to Johann Hofmann [:johannh] from comment #1) > > Hm, I thought it was necessary to use a single-consumer broadcaster for > > altering the state of the global TP toggle in bug 1462468, although I > > struggle to remember why exactly. I can take a look at that... > > Thanks for the heads up. Looks like that bug added a test so hopefully if > there is an issue it will be caught. If you remember any more details please > let me know. Does it maybe have something to do with Customize Mode? I see code where we are doing stuff like `let element = toolbox.palette.getElementsByAttribute("id", aId)[0];` [0] as opposed to document.getElementById(aId) which makes me wonder if we end up cloning nodes and appending elements with the same ID into the DOM twice? In which case the [observes] attribute would work on both nodes, but code directly setting attrs on the original node wouldn't. [0]: https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/components/customizableui/CustomizableUI.jsm#1423

(In reply to Brian Grinstead [:bgrins] from comment #3) > (In reply to Brian Grinstead [:bgrins] from comment #2) > > (In reply to Johann Hofmann [:johannh] from comment #1) > > > Hm, I thought it was necessary to use a single-consumer broadcaster for > > > altering the state of the global TP toggle in bug 1462468, although I > > > struggle to remember why exactly. I can take a look at that... > > > > Thanks for the heads up. Looks like that bug added a test so hopefully if > > there is an issue it will be caught. If you remember any more details please > > let me know. > > Does it maybe have something to do with Customize Mode? I see code where we > are doing stuff like `let element = > toolbox.palette.getElementsByAttribute("id", aId)[0];` [0] as opposed to > document.getElementById(aId) We do this because `toolbox.palette` is not in the document, so document.getElementById() won't find those nodes. The palette is a specific node that's in the markup somewhere and gets taken out of the document by the first toolbar binding whose XBL binding gets constructed (it finds the toolbox and then its palette, and puts it on a property but out of the document. > which makes me wonder if we end up cloning > nodes and appending elements with the same ID into the DOM twice? I don't think so. > In which > case the [observes] attribute would work on both nodes, but code directly > setting attrs on the original node wouldn't. > > [0]: > https://searchfox.org/mozilla-central/rev/ > a80651653faa78fa4dfbd238d099c2aad1cec304/browser/components/customizableui/ > CustomizableUI.jsm#1423 I wasn't involved with this change so I have no idea why the TP toggle does this.

(In reply to :Gijs (he/him) from comment #5) > (In reply to Brian Grinstead [:bgrins] from comment #3) > > (In reply to Brian Grinstead [:bgrins] from comment #2) > > > (In reply to Johann Hofmann [:johannh] from comment #1) > > > > Hm, I thought it was necessary to use a single-consumer broadcaster for > > > > altering the state of the global TP toggle in bug 1462468, although I > > > > struggle to remember why exactly. I can take a look at that... > > > > > > Thanks for the heads up. Looks like that bug added a test so hopefully if > > > there is an issue it will be caught. If you remember any more details please > > > let me know. > > > > Does it maybe have something to do with Customize Mode? I see code where we > > are doing stuff like `let element = > > toolbox.palette.getElementsByAttribute("id", aId)[0];` [0] as opposed to > > document.getElementById(aId) > > We do this because `toolbox.palette` is not in the document, so > document.getElementById() won't find those nodes. The palette is a specific > node that's in the markup somewhere and gets taken out of the document by > the first toolbar binding whose XBL binding gets constructed (it finds the > toolbox and then its palette, and puts it on a property but out of the > document. Ok good to know - thanks. > > which makes me wonder if we end up cloning > > nodes and appending elements with the same ID into the DOM twice? > > I don't think so. > > > In which > > case the [observes] attribute would work on both nodes, but code directly > > setting attrs on the original node wouldn't. > > > > [0]: > > https://searchfox.org/mozilla-central/rev/ > > a80651653faa78fa4dfbd238d099c2aad1cec304/browser/components/customizableui/ > > CustomizableUI.jsm#1423 > > I wasn't involved with this change so I have no idea why the TP toggle does > this. Try looks pretty good with the changes https://treeherder.mozilla.org/#/jobs?repo=try&revision=777cc467730de48d422764d211e0738ca55594a7, so I'll go ahead and request review.

Comment on attachment 8991741 [details] Bug 1475304 - Remove broadcasters that only have one observer Johann: could you look at the tracking protection change specifically and see if there are any bugs that aren't being caught by existing mochitests? Gijs: mind taking a look at the rest? Sorry there's some churn here, but I do think this ends up simpler by putting attributes on the element where they are actually going to apply makes it easier to read the markup.

Comment on attachment 8991741 [details] Bug 1475304 - Remove broadcasters that only have one observer https://reviewboard.mozilla.org/r/256678/#review263748 LGTM, thanks!

OK, thanks Gijs. Will await Johann's review for the TP side of things before landing.

Comment on attachment 8991741 [details] Bug 1475304 - Remove broadcasters that only have one observer https://reviewboard.mozilla.org/r/256678/#review264110 Yup, this seems fine :) Thanks! ::: browser/base/content/browser-trackingprotection.js:39 (Diff revision 3) > this.container = $("#tracking-protection-container"); > this.content = $("#tracking-protection-content"); > this.icon = $("#tracking-protection-icon"); > this.appMenuContainer = $("#appMenu-tp-container"); > this.appMenuSeparator = $("#appMenu-tp-separator"); > - this.broadcaster = $("#trackingProtectionBroadcaster"); > + this.button = $("#appMenu-tp-toggle"); nit: since the other menu items are called appMenu.., please call this appMenuButton or appMenuToggle

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d13a54a85e2f Remove broadcasters that only have one observer;r=Gijs,johannh