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

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

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?

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8193ee3d99f9

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

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.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d195d131008

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: MozReview Request: Bug 1276406 Remove "ime-enabled-state-changed" notification completely r?smaug

"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/

Comment 6

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

https://mxr.mozilla.org/addons/source/276754/chrome/content/virtualKeyboard.js#24 uses this.

Comment 7

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

Vivien, any comments to this?

Comment 8

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

(I was looking at https://mxr.mozilla.org/addons/source/276754/install.rdf)

Comment 9

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

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.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

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.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

How do you think, should we still wait a reply from Vivien?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6820ce1a14d1
Remove "ime-enabled-state-changed" notification completely r=smaug

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6820ce1a14d1