810179 - Make IMEContentObserver cycle collectable

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See comment 19. However, the instance is only created while an editable content has focus. I think the risk isn't so high but I agree that we should do this.

Comment 1

[Masatoshi Kimura [:emk]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0)
> See comment 19.
of which bug?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, bug 806996 comment 19.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8fe1cadabcc4

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 700880 [details] [diff] [review]
Patch

>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsTextStateManager)
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsTextStateManager)
>+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mWidget)
>+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mSel)
>+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mRootContent)
>+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mEditableNode)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsTextStateManager)
>+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWidget)
>+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSel)
>+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRootContent)
>+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mEditableNode)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

NS_IMPL_CYCLE_COLLECTION_4(nsTextStateManager, mWidget, mSel, mRootContent, mEditableNode)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Thank you, Kimura-san.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 700953 [details] [diff] [review]
Patch

And we need to make something to actually use this cycle collectable
stuff.
Should the focused ESM traverse TSM?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 700953 [details] [diff] [review]
> Patch
> 
> And we need to make something to actually use this cycle collectable
> stuff.
> Should the focused ESM traverse TSM?

Do you mean that ESM needs to call something of cycle collector?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

ESM would have something like
NS_IMPL_CYCLE_COLLECTION_TRAVERSE(TSM);

and

delete TSM;
in unlink

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, ESM should have nsRefPtr<nsTextStateManager> mTextStateManager;?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Yes, and it would be non-null only for the focused ESM or something like that.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I'll add patches for redesigning the stuff.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Make IMEContentObserver cycle collectable

Updated for the latest m-c.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 EventStateManager should include IMEContentObserver into cycle collector

When IMEContentObserver starts to observe the focused content, it registers to ESM itself. When it ends to observe the focused content, it notifies ESM of that.

I.e., while IMEContentObserver is observing content, it is held by ESM.

For safety, if ESM clears stored IMEContentObserver, e.g., destroying or coming new IMEContentObserver instance, it makes stored IMEContentObserver forget the reference to ESM.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8408844 [details] [diff] [review]
part.2 EventStateManager should include IMEContentObserver into cycle collector

Perhaps 
s/EnsureToReleaseIMEContentObserver/ReleaseCurrentIMEContentObserver/

Why OnStopObservingContent doesn't need to call 
mIMEContentObserver->OnReleaseFromEventStateManager(); ?
Looks like it should.

Maybe s/OnReleaseFromEventStateManager/DisconnectFromEventStateManager/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 EventStateManager should include IMEContentObserver into cycle collector

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8410884 [details] [diff] [review]
part.2 EventStateManager should include IMEContentObserver into cycle collector

stroing -> storing

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c939dbbed91
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be46125d679

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c939dbbed91
https://hg.mozilla.org/mozilla-central/rev/7be46125d679