Closed
Bug 810179
Opened 12 years ago
Closed 10 years ago
Make IMEContentObserver cycle collectable
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: masayuki, Assigned: masayuki)
Details
(Keywords: inputmethod)
Attachments
(2 files, 3 obsolete files)
3.19 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
10.83 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #0) > See comment 19. of which bug?
Assignee | ||
Comment 2•12 years ago
|
||
Oops, bug 806996 comment 19.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8fe1cadabcc4
Attachment #700880 -
Flags: review?(bugs)
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
Thank you, Kimura-san.
Attachment #700880 -
Attachment is obsolete: true
Attachment #700880 -
Flags: review?(bugs)
Attachment #700953 -
Flags: review?(bugs)
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
(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•11 years ago
|
||
ESM would have something like NS_IMPL_CYCLE_COLLECTION_TRAVERSE(TSM); and delete TSM; in unlink
Assignee | ||
Comment 9•11 years ago
|
||
Hmm, ESM should have nsRefPtr<nsTextStateManager> mTextStateManager;?
Comment 10•11 years ago
|
||
Yes, and it would be non-null only for the focused ESM or something like that.
Assignee | ||
Comment 11•11 years ago
|
||
Okay, I'll add patches for redesigning the stuff.
Assignee | ||
Updated•10 years ago
|
Summary: Make nsTextStateManager cycle collectable → Make IMEContentObserver cycle collectable
Assignee | ||
Comment 12•10 years ago
|
||
Updated for the latest m-c.
Attachment #700953 -
Attachment is obsolete: true
Attachment #8408840 -
Flags: review?(bugs)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8408840 -
Flags: review?(bugs) → review+
Comment 14•10 years ago
|
||
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-
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8408844 -
Attachment is obsolete: true
Attachment #8410884 -
Flags: review?(bugs)
Comment 16•10 years ago
|
||
Comment on attachment 8410884 [details] [diff] [review] part.2 EventStateManager should include IMEContentObserver into cycle collector stroing -> storing
Attachment #8410884 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c939dbbed91 https://hg.mozilla.org/integration/mozilla-inbound/rev/7be46125d679
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6c939dbbed91 https://hg.mozilla.org/mozilla-central/rev/7be46125d679
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•