Editing the custom spelling dictionary

Enable users to alter their custom dictionary from an overlay on chrome://settings/editDictionary. This overlay can also be accessed through:

Settings
|_Show advanced settings...
..|_Language and spell-checker settings...
....|_Custom spelling dictionary

When a user types an unknown word into a textarea, the browser underlines this word as a misspelling. The user can right-click on this word and select "Add to Dictionary". If the user made a mistake, the user needs a way to remove a word from their custom dictionary. This CL provides this functionality.

Automated tests are in https://codereview.chromium.org/11280013/ and https://codereview.chromium.org/11308076/.

BUG=18238
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168760

[please use gerrit instead, 2012-11-10 01:59:54]

Will need the following patch to src/third_party/hunspell to enable removing
words from the dictionary:

diff --git a/src/hunspell/hashmgr.cxx b/src/hunspell/hashmgr.cxx
index df37acf..408ea8d 100644
--- a/src/hunspell/hashmgr.cxx
+++ b/src/hunspell/hashmgr.cxx
@@ -324,6 +324,12 @@ int HashMgr::get_clen_and_captype(const char * word, int
wbl, int * captype) {
 // remove word (personal dictionary function for standalone applications)
 int HashMgr::remove(const char * word)
 {
+#ifdef HUNSPELL_CHROME_CLIENT
+    std::map<base::StringPiece, int>::iterator iter =
+            custom_word_to_affix_id_map_.find(word);
+    if (iter != custom_word_to_affix_id_map_.end())
+        custom_word_to_affix_id_map_.erase(iter);
+#else
     struct hentry * dp = lookup(word);
     while (dp) {
         if (dp->alen == 0 || !TESTAFF(dp->astr, forbiddenword, dp->alen)) {
@@ -338,6 +344,7 @@ int HashMgr::remove(const char * word)
         }
         dp = dp->next_homonym;
     }
+#endif
     return 0;
 }

[please use gerrit instead, 2012-11-14 18:03:41]

cpu -  chrome/app/generated_resources.grd

csilv - chrome/browser/resources/options
        chrome/browser/ui/webui/options

rlp - chrome/browser/spellchecker
      chrome/renderer/spellchecker

jhawkins - chrome/chrome_browser_ui.gypi

cdn - chrome/common/spellcheck_messages.h

[James Hawkins, 2012-11-14 18:31:25]

gypi lgtm

[rpetterson, 2012-11-14 19:47:09]

http://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

http://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:56: void
SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
Why doesn't this function notify observers of OnCustomDictionaryLoaded as well?

http://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:71: void
SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
Does this need to be called on the UI thread? If so, should we have a DCHECK? It
looks like it will be when it's called from in Load(), but I'm assuming someone
can call it elsewhere.

http://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:151: 
Replace the next 8 lines with LoadDictionaryIntoCustomWordList?

http://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecker...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

http://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecker...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:35: // Returns a
newly allocated list of words read from custom dictionary file.
Who owns this new list? Can you add that to the comment?

http://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc (right):

http://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc:50: // OSX doesn't
support the custom dictionary.
yet? :)

http://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/spellcheck.cc (right):

http://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/spellcheck.cc:133: if (!platform_spelling_engine_
||
Can you update this to platform_spelling_engine_.get() as well for consistency?
(since you updated the one above)

[csilv, 2012-11-14 20:01:52]

https://chromiumcodereview.appspot.com/11362063/diff/8081/chrome/browser/reso...
File chrome/browser/resources/options/language_dictionary_overlay.js (right):

https://chromiumcodereview.appspot.com/11362063/diff/8081/chrome/browser/reso...
chrome/browser/resources/options/language_dictionary_overlay.js:11:
OptionsPage.call(this, 'dictionary',
nit: I think 'editDictionary' might be a better identifier.

https://chromiumcodereview.appspot.com/11362063/diff/8081/chrome/browser/reso...
File chrome/browser/resources/options/options.js (right):

https://chromiumcodereview.appspot.com/11362063/diff/8081/chrome/browser/reso...
chrome/browser/resources/options/options.js:124: LanguageOptions.getInstance());
this should be associated with the edit-dictionary button.

https://chromiumcodereview.appspot.com/11362063/diff/8081/chrome/browser/ui/w...
File chrome/browser/ui/webui/options/language_dictionary_overlay_handler.h
(right):

https://chromiumcodereview.appspot.com/11362063/diff/8081/chrome/browser/ui/w...
chrome/browser/ui/webui/options/language_dictionary_overlay_handler.h:9:
#include "chrome/browser/spellchecker/spellcheck_custom_dictionary.h"
nit: order includes in alphabetical order.

[cpu_(ooo_6.6-7.5), 2012-11-14 20:25:00]

grd lgtm.

http://codereview.chromium.org/11362063/diff/8081/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11362063/diff/8081/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6882: +        Custom spelling dictionary
The word 'overlay' seems ambiguous to me. Maybe the translator might have this
issue too.

[Cris Neckar, 2012-11-14 20:33:03]

IPC changes lgtm

[csilv, 2012-11-14 20:54:19]

Rouslan, I'd like to see a more verbose changelist description that would give
someone a better understanding of what this CL is all about without having to
look at the code.

[please use gerrit instead, 2012-11-14 21:27:30]

On 2012/11/14 20:54:19, csilv wrote:
> Rouslan, I'd like to see a more verbose changelist description that would give
> someone a better understanding of what this CL is all about without having to
> look at the code.

Good point. I've updated the description. Here is a screenshot, too:
http://i.imgur.com/BWFY5.png

[please use gerrit instead, 2012-11-14 22:54:45]

csilv, rpetterson: PTAL.

https://codereview.chromium.org/11362063/diff/8081/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6882: +        Custom spelling dictionary
On 2012/11/14 20:25:00, cpu wrote:
> The word 'overlay' seems ambiguous to me. Maybe the translator might have this
> issue too.

I've replaced "custom dictionary overlay" with "chrome://settings/editDictionary
page" to correspond more closely with similar comments in this file.

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/resources/o...
File chrome/browser/resources/options/language_dictionary_overlay.js (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/resources/o...
chrome/browser/resources/options/language_dictionary_overlay.js:11:
OptionsPage.call(this, 'dictionary',
On 2012/11/14 20:01:52, csilv wrote:
> nit: I think 'editDictionary' might be a better identifier.

Done.

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/resources/o...
File chrome/browser/resources/options/options.js (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/resources/o...
chrome/browser/resources/options/options.js:124: LanguageOptions.getInstance());
On 2012/11/14 20:01:52, csilv wrote:
> this should be associated with the edit-dictionary button.

Done.

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecke...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.cc (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecke...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:56: void
SpellcheckCustomDictionary::LoadDictionaryIntoCustomWordList(
On 2012/11/14 19:47:09, rpetterson wrote:
> Why doesn't this function notify observers of OnCustomDictionaryLoaded as
well?

This method does not modify the private member variable |custom_words_|. If it
was not for |custom_dictionary_path_|, this method would have been static. 

This method runs on the file thread, whereas the listeners are on the UI thread.

Here is how loading the dictionary works here:

1) [UI]   Load()
2) [FILE] LoadDictionary()
3) [FILE] LoadDictionaryIntocustomWordList()
4) [UI]   SetCustomWordList()
5) [UI]   OnCustomDictionaryLoaded()

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecke...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:71: void
SpellcheckCustomDictionary::SetCustomWordList(WordList* custom_words) {
On 2012/11/14 19:47:09, rpetterson wrote:
> Does this need to be called on the UI thread? If so, should we have a DCHECK?
It
> looks like it will be when it's called from in Load(), but I'm assuming
someone
> can call it elsewhere.

Adding dchecks for the correct browser thread in all methods.

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecke...
chrome/browser/spellchecker/spellcheck_custom_dictionary.cc:151: 
On 2012/11/14 19:47:09, rpetterson wrote:
> Replace the next 8 lines with LoadDictionaryIntoCustomWordList?

Done.

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecke...
File chrome/browser/spellchecker/spellcheck_custom_dictionary.h (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/spellchecke...
chrome/browser/spellchecker/spellcheck_custom_dictionary.h:35: // Returns a
newly allocated list of words read from custom dictionary file.
On 2012/11/14 19:47:09, rpetterson wrote:
> Who owns this new list? Can you add that to the comment?

The caller owns this new list. Added to the comment.

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/language_dictionary_overlay_handler.h
(right):

https://codereview.chromium.org/11362063/diff/8081/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/language_dictionary_overlay_handler.h:9:
#include "chrome/browser/spellchecker/spellcheck_custom_dictionary.h"
On 2012/11/14 20:01:52, csilv wrote:
> nit: order includes in alphabetical order.

Done.

https://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellcheck...
File chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellcheck...
chrome/renderer/spellchecker/cocoa_spelling_engine_mac.cc:50: // OSX doesn't
support the custom dictionary.
On 2012/11/14 19:47:09, rpetterson wrote:
> yet? :)

Done.

https://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellcheck...
File chrome/renderer/spellchecker/spellcheck.cc (right):

https://codereview.chromium.org/11362063/diff/8081/chrome/renderer/spellcheck...
chrome/renderer/spellchecker/spellcheck.cc:133: if (!platform_spelling_engine_
||
On 2012/11/14 19:47:09, rpetterson wrote:
> Can you update this to platform_spelling_engine_.get() as well for
consistency?
> (since you updated the one above)

Done.

[csilv, 2012-11-15 01:12:38]

lgtm

[please use gerrit instead, 2012-11-15 21:36:07]

rpetterson: PTAL.

- Duplicates are no longer added to the dictionary file.
- No longer leaking memory when loading the dictionary file.
- Users cannot edit words in place: only add or remove.
- User interface lets users add duplicate words.
- All new words go to the bottom of the list.
- Page refresh or overlay open-close remove duplicates and sort all the words.

[rpetterson, 2012-11-15 23:01:05]

LGTM

[commit-bot: I haz the power, 2012-11-15 23:08:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11362063/30006

[commit-bot: I haz the power, 2012-11-15 23:55:11]

Retried try job too often for step(s) content_unittests

[commit-bot: I haz the power, 2012-11-16 16:48:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11362063/30006

[commit-bot: I haz the power, 2012-11-16 16:49:09]

Failed to apply patch for chrome/browser/spellchecker/spellcheck_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/spellchecker/spellcheck_service.cc
  Hunk #1 succeeded at 50 with fuzz 1 (offset -1 lines).
  Hunk #2 FAILED at 132.
  Hunk #3 succeeded at 179 (offset -6 lines).
  Hunk #4 succeeded at 224 (offset -6 lines).
  Hunk #5 FAILED at 242.
  2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/spellchecker/spellcheck_service.cc.rej

Patch:       chrome/browser/spellchecker/spellcheck_service.cc
Index: chrome/browser/spellchecker/spellcheck_service.cc
diff --git a/chrome/browser/spellchecker/spellcheck_service.cc
b/chrome/browser/spellchecker/spellcheck_service.cc
index
e4e79f71b6bb8c7bc412c42ca2f59e4223c3d42b..47cfcaf131f5a5ed8285c5554e5b4c8d62d9ccbb
100644
--- a/chrome/browser/spellchecker/spellcheck_service.cc
+++ b/chrome/browser/spellchecker/spellcheck_service.cc
@@ -51,6 +51,7 @@ SpellcheckService::SpellcheckService(Profile* profile)
   hunspell_dictionary_->Initialize();
 
   custom_dictionary_.reset(new SpellcheckCustomDictionary(profile_));
+  custom_dictionary_->AddObserver(this);
   custom_dictionary_->Load();
 
   registrar_.Add(weak_ptr_factory_.GetWeakPtr(),
@@ -131,32 +132,22 @@ bool SpellcheckService::SignalStatusEvent(
   return true;
 }
 
-// static
-void SpellcheckService::AttachStatusEvent(base::WaitableEvent* status_event) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  g_status_event = status_event;
-}
-
-// static
-SpellcheckService::EventType SpellcheckService::WaitStatusEvent() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  if (g_status_event)
-    g_status_event->Wait();
-  return g_status_type;
-}
-
-void SpellcheckService::Initialize() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  hunspell_dictionary_->Initialize();
-}
-
 void SpellcheckService::StartRecordingMetrics(bool spellcheck_enabled) {
   metrics_.reset(new SpellCheckHostMetrics());
   metrics_->RecordEnabledStats(spellcheck_enabled);
 }
 
+void SpellcheckService::InitForAllRenderers() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  for (content::RenderProcessHost::iterator i(
+          content::RenderProcessHost::AllHostsIterator());
+       !i.IsAtEnd(); i.Advance()) {
+    content::RenderProcessHost* process = i.GetCurrentValue();
+    if (process)
+      InitForRenderer(process);
+  }
+}
+
 void SpellcheckService::InitForRenderer(content::RenderProcessHost* process) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -184,15 +175,37 @@ void
SpellcheckService::InitForRenderer(content::RenderProcessHost* process) {
 #endif
   }
 
-  WordList custom_words = GetCustomDictionary()->GetCustomWords();
-
   process->Send(new SpellCheckMsg_Init(
       file,
-      custom_words,
+      custom_dictionary_->GetWords(),
       hunspell_dictionary_->GetLanguage(),
       prefs->GetBoolean(prefs::kEnableAutoSpellCorrect)));
 }
 
+bool SpellcheckService::IsReady() const {
+  return hunspell_dictionary_->IsReady();
+}
+
+const base::PlatformFile& SpellcheckService::GetDictionaryFile() const {
+  return hunspell_dictionary_->GetDictionaryFile();
+}
+
+const std::string& SpellcheckService::GetLanguage() const {
+  return hunspell_dictionary_->GetLanguage();
+}
+
+bool SpellcheckService::IsUsingPlatformChecker() const {
+  return hunspell_dictionary_->IsUsingPlatformChecker();
+}
+
+SpellCheckHostMetrics* SpellcheckService::GetMetrics() const {
+  return metrics_.get();
+}
+
+SpellcheckCustomDictionary* SpellcheckService::GetCustomDictionary() {
+  return custom_dictionary_.get();
+}
+
 void SpellcheckService::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
@@ -207,7 +220,7 @@ void SpellcheckService::OnPreferenceChanged(PrefServiceBase*
prefs,
   DCHECK(prefs);
   if (pref_name_in == prefs::kSpellCheckDictionary ||
       pref_name_in == prefs::kEnableSpellCheck) {
-    InformProfileOfInitializationWithCustomWords(NULL);
+    InitForAllRenderers();
   } else if (pref_name_in == prefs::kEnableAutoSpellCorrect) {
     bool enabled = prefs->GetBoolean(prefs::kEnableAutoSpellCorrect);
     for (content::RenderProcessHost::iterator i(
@@ -219,44 +232,28 @@ void
SpellcheckService::OnPreferenceChanged(PrefServiceBase* prefs,
   }
 }
 
-SpellCheckHostMetrics* SpellcheckService::GetMetrics() const {
-  return metrics_.get();
+void SpellcheckService::OnCustomDictionaryLoaded() {
+  InitForAllRenderers();
 }
 
-SpellcheckCustomDictionary* SpellcheckService::GetCustomDictionary() {
-  return custom_dictionary_.get();
+void SpellcheckService::OnCustomDictionaryWordAdded(const std::string& word) {
 }
 
-bool SpellcheckService::IsReady() const {
-  return hunspell_dictionary_->IsReady();
+void SpellcheckService::OnCustomDictionaryWordRemoved(const std::string& word)
{
 }
 
-bool SpellcheckService::IsUsingPlatformChecker() const {
-  return hunspell_dictionary_->IsUsingPlatformChecker();
-}
-
-const base::PlatformFile& SpellcheckService::GetDictionaryFile() const {
-  return hunspell_dictionary_->GetDictionaryFile();
-}
+// static
+void SpellcheckService::AttachStatusEvent(base::WaitableEvent* status_event) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-const std::string& SpellcheckService::GetLanguage() const {
-  return hunspell_dictionary_->GetLanguage();
+  g_status_event = status_event;
 }
 
-// TODO(rlp): rename to something more logical.
-void SpellcheckService::InformProfileOfInitializationWithCustomWords(
-    WordList* custom_words) {
+// static
+SpellcheckService::EventType SpellcheckService::WaitStatusEvent() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (custom_words) {
-    GetCustomDictionary()->SetCustomWordList(custom_words);
-  }
-
-  for (content::RenderProcessHost::iterator i(
-          content::RenderProcessHost::AllHostsIterator());
-       !i.IsAtEnd(); i.Advance()) {
-    content::RenderProcessHost* process = i.GetCurrentValue();
-    if (process)
-      InitForRenderer(process);
-  }
+  if (g_status_event)
+    g_status_event->Wait();
+  return g_status_type;
 }

[commit-bot: I haz the power, 2012-11-16 17:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11362063/38007

[commit-bot: I haz the power, 2012-11-16 19:00:57]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-11-16 19:33:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11362063/26039

[Dan Beam, 2012-11-16 20:02:27]

https://codereview.chromium.org/11362063/diff/26039/chrome/browser/resources/...
File chrome/browser/resources/options/language_dictionary_overlay.js (right):

https://codereview.chromium.org/11362063/diff/26039/chrome/browser/resources/...
chrome/browser/resources/options/language_dictionary_overlay.js:9: 
FYI: you might want to add JSDoc here in a follow up patch - we definitely do
this in the NTP, not as sure about options... (but we should)

[please use gerrit instead, 2012-11-16 20:09:58]

https://codereview.chromium.org/11362063/diff/26039/chrome/browser/resources/...
File chrome/browser/resources/options/language_dictionary_overlay.js (right):

https://codereview.chromium.org/11362063/diff/26039/chrome/browser/resources/...
chrome/browser/resources/options/language_dictionary_overlay.js:9: 
On 2012/11/16 20:02:27, Dan Beam wrote:
> FYI: you might want to add JSDoc here in a follow up patch - we definitely do
> this in the NTP, not as sure about options... (but we should)

Will do.

[commit-bot: I haz the power, 2012-11-17 08:29:55]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-17 16:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11362063/26039

[commit-bot: I haz the power, 2012-11-17 20:06:17]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-20 06:36:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/11362063/27018

[commit-bot: I haz the power, 2012-11-20 08:26:48]

Change committed as 168760