962005 - Make mozilla::TextComposition refcountable

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

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

TextComposition isn't a countable class now.

But for using/sharing it from editor and nsIMEStateManager, it's better the class to be refcountable.

Comment 1

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

Attachment: Patch

I worry about the memory fragmentation by this change. If you have better idea, let me know.

Comment 2

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

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

 > TextComposition::TextComposition(const TextComposition& aOther)
> {
>-  mNativeContext = aOther.mNativeContext;
>-  mPresContext = aOther.mPresContext;
>-  mNode = aOther.mNode;
>-  mLastData = aOther.mLastData;
>-  mCompositionStartOffset = aOther.mCompositionStartOffset;
>-  mCompositionTargetOffset = aOther.mCompositionTargetOffset;
>-  mIsSynthesizedForTests = aOther.mIsSynthesizedForTests;
>+  MOZ_CRASH("Don't use copy constructor");
> }
Can't you just have copy ctor unimplemented in the header file.



This makes the code much more manageable, thanks.

Comment 3

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

Thank you for the review.

(In reply to Olli Pettay [:smaug] from comment #2)
> Can't you just have copy ctor unimplemented in the header file.

The copy constructor becomes private. I.e., nobody except TextComposition itself cannot use the copy constructor. The MOZ_CRASH() prevents other developers to use it in TextComposition. Isn't it necessary? (copying refcounter makes a bug, I guess)

Comment 4

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

I meant that do I need to change something before landing the patch?

Comment 5

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

Does the patch compile if you just have private unimplemented copy-ctor?
If so, land it that way, otherwise land the patch as is

Comment 6

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

Thanks. Succeeded building TextComposition without the default constructor implementation.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d56032aebbed

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d56032aebbed