Closed Bug 1475304 Opened 6 years ago Closed 6 years ago

Audit <xul:broadcaster> usage for cases where there's only one consumer

Categories

(Firefox :: General, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Priority: P5 → P3
(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
Flags: needinfo?(jhofmann)
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(jhofmann)
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.
Attachment #8991741 - Flags: review?(jhofmann)
Attachment #8991741 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8991741 [details]
Bug 1475304 - Remove broadcasters that only have one observer

https://reviewboard.mozilla.org/r/256678/#review263748

LGTM, thanks!
Attachment #8991741 - Flags: review?(gijskruitbosch+bugs) → review+
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
Attachment #8991741 - Flags: review?(jhofmann) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d13a54a85e2f
Remove broadcasters that only have one observer;r=Gijs,johannh
https://hg.mozilla.org/mozilla-central/rev/d13a54a85e2f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
See Also: → 1479908
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: