Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1602303002:
[mac] Send the system spellchecker language to the renderer. (Closed)
|
Created:
4 years, 11 months ago by please use gerrit instead Modified:
4 years, 10 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[mac] Send the system spellchecker language to the renderer.
The browser process sends the system spellchecker language to the
renderer to initialize its text breaker. The text breaker is used when
providing suggestions in the context menu. If the text breaker is not
initialized, the menu has no suggestions.
The browser saves the system spellchecker language into the unsynced
preference prefs::kSpellCheckDictionaries. This preference is also used
by the spelling service client. If the preference is empty, then the
client is disabled.
BUG=577835
Committed: https://crrev.com/ff659a207ba7a0c42825963817cf3d342d00251e
Cr-Commit-Position: refs/heads/master@{#371605}
Patch Set 1 #
Total comments: 4
Patch Set 2 #
Total comments: 2
Patch Set 3 : Add comments. #
Messages
Total messages: 19 (11 generated)
Description was changed from ========== [mac] Initialize Mac spellchecker with a language. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. BUG=577835 ========== to ========== [mac] Initialize Mac spellchecker with a language. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ==========
Description was changed from ========== [mac] Initialize Mac spellchecker with a language. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ========== to ========== [mac] Initialize Mac spellchecker with a language. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by the spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ==========
Description was changed from ========== [mac] Initialize Mac spellchecker with a language. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by the spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ========== to ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by the spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ==========
Description was changed from ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by the spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ========== to ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by the spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ==========
Description was changed from ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language code to the renderer. The renderer needs a language code to initialize its text breaker. If the text breaker is not initialized, then the context menu will not have any spelling suggestions. The browser saves the language into the preference that is also used by the spelling service client: prefs::kSpellCheckDictionaries. If this preference is empty, then spelling service client is disabled. This preference is not synced. BUG=577835 ========== to ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language code to the renderer to initialize its text breaker. The text breaker is used when providing suggestions in the context menu. If the text breaker is not initialized, the menu has no suggestions. The browser saves the system spellchecker language into the unsynced preference prefs::kSpellCheckDictionaries. This preference is also used by the spelling service client. If the preference is empty, then the client is disabled. BUG=577835 ==========
Description was changed from ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language code to the renderer to initialize its text breaker. The text breaker is used when providing suggestions in the context menu. If the text breaker is not initialized, the menu has no suggestions. The browser saves the system spellchecker language into the unsynced preference prefs::kSpellCheckDictionaries. This preference is also used by the spelling service client. If the preference is empty, then the client is disabled. BUG=577835 ========== to ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language to the renderer to initialize its text breaker. The text breaker is used when providing suggestions in the context menu. If the text breaker is not initialized, the menu has no suggestions. The browser saves the system spellchecker language into the unsynced preference prefs::kSpellCheckDictionaries. This preference is also used by the spelling service client. If the preference is empty, then the client is disabled. BUG=577835 ==========
rouslan@chromium.org changed reviewers: + groby@chromium.org
Rachel, PTAL.
https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_platform_mac.mm (right): https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_platform_mac.mm:113: return base::SysNSStringToUTF8([SharedSpellChecker() language]); Don't you want to call ConvertLanguageCodeFromMac? Also, if the user has set the language to "Automatic", what happens here? https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.cc:85: if (dictionaries_pref.GetValue().empty()) { Do we really want to mess with prefs here? What happens in the cross-platform sync case?
Rachel, ptal patch 2. https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_platform_mac.mm (right): https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_platform_mac.mm:113: return base::SysNSStringToUTF8([SharedSpellChecker() language]); On 2016/01/19 23:54:14, groby wrote: > Don't you want to call ConvertLanguageCodeFromMac? Done. > Also, if the user has set the language to "Automatic", what happens here? I have "Automatic by language" selected in Mac spellcheck settings. This function returns "en" for me. Not sure how Mac determines what to return internally. https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1602303002/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.cc:85: if (dictionaries_pref.GetValue().empty()) { On 2016/01/19 23:54:14, groby wrote: > Do we really want to mess with prefs here? Yes, because the spelling service client is using this preference to determine whether to be enabled. Also, the client sends this language together with the spellcheck request. An alternative is to #ifdef both spelling service client and SpellCheckMsg_Init() here. I think changing one place is better than multiple. In fact, since a Mac user has no way of modifying this preference in Chrome, it should be set to platform spellchecker's language unconditionally. > What happens in the cross-platform > sync case? This preference is not synced, as you can see from its initialization here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/spe... user_prefs->RegisterListPref(prefs::kSpellCheckDictionaries, new base::ListValue); Absence of user_prefs::PrefRegistrySyncable::SYNCABLE_PREF flag indicates that it's not syncable.
LGTM w/ documentation nit. https://codereview.chromium.org/1602303002/diff/20001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1602303002/diff/20001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:55: first_of_dictionaries = dictionaries_pref.GetValue().front(); Can you add a short explanation what either code swath actually does? Or possible move the migration part into a MigratePreferences? It's hard to intuit why we need either 30 lines or 3 :)
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/1602303002/#ps40001 (title: "Add comments.")
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1602303002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1602303002/40001
Sending to cq. https://codereview.chromium.org/1602303002/diff/20001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1602303002/diff/20001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:55: first_of_dictionaries = dictionaries_pref.GetValue().front(); On 2016/01/26 01:30:10, groby wrote: > Can you add a short explanation what either code swath actually does? Or > possible move the migration part into a MigratePreferences? It's hard to intuit > why we need either 30 lines or 3 :) Done.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language to the renderer to initialize its text breaker. The text breaker is used when providing suggestions in the context menu. If the text breaker is not initialized, the menu has no suggestions. The browser saves the system spellchecker language into the unsynced preference prefs::kSpellCheckDictionaries. This preference is also used by the spelling service client. If the preference is empty, then the client is disabled. BUG=577835 ========== to ========== [mac] Send the system spellchecker language to the renderer. The browser process sends the system spellchecker language to the renderer to initialize its text breaker. The text breaker is used when providing suggestions in the context menu. If the text breaker is not initialized, the menu has no suggestions. The browser saves the system spellchecker language into the unsynced preference prefs::kSpellCheckDictionaries. This preference is also used by the spelling service client. If the preference is empty, then the client is disabled. BUG=577835 Committed: https://crrev.com/ff659a207ba7a0c42825963817cf3d342d00251e Cr-Commit-Position: refs/heads/master@{#371605} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ff659a207ba7a0c42825963817cf3d342d00251e Cr-Commit-Position: refs/heads/master@{#371605} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
