Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
654653002:
Enables the user to select multiple languages for spellchecking (UI) (Closed)
|
Created:
6 years, 2 months ago by Klemen Forstnerič Modified:
5 years, 5 months ago CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spellwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnables the user to select multiple languages for spellchecking (UI)
This patch enables the user to select multiple languages for
spellchecking using the right click menu, as well as through the
chrome://settings/languages. It can be enabled through a command
line flag "--enable-multilingual-spellchecker". It also adds the
option to enable or disable this flag using the chrome://flags page.
BUG=5102
Patch Set 1 : Initial patchset #Patch Set 2 : Temporarily disabled tests, fixed a bug #
Total comments: 7
Patch Set 3 : Made functions inline #
Total comments: 4
Patch Set 4 : Updated the chrome://settings/languages to support the selection of multiple languages for spellche… #Patch Set 5 : Reset unneccessary changes. #
Total comments: 61
Patch Set 6 : Addressed comments. #
Total comments: 22
Patch Set 7 : Addressed some of the comments. #Patch Set 8 : Addressed the remaining comments. #Patch Set 9 : Fixed the value of 'enable-multilingual-spellchecker' in histograms.xml #
Total comments: 14
Patch Set 10 : Addressed comments. #Patch Set 11 : Addressed remaining comments. #Patch Set 12 : Addressed comments. #Patch Set 13 : Possible empty menu entry fix. #
Total comments: 5
Patch Set 14 : Refactored spellcheck dictionary reading code. #Patch Set 15 : Fixed a regression. #
Total comments: 15
Messages
Total messages: 75 (20 generated)
Patchset #5 (id:160001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
klemen.forstneric@gmail.com changed reviewers: + rouslan@chromium.org
Rouslan: PTAL Patch Set 2.
https://codereview.chromium.org/654653002/diff/200001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/200001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:232: std::vector<std::string> SpellcheckService::GetAcceptLanguages( It's usually best for readability to inline the functions that are used only once. https://codereview.chromium.org/654653002/diff/200001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:252: std::vector<std::string> SpellcheckService::GetSpellCheckDictionaryLanguages( Ditto.
https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:177: void SpellCheckerSubMenuObserver::AddLanguagesAsRadioItems() { Ditto. https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:187: void SpellCheckerSubMenuObserver::AddLanguagesAsCheckItems() { Ditto.
https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:61: IDS_CONTENT_CONTEXT_LANGUAGE_SETTINGS); Whitespace changes should go into a whitespace-only patch. Whitespace changes are allowed only if code formatting was not following guidelines or clang-format thinks so.
Don't be scared of long functions :-) https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:197: void SpellCheckerSubMenuObserver::UpdateSpellcheckDictionariesWithLanguage( Please inline. https://codereview.chromium.org/654653002/diff/200001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:219: void SpellCheckerSubMenuObserver::SetLanguageAsSpellcheckDictionary( Please inline.
https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:55: if (command_line->HasSwitch(switches::kEnableMultilingualSpellChecker)) { It's best to put this line into a separate function that you call here and below. When we will move from a command-line argument to an experiment (enabled by default for 10% of users), it will be easier to make the change inside of this single function instead of multiple places in code. bool isMultilingualSpellcheckerEnabled() { return command_line->HasSwitch(switches::kEnableMultilingualSpellChecker); }
https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:49: std::vector<std::string> languages_; Please add comments for all three variables and think about better names for "languages_selected_" and "languages_". Maybe "spellcheck_languages_" and "accepted_languages_". I'm suggesting "accepted_languages_" because Chrome uses the languages from chrome://settings/languages to let the webpage know which languages the user can read and the browser can accept.
https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:49: std::vector<std::string> languages_; On 2014/11/07 16:24:49, Rouslan Solomakhin wrote: > Please add comments for all three variables and think about better names for > "languages_selected_" and "languages_". Maybe "spellcheck_languages_" and > "accepted_languages_". I'm suggesting "accepted_languages_" because Chrome uses > the languages from chrome://settings/languages to let the webpage know which > languages the user can read and the browser can accept. Acknowledged. https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/220001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:55: if (command_line->HasSwitch(switches::kEnableMultilingualSpellChecker)) { On 2014/11/07 16:20:08, Rouslan Solomakhin wrote: > It's best to put this line into a separate function that you call here and > below. When we will move from a command-line argument to an experiment (enabled > by default for 10% of users), it will be easier to make the change inside of > this single function instead of multiple places in code. > > bool isMultilingualSpellcheckerEnabled() { > return command_line->HasSwitch(switches::kEnableMultilingualSpellChecker); > } Good idea, I'll do it this way.
Hi Rouslan, PTAL Patch Set 5. I've updated the chrome://settings/languages page to display checkboxes (instead of a button) when the enable multilingual spellchecker flag is set (in addition to the right-click menu), as well as the option to enable or disable the feature itself through the experiments page (chrome://flags).
https://codereview.chromium.org/654653002/diff/260001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/654653002/diff/260001/chrome/app/generated_re... chrome/app/generated_resources.grd:6198: + <message name="IDS_ENABLE_MULTILINGUAL_SPELLCHECKER_NAME" desc="Title for the flag to turn on multilingual spellchecker."> s/IDS/IDS_FLAGS/ https://codereview.chromium.org/654653002/diff/260001/chrome/app/generated_re... chrome/app/generated_resources.grd:6201: + <message name="IDS_ENABLE_MULTILINGUAL_SPELLCHECKER_DESCRIPTION" desc="Description for the flag to turn on multilingual spellchecker."> s/IDS/IDS_FLAGS/ https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2200: #if defined(ENABLE_SPELLCHECK) && (defined(OS_LINUX) || defined(OS_WIN)) Also OS_CHROMEOS. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2205: kOsWin | kOsLinux, Also kOsCrOS. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (left): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:172: switch (command_id) { No need to change this code. Please change it back to original. Let me know if there's a good reason to change this, but I missed it. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:126: return spellcheck_languages_.count(command_id - Please use "spellcheck_languages_.find(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST) != spellcheck_languages_.end();" There're two benefits to this: 1) It's easier for the reader to understand that you're looking up something instead of counting it. 2) If we ever switch to a vector instead of a set, it would be somewhat wasteful to go through the whole vector looking for multiple occurrences of the item. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:174: const std::size_t language = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST; size_t should be OK. No need to change it to std::size_t. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:176: if (language >= languages_.size()) This should never happen. Put a DCHECK here: DCHECK_LT(language, languages_.size()); https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:184: ',', Put a "static const char kLanguagesSeparator = ',';" in the anonymous namespace at the top of the file and use it instead of the literal comma here and below. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:187: auto found_language = std::find(languages_split.begin(), #include <algorithm> https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:197: JoinString(languages_split, ',')); base::JoinString https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:199: prefs->SetString(prefs::kSpellCheckDictionary, languages_[language]); Is this equivalent to the original code? StringPrefMember dictionary_language; dictionary_language.Init(prefs::kSpellCheckDictionary, profile->GetPrefs()); dictionary_language.SetValue(languages_[language]); https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:655: No need to introduce extra newline. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:676: DOWNLOAD_STATUS_IN_PROGRESS) { Please unify the dictionary download progress code. No need to repeat it here and below. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1074: if (isChecked) Inline "e.target.checked" https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1080: for (var languageCode in this.spellCheckDictionaries_) { Don't redefine "languageCode" variable inside of the for loop. Use a different name. (See "var languageCode = e.target.languageCode" above.) https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:84: prefs::kSpellCheckDictionary, No need to register kSpellCheckDictionary pref when multi-lingual spellchecker is enabled. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:89: StringPrefMember dictionary_languages_pref; You can use "dictionary_languages_pref" for both clauses of the if statement. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:91: No need for this newline. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:93: dictionary_languages_pref.GetValue(), ',', &dictionary_languages); You may want to put "static const char kDictionaryLanguagesSeparator = ',';" into spellcheck_common.h. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:97: No need for this newline. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:102: No need for this newline. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:120: for (std::size_t i = 0; i < dictionary_languages.size(); ++i) No need for "std::" before "size_t". If I missed a PSA about us switching to std::size_t, please let me know. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:121: selected_language_indices.insert(i); Add a comment that the the first |dictionary_languages| indices work because the |languages| array begins with the |dictionary_languages| array. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:131: for (const std::string& accept_language : accept_languages) { No need to refactor this. It's not directly related to the goal of your patch. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service_unittest.cc:109: EXPECT_EQ("fr", languages[0]); This will probably fail. Please run all unit tests to verify that existing behavior works OK. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_handler_common.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_handler_common.cc:131: enable_multilingual_spellchecker); Please inline "enable_multilingual_spellchecker."
https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2210: // NOTE: Adding new command-line switches requires adding corresponding You should probably follow this advice.
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #6 (id:320001) has been deleted
Rouslan, PTAL Patch Set 6. https://codereview.chromium.org/654653002/diff/260001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/654653002/diff/260001/chrome/app/generated_re... chrome/app/generated_resources.grd:6198: + <message name="IDS_ENABLE_MULTILINGUAL_SPELLCHECKER_NAME" desc="Title for the flag to turn on multilingual spellchecker."> On 2015/02/23 18:51:46, Rouslan Solomakhin wrote: > s/IDS/IDS_FLAGS/ Done. https://codereview.chromium.org/654653002/diff/260001/chrome/app/generated_re... chrome/app/generated_resources.grd:6201: + <message name="IDS_ENABLE_MULTILINGUAL_SPELLCHECKER_DESCRIPTION" desc="Description for the flag to turn on multilingual spellchecker."> On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > s/IDS/IDS_FLAGS/ Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2200: #if defined(ENABLE_SPELLCHECK) && (defined(OS_LINUX) || defined(OS_WIN)) On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Also OS_CHROMEOS. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2205: kOsWin | kOsLinux, On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Also kOsCrOS. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2210: // NOTE: Adding new command-line switches requires adding corresponding On 2015/02/23 18:56:22, Rouslan Solomakhin wrote: > You should probably follow this advice. Acknowledged. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (left): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:172: switch (command_id) { On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > No need to change this code. Please change it back to original. Let me know if > there's a good reason to change this, but I missed it. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:126: return spellcheck_languages_.count(command_id - On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Please use "spellcheck_languages_.find(command_id - > IDC_SPELLCHECK_LANGUAGES_FIRST) != spellcheck_languages_.end();" > > There're two benefits to this: > 1) It's easier for the reader to understand that you're looking up something > instead of counting it. > 2) If we ever switch to a vector instead of a set, it would be somewhat wasteful > to go through the whole vector looking for multiple occurrences of the item. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:174: const std::size_t language = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST; On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > size_t should be OK. No need to change it to std::size_t. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:176: if (language >= languages_.size()) On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > This should never happen. Put a DCHECK here: > > DCHECK_LT(language, languages_.size()); Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:184: ',', On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Put a "static const char kLanguagesSeparator = ',';" in the anonymous namespace > at the top of the file and use it instead of the literal comma here and below. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:187: auto found_language = std::find(languages_split.begin(), On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > #include <algorithm> Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:197: JoinString(languages_split, ',')); On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > base::JoinString JoinString is in the global namespace. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:199: prefs->SetString(prefs::kSpellCheckDictionary, languages_[language]); On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Is this equivalent to the original code? > > StringPrefMember dictionary_language; > dictionary_language.Init(prefs::kSpellCheckDictionary, profile->GetPrefs()); > dictionary_language.SetValue(languages_[language]); Yes, |StringPrefMember::SetValue()| calls |StringPrefMember::UpdatePref()|, which in turn calls |PrefService::SetString()|. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:655: On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > No need to introduce extra newline. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:676: DOWNLOAD_STATUS_IN_PROGRESS) { On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Please unify the dictionary download progress code. No need to repeat it here > and below. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1074: if (isChecked) On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Inline "e.target.checked" Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1080: for (var languageCode in this.spellCheckDictionaries_) { On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > Don't redefine "languageCode" variable inside of the for loop. Use a different > name. (See "var languageCode = e.target.languageCode" above.) Whoops, I missed this one. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:84: prefs::kSpellCheckDictionary, On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > No need to register kSpellCheckDictionary pref when multi-lingual spellchecker > is enabled. Done. Do I also create a separate IDS_SPELLCHECK_DICTIONARIES or can I reuse the IDS_SPELLCHECK_DICTIONARY? https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:89: StringPrefMember dictionary_languages_pref; On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > You can use "dictionary_languages_pref" for both clauses of the if statement. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:91: On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > No need for this newline. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:93: dictionary_languages_pref.GetValue(), ',', &dictionary_languages); On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > You may want to put "static const char kDictionaryLanguagesSeparator = ',';" > into spellcheck_common.h. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:97: On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > No need for this newline. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:102: On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > No need for this newline. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:120: for (std::size_t i = 0; i < dictionary_languages.size(); ++i) On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > No need for "std::" before "size_t". If I missed a PSA about us switching to > std::size_t, please let me know. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:121: selected_language_indices.insert(i); On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > Add a comment that the the first |dictionary_languages| indices work because the > |languages| array begins with the |dictionary_languages| array. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:131: for (const std::string& accept_language : accept_languages) { On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > No need to refactor this. It's not directly related to the goal of your patch. Done. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service_unittest.cc:109: EXPECT_EQ("fr", languages[0]); On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > This will probably fail. Please run all unit tests to verify that existing > behavior works OK. Why will these tests fail? |SpellcheckService::GetSpellCheckLanguagesFromAcceptLanguages| (now) accepts only two parameters and fills the |languages| vector with language codes that are correct. The tests should pass, but I'll verify anyway. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_handler_common.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_handler_common.cc:131: enable_multilingual_spellchecker); On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > Please inline "enable_multilingual_spellchecker." Done. It was a conscious decision though, because the above |enableSpellingAutoCorrect| is also set this way.
Patchset #6 (id:340001) has been deleted
Patchset #6 (id:360001) has been deleted
Patchset #6 (id:380001) has been deleted
Good job so far! https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:84: prefs::kSpellCheckDictionary, On 2015/02/24 21:57:45, Klemen Forstnerič wrote: > On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > > No need to register kSpellCheckDictionary pref when multi-lingual spellchecker > > is enabled. > > Done. Do I also create a separate IDS_SPELLCHECK_DICTIONARIES or can I reuse the > IDS_SPELLCHECK_DICTIONARY? Let's use the new IDS_SPELLCHECK_DICTIONARIES, because there may be unforeseen consequences of storing two different data types in two different locations. The code that expects commas will handle no comma case, but the code that does not expect commas might choke if it encounters a comma. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service_unittest.cc:109: EXPECT_EQ("fr", languages[0]); On 2015/02/24 21:57:46, Klemen Forstnerič wrote: > On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > > This will probably fail. Please run all unit tests to verify that existing > > behavior works OK. > > Why will these tests fail? > |SpellcheckService::GetSpellCheckLanguagesFromAcceptLanguages| (now) accepts > only two parameters and fills the |languages| vector with language codes that > are correct. The tests should pass, but I'll verify anyway. I suspect that this unit test will fail because it expects "fr" to be in the 1st item in "languages" array. Although "fr" is the 4th item in "accept_languages", the original code in GetSpellCheckLanguagesFromAcceptLanguages() was placing the "fr" into the 1st position because it was the language used for spellchecking. If the test does not fail, that's fine, I guess :-D https://codereview.chromium.org/654653002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/language_options_handler_common.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/language_options_handler_common.cc:131: enable_multilingual_spellchecker); On 2015/02/24 21:57:46, Klemen Forstnerič wrote: > On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > > Please inline "enable_multilingual_spellchecker." > > Done. It was a conscious decision though, because the above > |enableSpellingAutoCorrect| is also set this way. I see. Although we usually prefer to follow the convention of the surrounding code, I strongly feel that the surrounding code is subpar. ;-) https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:32: static const char kLanguagesSeparator = ','; Let's use the constant from spellcheck_common here as well. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:70: submenu_model_.AddRadioItem( Please format submenu_model_.AddCheckItem() and submenu_model_.AddRadioItem() similarly for ease of reading. Unfortunately clang-format is not of much help. You'll have to do this manually :-\. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:177: const size_t language = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST; "const" is not necessary. We try to not go overboard on making everything that does not change "const".. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:183: if (command_line->HasSwitch(switches::kEnableMultilingualSpellChecker)) { Please inline "command_line", because it's used only once in this method. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". Since this is a list of strings instead of a comma-separated string, the example should probably be: ["en-US", "sl-SI"] https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:673: spellCheckLanguageCheckbox.checked = true; We usually put a newline after an if-statement to make it easier to read. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:997: handleSpellCheckDictionariesPrefChange_: function(e) { Add a comment please. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:82: // TODO(estade): IDS_SPELLCHECK_DICTIONARY should be an ASCII string. This TODO applies to both clauses of the if statement. Please move it immediately before the statement. https://codereview.chromium.org/654653002/diff/400001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/654653002/diff/400001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1079: // Enables the multilingual spellchecker There appears to be a period at the end of the other comments in this file.
https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:32: static const char kLanguagesSeparator = ','; On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > Let's use the constant from spellcheck_common here as well. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:70: submenu_model_.AddRadioItem( On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > Please format submenu_model_.AddCheckItem() and submenu_model_.AddRadioItem() > similarly for ease of reading. Unfortunately clang-format is not of much help. > You'll have to do this manually :-\. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:177: const size_t language = command_id - IDC_SPELLCHECK_LANGUAGES_FIRST; On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > "const" is not necessary. We try to not go overboard on making everything that > does not change "const".. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:183: if (command_line->HasSwitch(switches::kEnableMultilingualSpellChecker)) { On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > Please inline "command_line", because it's used only once in this method. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > Since this is a list of strings instead of a comma-separated string, the example > should probably be: > > ["en-US", "sl-SI"] Actually it's a dictionary. I forgot to change the comment. It might be better if I changed it to '{"en-US": true, "sl-SI": true}'. What do you think? https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:673: spellCheckLanguageCheckbox.checked = true; On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > We usually put a newline after an if-statement to make it easier to read. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:997: handleSpellCheckDictionariesPrefChange_: function(e) { On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > Add a comment please. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:82: // TODO(estade): IDS_SPELLCHECK_DICTIONARY should be an ASCII string. On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > This TODO applies to both clauses of the if statement. Please move it > immediately before the statement. Done. https://codereview.chromium.org/654653002/diff/400001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/654653002/diff/400001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:1079: // Enables the multilingual spellchecker On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > There appears to be a period at the end of the other comments in this file. Done.
https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:84: prefs::kSpellCheckDictionary, On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > On 2015/02/24 21:57:45, Klemen Forstnerič wrote: > > On 2015/02/23 18:51:47, Rouslan Solomakhin wrote: > > > No need to register kSpellCheckDictionary pref when multi-lingual > spellchecker > > > is enabled. > > > > Done. Do I also create a separate IDS_SPELLCHECK_DICTIONARIES or can I reuse > the > > IDS_SPELLCHECK_DICTIONARY? > > Let's use the new IDS_SPELLCHECK_DICTIONARIES, because there may be unforeseen > consequences of storing two different data types in two different locations. The > code that expects commas will handle no comma case, but the code that does not > expect commas might choke if it encounters a comma. I agree, I will make the change. https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/654653002/diff/260001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service_unittest.cc:109: EXPECT_EQ("fr", languages[0]); On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > On 2015/02/24 21:57:46, Klemen Forstnerič wrote: > > On 2015/02/23 18:51:48, Rouslan Solomakhin wrote: > > > This will probably fail. Please run all unit tests to verify that existing > > > behavior works OK. > > > > Why will these tests fail? > > |SpellcheckService::GetSpellCheckLanguagesFromAcceptLanguages| (now) accepts > > only two parameters and fills the |languages| vector with language codes that > > are correct. The tests should pass, but I'll verify anyway. > > I suspect that this unit test will fail because it expects "fr" to be in the 1st > item in "languages" array. Although "fr" is the 4th item in "accept_languages", > the original code in GetSpellCheckLanguagesFromAcceptLanguages() was placing the > "fr" into the 1st position because it was the language used for spellchecking. > > If the test does not fail, that's fine, I guess :-D "fr" will actually still be at index 0, due to the fact that it's the only valid language code ("en" is not a valid, according to this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/spel...). So it's all good :-)
Patchset #8 (id:440001) has been deleted
Rouslan, PTAL at Patch Set 9. Please also see my inline comments.
Great progress! Please run "git cl try" on your patch when you send it out for a review. https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". On 2015/02/25 09:48:23, Klemen Forstnerič wrote: > On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > > Since this is a list of strings instead of a comma-separated string, the > example > > should probably be: > > > > ["en-US", "sl-SI"] > > Actually it's a dictionary. I forgot to change the comment. It might be better > if I changed it to '{"en-US": true, "sl-SI": true}'. What do you think? I'm not sure why this is a dictionary instead of a list or array. Can you explain please? (Also add a comment about this.) https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:130: IDC_SPELLCHECK_LANGUAGES_FIRST) != spellcheck_languages_.end(); Please run the clang-format on these two lines as well. See my instructions for emacs in my other comment. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:175: No need for this newline. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:179: switches::kEnableMultilingualSpellChecker)) { These two lines would be a good place to run clang-format. Don't run clang-format on whole file because it will introduce whitespace changes in code you did not modify.. Don't use "git cl format" because you formatted submenu_model_.AddCheckItem() and submenu_model_.AddRadioItem() by hand. "git cl format" will destroy that. If you're using emacs, follow instructions on https://code.google.com/p/chromium/wiki/Emacs#Use_Google's_C++_style! to install clang-format in ~/.emacs config, restart emacs, bring the cursor to the first line you want to format, press Ctrl-Space, move the cursor to the last line you want to format, then press Alt-X, and type "clang-format-region". This will format first to last line inclusively, I think. Let me know if you need instructions for another editor. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:999: * Handles spellCheckDictionariesPref change. Please provide a better explanation like "Updates spellcheck dictionary UI (checkboxes, buttons, and labels) when preferences change." https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1000: * @param {Event} e Change event. Please provide a better explanation like "Preference change event where e.value.value is the comma separated list of languages currently used for spellchecking." I know other places have simpler explanations, but let's not follow that particular convention... (No need to fix other people's comments, though.) https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1046: * Handles spellCheckLanguageCheckbox click. Please improve this comment similar to what I suggested in my comment above. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/480001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:77: l10n_util::GetStringUTF8(IDS_SPELLCHECK_DICTIONARIES), No need to introduce IDS_SPELLCHECK_DICTIONARIES. This is the default spellcheck dictionary to use for a new profile. It's identical to IDS_SPELLCHECK_DICTIONARY, so let's re-use that. Reusing IDS_SPELLCHECK_DICTIONARY will make your patch smaller, too. If you think the reader will get confused, feel free to add a comment.
https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > On 2015/02/25 09:48:23, Klemen Forstnerič wrote: > > On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > > > Since this is a list of strings instead of a comma-separated string, the > > example > > > should probably be: > > > > > > ["en-US", "sl-SI"] > > > > Actually it's a dictionary. I forgot to change the comment. It might be better > > if I changed it to '{"en-US": true, "sl-SI": true}'. What do you think? > > I'm not sure why this is a dictionary instead of a list or array. Can you > explain please? (Also add a comment about this.) I figured that since we're determining whether something is in |spellCheckDictionaries_| most of the time, it might make sense to use a dictionary.
rouslan@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". On 2015/02/26 15:37:42, Klemen Forstnerič wrote: > On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > > On 2015/02/25 09:48:23, Klemen Forstnerič wrote: > > > On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > > > > Since this is a list of strings instead of a comma-separated string, the > > > example > > > > should probably be: > > > > > > > > ["en-US", "sl-SI"] > > > > > > Actually it's a dictionary. I forgot to change the comment. It might be > better > > > if I changed it to '{"en-US": true, "sl-SI": true}'. What do you think? > > > > I'm not sure why this is a dictionary instead of a list or array. Can you > > explain please? (Also add a comment about this.) > > I figured that since we're determining whether something is in > |spellCheckDictionaries_| most of the time, it might make sense to use a > dictionary. Dan, what's the best JS collection data structure to use for checking if an certain item is in that collection?
On 2015/02/26 16:03:12, Rouslan Solomakhin wrote: > https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... > File chrome/browser/resources/options/language_options.js (right): > > https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... > chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". > On 2015/02/26 15:37:42, Klemen Forstnerič wrote: > > On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > > > On 2015/02/25 09:48:23, Klemen Forstnerič wrote: > > > > On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > > > > > Since this is a list of strings instead of a comma-separated string, the > > > > example > > > > > should probably be: > > > > > > > > > > ["en-US", "sl-SI"] > > > > > > > > Actually it's a dictionary. I forgot to change the comment. It might be > > better > > > > if I changed it to '{"en-US": true, "sl-SI": true}'. What do you think? > > > > > > I'm not sure why this is a dictionary instead of a list or array. Can you > > > explain please? (Also add a comment about this.) > > > > I figured that since we're determining whether something is in > > |spellCheckDictionaries_| most of the time, it might make sense to use a > > dictionary. > > Dan, what's the best JS collection data structure to use for checking if an > certain item is in that collection? You can search in O(1) time in a hash set (or a hash map), which is an Object in JS. Is it not?
https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:130: IDC_SPELLCHECK_LANGUAGES_FIRST) != spellcheck_languages_.end(); On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > Please run the clang-format on these two lines as well. See my instructions for > emacs in my other comment. Done. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:175: On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > No need for this newline. Done. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:179: switches::kEnableMultilingualSpellChecker)) { On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > These two lines would be a good place to run clang-format. > > Don't run clang-format on whole file because it will introduce whitespace > changes in code you did not modify.. > > Don't use "git cl format" because you formatted submenu_model_.AddCheckItem() > and submenu_model_.AddRadioItem() by hand. "git cl format" will destroy that. > > If you're using emacs, follow instructions on > https://code.google.com/p/chromium/wiki/Emacs#Use_Google to install > clang-format in ~/.emacs config, restart emacs, bring the cursor to the first > line you want to format, press Ctrl-Space, move the cursor to the last line you > want to format, then press Alt-X, and type "clang-format-region". This will > format first to last line inclusively, I think. > > Let me know if you need instructions for another editor. I got it all set up in Sublime Text, and I usually run it, but this line looked okay so I didn't do it. Fixed now. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:999: * Handles spellCheckDictionariesPref change. On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > Please provide a better explanation like "Updates spellcheck dictionary UI > (checkboxes, buttons, and labels) when preferences change." Done. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1000: * @param {Event} e Change event. On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > Please provide a better explanation like "Preference change event where > e.value.value is the comma separated list of languages currently used for > spellchecking." > > I know other places have simpler explanations, but let's not follow that > particular convention... (No need to fix other people's comments, though.) Done. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:1046: * Handles spellCheckLanguageCheckbox click. On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > Please improve this comment similar to what I suggested in my comment above. Done. https://codereview.chromium.org/654653002/diff/480001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/480001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:77: l10n_util::GetStringUTF8(IDS_SPELLCHECK_DICTIONARIES), On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > No need to introduce IDS_SPELLCHECK_DICTIONARIES. This is the default spellcheck > dictionary to use for a new profile. It's identical to > IDS_SPELLCHECK_DICTIONARY, so let's re-use that. Reusing > IDS_SPELLCHECK_DICTIONARY will make your patch smaller, too. > > If you think the reader will get confused, feel free to add a comment. Done.
On 2015/02/26 16:17:46, Klemen Forstnerič wrote: > You can search in O(1) time in a hash set (or a hash map), which is an Object in > JS. Is it not? yup https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * "en-US, sl-SI". On 2015/02/26 16:03:12, Rouslan Solomakhin wrote: > On 2015/02/26 15:37:42, Klemen Forstnerič wrote: > > On 2015/02/25 18:16:15, Rouslan Solomakhin wrote: > > > On 2015/02/25 09:48:23, Klemen Forstnerič wrote: > > > > On 2015/02/24 23:38:38, Rouslan Solomakhin wrote: > > > > > Since this is a list of strings instead of a comma-separated string, the > > > > example > > > > > should probably be: > > > > > > > > > > ["en-US", "sl-SI"] > > > > > > > > Actually it's a dictionary. I forgot to change the comment. It might be > > better > > > > if I changed it to '{"en-US": true, "sl-SI": true}'. What do you think? > > > > > > I'm not sure why this is a dictionary instead of a list or array. Can you > > > explain please? (Also add a comment about this.) > > > > I figured that since we're determining whether something is in > > |spellCheckDictionaries_| most of the time, it might make sense to use a > > dictionary. > > Dan, what's the best JS collection data structure to use for checking if an > certain item is in that collection? yeah, an object (see: hash map) is good for this. var obj = {}; obj.blah = true; var key = 'blee'; obj[key] = false; and then test with 'blah' in obj // tests if key exists obj.blah // true note: key in obj // true because key is set obj[key] // false and also properties are inherited, so: obj.hasOwnProperty(key) is arguably the safest (if Object.prototype.randomName exists, 'randomName' in ({}) would throw you off)
Cool, let's continue using a map and check key existence with map.hasOwnProperty(key).
Klemen, please fix the WebUI test failures: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
On 2015/02/26 19:03:44, Rouslan Solomakhin wrote: > Klemen, please fix the WebUI test failures: > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... I think this error might have something to do with the failures: [990:990:0226/110537:ERROR:CONSOLE(13867)] "Uncaught ReferenceError: DOWNLOAD_STATUS_IN_PROGRESS is not defined", source: chrome://settings-frame/options_bundle.js (13867)
I'm getting checkmarks for empty spots in submenu: http://i.imgur.com/H3Js6Wu.png Let's investigate and fix that before committing the patch...
On 2015/02/26 19:10:55, Rouslan Solomakhin wrote: > I'm getting checkmarks for empty spots in submenu: > > http://i.imgur.com/H3Js6Wu.png > > Let's investigate and fix that before committing the patch... I've been trying to figure out in what case that happens. Were you doing anything special when you encountered that empty entry?
On 2015/02/26 19:06:51, Rouslan Solomakhin wrote: > On 2015/02/26 19:03:44, Rouslan Solomakhin wrote: > > Klemen, please fix the WebUI test failures: > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > I think this error might have something to do with the failures: > > [990:990:0226/110537:ERROR:CONSOLE(13867)] "Uncaught ReferenceError: > DOWNLOAD_STATUS_IN_PROGRESS is not defined", source: > chrome://settings-frame/options_bundle.js (13867) Oops. :-D That's a typo, I've written DOWNLOAD_STATUS.IN_PROGRESS as DOWNLOAD_STATUS_IN_PROGRESS (DOWNLOAD_STATUS seems to be an enum). Fixed now.
On 2015/02/26 19:27:03, Klemen Forstnerič wrote: > On 2015/02/26 19:06:51, Rouslan Solomakhin wrote: > > On 2015/02/26 19:03:44, Rouslan Solomakhin wrote: > > > Klemen, please fix the WebUI test failures: > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > > > I think this error might have something to do with the failures: > > > > [990:990:0226/110537:ERROR:CONSOLE(13867)] "Uncaught ReferenceError: > > DOWNLOAD_STATUS_IN_PROGRESS is not defined", source: > > chrome://settings-frame/options_bundle.js (13867) > > Oops. :-D That's a typo, I've written DOWNLOAD_STATUS.IN_PROGRESS as > DOWNLOAD_STATUS_IN_PROGRESS (DOWNLOAD_STATUS seems to be an enum). Fixed now. https://code.google.com/p/chromium/wiki/ClosureCompilation
On 2015/02/26 19:25:29, Klemen Forstnerič wrote: > On 2015/02/26 19:10:55, Rouslan Solomakhin wrote: > > I'm getting checkmarks for empty spots in submenu: > > > > http://i.imgur.com/H3Js6Wu.png > > > > Let's investigate and fix that before committing the patch... > > I've been trying to figure out in what case that happens. Were you doing > anything special when you encountered that empty entry? 1) Opened a window with a <textarea>. 2) Opened a window with chrome://settings/language side by side. 3) Type "Hellloo world" into <textarea>. 4) Disable spellcheck for all languages in chrome://settings/language. 5) Enable spellcheck for all languages in chrome://settings/language. 6) Check the right-click menu in <textarea>. Regardless, you should handle bad data in settings correctly. If the settings file has ",en,,es,,,," then you should sanitize the data into "en,es".
On 2015/02/26 19:28:38, Dan Beam wrote: > On 2015/02/26 19:27:03, Klemen Forstnerič wrote: > > On 2015/02/26 19:06:51, Rouslan Solomakhin wrote: > > > On 2015/02/26 19:03:44, Rouslan Solomakhin wrote: > > > > Klemen, please fix the WebUI test failures: > > > > > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > > > > > I think this error might have something to do with the failures: > > > > > > [990:990:0226/110537:ERROR:CONSOLE(13867)] "Uncaught ReferenceError: > > > DOWNLOAD_STATUS_IN_PROGRESS is not defined", source: > > > chrome://settings-frame/options_bundle.js (13867) > > > > Oops. :-D That's a typo, I've written DOWNLOAD_STATUS.IN_PROGRESS as > > DOWNLOAD_STATUS_IN_PROGRESS (DOWNLOAD_STATUS seems to be an enum). Fixed now. > > https://code.google.com/p/chromium/wiki/ClosureCompilation Thanks for the link, I'm getting a Server Error 500, though, so I'll take a look at it once it's back online.
On 2015/02/26 19:36:04, Rouslan Solomakhin wrote: > On 2015/02/26 19:25:29, Klemen Forstnerič wrote: > > On 2015/02/26 19:10:55, Rouslan Solomakhin wrote: > > > I'm getting checkmarks for empty spots in submenu: > > > > > > http://i.imgur.com/H3Js6Wu.png > > > > > > Let's investigate and fix that before committing the patch... > > > > I've been trying to figure out in what case that happens. Were you doing > > anything special when you encountered that empty entry? > > 1) Opened a window with a <textarea>. > 2) Opened a window with chrome://settings/language side by side. > 3) Type "Hellloo world" into <textarea>. > 4) Disable spellcheck for all languages in chrome://settings/language. > 5) Enable spellcheck for all languages in chrome://settings/language. > 6) Check the right-click menu in <textarea>. > > Regardless, you should handle bad data in settings correctly. If the settings > file has ",en,,es,,,," then you should sanitize the data into "en,es". Okay, I'll investigate why it happens, and remove any bad entries before loading them in the menu.
Patchset #13 (id:560001) has been deleted
On 2015/02/26 19:41:43, Klemen Forstnerič wrote: > On 2015/02/26 19:36:04, Rouslan Solomakhin wrote: > > On 2015/02/26 19:25:29, Klemen Forstnerič wrote: > > > On 2015/02/26 19:10:55, Rouslan Solomakhin wrote: > > > > I'm getting checkmarks for empty spots in submenu: > > > > > > > > http://i.imgur.com/H3Js6Wu.png > > > > > > > > Let's investigate and fix that before committing the patch... > > > > > > I've been trying to figure out in what case that happens. Were you doing > > > anything special when you encountered that empty entry? > > > > 1) Opened a window with a <textarea>. > > 2) Opened a window with chrome://settings/language side by side. > > 3) Type "Hellloo world" into <textarea>. > > 4) Disable spellcheck for all languages in chrome://settings/language. > > 5) Enable spellcheck for all languages in chrome://settings/language. > > 6) Check the right-click menu in <textarea>. > > > > Regardless, you should handle bad data in settings correctly. If the settings > > file has ",en,,es,,,," then you should sanitize the data into "en,es". > > Okay, I'll investigate why it happens, and remove any bad entries before loading > them in the menu. There, I think I fixed it. I haven't tested it yet, though, due to my slow Linux machine. I'll do that tomorrow.
https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:105: dictionary_languages.erase(partition_point, dictionary_languages.end()); Instead of repeating this code, perhaps we can put it in spellcheck_common? std::vector<std::string> GetDictionaryLanguagesPref(); Also, please place a "DCHECK_LT(0, language.length());" inside of the lambda function. This will help you catch any code that mistakenly writes empty entries into your preference string. (DCHECKs trigger only in debug builds.)
https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:105: dictionary_languages.erase(partition_point, dictionary_languages.end()); On 2015/02/26 21:30:19, Rouslan Solomakhin wrote: > Instead of repeating this code, perhaps we can put it in spellcheck_common? > > std::vector<std::string> GetDictionaryLanguagesPref(); > > Also, please place a "DCHECK_LT(0, language.length());" inside of the lambda > function. This will help you catch any code that mistakenly writes empty entries > into your preference string. (DCHECKs trigger only in debug builds.) size_t < 0?! mind == blown. i think rouslan@ meant GT (greater than, >), but the easiest would be: DCHECK(!language.empty());
https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:105: dictionary_languages.erase(partition_point, dictionary_languages.end()); On 2015/02/26 22:04:32, Dan Beam wrote: > On 2015/02/26 21:30:19, Rouslan Solomakhin wrote: > > Instead of repeating this code, perhaps we can put it in spellcheck_common? > > > > std::vector<std::string> GetDictionaryLanguagesPref(); > > > > Also, please place a "DCHECK_LT(0, language.length());" inside of the lambda > > function. This will help you catch any code that mistakenly writes empty > entries > > into your preference string. (DCHECKs trigger only in debug builds.) > > size_t < 0?! mind == blown. i think rouslan@ meant GT (greater than, >), but the > easiest would be: > > DCHECK(!language.empty()); oh, I guess the operand order is right in rouslan@'s command, but I think !empty() is easier to grok
https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:105: dictionary_languages.erase(partition_point, dictionary_languages.end()); On 2015/02/26 21:30:19, Rouslan Solomakhin wrote: > Instead of repeating this code, perhaps we can put it in spellcheck_common? > > std::vector<std::string> GetDictionaryLanguagesPref(); > > Also, please place a "DCHECK_LT(0, language.length());" inside of the lambda > function. This will help you catch any code that mistakenly writes empty entries > into your preference string. (DCHECKs trigger only in debug builds.) Good idea. Done. https://codereview.chromium.org/654653002/diff/580001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:105: dictionary_languages.erase(partition_point, dictionary_languages.end()); On 2015/02/26 22:30:23, Dan Beam wrote: > On 2015/02/26 22:04:32, Dan Beam wrote: > > On 2015/02/26 21:30:19, Rouslan Solomakhin wrote: > > > Instead of repeating this code, perhaps we can put it in spellcheck_common? > > > > > > std::vector<std::string> GetDictionaryLanguagesPref(); > > > > > > Also, please place a "DCHECK_LT(0, language.length());" inside of the lambda > > > function. This will help you catch any code that mistakenly writes empty > > entries > > > into your preference string. (DCHECKs trigger only in debug builds.) > > > > size_t < 0?! mind == blown. i think rouslan@ meant GT (greater than, >), but > the > > easiest would be: > > > > DCHECK(!language.empty()); > > oh, I guess the operand order is right in rouslan@'s command, but I think > !empty() is easier to grok I think so too, DCHECK(!language.empty()) is more readable. Done.
Patchset #15 (id:620001) has been deleted
Patchset #15 (id:640001) has been deleted
Rouslan, PTAL at Patch Set 15.
On 2015/03/02 19:28:53, Klemen Forstnerič wrote: > Rouslan, PTAL at Patch Set 15. It works! Please clean up the patch description and ask OWNERs for a review. Ping me if you need help with these tasks.
klemen.forstneric@gmail.com changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes in tools/metrics/histograms/histograms.xml
klemen.forstneric@gmail.com changed reviewers: + groby@chromium.org
groby@chromium.org: Please review changes in chrome/browser/spellchecker/spellcheck_factory.cc chrome/browser/spellchecker/spellcheck_service.h chrome/browser/spellchecker/spellcheck_service.cc chrome/browser/spellchecker/spellcheck_service_unittest.cc
klemen.forstneric@gmail.com changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc
Overall design lgtm
dbeam@chromium.org: Please review changes in chrome/browser/resources/options/language_options.html chrome/browser/resources/options/language_options.js in Patch Set 15.
On 2015/03/02 22:08:51, Rouslan Solomakhin wrote: > Overall design lgtm The context menu stuff LGTM. What's the intended timeline for moving this from out behind the flag and removing the flag you're adding here?
On 2015/03/02 22:17:24, Avi wrote: > On 2015/03/02 22:08:51, Rouslan Solomakhin wrote: > > Overall design lgtm > > The context menu stuff LGTM. > > What's the intended timeline for moving this from out behind the flag and > removing the flag you're adding here? I can answer this question as a shepherd of the patch: we intend to remove the flag after the feature is implemented 100% and is well tested. This patch changes the way preferences are handled, but spellcheck itself still still uses only a single dictionary for spellchecking.
https://codereview.chromium.org/654653002/diff/660001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/renderer... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:172: PrefService* prefs = profile->GetPrefs(); nit: either declare this closer to where it's used or re-use it more https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... chrome/browser/resources/options/language_options.html:45: id="language-options-spell-check-language-checkbox-div"> super-duper-long-ids-make-me-sad :( (probably not much you can do, though) https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:65: * @type {string} nit: @const {string} is shorter (and a newer syntax) https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:156: * like "{"en-US": true, "sl-SI": true}". nit: remove quotes around the dictionary, e.g. like {"en-US": true, "sl-SI": true}. https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:673: spellCheckLanguageCheckbox.checked = true; nit: spellCheckLanguageCheckbox.checked = this.spellCheckDictionaries_.hasOwnProperty(languageCode); if this checkbox should also ensuredly be unchecked if languageCode *isn't* in spellCheckDictionaries_ https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:676: } else if (languageCode == this.spellCheckDictionary_) { how will a string ever be equal to a dictionary? (at least in a non-evil way) https://codereview.chromium.org/654653002/diff/660001/chrome/browser/resource... chrome/browser/resources/options/language_options.js:688: var status = this.spellcheckDictionaryDownloadStatus_[languageCode]; // ... re-use status ... or better yet, use a `switch () {` here https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:83: user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF); do these preferences need to be migrated?
histograms.xml lgtm
c/b/spellchecker only https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:74: switches::kEnableMultilingualSpellChecker)) { How is preference migration handled? (I.e. what happens if I enable/disable multiple spellchecks) https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_factory.cc:83: user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF); On 2015/03/02 22:40:08, Dan Beam wrote: > do these preferences need to be migrated? Heh. That was off-screen when I asked :) https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:100: *languages = dictionary_languages; Can you please leave a short comment somewhere how the function decides which languages to use? https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.cc:105: std::set<int> selected_language_indices; If it's always the first <n> elements, why not return <n> instead of a set/vector? https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.h:59: // returns the set of indices of the current spell check languages in the nit: can you change "in the vector" to "in |languages|"? https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service.h:63: static std::set<int> GetSpellCheckLanguages( Is there a particular reason to make it a set? Would vector do? https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/654653002/diff/660001/chrome/browser/spellche... chrome/browser/spellchecker/spellcheck_service_unittest.cc:87: SpellcheckService::GetSpellCheckLanguagesFromAcceptLanguages( There don't seem to be any tests for the return value. If we don't need it at all, let's void it. Otherwise, please add a test :)
ping?
FYI: Klemen is currently busy, will return to this.
Hi, could you (Rouslan or Klemen) shortly report the status of this? I thought it was nearly finished and wanted to push it further (probably to the state that would satisfy me, not neccessary to the shippable state), but apparently right now only UI works but don't do the spellchecking - am I right? Anyway, thanks for starting to write this code.
On 2015/04/15 19:29:31, sygi wrote: > Hi, > could you (Rouslan or Klemen) shortly report the status of this? I thought it > was nearly finished and wanted to push it further (probably to the state that > would satisfy me, not neccessary to the shippable state), but apparently right > now only UI works but don't do the spellchecking - am I right? > Anyway, thanks for starting to write this code. You're right, the UI is almost finished. Klemen needs to address reviewer's feedback, but is busy with school work. If you want to coordinate with Klemen, you can pick up where he left off and complete the UI part in this patch. The next patch would need to be the back end implementation. Keep in mind that this is a significant time commitment, even if you're an experienced engineer, because Chromium's standards are quite high... Let me know what you think.
As I needed this feature myself, I implemented a quick & dirty solution. It will definitely need a lot of refactoring, testing, matching chromium codestyle, etc., but the basic functionality is there. I can try sharing it with you if you want to take the inspiration from it. But to do it I think I need edit privileges in this issue and later - how do I add the code to the current CL? Should I checkout master, create own branch, set the issue number, git cl try, git cl upload?
On 2015/04/20 07:49:20, sygi wrote: > As I needed this feature myself, I implemented a quick & dirty solution. It will > definitely need a lot of refactoring, testing, matching chromium codestyle, > etc., but the basic functionality is there. I can try sharing it with you if you > want to take the inspiration from it. > But to do it I think I need edit privileges in this issue and later - how do I > add the code to the current CL? Should I checkout master, create own branch, set > the issue number, git cl try, git cl upload? You should upload a new CL: create own branch, apply changes, git cl upload. |
