For suspended IME extension (e.g. XKB extension), don't awake it by IME events except onActivate. T…

Don't awake suspended IME extension's background except for onActivate.

For suspended IME extension (e.g. XKB extension), don't awake it by IME
 events except onActivate. The IME extension should be awake by other
events (e.g. runtime.onMessage) from its other pages. This is to save
memory for steady state Chrome OS on which the users don't want any
IME features.

BUG=451627
TEST=Verified on linux_chromeos.

Committed: https://crrev.com/bef0893a2994abc9e68f8e98027a4d04590ddc7c
Cr-Commit-Position: refs/heads/master@{#314100}

[Shu Chen, 2015-01-29 04:17:11]

shuchen@chromium.org changed reviewers:
+ nona@chromium.org

[Shu Chen, 2015-01-29 04:17:12]

Nona, can you please review this cl? Thanks

[Seigo Nonaka, 2015-01-29 09:11:29]

Could you please update the description as git-log friendly format?
Like https://codereview.chromium.org/24123006/,
1. please put short one-line description on the top.
2. after one line break, please put detailed description with breaking about 80
chars.

Thank you.

=========== Example =============
Don't awake suspended IME extension except for onActivate.

For suspended IME extension (e.g. XKB extension), don't awake it by IME
 events except onActivate. The IME extension should be awake by other 
events (e.g. runtime.onMessage) from its other pages. This is to save 
memory for steady state Chrome OS on which the users don't want any 
IME features.

https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For suspended
IME extension (e.g. XKB extension), don't awake it by IME
I'm sorry but "suspended IME" and its example is not clear for me.

IIRC, there four state in extension IMEs:
1. not installed.
2. installed but not enabled(not checked by user in settings page)
3. enabled but not activated(listed in system tray but the indicator is
different from this).
4. active.

Looks like the suspended IMEs are 3., right?
If so, please write like as,

For the IMEs which enabled but not activated, don't awake it by IME ...

[Shu Chen, 2015-01-30 14:38:29]

https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For suspended
IME extension (e.g. XKB extension), don't awake it by IME
On 2015/01/29 09:11:29, Seigo Nonaka wrote:
> I'm sorry but "suspended IME" and its example is not clear for me.
> 
> IIRC, there four state in extension IMEs:
> 1. not installed.
> 2. installed but not enabled(not checked by user in settings page)
> 3. enabled but not activated(listed in system tray but the indicator is
> different from this).
> 4. active.
> 
> Looks like the suspended IMEs are 3., right?
> If so, please write like as,
> 
> For the IMEs which enabled but not activated, don't awake it by IME ...

"suspend" means the extension background is killed by extension system for
"event page":
https://developer.chrome.com/extensions/event_pages
The logic here is to make sure once an IME extension background is killed, it
should not be awaken by IME API events, until a listened non-IME-API event
arrives.

[Shu Chen, 2015-02-02 00:35:29]

On 2015/01/30 14:38:29, Shu Chen wrote:
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):
> 
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For suspended
> IME extension (e.g. XKB extension), don't awake it by IME
> On 2015/01/29 09:11:29, Seigo Nonaka wrote:
> > I'm sorry but "suspended IME" and its example is not clear for me.
> > 
> > IIRC, there four state in extension IMEs:
> > 1. not installed.
> > 2. installed but not enabled(not checked by user in settings page)
> > 3. enabled but not activated(listed in system tray but the indicator is
> > different from this).
> > 4. active.
> > 
> > Looks like the suspended IMEs are 3., right?
> > If so, please write like as,
> > 
> > For the IMEs which enabled but not activated, don't awake it by IME ...
> 
> "suspend" means the extension background is killed by extension system for
> "event page":
> https://developer.chrome.com/extensions/event_pages
> The logic here is to make sure once an IME extension background is killed, it
> should not be awaken by IME API events, until a listened non-IME-API event
> arrives.

With the second thought, I think maybe this logic should only apply to 1st party
IME extensions.
WDYT?

[Seigo Nonaka, 2015-02-02 00:56:38]

https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):

https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For suspended
IME extension (e.g. XKB extension), don't awake it by IME
On 2015/01/30 14:38:29, Shu Chen wrote:
> On 2015/01/29 09:11:29, Seigo Nonaka wrote:
> > I'm sorry but "suspended IME" and its example is not clear for me.
> > 
> > IIRC, there four state in extension IMEs:
> > 1. not installed.
> > 2. installed but not enabled(not checked by user in settings page)
> > 3. enabled but not activated(listed in system tray but the indicator is
> > different from this).
> > 4. active.
> > 
> > Looks like the suspended IMEs are 3., right?
> > If so, please write like as,
> > 
> > For the IMEs which enabled but not activated, don't awake it by IME ...
> 
> "suspend" means the extension background is killed by extension system for
> "event page":
> https://developer.chrome.com/extensions/event_pages
> The logic here is to make sure once an IME extension background is killed, it
> should not be awaken by IME API events, until a listened non-IME-API event
> arrives.

Oh, I didn't know extension background page can be suspended.

Then, I have another questions.

1. Does this function is called only for IME event? Looks like all event except
for OnActivate  will be ignored, is it correct?

2. Also, no need to call other events? IIRC, extension suspension can happen at
any time, so looks like need to dispatch all event if the IME is activated.

[Shu Chen, 2015-02-02 01:17:11]

On 2015/02/02 00:56:38, Seigo Nonaka wrote:
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):
> 
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For suspended
> IME extension (e.g. XKB extension), don't awake it by IME
> On 2015/01/30 14:38:29, Shu Chen wrote:
> > On 2015/01/29 09:11:29, Seigo Nonaka wrote:
> > > I'm sorry but "suspended IME" and its example is not clear for me.
> > > 
> > > IIRC, there four state in extension IMEs:
> > > 1. not installed.
> > > 2. installed but not enabled(not checked by user in settings page)
> > > 3. enabled but not activated(listed in system tray but the indicator is
> > > different from this).
> > > 4. active.
> > > 
> > > Looks like the suspended IMEs are 3., right?
> > > If so, please write like as,
> > > 
> > > For the IMEs which enabled but not activated, don't awake it by IME ...
> > 
> > "suspend" means the extension background is killed by extension system for
> > "event page":
> > https://developer.chrome.com/extensions/event_pages
> > The logic here is to make sure once an IME extension background is killed,
it
> > should not be awaken by IME API events, until a listened non-IME-API event
> > arrives.
> 
> Oh, I didn't know extension background page can be suspended.
> 
> Then, I have another questions.
> 
> 1. Does this function is called only for IME event? Looks like all event
except
> for OnActivate  will be ignored, is it correct?
Yes & Yes.
> 
> 2. Also, no need to call other events? IIRC, extension suspension can happen
at
> any time, so looks like need to dispatch all event if the IME is activated.
This cl is to avoid awaking by IME events. US keyboard is provided by XKB
extension whose background could be suspended after idling for ~20 seconds.
If Chrome OS is in steady state (no IME features are enabled), the background
should remain idle to save ~40MB memory.
So IME events including key events, focus events should not awake the
background.
If user enables IME features (e.g. physical keyboard autocorrect, virtual
keyboard, etc.), the background will be awaken by runtime.sendMessage events
 (for physical keyboard autocorrect, it will periodically send messages as
heartbeats).
onActivate should also awake the background in case the user is switching from
US keyboard to FR keyboard with some IME features enabled.

[Seigo Nonaka, 2015-02-02 01:55:40]

On 2015/02/02 01:17:11, Shu Chen wrote:
> On 2015/02/02 00:56:38, Seigo Nonaka wrote:
> >
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> > File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> > chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For
suspended
> > IME extension (e.g. XKB extension), don't awake it by IME
> > On 2015/01/30 14:38:29, Shu Chen wrote:
> > > On 2015/01/29 09:11:29, Seigo Nonaka wrote:
> > > > I'm sorry but "suspended IME" and its example is not clear for me.
> > > > 
> > > > IIRC, there four state in extension IMEs:
> > > > 1. not installed.
> > > > 2. installed but not enabled(not checked by user in settings page)
> > > > 3. enabled but not activated(listed in system tray but the indicator is
> > > > different from this).
> > > > 4. active.
> > > > 
> > > > Looks like the suspended IMEs are 3., right?
> > > > If so, please write like as,
> > > > 
> > > > For the IMEs which enabled but not activated, don't awake it by IME ...
> > > 
> > > "suspend" means the extension background is killed by extension system for
> > > "event page":
> > > https://developer.chrome.com/extensions/event_pages
> > > The logic here is to make sure once an IME extension background is killed,
> it
> > > should not be awaken by IME API events, until a listened non-IME-API event
> > > arrives.
> > 
> > Oh, I didn't know extension background page can be suspended.
> > 
> > Then, I have another questions.
> > 
> > 1. Does this function is called only for IME event? Looks like all event
> except
> > for OnActivate  will be ignored, is it correct?
> Yes & Yes.
> > 
> > 2. Also, no need to call other events? IIRC, extension suspension can happen
> at
> > any time, so looks like need to dispatch all event if the IME is activated.
> This cl is to avoid awaking by IME events. US keyboard is provided by XKB
> extension whose background could be suspended after idling for ~20 seconds.
> If Chrome OS is in steady state (no IME features are enabled), the background
> should remain idle to save ~40MB memory.
> So IME events including key events, focus events should not awake the
> background.
> If user enables IME features (e.g. physical keyboard autocorrect, virtual
> keyboard, etc.), the background will be awaken by runtime.sendMessage events
>  (for physical keyboard autocorrect, it will periodically send messages as
> heartbeats).
> onActivate should also awake the background in case the user is switching from
> US keyboard to FR keyboard with some IME features enabled.

Okay, thank you for your detailed explanation.
Please double check this doesn't break non XKB keyboard IMEs.

Thank you.

lgtm.

[Shu Chen, 2015-02-02 02:00:15]

On 2015/02/02 01:55:40, Seigo Nonaka wrote:
> On 2015/02/02 01:17:11, Shu Chen wrote:
> > On 2015/02/02 00:56:38, Seigo Nonaka wrote:
> > >
> >
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> > > File chrome/browser/extensions/api/input_ime/input_ime_api.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/884713005/diff/1/chrome/browser/extensions/ap...
> > > chrome/browser/extensions/api/input_ime/input_ime_api.cc:86: // For
> suspended
> > > IME extension (e.g. XKB extension), don't awake it by IME
> > > On 2015/01/30 14:38:29, Shu Chen wrote:
> > > > On 2015/01/29 09:11:29, Seigo Nonaka wrote:
> > > > > I'm sorry but "suspended IME" and its example is not clear for me.
> > > > > 
> > > > > IIRC, there four state in extension IMEs:
> > > > > 1. not installed.
> > > > > 2. installed but not enabled(not checked by user in settings page)
> > > > > 3. enabled but not activated(listed in system tray but the indicator
is
> > > > > different from this).
> > > > > 4. active.
> > > > > 
> > > > > Looks like the suspended IMEs are 3., right?
> > > > > If so, please write like as,
> > > > > 
> > > > > For the IMEs which enabled but not activated, don't awake it by IME
...
> > > > 
> > > > "suspend" means the extension background is killed by extension system
for
> > > > "event page":
> > > > https://developer.chrome.com/extensions/event_pages
> > > > The logic here is to make sure once an IME extension background is
killed,
> > it
> > > > should not be awaken by IME API events, until a listened non-IME-API
event
> > > > arrives.
> > > 
> > > Oh, I didn't know extension background page can be suspended.
> > > 
> > > Then, I have another questions.
> > > 
> > > 1. Does this function is called only for IME event? Looks like all event
> > except
> > > for OnActivate  will be ignored, is it correct?
> > Yes & Yes.
> > > 
> > > 2. Also, no need to call other events? IIRC, extension suspension can
happen
> > at
> > > any time, so looks like need to dispatch all event if the IME is
activated.
> > This cl is to avoid awaking by IME events. US keyboard is provided by XKB
> > extension whose background could be suspended after idling for ~20 seconds.
> > If Chrome OS is in steady state (no IME features are enabled), the
background
> > should remain idle to save ~40MB memory.
> > So IME events including key events, focus events should not awake the
> > background.
> > If user enables IME features (e.g. physical keyboard autocorrect, virtual
> > keyboard, etc.), the background will be awaken by runtime.sendMessage events
> >  (for physical keyboard autocorrect, it will periodically send messages as
> > heartbeats).
> > onActivate should also awake the background in case the user is switching
from
> > US keyboard to FR keyboard with some IME features enabled.
> 
> Okay, thank you for your detailed explanation.
> Please double check this doesn't break non XKB keyboard IMEs.
> 
> Thank you.
> 
> lgtm.

Thanks for the review! The non-XKB keyboard IMEs' backgrounds are not event
pages.
So this cl doesn't affect non-XKB IMEs.

[Shu Chen, 2015-02-02 02:02:46]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2015-02-02 02:03:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/884713005/1

[commit-bot: I haz the power, 2015-02-02 02:50:01]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-02 02:50:45]

Patchset 1 (id:??) landed as
https://crrev.com/bef0893a2994abc9e68f8e98027a4d04590ddc7c
Cr-Commit-Position: refs/heads/master@{#314100}