Adds extension events for custom dictionary load/change.

Adds extension events for custom dictionary load/change.

Events exposed through inputMethodPrivate extension API.

BUG=455026
Committed: https://crrev.com/38625727a94921d4773b267b4d5f4692dd688689
Cr-Commit-Position: refs/heads/master@{#320940}

[Peter Wen, 2015-03-04 16:38:50]

wnwen@chromium.org changed reviewers:
+ bshe@chromium.org, rouslan@chromium.org, shuchen@chromium.org

[Peter Wen, 2015-03-04 16:38:51]

Added the loaded/changed events for custom spelling dictionary in
inputMethodPrivate. After this CL lands I will work on front-end syncing with
NaCl through these new private APIs.

[please use gerrit instead, 2015-03-05 21:42:52]

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:31: if
(dictionary_) {
No need to save "dictionary_". SpellcheckService can be null on shutdown and it
owns SpellcheckCustomDictionary, so that can be null on shutdown as well.
However "dictionary_" is never set to null. Do this in the destructor instead:

SpellcheckService* spellcheck =
SpellcheckServiceFactory::GetForContext(context_);
if (spellcheck)
  spellcheck->GetCustomDictionary()->RemoveObserver(this);

Alternatively, if you feel super productive, you can make either
SpellcheckService or SpellcheckCustomDictionary implement base::WeakPtr
interface. Then you can save either "spellcheck_" or "dictionary_" and use them
any time you want. (A base::WeakPtr will become NULL when the object is
deleted.)

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.h (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.h:25: virtual
~ExtensionDictionaryEventRouter();
I think we've changed from "vurtual ~ClassName();" to "~ClassName() override;"
for destructors.

[please use gerrit instead, 2015-03-05 21:43:33]

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
2015

[Peter Wen, 2015-03-06 16:09:09]

PTAL.

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2015/03/05 21:43:33, Rouslan Solomakhin wrote:
> 2015

Done.

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:31: if
(dictionary_) {
On 2015/03/05 21:42:51, Rouslan Solomakhin wrote:
> No need to save "dictionary_". SpellcheckService can be null on shutdown and
it
> owns SpellcheckCustomDictionary, so that can be null on shutdown as well.
> However "dictionary_" is never set to null. Do this in the destructor instead:
> 
> SpellcheckService* spellcheck =
> SpellcheckServiceFactory::GetForContext(context_);
> if (spellcheck)
>   spellcheck->GetCustomDictionary()->RemoveObserver(this);
> 
> Alternatively, if you feel super productive, you can make either
> SpellcheckService or SpellcheckCustomDictionary implement base::WeakPtr
> interface. Then you can save either "spellcheck_" or "dictionary_" and use
them
> any time you want. (A base::WeakPtr will become NULL when the object is
> deleted.)

I changed it to use base::WeakPtr for SpellcheckService. Do you have an example
where a class can implement WeakPtr? :)

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.h (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2015/03/05 21:42:51, Rouslan Solomakhin wrote:
> 2015

Done.

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.h:25: virtual
~ExtensionDictionaryEventRouter();
On 2015/03/05 21:42:52, Rouslan Solomakhin wrote:
> I think we've changed from "vurtual ~ClassName();" to "~ClassName() override;"
> for destructors.

Since this class only inherits from Observer, and observer does not define a
destructor, There is nothing to override, and I need to create my own virtual
destructor.

Is there a better way?

[please use gerrit instead, 2015-03-06 17:54:34]

rouslan@chromium.org changed reviewers:
+ groby@chromium.org

[please use gerrit instead, 2015-03-06 17:54:35]

spellcheck_service.{cc,h} changes and usage of spellcheck API LGTM.

R=groby for OWNERS review of spellcheck_service.{cc,h}.

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:31: if
(dictionary_) {
On 2015/03/06 16:09:09, Peter Wen wrote:
> On 2015/03/05 21:42:51, Rouslan Solomakhin wrote:
> > No need to save "dictionary_". SpellcheckService can be null on shutdown and
> it
> > owns SpellcheckCustomDictionary, so that can be null on shutdown as well.
> > However "dictionary_" is never set to null. Do this in the destructor
instead:
> > 
> > SpellcheckService* spellcheck =
> > SpellcheckServiceFactory::GetForContext(context_);
> > if (spellcheck)
> >   spellcheck->GetCustomDictionary()->RemoveObserver(this);
> > 
> > Alternatively, if you feel super productive, you can make either
> > SpellcheckService or SpellcheckCustomDictionary implement base::WeakPtr
> > interface. Then you can save either "spellcheck_" or "dictionary_" and use
> them
> > any time you want. (A base::WeakPtr will become NULL when the object is
> > deleted.)
> 
> I changed it to use base::WeakPtr for SpellcheckService. Do you have an
example
> where a class can implement WeakPtr? :)

Sorry I meant SupportsWeakPtr, like this for example:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Your approach is also OK and is actually preferred.

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.h (right):

https://codereview.chromium.org/973213003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.h:25: virtual
~ExtensionDictionaryEventRouter();
On 2015/03/06 16:09:09, Peter Wen wrote:
> On 2015/03/05 21:42:52, Rouslan Solomakhin wrote:
> > I think we've changed from "vurtual ~ClassName();" to "~ClassName()
override;"
> > for destructors.
> 
> Since this class only inherits from Observer, and observer does not define a
> destructor, There is nothing to override, and I need to create my own virtual
> destructor.

SGTM.

https://codereview.chromium.org/973213003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:29: if (service_)
{
nit: no need for { and }.

[Peter Wen, 2015-03-06 18:46:04]

Thanks Rouslan!

https://codereview.chromium.org/973213003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:29: if (service_)
{
On 2015/03/06 17:54:34, Rouslan Solomakhin wrote:
> nit: no need for { and }.

Done.

[groby-ooo-7-16, 2015-03-06 18:55:19]

c/b/spellchecker syntactically LGTM

rouslan: I assume you're the main reviewer and have reviewed that there's a good
reason for exposing a WeakPtr to the service?

[please use gerrit instead, 2015-03-06 19:03:56]

On 2015/03/06 18:55:19, groby wrote:
> c/b/spellchecker syntactically LGTM
> 
> rouslan: I assume you're the main reviewer

Correct.

> and have reviewed that there's a good
> reason for exposing a WeakPtr to the service?

Also correct.

[bshe, 2015-03-09 13:16:40]

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:21: context_);
Not sure when spellcheck will be null. But is it possible to get unexpected
behavior if spellcheck is null? Looks like we then stopped observing dictionary.

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/input_method_api.cc (left):

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/input_method_api.cc:197:
input_method_event_router_.reset(
I am not familiar with the code. But could this function being called multiple
times? Looks like you changed the logic here when that happen. Especially for
multi profiles case, you might want to test if it still works.

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/input_method_api.cc:199:
EventRouter::Get(context_)->UnregisterObserver(this);
Looks like you removed the UnregisterObserver step in the new code path. Could
this be a problem? If not a problem, you can remove the comment at line 200 and
201.

[Peter Wen, 2015-03-09 14:56:08]

PTAL. :)

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/dictionary_event_router.cc (right):

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/dictionary_event_router.cc:21: context_);
On 2015/03/09 13:16:39, bshe wrote:
> Not sure when spellcheck will be null. But is it possible to get unexpected
> behavior if spellcheck is null? Looks like we then stopped observing
dictionary.

If I understood correctly, spellcheck is null'ed on shutdown, in which case we
don't mind not observing.

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/input_method_api.cc (left):

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/input_method_api.cc:197:
input_method_event_router_.reset(
On 2015/03/09 13:16:39, bshe wrote:
> I am not familiar with the code. But could this function being called multiple
> times? Looks like you changed the logic here when that happen. Especially for
> multi profiles case, you might want to test if it still works.

Thanks for the heads up. Shu, thoughts?

I changed it so this function is called every time a listener is added instead
of exactly once, but each of the event routers is only initialized once, like
the original. We don't add too many listeners, so I think there is very little
overhead.

https://codereview.chromium.org/973213003/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/input_method_api.cc:199:
EventRouter::Get(context_)->UnregisterObserver(this);
On 2015/03/09 13:16:39, bshe wrote:
> Looks like you removed the UnregisterObserver step in the new code path. Could
> this be a problem? If not a problem, you can remove the comment at line 200
and
> 201.

Done.

[bshe, 2015-03-09 17:00:08]

great! lgtm

[Peter Wen, 2015-03-10 17:25:41]

@shuchen - ping for input_method_private.json OWNERS.

[groby-ooo-7-16, 2015-03-10 23:37:05]

I suppose it's too late to ask why this is a private API?

[Shu Chen, 2015-03-11 03:34:35]

Sorry I don't understand the flow of onDictionaryLoaded event.

Can you please elaborate how onDictionaryLoaded event is triggered?

I just want to make sure the event can be received by IME extension correctly,
even when the IME extension requires quite amount of time to be loaded.

Thanks

[Peter Wen, 2015-03-11 14:05:02]

On 2015/03/10 23:37:05, groby wrote:
> I suppose it's too late to ask why this is a private API?

This API arose out of the need for Chrome OS input tools to include
user-selected entries in automatic typing correction. This way when automatic
corrections are applied, users can specify that they prefer the original to be
ranked higher than the automatically corrected one. We have our own native
client dictionaries on Chrome OS that are used to select the automatic
correction candidates.

We built our own personal dictionary interface, but since Chrome OS already had
the custom spelling dictionary, we needed to consolidate the two. The decision
was to create this API and to exclusively use the custom spelling dictionary
instead of yet another dictionary management interface.

So our front-ends will call this API when users want to add words that they
prefer (pronouns or otherwise misspelled words), directly to the custom spelling
dictionary, and our backend will listen to changes to the custom spelling
dictionary and update its data models appropriately to provide the right
automatic correction mechanisms next time the user types the same word.

To answer your original question, it is currently a private API because it's the
easiest to add it to our existing private input method API. :)

[Peter Wen, 2015-03-11 14:18:38]

On 2015/03/11 03:34:35, Shu Chen wrote:
> Sorry I don't understand the flow of onDictionaryLoaded event.
> 
> Can you please elaborate how onDictionaryLoaded event is triggered?
> 
> I just want to make sure the event can be received by IME extension correctly,
> even when the IME extension requires quite amount of time to be loaded.
> 
> Thanks

From what I understand to be the case:

When the browser-context-specific spellcheck service first initializes the
spellcheck custom dictionary, the custom dictionary begins reading the local
file storage for the dictionary entries. When it is done, it fires the
OnCustomDictionaryLoaded event only then to all its observers, exactly once.
After it is loaded, all other calls to get entries and add entries will succeed.
This is only an issue if we are the first ones to request the spellcheck
service, which may be the case.

The use case for our onDictionaryLoaded event would be the initial fetch for all
entries. If the custom dictionary is not done loading yet, the fetch will fail,
but in this case we know that the OnCustomDictionaryLoaded event has not fired
yet. Thus if we first add a listener for the event and then call fetch, one of
two things will happen:
1. The dictionary was already loaded, in this case the onDictionaryLoaded event
will never fire, and our fetch will succeed.
2. The dictionary is not loaded yet, in this case the onDictionaryLoaded event
will eventually fire, and our initial fetch request will fail.
  2.1. In the onDictionaryLoaded event handler, we can make another fetch
request which is guaranteed to succeed.

This is why I make use of the new Promise construct in our extension API test,
it covers both asynchronous cases very well.

Another approach I considered is to have the fetch request itself wait on the
OnCustomDictionaryLoaded event if the dictionary is not yet loaded, but I was
not familiar enough with asynchronous C++ event handling to do so for both fetch
requests and add word requests. It would take longer for me than this approach,
but if you feel that is more appropriate then I'd be happy to learn the
necessary steps to do so.

[groby-ooo-7-16, 2015-03-11 17:58:40]

On 2015/03/11 14:18:38, Peter Wen wrote:
> On 2015/03/11 03:34:35, Shu Chen wrote:
> > Sorry I don't understand the flow of onDictionaryLoaded event.
> > 
> > Can you please elaborate how onDictionaryLoaded event is triggered?
> > 
> > I just want to make sure the event can be received by IME extension
correctly,
> > even when the IME extension requires quite amount of time to be loaded.
> > 
> > Thanks
> 
> From what I understand to be the case:
> 
> When the browser-context-specific spellcheck service first initializes the
> spellcheck custom dictionary, the custom dictionary begins reading the local
> file storage for the dictionary entries. When it is done, it fires the
> OnCustomDictionaryLoaded event only then to all its observers, exactly once.
> After it is loaded, all other calls to get entries and add entries will
succeed.
> This is only an issue if we are the first ones to request the spellcheck
> service, which may be the case.
> 
> The use case for our onDictionaryLoaded event would be the initial fetch for
all
> entries. If the custom dictionary is not done loading yet, the fetch will
fail,
> but in this case we know that the OnCustomDictionaryLoaded event has not fired
> yet. Thus if we first add a listener for the event and then call fetch, one of
> two things will happen:
> 1. The dictionary was already loaded, in this case the onDictionaryLoaded
event
> will never fire, and our fetch will succeed.
> 2. The dictionary is not loaded yet, in this case the onDictionaryLoaded event
> will eventually fire, and our initial fetch request will fail.
>   2.1. In the onDictionaryLoaded event handler, we can make another fetch
> request which is guaranteed to succeed.
> 
> This is why I make use of the new Promise construct in our extension API test,
> it covers both asynchronous cases very well.
> 
> Another approach I considered is to have the fetch request itself wait on the
> OnCustomDictionaryLoaded event if the dictionary is not yet loaded, but I was
> not familiar enough with asynchronous C++ event handling to do so for both
fetch
> requests and add word requests. It would take longer for me than this
approach,
> but if you feel that is more appropriate then I'd be happy to learn the
> necessary steps to do so.

I still think this should be at least considered as a public API, and an API on
spellcheck, not inputMethod.

[Peter Wen, 2015-03-11 22:34:16]

On 2015/03/11 17:58:40, groby wrote:
> I still think this should be at least considered as a public API, and an API
on
> spellcheck, not inputMethod.

Agreed, this API can and probably should eventually be exposed as a
chrome.spellcheck API.

For now, to unblock our front-end work, would you be okay with us hosting it in
chrome.inputMethodPrivate? Once it's stabilized and the public API is added to
chrome.spellcheck, it will not be hard to rewire our private front-end calls to
the new public API.

[groby-ooo-7-16, 2015-03-12 01:01:36]

Yeah, absolutely OK w/ private API as interim step. c/b/spellchecker still LGTM
:)

[Shu Chen, 2015-03-12 01:18:29]

On 2015/03/11 14:18:38, Peter Wen wrote:
> On 2015/03/11 03:34:35, Shu Chen wrote:
> > Sorry I don't understand the flow of onDictionaryLoaded event.
> > 
> > Can you please elaborate how onDictionaryLoaded event is triggered?
> > 
> > I just want to make sure the event can be received by IME extension
correctly,
> > even when the IME extension requires quite amount of time to be loaded.
> > 
> > Thanks
> 
> From what I understand to be the case:
> 
> When the browser-context-specific spellcheck service first initializes the
> spellcheck custom dictionary, the custom dictionary begins reading the local
> file storage for the dictionary entries. When it is done, it fires the
> OnCustomDictionaryLoaded event only then to all its observers, exactly once.
> After it is loaded, all other calls to get entries and add entries will
succeed.
> This is only an issue if we are the first ones to request the spellcheck
> service, which may be the case.
> 
> The use case for our onDictionaryLoaded event would be the initial fetch for
all
> entries. If the custom dictionary is not done loading yet, the fetch will
fail,
> but in this case we know that the OnCustomDictionaryLoaded event has not fired
> yet. Thus if we first add a listener for the event and then call fetch, one of
> two things will happen:
> 1. The dictionary was already loaded, in this case the onDictionaryLoaded
event
> will never fire, and our fetch will succeed.
> 2. The dictionary is not loaded yet, in this case the onDictionaryLoaded event
> will eventually fire, and our initial fetch request will fail.
>   2.1. In the onDictionaryLoaded event handler, we can make another fetch
> request which is guaranteed to succeed.
> 
> This is why I make use of the new Promise construct in our extension API test,
> it covers both asynchronous cases very well.
> 
> Another approach I considered is to have the fetch request itself wait on the
> OnCustomDictionaryLoaded event if the dictionary is not yet loaded, but I was
> not familiar enough with asynchronous C++ event handling to do so for both
fetch
> requests and add word requests. It would take longer for me than this
approach,
> but if you feel that is more appropriate then I'd be happy to learn the
> necessary steps to do so.

Adding 2 APIs for 1 thing would make things complicated. I would suggest to use
one API instead.

chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function(e) {
  // Gets custom dictionary entries from |e|.
});

When adding the listener, if the custom dictionary is already been loaded, just
issue the onDictionaryLoaded immediately.

There are similar designs for other APIs. For example,
chrome.input.ime.onActivate event works that way.
When the listener is firstly added, it issue the event immediately to tell the
extension about the current IME.

[Peter Wen, 2015-03-12 21:48:44]

On 2015/03/12 01:18:29, Shu Chen wrote:
> Adding 2 APIs for 1 thing would make things complicated. I would suggest to
use
> one API instead.
> 
> chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function(e) {
>   // Gets custom dictionary entries from |e|.
> });
> 
> When adding the listener, if the custom dictionary is already been loaded,
just
> issue the onDictionaryLoaded immediately.
> 
> There are similar designs for other APIs. For example,
> chrome.input.ime.onActivate event works that way.
> When the listener is firstly added, it issue the event immediately to tell the
> extension about the current IME.

I agree that it is nice to just wrap all dictionary code with
onDictionaryLoaded. I would prefer to separate fetch/add from the event, as if
we merge fetch and the load event, then adding a single word is left hanging.

How about this:

If the dictionary is loaded, then the callback is run immediately (preferably
async to prevent UI jank).
If the dictionary is not loaded, then the callback gets called when the
dictionary actually loads.

Under this model, we can just wrap all fetch/add calls inside an
onDictionaryLoaded.addListener callback and remove the listener when we're done.
This way we are not constrained by having to always do a fetch if we are just
adding a single word to the dictionary.

Example 1 - this would be for initializing a particular native client language
model:

var listener =
chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function() {
  chrome.inputMethodPrivate.onDictionaryLoaded.removeListener(listener);
  
  chrome.inputMethodPrivate.fetchAllDictionaryEntries(function(entries) {
    // process entries
  });
});

Example 2 - this would be for user clicking on VK "add to dictionary" or PK menu
item "add to dictionary":

var listener =
chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function() {
  chrome.inputMethodPrivate.onDictionaryLoaded.removeListener(listener);
  
  chrome.inputMethodPrivate.addWordToDictionary("helloworld");
});

[Shu Chen, 2015-03-13 07:14:49]

On 2015/03/12 21:48:44, Peter Wen wrote:
> On 2015/03/12 01:18:29, Shu Chen wrote:
> > Adding 2 APIs for 1 thing would make things complicated. I would suggest to
> use
> > one API instead.
> > 
> > chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function(e) {
> >   // Gets custom dictionary entries from |e|.
> > });
> > 
> > When adding the listener, if the custom dictionary is already been loaded,
> just
> > issue the onDictionaryLoaded immediately.
> > 
> > There are similar designs for other APIs. For example,
> > chrome.input.ime.onActivate event works that way.
> > When the listener is firstly added, it issue the event immediately to tell
the
> > extension about the current IME.
> 
> I agree that it is nice to just wrap all dictionary code with
> onDictionaryLoaded. I would prefer to separate fetch/add from the event, as if
> we merge fetch and the load event, then adding a single word is left hanging.
> 
> How about this:
> 
> If the dictionary is loaded, then the callback is run immediately (preferably
> async to prevent UI jank).
> If the dictionary is not loaded, then the callback gets called when the
> dictionary actually loads.
Thanks for your elaboration. That makes a lot of sense.
I would love & prefer this solution.
> 
> Under this model, we can just wrap all fetch/add calls inside an
> onDictionaryLoaded.addListener callback and remove the listener when we're
done.
> This way we are not constrained by having to always do a fetch if we are just
> adding a single word to the dictionary.
> 
> Example 1 - this would be for initializing a particular native client language
> model:
> 
> var listener =
> chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function() {
>   chrome.inputMethodPrivate.onDictionaryLoaded.removeListener(listener);
>   
>   chrome.inputMethodPrivate.fetchAllDictionaryEntries(function(entries) {
>     // process entries
>   });
> });
> 
> Example 2 - this would be for user clicking on VK "add to dictionary" or PK
menu
> item "add to dictionary":
> 
> var listener =
> chrome.inputMethodPrivate.onDictionaryLoaded.addListener(function() {
>   chrome.inputMethodPrivate.onDictionaryLoaded.removeListener(listener);
>   
>   chrome.inputMethodPrivate.addWordToDictionary("helloworld");
> });

[Peter Wen, 2015-03-13 21:02:21]

On 2015/03/13 07:14:49, Shu Chen wrote:
> Thanks for your elaboration. That makes a lot of sense.
> I would love & prefer this solution.

I revised the implementation to fire the event again if a new listener is added
and the dictionary is already loaded.

This requires us to ensure on the front-end that we *always* remove the listener
inside the listener, but since we'll be the only ones consuming this API I can
just add a wrapper in our code to make sure of this. :)

Will revise test on Monday to verify asychrony.

[Peter Wen, 2015-03-16 21:18:23]

On 2015/03/13 21:02:21, Peter Wen wrote:
> On 2015/03/13 07:14:49, Shu Chen wrote:
> > Thanks for your elaboration. That makes a lot of sense.
> > I would love & prefer this solution.
> 
> I revised the implementation to fire the event again if a new listener is
added
> and the dictionary is already loaded.
> 
> This requires us to ensure on the front-end that we *always* remove the
listener
> inside the listener, but since we'll be the only ones consuming this API I can
> just add a wrapper in our code to make sure of this. :)
> 
> Will revise test on Monday to verify asychrony.

Shu - updated tests, PTAL.

Peter

[Shu Chen, 2015-03-17 01:41:45]

lgtm

[Peter Wen, 2015-03-17 17:53:46]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2015-03-17 17:53:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, groby@chromium.org, bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/973213003/#ps180001
(title: "Add async test.")

[commit-bot: I haz the power, 2015-03-17 17:54:07]

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

[commit-bot: I haz the power, 2015-03-17 18:02:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-03-17 18:03:48]

Patchset 10 (id:??) landed as
https://crrev.com/38625727a94921d4773b267b4d5f4692dd688689
Cr-Commit-Position: refs/heads/master@{#320940}