Closed Bug 810179 Opened 7 years ago Closed 6 years ago

Make IMEContentObserver cycle collectable

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: masayuki, Assigned: masayuki)

Details

(Keywords: inputmethod)

Attachments

(2 files, 3 obsolete files)

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.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0)
> See comment 19.
of which bug?
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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)
Attached patch Patch (obsolete) — Splinter Review
Thank you, Kimura-san.
Attachment #700880 - Attachment is obsolete: true
Attachment #700880 - Flags: review?(bugs)
Attachment #700953 - Flags: review?(bugs)
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?
Attachment #700953 - Flags: review?(bugs) → review+
(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?
ESM would have something like
NS_IMPL_CYCLE_COLLECTION_TRAVERSE(TSM);

and

delete TSM;
in unlink
Hmm, ESM should have nsRefPtr<nsTextStateManager> mTextStateManager;?
Yes, and it would be non-null only for the focused ESM or something like that.
Okay, I'll add patches for redesigning the stuff.
Summary: Make nsTextStateManager cycle collectable → Make IMEContentObserver cycle collectable
Updated for the latest m-c.
Attachment #700953 - Attachment is obsolete: true
Attachment #8408840 - Flags: review?(bugs)
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.
Attachment #8408844 - Flags: review?(bugs)
Attachment #8408840 - Flags: review?(bugs) → review+
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/
Attachment #8408844 - Flags: review?(bugs) → review-
Comment on attachment 8410884 [details] [diff] [review]
part.2 EventStateManager should include IMEContentObserver into cycle collector

stroing -> storing
Attachment #8410884 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/6c939dbbed91
https://hg.mozilla.org/mozilla-central/rev/7be46125d679
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.