remove broadcasters from editor/

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: mkmelin, Assigned: mkmelin)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

6 months ago
+++ This bug was initially created as a clone of Bug #1489447 +++

Remove broadcasters from editor/.

AFAICT, the <broadcaster id="args" value=""/> is some ancient artefact which isn't used for anything anymore. I can't find any reference to it being used, and composition seems to work fine without it. Most other broadcasters here aren't used in Thunderbird. There is some usage in SeaMonkey.
Assignee

Comment 1

6 months ago
Attachment #9030201 - Flags: review?(acelists)
Assignee

Updated

6 months ago
Status: NEW → ASSIGNED

Comment 2

5 months ago
Comment on attachment 9030201 [details] [diff] [review]
bug1512936_editor_broadcaster.patch

Review of attachment 9030201 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editor.xul
@@ -110,5 @@
>  
>    <tooltip id="aHTMLTooltip" onpopupshowing="return FillInHTMLTooltipEditor(this);"/>
>  
> -  <broadcasterset id="editorBroadcasters">
> -    <broadcaster id="Editor:Throbber" busy="false"/>

Where is this one set/changed?

@@ -111,5 @@
>    <tooltip id="aHTMLTooltip" onpopupshowing="return FillInHTMLTooltipEditor(this);"/>
>  
> -  <broadcasterset id="editorBroadcasters">
> -    <broadcaster id="Editor:Throbber" busy="false"/>
> -    <broadcaster id="Communicator:WorkMode"/>

Seems only used in SM. Can we remove it like this?

@@ -115,5 @@
> -    <broadcaster id="Communicator:WorkMode"/>
> -    <broadcaster id="args" value="about:blank"/>
> -  </broadcasterset>
> -
> -  <broadcasterset id="mainBroadcasterSet"/>

Also used in SM.
Assignee

Comment 3

5 months ago
(In reply to :aceman from comment #2)
> Where is this one set/changed?

Unused, AFAIKT. That's why I'm removing it.

> Seems only used in SM. Can we remove it like this?

I think so. They will have to catch up in the unlikely event they do release anything past 60.
The alternative of leaving code that will be dead-but-used-in-suite around isn't very enticing.

Comment 4

5 months ago
Yes, but there is difference between visible compilation failure and silent non-working JS code that nobody notices.

But OK, having frg on the bug should prevent this change going unnoticed for SM.

You copy the busy="false" attribute to the element. Is that one used? Or you try to not touch that for now?
Assignee

Comment 5

5 months ago
With editor it is a bit hard to tell sometimes if things are used or not. I think it's used at least here: https://searchfox.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js#361 and I'm just setting it the way the broadcaster would have set it.

Comment 6

5 months ago
Comment on attachment 9030201 [details] [diff] [review]
bug1512936_editor_broadcaster.patch

Review of attachment 9030201 [details] [diff] [review]:
-----------------------------------------------------------------

Inheriting the 'busy' attribute value via Editor:Throbber and then setting it directly on navigator-throbber instead of the broadcaster seems strange. So the new way seems better.
Attachment #9030201 - Flags: review?(acelists) → review+
Assignee

Comment 7

5 months ago
Thx
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.