Voicing can be initialized twice in RawRichTextLink

[jessegreenberg]

Discovered with @zepumph - Voicing is mixed into RawRichTextLink. Its constructor looks like

    super();

    this.fireListener = null;
    this.accessibleInputListener = null;

    this.initialize( innerContent, href );

Voicing is then initialized in the first super call, and then also in the initialize, which follows up with a super.initialize(). We fixed this by conditionally calling the super.initialize in RawRichTextLink.initialize. See the following commit. We weren't sure about this as a long term solution, @jonathanolson would you recommend anything different here?

[jonathanolson]

Actually, this seems somewhat backwards to me, and might be broken.

It looks like we only want to call Voicing's initialize in the constructor, and NOT in the repeated "initialize" that can later happen multiple times. I'll look into a solution.

[jonathanolson]

Actually, I take that back. If Voicing is creating something that has an initialize method, that NOTES poolability, it should be friendly to pooling. Right now it is not lazily creating Properties, but it is doing a lot of work that breaks pooling.

[jonathanolson]

I don't think I understand what is going on.

If something is in initialize, its purpose is to take the arguments provided to initialize, and mutate the object so that it is AS IF you called the constructor with those arguments.

Voicing's initialize() uses none of its arguments, so it should... be a no-op. I don't see any cleanup.

Voicing's initialize() seems to do the opposite of what pooling initialize is for.

Instead of

this._voicingCanSpeakProperty = new TinyProperty<boolean>( true );

pooling initialization would do lazy construction:

this._voicingCanSpeakProperty = this._voicingCanSpeakProperty || new TinyProperty<boolean>( true );

So... currently when intialize is called a second time on a RichTextLink, it is recreating Properties, and leaking memory.

[jonathanolson]

Looks like we aren't leaking in this specific case, because Voicing provides... a clean(), which is implementing RichTextCleanable?

So that does pair things up with initialize/clean cycles, so it looks like we aren't explicitly leaking directly.

Maybe I'm confused that Voicing is set up to API-match with RichTextCleanable, and that it is not reusing things in initialize() - why is that?

@jessegreenberg and @zepumph, perhaps we can schedule a call on this?

[zepumph]

The issue is this:

scenery/js/util/rich-text/RichTextLink.ts

Lines 20 to 24 in 45ea411

	super();
	// Voicing was already initialized in the super call, we do not want to initialize super again. But we do want to
	// initialize the RichText portion of the implementation.
	this.initialize( innerContent, href, false );

Because of this:

scenery/js/accessibility/voicing/Voicing.ts

Line 245 in e8bf225

VoicingClass.prototype.initialize.call( this );

So initialize() is called twice, once in a super() that is meant to basically be a no-op for RichTextLink. Can you and @jessegreenberg recommend any improvements here? Is it actually buggy?