Closed Bug 1276406 Opened 6 years ago Closed 5 years ago

It seems that nobody is observing "ime-enabled-state-changed", if so, we can remove it

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, Whiteboard: btpp-active)

Attachments

(1 file)

Looks like that nobody is observing "ime-enabled-state-changed" which was implemented in bug 603848 for Android.
http://mxr.mozilla.org/mozilla-central/search?string=%22ime-enabled-state-changed%22
http://mxr.mozilla.org/gaia/search?string=ime-enabled-state-changed

So, I'd like to remove this call of IMEStateManager. However, I've not found the bug to have removed the observer. Is somebody sure if this is not used anymore?
Flags: needinfo?(cpeterson)
Flags: needinfo?(21)
Status: NEW → ASSIGNED
Whiteboard: btpp-active
It looks like I removed a (the?) "ime-enabled-state-changed" observer in bug 766066: https://hg.mozilla.org/mozilla-central/rev/69b6541efafc. Bug 819515 also proposed to remove this same "ime-enabled-state-changed" observer for some other problems. I am guessing no one uses this event.
Flags: needinfo?(cpeterson)
Thank you for your reply.

I also tried to crash if the topic is observed, but there are no crashed tests in comment 1. So, we should be able to remove this completely.
Flags: needinfo?(21)
"ime-enabled-state-changed" notification was implemented for Android in bug 603848 but nobody currently observes this notification.  Therefore, we can stop notify the observer service of this.

Review commit: https://reviewboard.mozilla.org/r/56770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56770/
Attachment #8758506 - Flags: review?(bugs)
Vivien, any comments to this?
Flags: needinfo?(21)
Comment on attachment 8758506 [details]
MozReview Request: Bug 1276406 Remove "ime-enabled-state-changed" notification completely r?smaug

https://reviewboard.mozilla.org/r/56770/#review53554

So, conditional r+. Only land the patch if Vivien is ok with it.
Attachment #8758506 - Flags: review?(bugs) → review+
Ah, add-ons...

But they can use focus event (listen to focus event of editor or its ancestor in the system event group) and retrieve the new IME state with nsIDOMWindowUtils.IMEStatus.
How do you think, should we still wait a reply from Vivien?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #7)
> Vivien, any comments to this?

Sorry for the long delay. I'm completely fine to remove it.
Flags: needinfo?(21)
Thank you!
Flags: needinfo?(bugs)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6820ce1a14d1
Remove "ime-enabled-state-changed" notification completely r=smaug
https://hg.mozilla.org/mozilla-central/rev/6820ce1a14d1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.