Closed Bug 810179 Opened 7 years ago Closed 6 years ago
Observer cycle collectable
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?
Oops, bug 806996 comment 19.
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)
Thank you, Kimura-san.
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.
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)
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+
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.